fix(crd): restrict database secret name to be immutable

[tthvo]

Welcome to Cryostat! 👋

Before contributing, make sure you have:

• Read the contributing guidelines
• Linked a relevant issue which this PR resolves
• Linked any other relevant issues, PR's, or documentation, if any
• Resolved all conflicts, if any
• Rebased your branch PR on top of the latest upstream main branch
• Attached at least one of the following labels to the PR: [chore, ci, docs, feat, fix, test]
• Signed all commits: git commit -S -m "YOUR_COMMIT_MESSAGE"

---

Fixes: #475

Description of the change:

Updated the reconciler logic to emit an Event to warn against mismatched secret name for database and return an approriate error.

Motivation for the change:

See #475. The secret used to database encryption must not change (i.e. databaseOptions.secretName is immutable).

How to manually test:

Create a secret:

cat <<EOF | oc apply -f -
apiVersion: v1
stringData:
  CONNECTION_KEY: connection_key
  ENCRYPTION_KEY: encryption_key
kind: Secret
metadata:
  name: db-secret
type: Opaque
EOF

Deploy the operator and create a CR:

export IMAGE_NAMESPACE=quay.io/some-user
export IMAGE_VERSION=test-version
make oci-build bundle bundle-build
podman push $IMAGE_NAMESPACE/cryostat-operator:$IMAGE_VERSION
podman push $IMAGE_NAMESPACE/cryostat-operator-bundle:$IMAGE_VERSION
make deploy_bundle

cat <<EOF | oc apply -f -
apiVersion: operator.cryostat.io/v1beta2
kind: Cryostat
metadata:
  name: cryostat-sample
spec:
  enableCertManager: true
  databaseOptions:
    secretName: db-secret
EOF

Wait for the controller finishes reconciling. Then, update the CR to remove the databaseOptions.secretName spec. The controller should show an error in reconciler loop and an event emitted. Vice versa, create an empty CR, wait for reconciling, and update the CR to specify the databaseOptions.secretName spec. The same behaviour should occur.

Controller's log:

2024-05-31T05:48:59Z	ERROR	Reconciler error	{"controller": "cryostat", "controllerGroup": "operator.cryostat.io", "controllerKind": "Cryostat", "Cryostat": {"name":"cryostat-sample","namespace":"myns"}, "namespace": "myns", "name": "cryostat-sample", "reconcileID": "f2dcb01e-d6a4-4d92-b949-11727633e32b", "error": "database secret cannot be updated, but another secret is specified"}
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).reconcileHandler
	/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:329
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).processNextWorkItem
	/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:266
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Start.func2.2
	/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:227
2024-05-31T05:48:59Z	DEBUG	events	"databaseOptions.secretName" field cannot be updated, please revert its value or re-create this Cryostat custom resource	{"type": "Warning", "object": {"kind":"Cryostat","namespace":"myns","name":"cryostat-sample","uid":"21527815-9aa8-4196-8bce-0b0625e28596","apiVersion":"operator.cryostat.io/v1beta2","resourceVersion":"165990"}, "reason": "DatabaseSecretMismatched"}

Events

[tthvo]

/build_test

[tthvo]

There might be cases where the user simply migrate the content of the custom database secret to another one. This means the keys are the same but the operator will consider it as updated.

I am not sure if it's good practice or secure to check secret contents or I am overthinking. Let me know what you think :D

[github-actions]

/build_test completed successfully ✅.
View Actions Run.

[andrewazores]

I am not sure if it's good practice or secure to check secret contents or I am overthinking. Let me know what you think :D

Seems to me like it should be OK to do in general, so long as we are careful with how we handle the secret after we have accessed it. ie don't log the secret contents, don't copy it to another file/secret/configmap/etc., avoid holding it in memory on heap and if this can't be avoided then be sure to write over the char/byte array so that the secret is unlikely to be seen within a heap dump.

[tthvo]

I am not sure if it's good practice or secure to check secret contents or I am overthinking. Let me know what you think :D

Seems to me like it should be OK to do in general, so long as we are careful with how we handle the secret after we have accessed it. ie don't log the secret contents, don't copy it to another file/secret/configmap/etc., avoid holding it in memory on heap and if this can't be avoided then be sure to write over the char/byte array so that the secret is unlikely to be seen within a heap dump.

Thanks! Make sense to me! Are we looking to implement the checks at secret field level here? If so, I make this draft and move in that direction. Or is this good?

[andrewazores]

I don't think it's necessary to do that, but if we were to implement something like that, those would be the constraints. For example,

