CBC Reconcile refactor. Added code to avoid overwriting secrets with …

…the same name. Rename some API fields. Use pointers for booleans so false values still show up. Other minor changes.
CrunchyData · Feb 27, 2024 · f91d8a9 · f91d8a9
1 parent 2267ade
commit f91d8a9
Show file tree

Hide file tree

Showing 10 changed files with 212 additions and 116 deletions.
diff --git a/build/crd/crunchybridgeclusters/immutable.yaml b/build/crd/crunchybridgeclusters/immutable.yaml
@@ -3,9 +3,9 @@
   value: [{ message: 'immutable', rule: 'self == oldSelf'}]
 - op: copy
   from: /work
-  path: /spec/versions/0/schema/openAPIV3Schema/properties/spec/properties/providerId/x-kubernetes-validations
+  path: /spec/versions/0/schema/openAPIV3Schema/properties/spec/properties/provider/x-kubernetes-validations
 - op: copy
   from: /work
-  path: /spec/versions/0/schema/openAPIV3Schema/properties/spec/properties/regionId/x-kubernetes-validations
+  path: /spec/versions/0/schema/openAPIV3Schema/properties/spec/properties/region/x-kubernetes-validations
 - op: remove
   path: /work
diff --git a/config/crd/bases/postgres-operator.crunchydata.com_crunchybridgeclusters.yaml b/config/crd/bases/postgres-operator.crunchydata.com_crunchybridgeclusters.yaml
@@ -73,11 +73,11 @@ spec:
                       type: string
                     type: object
                 type: object
-              planId:
+              plan:
                 description: The ID of the cluster's plan. Determines instance, CPU,
                   and memory.
                 type: string
-              providerId:
+              provider:
                 description: The cloud provider where the cluster is located. Currently
                   Bridge offers aws, azure, and gcp only
                 enum:
@@ -88,7 +88,7 @@ spec:
                 x-kubernetes-validations:
                 - message: immutable
                   rule: self == oldSelf
-              regionId:
+              region:
                 description: The provider region where the cluster is located.
                 type: string
                 x-kubernetes-validations:
@@ -109,6 +109,7 @@ spec:
                     secretName:
                       description: The name of the Secret that will hold the role
                         credentials.
+                      maxLength: 253
                       pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$
                       type: string
                   required:
@@ -138,9 +139,9 @@ spec:
             - clusterName
             - isHa
             - majorVersion
-            - planId
-            - providerId
-            - regionId
+            - plan
+            - provider
+            - region
             - storage
             type: object
           status:
@@ -265,7 +266,7 @@ spec:
                   - state
                   type: object
                 type: array
-              planId:
+              plan:
                 description: The ID of the cluster's plan. Determines instance, CPU,
                   and memory.
                 type: string

diff --git a/examples/crunchybridgecluster/crunchybridgecluster.yaml b/examples/crunchybridgecluster/crunchybridgecluster.yaml
@@ -5,9 +5,9 @@ metadata:
 spec:
   isHa: false
   clusterName: sigil
-  planId: standard-8
+  plan: standard-8
   majorVersion: 15
-  providerId: aws
-  regionId: us-east-2
+  provider: aws
+  region: us-east-2
   secret: crunchy-bridge-api-key
   storage: 10G
