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

* fix(database): emit an Event when database secret is mismatched Signed-off-by: Thuan Vo <[email protected]> * fix(secret): mark generated db secret as immutable * docs(crd): more details on immutable secrets * docs(crd): fix typo in crd markers * docs(readme): mention min k8s version in README * docs(readme): re-organize readme * fix(secret): ensure secret is immutable in createOrUpdate --------- Signed-off-by: Thuan Vo <[email protected]>
cryostatio · Jun 5, 2024 · eba4fb2 · eba4fb2
1 parent 48128d1
commit eba4fb2
Show file tree

Hide file tree

Showing 9 changed files with 62 additions and 24 deletions.
diff --git a/README.md b/README.md
@@ -26,6 +26,14 @@ the JFR datasource for Grafana.
 for the Grafana dashboard.
 
 # Using
+
+## Requirements
+
+- `kubernetes` v1.21+ with [`Operator Lifecycle Manager`](https://olm.operatorframework.io/)
+- [`cert-manager`](https://github.com/cert-manager/cert-manager) v1.11.5+ (Recommended)
+
+## Instructions
+
 Once deployed, the `cryostat` instance can be accessed via web browser
 at the URL provided by:
 ```
@@ -41,7 +49,6 @@ the need to expose a JMX port over the network.
 ## Requirements
 - `go` v1.21+
 - [`operator-sdk`](https://github.com/operator-framework/operator-sdk) v1.31.0
-- [`cert-manager`](https://github.com/cert-manager/cert-manager) v1.11.5+ (Recommended)
 - `podman` or `docker`
 - [`jq`](https://stedolan.github.io/jq/) v1.6+
 - `ginkgo` (Optional)

diff --git a/api/v1beta2/cryostat_types.go b/api/v1beta2/cryostat_types.go
@@ -603,7 +603,9 @@ type TargetDiscoveryOptions struct {
 type DatabaseOptions struct {
 	// Name of the secret containing database keys. This secret must contain a CONNECTION_KEY secret which is the
 	// database connection password, and an ENCRYPTION_KEY secret which is the key used to encrypt sensitive data
-	// stored within the database, such as the target credentials keyring.
+	// stored within the database, such as the target credentials keyring. This field cannot be updated.
+	// It is recommended that the secret should be marked as immutable to avoid accidental changes to secret's data.
+	// More details: https://kubernetes.io/docs/concepts/configuration/secret/#secret-immutable
 	// +optional
 	// +operator-sdk:csv:customresourcedefinitions:type=spec,xDescriptors={"urn:alm:descriptor:io.kubernetes:Secret"}
 	SecretName *string `json:"secretName,omitempty"`

diff --git a/bundle/manifests/cryostat-operator.clusterserviceversion.yaml b/bundle/manifests/cryostat-operator.clusterserviceversion.yaml
@@ -53,7 +53,7 @@ metadata:
     capabilities: Seamless Upgrades
     categories: Monitoring, Developer Tools
     containerImage: quay.io/cryostat/cryostat-operator:3.0.0-dev
-    createdAt: "2024-05-28T20:14:14Z"
+    createdAt: "2024-05-31T05:27:37Z"
     description: JVM monitoring and profiling tool
     operatorframework.io/initialization-resource: |-
       {
@@ -576,10 +576,13 @@ spec:
       - description: Options to configure the Cryostat application's database.
         displayName: Database Options
         path: databaseOptions
-      - description: Name of the secret containing database keys. This secret must
+      - description: 'Name of the secret containing database keys. This secret must
           contain a CONNECTION_KEY secret which is the database connection password,
           and an ENCRYPTION_KEY secret which is the key used to encrypt sensitive
           data stored within the database, such as the target credentials keyring.
+          This field cannot be updated. It is recommended that the secret should be
+          marked as immutable to avoid accidental changes to secret''s data. More
+          details: https://kubernetes.io/docs/concepts/configuration/secret/#secret-immutable'
         displayName: Secret Name
         path: databaseOptions.secretName
         x-descriptors:

diff --git a/bundle/manifests/operator.cryostat.io_cryostats.yaml b/bundle/manifests/operator.cryostat.io_cryostats.yaml
@@ -5208,11 +5208,14 @@ spec:
                 description: Options to configure the Cryostat application's database.
                 properties:
                   secretName:
-                    description: Name of the secret containing database keys. This
+                    description: 'Name of the secret containing database keys. This
                       secret must contain a CONNECTION_KEY secret which is the database
                       connection password, and an ENCRYPTION_KEY secret which is the
                       key used to encrypt sensitive data stored within the database,
-                      such as the target credentials keyring.
+                      such as the target credentials keyring. This field cannot be
+                      updated. It is recommended that the secret should be marked
+                      as immutable to avoid accidental changes to secret''s data.
+                      More details: https://kubernetes.io/docs/concepts/configuration/secret/#secret-immutable'
                     type: string
                 type: object
               enableCertManager:

diff --git a/config/crd/bases/operator.cryostat.io_cryostats.yaml b/config/crd/bases/operator.cryostat.io_cryostats.yaml
@@ -5198,11 +5198,14 @@ spec:
                 description: Options to configure the Cryostat application's database.
                 properties:
                   secretName:
-                    description: Name of the secret containing database keys. This
+                    description: 'Name of the secret containing database keys. This
                       secret must contain a CONNECTION_KEY secret which is the database
                       connection password, and an ENCRYPTION_KEY secret which is the
                       key used to encrypt sensitive data stored within the database,
-                      such as the target credentials keyring.
+                      such as the target credentials keyring. This field cannot be
+                      updated. It is recommended that the secret should be marked
+                      as immutable to avoid accidental changes to secret''s data.
+                      More details: https://kubernetes.io/docs/concepts/configuration/secret/#secret-immutable'
                     type: string
                 type: object
               enableCertManager:

diff --git a/config/manifests/bases/cryostat-operator.clusterserviceversion.yaml b/config/manifests/bases/cryostat-operator.clusterserviceversion.yaml
@@ -121,10 +121,13 @@ spec:
       - description: Options to configure the Cryostat application's database.
         displayName: Database Options
         path: databaseOptions
-      - description: Name of the secret containing database keys. This secret must
+      - description: 'Name of the secret containing database keys. This secret must
           contain a CONNECTION_KEY secret which is the database connection password,
           and an ENCRYPTION_KEY secret which is the key used to encrypt sensitive
           data stored within the database, such as the target credentials keyring.
+          This field cannot be updated. It is recommended that the secret should be
+          marked as immutable to avoid accidental changes to secret''s data. More
+          details: https://kubernetes.io/docs/concepts/configuration/secret/#secret-immutable'
         displayName: Secret Name
         path: databaseOptions.secretName
         x-descriptors:

diff --git a/internal/controllers/reconciler_test.go b/internal/controllers/reconciler_test.go
@@ -2352,6 +2352,7 @@ func (t *cryostatTestInput) expectDatabaseSecret() {
 	expectedSecret := t.NewDatabaseSecret()
 	t.checkMetadata(secret, expectedSecret)
 	Expect(secret.StringData).To(Equal(expectedSecret.StringData))
+	Expect(secret.Immutable).To(Equal(expectedSecret.Immutable))
 }
 
 func (t *cryostatTestInput) expectStorageSecret() {

diff --git a/internal/controllers/secrets.go b/internal/controllers/secrets.go
@@ -16,12 +16,13 @@ package controllers
 
 import (
 	"context"
+	"errors"
 	"fmt"
 
 	"github.com/cryostatio/cryostat-operator/internal/controllers/constants"
 	"github.com/cryostatio/cryostat-operator/internal/controllers/model"
 	corev1 "k8s.io/api/core/v1"
-	"k8s.io/apimachinery/pkg/api/errors"
+	kerrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 )
@@ -57,23 +58,35 @@ func (r *Reconciler) reconcileAuthProxyCookieSecret(ctx context.Context, cr *mod
 	})
 }
 
-// databaseSecretNameSuffix is the suffix to be appended to the name of a
-// Cryostat CR to name its database secret
-const databaseSecretNameSuffix = "-db"
+const (
+	// The suffix to be appended to the name of a Cryostat CR to name its database secret
+	databaseSecretNameSuffix          = "-db"
+	eventDatabaseSecretMismatchedType = "DatabaseSecretMismatched"
+	eventDatabaseMismatchMsg          = "\"databaseOptions.secretName\" field cannot be updated, please revert its value or re-create this Cryostat custom resource"
+)
+
+var errDatabaseSecretUpdated = errors.New("database secret cannot be updated, but another secret is specified")
 
 func (r *Reconciler) reconcileDatabaseConnectionSecret(ctx context.Context, cr *model.CryostatInstance) error {
-	var secretName string
+	secret := &corev1.Secret{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      cr.Name + databaseSecretNameSuffix,
+			Namespace: cr.InstallNamespace,
+		},
+	}
+	secretName := secret.Name
 	secretProvided := cr.Spec.DatabaseOptions != nil && cr.Spec.DatabaseOptions.SecretName != nil
 	if secretProvided {
-		// Do not delete default secret to allow reverting to use default if needed
 		secretName = *cr.Spec.DatabaseOptions.SecretName
-	} else {
-		secret := &corev1.Secret{
-			ObjectMeta: metav1.ObjectMeta{
-				Name:      cr.Name + databaseSecretNameSuffix,
-				Namespace: cr.InstallNamespace,
-			},
-		}
+	}
+
+	// If the CR status contains the secret name, emit an Event in case the configured secret's name does not match.
+	if len(cr.Status.DatabaseSecret) > 0 && cr.Status.DatabaseSecret != secretName {
+		r.EventRecorder.Event(cr.Object, corev1.EventTypeWarning, eventDatabaseSecretMismatchedType, eventDatabaseMismatchMsg)
+		return errDatabaseSecretUpdated
+	}
+
+	if !secretProvided {
 		err := r.createOrUpdateSecret(ctx, secret, cr.Object, func() error {
 			if secret.StringData == nil {
 				secret.StringData = map[string]string{}
@@ -84,14 +97,16 @@ func (r *Reconciler) reconcileDatabaseConnectionSecret(ctx context.Context, cr *
 				secret.StringData[constants.DatabaseSecretConnectionKey] = r.GenPasswd(32)
 				secret.StringData[constants.DatabaseSecretEncryptionKey] = r.GenPasswd(32)
 			}
+
+			secret.Immutable = &[]bool{true}[0]
 			return nil
 		})
 
 		if err != nil {
 			return err
 		}
-		secretName = secret.Name
 	}
+
 	cr.Status.DatabaseSecret = secretName
 	return r.Client.Status().Update(ctx, cr.Object)
 }
@@ -150,7 +165,7 @@ func (r *Reconciler) createOrUpdateSecret(ctx context.Context, secret *corev1.Se
 
 func (r *Reconciler) deleteSecret(ctx context.Context, secret *corev1.Secret) error {
 	err := r.Client.Delete(ctx, secret)
-	if err != nil && !errors.IsNotFound(err) {
+	if err != nil && !kerrors.IsNotFound(err) {
 		r.Log.Error(err, "Could not delete secret", "name", secret.Name, "namespace", secret.Namespace)
 		return err
 	}

diff --git a/internal/test/resources.go b/internal/test/resources.go
@@ -852,6 +852,7 @@ func (r *TestResources) NewDatabaseSecret() *corev1.Secret {
 			"CONNECTION_KEY": "connection_key",
 			"ENCRYPTION_KEY": "encryption_key",
 		},
+		Immutable: &[]bool{true}[0],
 	}
 }
-Original file line number
+Diff line change
@@ Expand Up @@
     			"CONNECTION_KEY": "connection_key",
     			"ENCRYPTION_KEY": "encryption_key",
     		},
+    		Immutable: &[]bool{true}[0],
     	}
     }
@@ Expand Down @@