Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add tls settings for BackupPolicy #580

Merged
merged 13 commits into from
Oct 27, 2023
Merged

Add tls settings for BackupPolicy #580

merged 13 commits into from
Oct 27, 2023

Conversation

kmdkuk
Copy link
Contributor

@kmdkuk kmdkuk commented Oct 17, 2023

  • Add ca-cert flag to moco-backup
  • Add caCert field to BackupPolicy
  • Enable minio tls used for testing
  • Add test using minio with TLS
    • refactoring: Use Gomega matcher in Eventually on backup_test

Signed-off-by: kouki [email protected]

@kmdkuk kmdkuk self-assigned this Oct 17, 2023
@kmdkuk kmdkuk force-pushed the backup-enable-tls branch 5 times, most recently from eedf619 to 19bb6c6 Compare October 20, 2023 06:03
@kmdkuk kmdkuk changed the title wip: enable tls for backup wip: Add tls settings for BackupPolicy Oct 20, 2023
@kmdkuk kmdkuk changed the title wip: Add tls settings for BackupPolicy Add tls settings for BackupPolicy Oct 20, 2023
@kmdkuk kmdkuk marked this pull request as ready for review October 23, 2023 00:14
api/v1beta2/job_types.go Outdated Show resolved Hide resolved
cmd/moco-backup/cmd/root.go Outdated Show resolved Hide resolved
e2e/backup_test.go Show resolved Hide resolved
e2e/testdata/makebucket.yaml Outdated Show resolved Hide resolved
e2e/minio.yaml Outdated Show resolved Hide resolved
yamatcha
yamatcha previously approved these changes Oct 24, 2023
Copy link
Member

@ymmt2005 ymmt2005 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, we should run 2 minio instances, one with TLS enabled and another with disabled.
And run the tests for both.

@kmdkuk
Copy link
Contributor Author

kmdkuk commented Oct 25, 2023

@ymmt2005

Thank you for your comment.
I intentionally created only tests with TLS enabled, thinking that it would be enough to have tests with TLS enabled.
Why is it better to have both cases?
Is it a test coverage issue?

@ymmt2005
Copy link
Member

I just thought it was not enough.

@kmdkuk
Copy link
Contributor Author

kmdkuk commented Oct 25, 2023

I wanted to add test cases a little more intelligently, but I added tests using two minio.
137c8c0

@ymmt2005
Copy link
Member

@kmdkuk thank you!

@kmdkuk kmdkuk force-pushed the backup-enable-tls branch 2 times, most recently from 5633a2f to 87da18d Compare October 25, 2023 04:31
@kmdkuk
Copy link
Contributor Author

kmdkuk commented Oct 25, 2023

There was conflict, so I resolve it.

docs/moco-backup.md Outdated Show resolved Hide resolved
@ymmt2005
Copy link
Member

This looks like a fundamental error.

STEP: getting process list

• Failure [29.555 seconds]
kill
/home/runner/work/moco/moco/pkg/dbop/kill_test.go:20
  should kill non-system processes only [It]
  /home/runner/work/moco/moco/pkg/dbop/kill_test.go:21

  Unexpected error:
      <*fmt.wrapError | 0xc000[202](https://github.com/cybozu-go/moco/actions/runs/6647568774/job/18063192309?pr=580#step:6:203)4e0>: 
      sql: Scan error on column index 3, name "STATE": converting NULL to string is unsupported
      {
          msg: "sql: Scan error on column index 3, name \"STATE\": converting NULL to string is unsupported",
          err: <*errors.errorString | 0xc0000563c0>{
              s: "converting NULL to string is unsupported",
          },
      }
  occurred

  /home/runner/work/moco/moco/pkg/dbop/kill_test.go:46

Cc: @zoetrope

@ymmt2005
Copy link
Member

No it's not. It's just due to a poor test code...

type process struct {
        ID    uint64 `db:"ID"`
        User  string `db:"USER"`
        Host  string `db:"HOST"`
        State string `db:"STATE"` // for debugging
}

var _ = Describe("kill", func() {
        It("should kill non-system processes only", func() {
                By("preparing a single node cluster")
                cluster := &mocov1beta2.MySQLCluster{}
                cluster.Namespace = "test"
                cluster.Name = "kill"
                cluster.Spec.Replicas = 1

                passwd, err := password.NewMySQLPassword()
                Expect(err).NotTo(HaveOccurred())

                op, err := factory.New(context.Background(), cluster, passwd, 0)
                Expect(err).NotTo(HaveOccurred())

                By("creating a user and making a connection with the user")
                _, err = op.(*operator).db.Exec("SET GLOBAL read_only=0")
                Expect(err).NotTo(HaveOccurred())
                _, err = op.(*operator).db.Exec("CREATE USER 'foo'@'%' IDENTIFIED BY 'bar'")
                Expect(err).NotTo(HaveOccurred())
                db, err := factory.(*testFactory).newConn(context.Background(), cluster, "foo", "bar", 0)
                Expect(err).NotTo(HaveOccurred())
                defer db.Close()

                By("getting process list")
                var procs []process
                err = op.(*operator).db.Select(&procs, `SELECT ID, USER, HOST, STATE FROM information_schema.PROCESSLIST`)
                Expect(err).NotTo(HaveOccurred())

@ymmt2005
Copy link
Member

@kmdkuk @zoetrope
can you fix that bug in kill_test.go? I think we should use sql.NullString.
https://pkg.go.dev/database/sql#NullString

@ymmt2005
Copy link
Member

@masa213f
That State field is added by you.
I have a question; why did you define a new type process instead of adding the State field to the existing type Process?

e2e/backup_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@morimoto-cybozu morimoto-cybozu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ymmt2005 ymmt2005 merged commit c17a4a2 into main Oct 27, 2023
15 checks passed
@ymmt2005 ymmt2005 deleted the backup-enable-tls branch October 27, 2023 01:07
@masa213f
Copy link
Contributor

@ymmt2005

I have a question; why did you define a new type process instead of adding the State field to the existing type Process?

It is just for debugging. I meant to delete the type process when I finished the debug, but I forgot.
The kill_test.go has been flaky before #515. But I don't know the reason why kill connection failed. So I tried to get more information.

@ymmt2005
Copy link
Member

@masa213f Thank you for answering. So, are you going to remove it?

@masa213f
Copy link
Contributor

@ymmt2005
Yes. I'll revert the PR. I had not been sure that the flaky test was fixed by the Eventually.
#595

@masa213f masa213f mentioned this pull request Oct 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants