ROX-12934: Fix operator reconciliation when external DB is in use by misberner · Pull Request #3322 · stackrox/stackrox

misberner · 2022-10-05T18:22:38Z

Description

This PR fixes the reconciliation of the central-db persistent volume and password.

Unfortunately, the JIRA is missing a concrete description of the issue (and also doesn't cover all the existing issues). However, this PR functionally addresses the following:

a generated password secret is deleted when central DB is disabled
when an external DB is used, the use of a manually specified secret is enforced
when an external DB is used, and the specified secret is not called central-db-secret, the contents of the user-supplied secret are mirrored into a central-db-secret secret (as only that is mounted into Central)
when the password entry in the manually specified secret changed, the central-db-secret password is also updated upon reconciliation
when there exists an unmanaged central-db-secret, and a password is manually specified in a different secret, reconciliation will fail if the passwords are different
if the central DB deployment is not used, no PVC will be created
if the central DB deployment is turned off, an existing, managed PVC will be disowned
better error checking to prevent inconsistent settings, such as using central-db persistence setting with an external DB

On the non-functional sides, this PR improves some naming and adds extensive unit tests for central DB password reconciler, as well as kuttl tests.

This PR is based upon #3293 by @connorgorman .

Checklist

Investigated and inspected CI test results
Unit test and regression tests added
~~Evaluated and added CHANGELOG entry if required~~
~~Determined and documented upgrade steps~~
~~Documented user facing changes (create PR based on openshift/openshift-docs and merge into rhacs-docs)~~

If any of these don't apply, please comment below.

Testing Performed

Manual testing
CI

ghost · 2022-10-05T18:44:22Z

Images are ready for the commit at e3059f6.

To use with deploy scripts, first export MAIN_IMAGE_TAG=3.72.x-498-ge3059f6665.

operator/tests/central/basic/75-assert.yaml

operator/tests/central/basic/76-assert.yaml

operator/apis/platform/v1alpha1/central_types.go

operator/pkg/central/extensions/reconcile_central_db_password.go

porridge · 2022-10-20T07:16:28Z

operator/tests/central/basic/76-assert.yaml

+  tail: -1
+---
+apiVersion: apps/v1
+kind: Deployment


Might be good to also assert health of the central deployment in addition to central-db's health?

porridge · 2022-10-20T07:19:45Z

operator/pkg/central/values/translation/translation.go

+			cv.SetError(errors.New("if a connection string is provided, no persistence settings must be supplied"))
+		}
+
+		// TODO: there are other settings which are ignored in external mode - should we error if those are set, too?


+1 for erroring loud and clear on dubious configurations

I can add it. My main reservation against this stemmed from the fact that switching between configurations gets challenging, as you cannot comment out stuff in objects on etcd when doing, e.g., oc edit. But I guess it's not a very important use case to switch back and forth between internal and external DB.

Yeah, I think now I remember the problem, but that problem is even bigger.

The OpenShift console has the very unfortunate property of importing OpenAPI field defaults into the form and then acting as if the user has set those field values. See:

Screen.Recording.2022-10-26.at.18.26.49.mov

Currently there's no issue because Gorman didn't introduce a default value for the PVC name for the DB, but, arguably, there should be one, as we also have a stackrox-db default for the central persistence, and setting a default for a field is really nothing very unreasonable.

The bigger problem now is that setting a default for any of the fields means a user using OpenShift console can no longer disable central DB. OpenShift console simply does not respect the fact that (non-)nullness of fields in the CR might carry any meaning, it uses a lossy round-trip between the two representations. This, by the way, also applies in the other direction: with the existing CR structure, it is impossible to specify "use central DB, but with default settings" in the form view in OpenShift console, you have to set any of the fields (such as explicitly typing in the default central-db pvc name) or use YAML view.

@porridge I know you had some discussion with Gorman on the original PR re: an explicit boolean field vs the mere presence of another field (this was about external, not the DB as a whole). Putting the aspect of bool vs enum aside: what is your opinion on (a) avoiding explicit enabled/disabled fields and (b) erroring on inconsistent combinations of set fields, in light of these OpenShift console form view realities?

Oh, wow. This is a game-changer. 😞
I guess this means that mere presence of a struct can no longer have any meaning.
This in turn means CentralDBEnabled() needs to look at a new explicit toggle (I would very much prefer an enum on the CentralComponentSpec level rather than bools) rather than at the nilness of .DB 🤔

Please also mention in a (non-user visible) comment somewhere why we don't look at struct nilness. This is probably not obvious for a non-openshift developer.

SimonBaeumer · 2022-10-24T13:42:28Z

operator/pkg/central/extensions/reconcile_central_db_password.go

+		return errors.New("no password secret was specified")
+	}
+
+	passwordSecretName := r.centralObj.Spec.Central.DB.PasswordSecret.Name


Suggested change

passwordSecretName := r.centralObj.Spec.Central.DB.PasswordSecret.Name

passwordSecretName := r.centralObj.Spec.Central.DB.GetPasswordSecret().Name

I think this only makes sense if there's a nil-safe getter for Name. Adding this.

actually, we already check that GetPasswordSecret() is non-nil above, so I don't think this adds any value.

SimonBaeumer · 2022-10-24T13:44:42Z

operator/pkg/central/extensions/reconcile_central_db_password.go

+		return errors.Wrapf(err, "reading central db password from secret %s", passwordSecretName)
+	}
+
+	r.password = password


Why not returning the password and setting it from the main function? It feels odd to me because the result of that function is not returned, instead in sets state on the struct.
Setting it from the caller makes it more explicit to me.

I can change it. I don't feel very strongly tbh because the reconciliation execution structs are heavily dependent on modifying state. For example, validate also sets the current password, and this is intended.

Actually, I think it makes more sense to leave the behavior as-is. I will rename the function to readAndSetPassword...

SimonBaeumer · 2022-10-24T13:46:19Z

operator/pkg/central/extensions/reconcile_central_db_password_test.go

+			Namespace: testutils.TestNamespace,
+		},
+		Data: map[string][]byte{
+			"password": []byte(fmt.Sprintf("%s\n", pw1)),


Nice job, did not thought about this test-case.

SimonBaeumer · 2022-10-24T13:48:41Z

operator/pkg/central/extensions/reconcile_pvc.go

-		return central.Spec.Central.GetPersistence()
-	case DefaultCentralDBPVCName:
-		return convertDBPersistenceToPersistence(central.Spec.Central.DB.GetPersistence())
+func getPersistenceByTarget(central *platform.Central, target PVCTarget) *platform.Persistence {


Can you add a small doc string what this function is used for? I found it hard to understand, looking into the caller also did not really helped me.

porridge · 2022-10-28T07:34:19Z

operator/apis/platform/v1alpha1/central_types.go

+	// When using this option, you must explicitly set a password secret; automatically generating a password will not
+	// be supported.


Suggested change

// When using this option, you must explicitly set a password secret; automatically generating a password will not

// be supported.

// When using this option, you must explicitly set a password secret; a password will not be automatically generated.

porridge · 2022-10-28T07:40:24Z

operator/pkg/central/values/translation/translation.go

+			cv.SetError(errors.New("if a connection string is provided, no persistence settings must be supplied"))
+		}
+
+		// TODO: there are other settings which are ignored in external mode - should we error if those are set, too?


Oh, wow. This is a game-changer. 😞
I guess this means that mere presence of a struct can no longer have any meaning.
This in turn means CentralDBEnabled() needs to look at a new explicit toggle (I would very much prefer an enum on the CentralComponentSpec level rather than bools) rather than at the nilness of .DB 🤔

Please also mention in a (non-user visible) comment somewhere why we don't look at struct nilness. This is probably not obvious for a non-openshift developer.

operator/pkg/central/extensions/reconcile_central_db_password_test.go

openshift-ci · 2022-11-03T11:46:28Z

@misberner: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/eks-qa-e2e-tests	`5f7c4e7`	link	false	`/test eks-qa-e2e-tests`
ci/prow/osd-aws-qa-e2e-tests	`5f7c4e7`	link	false	`/test osd-aws-qa-e2e-tests`
ci/prow/rosa-qa-e2e-tests	`5f7c4e7`	link	false	`/test rosa-qa-e2e-tests`
ci/prow/aro-qa-e2e-tests	`5f7c4e7`	link	false	`/test aro-qa-e2e-tests`
ci/prow/gke-upgrade-tests	`5f7c4e7`	link	false	`/test gke-upgrade-tests`
ci/prow/gke-postgres-upgrade-tests	`5213d5f`	link	false	`/test gke-postgres-upgrade-tests`

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

…ing before)

…test.go

) Co-authored-by: Connor Gorman <cgorman@redhat.com> Co-authored-by: Vlad Bologa <vbologa@redhat.com>

openshift-ci bot added the do-not-merge/work-in-progress label Oct 5, 2022