reconcile() {
  ....
  if len(cr.Status.SecretHash) == 0 {
    databaseSecret := generatePassword()
    defer clearString(databaseSecret)
    cr.Status.SecretHash = sha256(databaseSecret)
    createSecret("database", databaseSecret)
  } else {
    currentSecret = getSecret("database")
    hash = sha256(currentSecret)
    defer clearString(currentSecret)
    if hash != cr.Status.SecretHash {
      return badSecretError
    }
  }
}

as a very rough pseudocode example. Here the idea is that the generated secret is hashed and the hash stored in the CR status, so we can use this hash for change detection rather than relying on Secret naming for change detection.

There are probably better k8s-specific ways to do this, but if you were to directly examine the Secret contents to do change detection, I think it would look something like this.

[tthvo]

Ahh how about the cases where the secret contains more than just CONNECTION_KEY and ENCRYPTION_KEY? That should make the hash different right even though the secret values are valid. We would need to save hash for each field...

I guess I am also a bit hesitant to save to secret hash in CR status. Another way is to save the hashes in another secrets but that seems not ideal.

I wonder if it is worth it to handle these unlikely cases or just make the databaseOptions.secretName immutable like now, similar to deployment selector or metadata name?

[andrewazores]

Making it immutable sounds like another good option. Basically, I think the right options are the ones that are intuitive for the user. So if it's immutable, they will understand that they need to delete and recreate things over again if they want to make any changes. If it is mutable then I think they would expect full change detection and automatic redeploy as needed - having it mutable but only checking for name changes seems like it could be confusing, since a rename with no content change should not actually do anything, whereas keeping the same name with updated content should cause a redeploy.

[tthvo]

Ahh thanks make sense! I think, IMO, the more simple and easier way with more restrictions (i.e. immutable field) would fit more since it's common in k8s too.

I can try: https://kubernetes.io/blog/2022/09/29/enforce-immutability-using-cel/ (k8s 1.25+) or with an admission webhook. Though, the PR essentially does the similar thing by erroring out from the changes and emit the Event.

We can also suggest the users to make their secret immutable: https://kubernetes.io/docs/concepts/configuration/secret/#secret-immutable (k8s 1.21+). That being said, we are still supporting k8s 1.19+?

[ebaron]

We can also suggest the users to make their secret immutable: https://kubernetes.io/docs/concepts/configuration/secret/#secret-immutable (k8s 1.21+). That being said, we are still supporting k8s 1.19+?

I'd be fine with us bumping the minimum support to 1.25.

[tthvo]

We can also suggest the users to make their secret immutable: https://kubernetes.io/docs/concepts/configuration/secret/#secret-immutable (k8s 1.21+). That being said, we are still supporting k8s 1.19+?

I'd be fine with us bumping the minimum support to 1.25.

Ah nice, considering pretty much k8s 1.25- are EOL. Another thing, would it make sense to to emit a warning if the secret provided is immutable? For our own generated one, we can add the immutable: true.

[tthvo]

A transition rule is a validation rule that references 'oldSelf'. The API server only evaluates transition rules when both an old value and new value exist.

https://kubernetes.io/blog/2022/09/23/crd-validation-rules-beta/#crd-transition-rules

Both an old and a new value exist. It remains possible to check if a value has been added or removed by placing a transition rule on the parent node. Transition rules are never applied to custom resource creation. When placed on an optional field, a transition rule will not apply to update operations that set or unset the field.

https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/#transition-rules

Unfortunately, I ran into a bit of a roadblock here. The CRD's validation/transition rules have above restrictions for optional fields (i.e. both databaseOptions and databaseOptions.secretName are optional). Indeed, the updates from unset to set databaseOptions.secretName (and vice versa) are totally ignored while changing custom secret names are invalidated correctly :(

There are possible workaround that requires k8s 1.30+ or with explicit enabling of CRD validation ratcheting feature gate. IMO, I don't think this is ideal to enforce it.

So, I kept the original implementation that would detect the change in databaseOptions.secretName and emit a warning Event. This behaves as if the field is immutable and allows reverting CR specs. I also added immutable to the generated database secret to avoid accidental mutations. What are your thoughts @ebaron @andrewazores?

[andrewazores]

I think the change itself makes sense, just the two documentation notes above.

[andrewazores]

LGTM, @ebaron?

[ebaron]

Looks good!

[ebaron]

/build_test

[github-actions]

/build_test completed successfully ✅.
View Actions Run.