Feature db operator watches for db passwords

[vladimir22]

Hi colleagues, I like your db-operator solution and believe it has a bright future!

I would like to propose you to add some enhancement into the database_controller.go to watch for the Secrets Update Events related to corresponding database resource.

In this case we can easily add new "DB Password Rotation" feature, to rotate already existing DB passwords.

Could you review my PR and share your thoughts about that?

Thanks.

P.S: I have added logic for WATCH_NAMESPACE env because not found that, it is optional, probably you have better solution. The main purpose of the PR is to watch for DB password updates.

[dabde]

Hi, had a quick look and already some questions coming in my mind.

db-operator support to use existing secrets, but can also auto generate this if not existing (so you don't need to manage secrets at all in some kind of source code). For each database you could have separate credentials. Looks with your function getDatabasesBySecret, you using one credential for several db's. Is this correct?

The functions under SetupWithManager, I would split out to a dedicated file, to keep the function call there simple.

General I like the idea of rotating secrets/credentials. But except the change and event against the db-operator. Also your application need to support this. Or have a transition phase where both credentials are valid.

Think also you code is not working. We have a checksum check to know when something has changed on the db CR, to skip events without changes onto the specs field. See https://github.com/kloeckner-i/db-operator/blob/master/controllers/helper.go#L27 ... related we don't have any checksum over the secrets, it will just ignore any changes there atm.

[vladimir22]

Hi @dabde , thanks for your review and your tips. Let me share my updates regarding your answer:

db-operator support to use existing secrets, but can also auto generate this if not existing (so you don't need to manage secrets at all in some kind of source code).

It should not conflict with existing design, because Secret Update Event will be processed only. (Secret Create and Secret Delete Events will be ignored).

For each database you could have separate credentials. Looks with your function getDatabasesBySecret, you using one credential for several db's. Is this correct?

I think you right, it is NOT correct to allow using shared DB credentials by customers because of possible conflicts. I have fixed that.

The functions under SetupWithManager, I would split out to a dedicated file, to keep the function call there simple.

Done , created separate database_secret_handler.go

General I like the idea of rotating secrets/credentials. But except the change and event against the db-operator.

I like too :). Currently, I think that your db-operator is the best place for watching for the "Db Secret Update" events.

Also your application need to support this. Or have a transition phase where both credentials are valid.

We are going to implement "Cluster Level Secret Rotation" logic using your db-operator + reloader operator for that.

Think also you code is not working. We have a checksum check to know when something has changed on the db CR, to skip events without changes onto the specs field. See https://github.com/kloeckner-i/db-operator/blob/master/controllers/helper.go#L27 ... related we don't have any checksum over the secrets, it will just ignore any changes there atm.

Yes, you right, to fix that I have introduced additional checksum/secret custom annotation. What do you think about that ?

P.S.: Currently , I see that logic is working on my local cluster. Please share your thoughts about these changes, after your approve I will fix minor issues and cover new logic by unit tests.

Thanks.

[hyunysmile]

Can please update our db-operator helm chart to configure namespace through values file?
https://github.com/kloeckner-i/db-operator/blob/master/charts/db-operator/templates/operator.yaml#L44
Afterwards, please increment version in Chart.yaml

[vladimir22]

Can please update our db-operator helm chart to configure namespace through values file?
https://github.com/kloeckner-i/db-operator/blob/master/charts/db-operator/templates/operator.yaml#L44
Afterwards, please increment version in Chart.yaml

added watchNamespace value into the helm chart.

@hyunysmile please review this PR again.
Thanks

[vladimir22]

@dabde @hyunysmile
I have done with parsing comma-separated values

Please review this PR again.
Thanks.

[dabde]

For the failing test. think the system is going slower than in the past. Think it's fine to increate the reties and the interval like this in the integration/test.sh file

retry=30
interval=15

[vladimir22]

@dabde done, I have updated integration/test.sh values to

retry=30
interval=15

[dabde]

okay, was try to run the integration test locally. looks like the databaseSecrets pointer is not correct. Will also debug it on my side.

time="2022-03-15T08:50:50Z" level=info msg="Instance: name=pg-gsql-instance Creating"
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x120 pc=0x144dd9f]

goroutine 326 [running]:
github.com/kloeckner-i/db-operator/controllers.addDBChecksum(0xc0001dac00, 0x0)
	/opt/db-operator/controllers/helper.go:43 +0xbf
github.com/kloeckner-i/db-operator/controllers.(*DatabaseReconciler).Reconcile(0xc000590de0, {0x1a30e78, 0xc000299320}, {{{0xc0003b963c, 0x4}, {0xc0003b9620, 0xd}}})
	/opt/db-operator/controllers/database_controller.go:175 +0x229c
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Reconcile(0xc0002dea00, {0x1a30e78, 0xc000299260}, {{{0xc0003b963c, 0x16daec0}, {0xc0003b9620, 0xc0003bf600}}})
	/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.10.0/pkg/internal/controller/controller.go:114 +0x222
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).reconcileHandler(0xc0002dea00, {0x1a30dd0, 0xc0003bf580}, {0x165e940, 0xc0003c20a0})
	/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.10.0/pkg/internal/controller/controller.go:311 +0x2f2
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).processNextWorkItem(0xc0002dea00, {0x1a30dd0, 0xc0003bf580})
	/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.10.0/pkg/internal/controller/controller.go:266 +0x205
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Start.func2.2()
	/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.10.0/pkg/internal/controller/controller.go:227 +0x85
created by sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Start.func2
	/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.10.0/pkg/internal/controller/controller.go:223 +0x354

[vladimir22]

@dabde yes, you right, it is because current logic allows to generate DatabaseSecret with random values if it is not exist.

I have committed fix for that case.
What do you think about that ?
Thanks.

[dabde]

@vladimir22 can you increase the app version to "1.5.0". Then I will approve and release the new version.

[vladimir22]

@dabde done, thank you very much :)