github-actions bot added the area/operator label Oct 5, 2022

misberner force-pushed the mi/rox-12934-operator-reconcile-external-db branch 2 times, most recently from 5f7c4e7 to 89fefa6 Compare October 18, 2022 11:10

misberner commented Oct 18, 2022

View reviewed changes

operator/tests/central/basic/75-assert.yaml Outdated Show resolved Hide resolved

misberner commented Oct 18, 2022

View reviewed changes

operator/tests/central/basic/76-assert.yaml Outdated Show resolved Hide resolved

misberner force-pushed the mi/rox-12934-operator-reconcile-external-db branch from 83cf14a to 823b611 Compare October 19, 2022 13:24

misberner requested review from SimonBaeumer, c-du, porridge and vladbologa October 19, 2022 13:49

misberner changed the title ~~[wip] ROX-12934: Fix operator reconciliation when external DB is in use~~ ROX-12934: Fix operator reconciliation when external DB is in use Oct 19, 2022

openshift-ci bot removed the do-not-merge/work-in-progress label Oct 19, 2022

misberner force-pushed the mi/rox-12934-operator-reconcile-external-db branch from 0b709ce to 5213d5f Compare October 19, 2022 21:37

porridge approved these changes Oct 20, 2022

View reviewed changes

SimonBaeumer suggested changes Oct 24, 2022

View reviewed changes

misberner force-pushed the mi/rox-12934-operator-reconcile-external-db branch from 5213d5f to da160c9 Compare October 26, 2022 17:04

porridge reviewed Oct 28, 2022

View reviewed changes

vladbologa requested changes Oct 31, 2022

View reviewed changes

operator/pkg/central/extensions/reconcile_central_db_password_test.go Outdated Show resolved Hide resolved

SimonBaeumer approved these changes Oct 31, 2022

View reviewed changes

misberner and others added 8 commits November 3, 2022 12:54

X-Smart-Branch-Parent: origin/master

c393b13

Fix Operator reconciliation when using secrets and external DB

f4bcbfc

rm unused password

1f6771d

kuttl test, unit tests, some bug fixes

c76f548

style fixes

c698c56

regenerate operator assets

98ecab2

fix reconciliation of central DB TLS secret

5a244a5

don't return a nil PVC for central even if it's not set in the CR

202e00b

misberner and others added 8 commits November 3, 2022 12:54

Apply suggestions from code review

67d0e1d

create DB persistence even if not explicitly specified

d77bcc1

disown PVC when not in use

f0a52dd

fix

46feaf3

fix pvc reconciliation unit test (note to self: check wth it was work…

6af61ed

…ing before)

review comments

bdbbdbd

Update operator/pkg/central/extensions/reconcile_central_db_password_…

e33b999

…test.go

Fix expected error message

e3059f6

vladbologa force-pushed the mi/rox-12934-operator-reconcile-external-db branch from d806cde to e3059f6 Compare November 3, 2022 11:54

vladbologa approved these changes Nov 3, 2022

View reviewed changes

vladbologa mentioned this pull request Nov 3, 2022

ROX-12934: WIP - Fix Operator reconciliation when using secrets and external DB #3293

Closed

5 tasks

vladbologa merged commit 7f8bf59 into master Nov 3, 2022

vladbologa deleted the mi/rox-12934-operator-reconcile-external-db branch November 3, 2022 14:26

ivan-degtiarenko pushed a commit that referenced this pull request Nov 5, 2022

ROX-12934: Fix operator reconciliation when external DB is in use (#3322

b1e6974

) Co-authored-by: Connor Gorman <cgorman@redhat.com> Co-authored-by: Vlad Bologa <vbologa@redhat.com>

RTann mentioned this pull request Nov 12, 2022

fix Operator reconciliation for external Central DB #3796

Merged

1 task

	passwordSecretName := r.centralObj.Spec.Central.DB.PasswordSecret.Name
	passwordSecretName := r.centralObj.Spec.Central.DB.GetPasswordSecret().Name

		// When using this option, you must explicitly set a password secret; automatically generating a password will not
		// be supported.

Conversation

misberner commented Oct 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Testing Performed

Uh oh!

ghost commented Oct 5, 2022 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

openshift-ci bot commented Nov 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

misberner commented Oct 5, 2022 •

edited

Loading

ghost commented Oct 5, 2022 •

edited by ghost

Loading

openshift-ci bot commented Nov 3, 2022 •

edited

Loading