diff --git a/internal/bridge/client.go b/internal/bridge/client.go
@@ -58,9 +58,9 @@ type ClusterApiResource struct {
 	DiskUsage              *ClusterDiskUsageApiResource `json:"disk_usage,omitempty"`
 	Environment            string                       `json:"environment,omitempty"`
 	Host                   string                       `json:"host,omitempty"`
-	IsHA                   bool                         `json:"is_ha,omitempty"`
-	IsProtected            bool                         `json:"is_protected,omitempty"`
-	IsSuspended            bool                         `json:"is_suspended,omitempty"`
+	IsHA                   *bool                        `json:"is_ha,omitempty"`
+	IsProtected            *bool                        `json:"is_protected,omitempty"`
+	IsSuspended            *bool                        `json:"is_suspended,omitempty"`
 	Keychain               string                       `json:"keychain_id,omitempty"`
 	MaintenanceWindowStart int64                        `json:"maintenance_window_start,omitempty"`
 	MajorVersion           int                          `json:"major_version,omitempty"`
@@ -74,7 +74,7 @@ type ClusterApiResource struct {
 	Region                 string                       `json:"region_id,omitempty"`
 	Replicas               []*ClusterApiResource        `json:"replicas,omitempty"`
 	Storage                int64                        `json:"storage,omitempty"`
-	Tailscale              bool                         `json:"tailscale_active,omitempty"`
+	Tailscale              *bool                        `json:"tailscale_active,omitempty"`
 	Team                   string                       `json:"team_id,omitempty"`
 	LastUpdate             string                       `json:"updated_at,omitempty"`
 	ResponsePayload        v1beta1.SchemalessObject     `json:""`
@@ -185,12 +185,13 @@ type PostClustersUpgradeRequestPayload struct {
 }
 
 // PutClustersUpgradeRequestPayload is used for updating an ongoing or scheduled upgrade.
+// TODO: Implement the ability to update an upgrade (this isn't currently being used)
 type PutClustersUpgradeRequestPayload struct {
 	Plan                 string             `json:"plan_id,omitempty"`
 	PostgresVersion      intstr.IntOrString `json:"postgres_version_id,omitempty"`
 	UpgradeStartTime     string             `json:"starting_from,omitempty"`
 	Storage              int64              `json:"storage,omitempty"`
-	UseMaintenanceWindow bool               `json:"use_cluster_maintenance_window,omitempty"`
+	UseMaintenanceWindow *bool              `json:"use_cluster_maintenance_window,omitempty"`
 }
 
 // ClusterRoleApiResource is used for retrieving details on ClusterRole from the Bridge API

diff --git a/internal/bridge/crunchybridgecluster/crunchybridgecluster_controller.go b/internal/bridge/crunchybridgecluster/crunchybridgecluster_controller.go
@@ -43,8 +43,6 @@ import (
 	"github.com/crunchydata/postgres-operator/pkg/apis/postgres-operator.crunchydata.com/v1beta1"
 )
 
-const finalizer = "crunchybridgecluster.postgres-operator.crunchydata.com/finalizer"
-
 // CrunchyBridgeClusterReconciler reconciles a CrunchyBridgeCluster object
 type CrunchyBridgeClusterReconciler struct {
 	client.Client
@@ -91,7 +89,7 @@ func (r *CrunchyBridgeClusterReconciler) SetupWithManager(
 // creator of such a reference have either "delete" permission on the owner or
 // "update" permission on the owner's "finalizers" subresource.
 // - https://docs.k8s.io/reference/access-authn-authz/admission-controllers/
-// +kubebuilder:rbac:groups="postgres-operator.crunchydata.com",resources="pgupgrades/finalizers",verbs={update}
+// +kubebuilder:rbac:groups="postgres-operator.crunchydata.com",resources="crunchybridgeclusters/finalizers",verbs={update}
 
 // setControllerReference sets owner as a Controller OwnerReference on controlled.
 // Only one OwnerReference can be a controller, so it returns an error if another
@@ -226,83 +224,40 @@ func (r *CrunchyBridgeClusterReconciler) Reconcile(ctx context.Context, req ctrl
 		return ctrl.Result{}, client.IgnoreNotFound(err)
 	}
 
-	// START SECRET HANDLING -- SPIN OFF INTO ITS OWN FUNC?
-
-	// Get and validate secret for req
-	key, team, err := r.GetSecretKeys(ctx, crunchybridgecluster)
+	// Get and validate connection secret for requests
+	key, team, err := r.reconcileBridgeConnectionSecret(ctx, crunchybridgecluster)
 	if err != nil {
-		log.Error(err, "whoops, secret issue")
-
-		meta.SetStatusCondition(&crunchybridgecluster.Status.Conditions, metav1.Condition{
-			ObservedGeneration: crunchybridgecluster.GetGeneration(),
-			Type:               v1beta1.ConditionCreating,
-			Status:             metav1.ConditionFalse,
-			Reason:             "SecretInvalid",
-			Message: fmt.Sprintf(
-				"Cannot create with bad secret: %v", "TODO(crunchybridgecluster)"),
-		})
+		log.Error(err, "issue reconciling bridge connection secret")
 
-		// Don't automatically requeue Secret issues
-		// We are watching for related secrets,
-		// so will requeue when a related secret is touched
+		// Don't automatically requeue Secret issues. We are watching for
+		// related secrets, so will requeue when a related secret is touched.
 		// lint:ignore nilerr Return err as status, no requeue needed
 		return ctrl.Result{}, nil
 	}
 
-	// Remove SecretInvalid condition if found
-	invalid := meta.FindStatusCondition(crunchybridgecluster.Status.Conditions,
-		v1beta1.ConditionCreating)
-	if invalid != nil && invalid.Status == metav1.ConditionFalse && invalid.Reason == "SecretInvalid" {
-		meta.RemoveStatusCondition(&crunchybridgecluster.Status.Conditions,
-			v1beta1.ConditionCreating)
-	}
-
-	// END SECRET HANDLING
-
-	// If the CrunchyBridgeCluster isn't being deleted, add the finalizer
-	if crunchybridgecluster.ObjectMeta.DeletionTimestamp.IsZero() {
-		if !controllerutil.ContainsFinalizer(crunchybridgecluster, finalizer) {
-			controllerutil.AddFinalizer(crunchybridgecluster, finalizer)
-			if err := r.Update(ctx, crunchybridgecluster); err != nil {
-				return ctrl.Result{}, err
-			}
-		}
-		// If the CrunchyBridgeCluster is being deleted,
-		// handle the deletion, and remove the finalizer
-	} else {
-		if controllerutil.ContainsFinalizer(crunchybridgecluster, finalizer) {
-			log.Info("deleting cluster", "clusterName", crunchybridgecluster.Spec.ClusterName)
-
-			// TODO(crunchybridgecluster): If is_protected is true, maybe skip this call, but allow the deletion of the K8s object?
-			_, deletedAlready, err := r.NewClient().DeleteCluster(ctx, key, crunchybridgecluster.Status.ID)
-			// Requeue if error
-			if err != nil {
-				return ctrl.Result{}, err
-			}
-
-			if !deletedAlready {
-				return ctrl.Result{RequeueAfter: 1 * time.Second}, err
-			}
-
-			// Remove finalizer if deleted already
-			if deletedAlready {
-				log.Info("cluster deleted", "clusterName", crunchybridgecluster.Spec.ClusterName)
-
-				controllerutil.RemoveFinalizer(crunchybridgecluster, finalizer)
-				if err := r.Update(ctx, crunchybridgecluster); err != nil {
-					return ctrl.Result{}, err
-				}
+	// Check for and handle deletion of cluster. Return early if it is being
+	// deleted or there was an error. Make sure finalizer is added if cluster
+	// is not being deleted.
+	if result, err := r.handleDelete(ctx, crunchybridgecluster, key); err != nil {
+		log.Error(err, "deleting")
+		return ctrl.Result{}, err
+	} else if result != nil {
+		if log := log.V(1); log.Enabled() {
+			if result.RequeueAfter > 0 {
+				// RequeueAfter implies Requeue, but set both to make the next
+				// log message more clear.
+				result.Requeue = true
 			}
+			log.Info("deleting", "result", fmt.Sprintf("%+v", *result))
 		}
-		// Stop reconciliation as the item is being deleted
-		return ctrl.Result{}, nil
+		return *result, err
 	}
 
 	// Wonder if there's a better way to handle adding/checking/removing statuses
 	// We did something in the upgrade controller
 	// Exit early if we can't create from this K8s object
 	// unless this K8s object has been changed (compare ObservedGeneration)
-	invalid = meta.FindStatusCondition(crunchybridgecluster.Status.Conditions,
+	invalid := meta.FindStatusCondition(crunchybridgecluster.Status.Conditions,
 		v1beta1.ConditionCreating)
 	if invalid != nil &&
 		invalid.Status == metav1.ConditionFalse &&
@@ -321,7 +276,7 @@ func (r *CrunchyBridgeClusterReconciler) Reconcile(ctx context.Context, req ctrl
 
 	storageVal, err := handleStorage(crunchybridgecluster.Spec.Storage)
 	if err != nil {
-		log.Error(err, "whoops, storage issue")
+		log.Error(err, "issue handling storage value")
 		// TODO(crunchybridgecluster)
 		// lint:ignore nilerr no requeue needed
 		return ctrl.Result{}, nil
@@ -330,18 +285,10 @@ func (r *CrunchyBridgeClusterReconciler) Reconcile(ctx context.Context, req ctrl
 	// We should only be missing the ID if no create has been issued
 	// or the create was interrupted and we haven't received the ID.
 	if crunchybridgecluster.Status.ID == "" {
-		// START FIND
-
-		// TODO(crunchybridgecluster) If the CreateCluster response was interrupted, we won't have the ID
-		// so we can get by name
-		// BUT if we do that, there's a chance for the K8s object to grab a preexisting Bridge cluster
-		// which means there's a chance to delete a Bridge cluster through K8s actions
-		// even though that cluster didn't originate from K8s.
-
 		// Check if the cluster exists
 		clusters, err := r.NewClient().ListClusters(ctx, key, team)
 		if err != nil {
-			log.Error(err, "whoops, cluster listing issue")
+			log.Error(err, "issue listing existing clusters in Bridge")
 			return ctrl.Result{}, err
 		}
 
@@ -381,12 +328,7 @@ func (r *CrunchyBridgeClusterReconciler) Reconcile(ctx context.Context, req ctrl
 			}
 		}
 
-		// END FIND
-
 		// if we've gotten here then no cluster exists with that name and we're missing the ID, ergo, create cluster
-
-		// TODO(crunchybridgecluster) Can almost just use the crunchybridgecluster.Spec... except for the team,
-		// which we don't want users to set on the spec. Do we?
 		createClusterRequestPayload := &bridge.PostClustersRequestPayload{
 			IsHA:            crunchybridgecluster.Spec.IsHA,
 			Name:            crunchybridgecluster.Spec.ClusterName,
@@ -399,7 +341,7 @@ func (r *CrunchyBridgeClusterReconciler) Reconcile(ctx context.Context, req ctrl
 		}
 		cluster, err := r.NewClient().CreateCluster(ctx, key, createClusterRequestPayload)
 		if err != nil {
-			log.Error(err, "whoops, cluster creating issue")
+			log.Error(err, "issue creating cluster in Bridge")
 			// TODO(crunchybridgecluster): probably shouldn't set this condition unless response from Bridge
 			// indicates the payload is wrong
 			// Otherwise want a different condition
@@ -427,29 +369,35 @@ func (r *CrunchyBridgeClusterReconciler) Reconcile(ctx context.Context, req ctrl
 	// Get Cluster
 	clusterDetails, err := r.NewClient().GetCluster(ctx, key, crunchybridgecluster.Status.ID)
 	if err != nil {
-		log.Error(err, "whoops, issue getting cluster")
+		log.Error(err, "issue getting cluster information from Bridge")
 		return ctrl.Result{}, err
 	}
 	clusterDetails.AddDataToClusterStatus(crunchybridgecluster)
 
 	// Get Cluster Status
 	clusterStatus, err := r.NewClient().GetClusterStatus(ctx, key, crunchybridgecluster.Status.ID)
 	if err != nil {
-		log.Error(err, "whoops, issue getting cluster status")
+		log.Error(err, "issue getting cluster status from Bridge")
 		return ctrl.Result{}, err
 	}
 	clusterStatus.AddDataToClusterStatus(crunchybridgecluster)
+	// TODO: Update the ConditionReady status here
 
 	// Get Cluster Upgrade
 	clusterUpgradeDetails, err := r.NewClient().GetClusterUpgrade(ctx, key, crunchybridgecluster.Status.ID)
 	if err != nil {
-		log.Error(err, "whoops, issue getting cluster upgrade")
+		log.Error(err, "issue getting cluster upgrade from Bridge")
 		return ctrl.Result{}, err
 	}
 	clusterUpgradeDetails.AddDataToClusterStatus(crunchybridgecluster)
+	// TODO: Update the ConditionUpdating status here
 
 	// Reconcile roles and their secrets
 	err = r.reconcilePostgresRoles(ctx, key, crunchybridgecluster)
+	if err != nil {
+		log.Error(err, "issue reconciling postgres user roles/secrets")
+		return ctrl.Result{}, err
+	}
 
 	// For now, we skip updating until the upgrade status is cleared.
 	// For the future, we may want to update in-progress upgrades,
@@ -476,7 +424,7 @@ func (r *CrunchyBridgeClusterReconciler) Reconcile(ctx context.Context, req ctrl
 	// Are there diffs between the cluster response from the Bridge API and the spec?
 	// HA diffs are sent to /clusters/{cluster_id}/actions/[enable|disable]-ha
 	// so have to know (a) to send and (b) which to send to
-	if crunchybridgecluster.Spec.IsHA != crunchybridgecluster.Status.IsHA {
+	if crunchybridgecluster.Spec.IsHA != *crunchybridgecluster.Status.IsHA {
 		return r.handleUpgradeHA(ctx, key, crunchybridgecluster)
 	}
 
@@ -491,6 +439,34 @@ func (r *CrunchyBridgeClusterReconciler) Reconcile(ctx context.Context, req ctrl
 	return ctrl.Result{RequeueAfter: 3 * time.Minute}, nil
 }
 
+func (r *CrunchyBridgeClusterReconciler) reconcileBridgeConnectionSecret(
+	ctx context.Context, crunchybridgecluster *v1beta1.CrunchyBridgeCluster,
+) (string, string, error) {
+	key, team, err := r.GetSecretKeys(ctx, crunchybridgecluster)
+	if err != nil {
+		meta.SetStatusCondition(&crunchybridgecluster.Status.Conditions, metav1.Condition{
+			ObservedGeneration: crunchybridgecluster.GetGeneration(),
+			Type:               v1beta1.ConditionCreating,
+			Status:             metav1.ConditionFalse,
+			Reason:             "SecretInvalid",
+			Message: fmt.Sprintf(
+				"Cannot create with bad secret: %v", "TODO(crunchybridgecluster)"),
+		})
+
+		return "", "", err
+	}
+
+	// Remove SecretInvalid condition if found
+	invalid := meta.FindStatusCondition(crunchybridgecluster.Status.Conditions,
+		v1beta1.ConditionCreating)
+	if invalid != nil && invalid.Status == metav1.ConditionFalse && invalid.Reason == "SecretInvalid" {
+		meta.RemoveStatusCondition(&crunchybridgecluster.Status.Conditions,
+			v1beta1.ConditionCreating)
+	}
+
+	return key, team, err
+}
+
 // handleStorage returns a usable int in G (rounded up if the original storage was in Gi).
 // Returns an error if the int is outside the range for Bridge min (10) or max (65535).
 func handleStorage(storageSpec resource.Quantity) (int64, error) {