Better subsets/consumers names (#16)

* Better subset/consumer status names
Riskified · Jan 2, 2024 · b543940 · b543940
1 parent 9ca8b08
commit b543940
Show file tree

Hide file tree

Showing 25 changed files with 70 additions and 58 deletions.
diff --git a/controllers/dynamicenv_controller.go b/controllers/dynamicenv_controller.go
@@ -145,7 +145,7 @@ func (r *DynamicEnvReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 
 	subsetsAndConsumers := mergeSubsetsAndConsumers(dynamicEnv.Spec.Subsets, dynamicEnv.Spec.Consumers)
 
-	if err := r.cleanupRemovedSubsetsOrConsumers(ctx, subsetsAndConsumers, uniqueVersion, dynamicEnv); err != nil {
+	if err := r.cleanupRemovedSubsetsOrConsumers(ctx, subsetsAndConsumers, dynamicEnv); err != nil {
 		rls.cleanupError = err
 	}
 
@@ -157,21 +157,23 @@ func (r *DynamicEnvReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 			defaultVersionForSubset = s.DefaultVersion
 		}
 
+		subsetName := mkSubsetName(s)
 		baseDeployment := &appsv1.Deployment{}
 		err := r.Get(ctx, types.NamespacedName{Name: s.Name, Namespace: s.Namespace}, baseDeployment)
 		if err != nil {
 			log.Error(err, "couldn't find the deployment we need to override", "deployment-name", s.Name, "namespace", s.Namespace)
 			msg := fmt.Sprintf("couldn't find the deployment we need to override (name: %s, ns: %s)", s.Name, s.Namespace)
 			rls.returnError = fmt.Errorf("%s: %w", msg, err)
-			rls.addDeploymentMessage(uniqueName, st.Type, "%s, %v", msg, err)
-			rls.nonReadyCS[uniqueName] = true
+			rls.addDeploymentMessage(subsetName, st.Type, "%s, %v", msg, err)
+			rls.nonReadyCS[subsetName] = true
 			break
 		}
 
 		deploymentHandler := handlers.DeploymentHandler{
 			Client:         r.Client,
 			UniqueName:     uniqueName,
 			UniqueVersion:  uniqueVersion,
+			SubsetName:     subsetName,
 			Owner:          owner,
 			BaseDeployment: baseDeployment,
 			DeploymentType: st.Type,
@@ -186,7 +188,7 @@ func (r *DynamicEnvReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 		deploymentHandlers = append(deploymentHandlers, &deploymentHandler)
 		if err := deploymentHandler.Handle(); err != nil {
 			rls.returnError = err
-			rls.addDeploymentMessage(uniqueName, st.Type, err.Error())
+			rls.addDeploymentMessage(subsetName, st.Type, err.Error())
 			break
 		}
 
@@ -195,7 +197,7 @@ func (r *DynamicEnvReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 			if err != nil {
 				msg := fmt.Sprintf("locating service hostname for deployment '%s'", baseDeployment.Name)
 				rls.returnError = fmt.Errorf("%s: %w", msg, err)
-				rls.subsetMessages[uniqueName] = rls.subsetMessages[uniqueName].AppendGlobalMsg("%s: %v", msg, err)
+				rls.subsetMessages[subsetName] = rls.subsetMessages[subsetName].AppendGlobalMsg("%s: %v", msg, err)
 				break
 			}
 
@@ -204,6 +206,7 @@ func (r *DynamicEnvReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 				UniqueName:     uniqueName,
 				UniqueVersion:  uniqueVersion,
 				Namespace:      s.Namespace,
+				SubsetName:     subsetName,
 				VersionLabel:   r.VersionLabel,
 				DefaultVersion: defaultVersionForSubset,
 				StatusHandler:  &statusHandler,
@@ -215,7 +218,7 @@ func (r *DynamicEnvReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 			mrHandlers = append(mrHandlers, &destinationRuleHandler)
 			if err := destinationRuleHandler.Handle(); err != nil {
 				rls.returnError = err
-				rls.subsetMessages[uniqueName] = rls.subsetMessages[uniqueName].AppendDestinationRuleMsg(err.Error())
+				rls.subsetMessages[subsetName] = rls.subsetMessages[subsetName].AppendDestinationRuleMsg(err.Error())
 				break
 			}
 
@@ -225,6 +228,7 @@ func (r *DynamicEnvReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 				UniqueVersion:  uniqueVersion,
 				RoutePrefix:    helpers.CalculateVirtualServicePrefix(uniqueVersion, s.Name),
 				Namespace:      s.Namespace,
+				SubsetName:     subsetName,
 				ServiceHosts:   serviceHosts,
 				DefaultVersion: defaultVersionForSubset,
 				DynamicEnv:     dynamicEnv,
@@ -241,15 +245,15 @@ func (r *DynamicEnvReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 					log.Error(err, "error updating virtual service for subset", "subset", s.Name)
 					msg := fmt.Sprintf("error updating virtual service for subset (%s)", uniqueName)
 					rls.returnError = fmt.Errorf("%s: %w", msg, err)
-					rls.subsetMessages[uniqueName] = rls.subsetMessages[uniqueName].AppendVirtualServiceMsg("%s: %s", msg, err)
+					rls.subsetMessages[subsetName] = rls.subsetMessages[subsetName].AppendVirtualServiceMsg("%s: %s", msg, err)
 				}
 			}
 
 			commonHostExists := helpers.CommonValueExists(destinationRuleHandler.GetHosts(), virtualServiceHandler.GetHosts())
 			if !commonHostExists {
 				degradedExists = true
-				rls.nonReadyCS[uniqueName] = true
-				rls.subsetMessages[uniqueName] = rls.subsetMessages[uniqueName].AppendGlobalMsg("Couldn't find common active service hostname across DestinationRules and VirtualServices")
+				rls.nonReadyCS[subsetName] = true
+				rls.subsetMessages[subsetName] = rls.subsetMessages[subsetName].AppendGlobalMsg("Couldn't find common active service hostname across DestinationRules and VirtualServices")
 			}
 		}
 	}
@@ -522,11 +526,11 @@ func (r *DynamicEnvReconciler) SetupWithManager(mgr ctrl.Manager) error {
 }
 
 // Cleanup subsets and consumers that are removed from the dynamic environment CRD.
-func (r *DynamicEnvReconciler) cleanupRemovedSubsetsOrConsumers(ctx context.Context, subsetsAndConsumers []SubsetType, version string, de *riskifiedv1alpha1.DynamicEnv) error {
+func (r *DynamicEnvReconciler) cleanupRemovedSubsetsOrConsumers(ctx context.Context, subsetsAndConsumers []SubsetType, de *riskifiedv1alpha1.DynamicEnv) error {
 	var allSC = make(map[string]riskifiedv1alpha1.SubsetOrConsumer)
 	for _, st := range subsetsAndConsumers {
-		uniqueName := mkSubsetUniqueName(st.Subset.Name, version)
-		allSC[uniqueName] = st.Type
+		subsetName := mkSubsetName(st.Subset)
+		allSC[subsetName] = st.Type
 	}
 	deletedSC := findDeletedSC(de, allSC)
 	for name, typ := range deletedSC {
@@ -703,3 +707,7 @@ func markedForDeletion(de *riskifiedv1alpha1.DynamicEnv) bool {
 func mkSubsetUniqueName(name, version string) string {
 	return name + "-" + version
 }
+
+func mkSubsetName(subset riskifiedv1alpha1.Subset) string {
+	return fmt.Sprintf("%s/%s", subset.Namespace, subset.Name)
+}
diff --git a/e2e-testing/kuttl/add-remove-subsets/00-assert.yaml b/e2e-testing/kuttl/add-remove-subsets/00-assert.yaml
@@ -7,7 +7,7 @@ metadata:
 status:
   state: ready
   subsetsStatus:
-    details-default-dynamicenv-add-remove-subsets:
+    add-remove-subsets/details:
       deployment:
         name: details-default-dynamicenv-add-remove-subsets
         namespace: add-remove-subsets

diff --git a/e2e-testing/kuttl/add-remove-subsets/01-assert.yaml b/e2e-testing/kuttl/add-remove-subsets/01-assert.yaml
@@ -6,13 +6,13 @@ metadata:
   namespace: default
 status:
   consumersStatus:
-    details-worker-default-dynamicenv-add-remove-subsets:
+    add-remove-subsets/details-worker:
       name: details-worker-default-dynamicenv-add-remove-subsets
       namespace: add-remove-subsets
       status: running
   state: ready
   subsetsStatus:
-    details-default-dynamicenv-add-remove-subsets:
+    add-remove-subsets/details:
       deployment:
         name: details-default-dynamicenv-add-remove-subsets
         namespace: add-remove-subsets
@@ -25,7 +25,7 @@ status:
         - name: details
           namespace: add-remove-subsets
           status: running
-    reviews-default-dynamicenv-add-remove-subsets:
+    add-remove-subsets/reviews:
       deployment:
         name: reviews-default-dynamicenv-add-remove-subsets
         namespace: add-remove-subsets

diff --git a/e2e-testing/kuttl/add-remove-subsets/02-assert.yaml b/e2e-testing/kuttl/add-remove-subsets/02-assert.yaml
@@ -7,7 +7,7 @@ metadata:
 status:
   state: ready
   subsetsStatus:
-    reviews-default-dynamicenv-add-remove-subsets:
+    add-remove-subsets/reviews:
       deployment:
         name: reviews-default-dynamicenv-add-remove-subsets
         namespace: add-remove-subsets

diff --git a/e2e-testing/kuttl/consumers-with-and-without-errors/00-assert.yaml b/e2e-testing/kuttl/consumers-with-and-without-errors/00-assert.yaml
@@ -8,12 +8,12 @@ status:
   totalCount: 3
   totalReady: 2
   consumersStatus:
-    details-worker-default-dynamicenv-consumers-with-and-without-er:
+    consumers-with-and-without-errors/details-worker:
       name: details-worker-default-dynamicenv-consumers-with-and-without-er
       namespace: consumers-with-and-without-errors
       status: running
       hash: -7893432429265961114
-    details-worker-invalid-default-dynamicenv-consumers-with-and-without-er:
+    consumers-with-and-without-errors/details-worker-invalid:
       errors:
         - error: 'couldn''t find the deployment we need to override (name: details-worker-invalid,
           ns: consumers-with-and-without-errors), Deployment.apps "details-worker-invalid"

diff --git a/e2e-testing/kuttl/delegate-virtual-service/00-assert.yaml b/e2e-testing/kuttl/delegate-virtual-service/00-assert.yaml
@@ -124,7 +124,7 @@ metadata:
   namespace: default
 status:
   subsetsStatus:
-    test-app-default-dynamicenv-delegate-virtual-service:
+    delegate-virtual-service/test-app:
       subsetErrors:
         virtualServices:
           - error: 'Wierd, Couldn''t find a service with name: does-not-exist-delegate,

diff --git a/e2e-testing/kuttl/global-virtual-service-errors/00-assert.yaml b/e2e-testing/kuttl/global-virtual-service-errors/00-assert.yaml
@@ -9,9 +9,9 @@ status:
   totalCount: 1
   totalReady: 0
   subsetsStatus:
-    details-default-dynamicenv-global-virtual-service-errors:
+    global-virtual-service-errors/details:
       subsetErrors:
         virtualServices:
           - error: 'error updating virtual service for subset (details-default-dynamicenv-global-virtual-service-errors):
-                     could not find even one virtual service that handles subset "details-default-dynamicenv-global-virtual-service-errors"'
+                     could not find even one virtual service that handles subset "global-virtual-service-errors/details"'
 ...
diff --git a/e2e-testing/kuttl/multiple-services-scenarios-no-vs/00-assert.yaml b/e2e-testing/kuttl/multiple-services-scenarios-no-vs/00-assert.yaml
@@ -7,7 +7,7 @@ metadata:
 status:
   state: degraded
   subsetsStatus:
-    details-default-dynamicenv-multiple-services-scenarios-n:
+    multiple-services-scenarios-no-vs/details:
       deployment:
         name: details-default-dynamicenv-multiple-services-scenarios-n
         namespace: multiple-services-scenarios-no-vs
@@ -22,7 +22,7 @@ status:
       subsetErrors:
         virtualServices:
         - error: 'error updating virtual service for subset (details-default-dynamicenv-multiple-services-scenarios-n):
-            could not find even one virtual service that handles subset "details-default-dynamicenv-multiple-services-scenarios-n"'
+            could not find even one virtual service that handles subset "multiple-services-scenarios-no-vs/details"'
   totalCount: 1
   totalReady: 0
 ...
diff --git a/e2e-testing/kuttl/multiple-services-scenarios-no-working-single-host/00-assert.yaml b/e2e-testing/kuttl/multiple-services-scenarios-no-working-single-host/00-assert.yaml
@@ -21,7 +21,7 @@ spec:
 status:
   state: degraded
   subsetsStatus:
-    details-default-dynamicenv-multiple-services-scenarios-n:
+    multiple-services-scenarios-no-working-single-host/details:
       deployment:
         name: details-default-dynamicenv-multiple-services-scenarios-n
         namespace: multiple-services-scenarios-no-working-single-host

diff --git a/e2e-testing/kuttl/multiple-services-scenarios/00-assert.yaml b/e2e-testing/kuttl/multiple-services-scenarios/00-assert.yaml
@@ -7,7 +7,7 @@ metadata:
 status:
   state: ready
   subsetsStatus:
-    details-default-dynamicenv-multiple-services-per-deploym:
+    multiple-services-per-deployment/details:
       deployment:
         name: details-default-dynamicenv-multiple-services-per-deploym
         namespace: multiple-services-per-deployment

diff --git a/e2e-testing/kuttl/multiple-services-scenarios/02-assert.yaml b/e2e-testing/kuttl/multiple-services-scenarios/02-assert.yaml
@@ -7,7 +7,7 @@ metadata:
 status:
   state: ready
   subsetsStatus:
-    details-default-dynamicenv-multiple-services-per-deploym:
+    multiple-services-per-deployment/details:
       deployment:
         name: details-default-dynamicenv-multiple-services-per-deploym
         namespace: multiple-services-per-deployment

diff --git a/e2e-testing/kuttl/multiple-services-scenarios/04-assert.yaml b/e2e-testing/kuttl/multiple-services-scenarios/04-assert.yaml
@@ -7,7 +7,7 @@ metadata:
 status:
   state: degraded
   subsetsStatus:
-    details-default-dynamicenv-multiple-services-per-deploym:
+    multiple-services-per-deployment/details:
       deployment:
         name: details-default-dynamicenv-multiple-services-per-deploym
         namespace: multiple-services-per-deployment

diff --git a/e2e-testing/kuttl/multiple-services-scenarios/05-assert.yaml b/e2e-testing/kuttl/multiple-services-scenarios/05-assert.yaml
@@ -7,7 +7,7 @@ metadata:
 status:
   state: ready
   subsetsStatus:
-    details-default-dynamicenv-multiple-services-per-deploym:
+    multiple-services-per-deployment/details:
       deployment:
         name: details-default-dynamicenv-multiple-services-per-deploym
         namespace: multiple-services-per-deployment

diff --git a/e2e-testing/kuttl/reconcile-flow/00-assert.yaml b/e2e-testing/kuttl/reconcile-flow/00-assert.yaml
@@ -8,7 +8,7 @@ status:
   totalCount: 1
   totalReady: 0
   subsetsStatus:
-    details-default-dynamicenv-reconcile-flow:
+    reconcile-flow/details:
       subsetErrors:
         deployment:
           - error: 'couldn''t find the deployment we need to override (name: details,

diff --git a/e2e-testing/kuttl/response-headers/00-assert.yaml b/e2e-testing/kuttl/response-headers/00-assert.yaml
@@ -126,7 +126,7 @@ metadata:
   namespace: default
 status:
   subsetsStatus:
-    test-app-default-dynamicenv-response-headers:
+    response-headers/test-app:
       virtualServices:
         - name: test-app-vs
           namespace: response-headers-2

diff --git a/e2e-testing/kuttl/status-updates/00-assert.yaml b/e2e-testing/kuttl/status-updates/00-assert.yaml
@@ -8,7 +8,7 @@ status:
   totalCount: 1
   totalReady: 1
   subsetsStatus:
-    details-default-dynamicenv-status-updates:
+    status-updates/details:
       deployment:
         name: details-default-dynamicenv-status-updates
         namespace: status-updates

diff --git a/e2e-testing/kuttl/update-subset-resources/00-assert.yaml b/e2e-testing/kuttl/update-subset-resources/00-assert.yaml
@@ -27,5 +27,5 @@ metadata:
   name: dynamicenv-update-subset-resources
 status:
   subsetsStatus:
-    details-default-dynamicenv-update-subset-resources:
+    update-subset-resources/details:
       hash: -5576677875946656812
diff --git a/e2e-testing/kuttl/update-subset-resources/01-assert.yaml b/e2e-testing/kuttl/update-subset-resources/01-assert.yaml
@@ -19,7 +19,7 @@ metadata:
   name: dynamicenv-update-subset-resources
 status:
   subsetsStatus:
-    details-default-dynamicenv-update-subset-resources:
+    update-subset-resources/details:
       hash: 3856039101763933687
       deployment:
         name: details-default-dynamicenv-update-subset-resources

diff --git a/e2e-testing/kuttl/valid-state/00-assert.yaml b/e2e-testing/kuttl/valid-state/00-assert.yaml
@@ -4,7 +4,7 @@ metadata:
   name: dynamicenv-valid-state
 status:
   subsetsStatus:
-    details-default-dynamicenv-valid-state:
+    valid-state/details:
       deployment:
         name: details-default-dynamicenv-valid-state
         namespace: valid-state

diff --git a/e2e-testing/kuttl/vs-with-multiple-services/00-assert.yaml b/e2e-testing/kuttl/vs-with-multiple-services/00-assert.yaml
@@ -69,7 +69,7 @@ metadata:
   namespace: default
 status:
   subsetsStatus:
-    details-default-dynamicenv-vs-with-multiple-services:
+    vs-with-multiple-services/details:
       virtualServices:
         - name: all-services
           namespace: vs-with-multiple-services

diff --git a/pkg/handlers/deployment_handler.go b/pkg/handlers/deployment_handler.go
@@ -43,6 +43,8 @@ type DeploymentHandler struct {
 	UniqueName string
 	// The unique version of the deployment we need to handle
 	UniqueVersion string
+	// The name of the subset/consumer as it appears in the Status map
+	SubsetName string
 	// THe owner of the deployment we need to handle (e.g. to configure watches)
 	Owner types.NamespacedName
 	// The deployment we should use as base
@@ -89,7 +91,7 @@ func (h *DeploymentHandler) Handle() error {
 		if err != nil {
 			return fmt.Errorf("calculating hash for %q: %w", h.UniqueName, err)
 		}
-		if err := h.StatusHandler.ApplyHash(h.UniqueName, hash, h.DeploymentType); err != nil {
+		if err := h.StatusHandler.ApplyHash(h.SubsetName, hash, h.DeploymentType); err != nil {
 			return fmt.Errorf("setting subset hash on deployment creation %q: %w", h.UniqueName, err)
 		}
 
@@ -146,24 +148,21 @@ func (h *DeploymentHandler) GetStatus() (riskifiedv1alpha1.ResourceStatus, error
 }
 
 func (h *DeploymentHandler) ApplyStatus(rs riskifiedv1alpha1.ResourceStatus) error {
-	return h.StatusHandler.AddDeploymentStatusEntry(h.UniqueName, rs, h.DeploymentType)
+	return h.StatusHandler.AddDeploymentStatusEntry(h.SubsetName, rs, h.DeploymentType)
 }
 
 func (h *DeploymentHandler) GetSubset() string {
-	return h.UniqueName
+	return h.SubsetName
 }
 
 func (h *DeploymentHandler) UpdateIfRequired() error {
 	h.Log.V(1).Info("Checking whether it's required to update subset", "subset namespace", h.Subset.Namespace, "subset name", h.Subset.Name)
-	// Trusting the webhook validator we assume the following:
-	// - Subset name/namespace can not be changed
-	// - Container and initContainer overrides must have the same names
 	s := h.Subset
 	var existingHash uint64
 	if h.DeploymentType == riskifiedv1alpha1.CONSUMER {
-		existingHash = h.StatusHandler.GetHashForConsumer(h.UniqueName)
+		existingHash = h.StatusHandler.GetHashForConsumer(h.SubsetName)
 	} else {
-		existingHash = h.StatusHandler.GetHashForSubset(h.UniqueName)
+		existingHash = h.StatusHandler.GetHashForSubset(h.SubsetName)
 	}
 	hash, err := hashstructure.Hash(h.Subset, hashstructure.FormatV2, nil)
 	if err != nil {
@@ -183,7 +182,7 @@ func (h *DeploymentHandler) UpdateIfRequired() error {
 			h.Log.Error(err, "Updating Subset", "subset full name", h.UniqueName)
 			return fmt.Errorf("error updating subset %s: %w", h.UniqueName, err)
 		}
-		if err := h.StatusHandler.ApplyHash(h.UniqueName, hash, h.DeploymentType); err != nil {
+		if err := h.StatusHandler.ApplyHash(h.SubsetName, hash, h.DeploymentType); err != nil {
 			return fmt.Errorf("setting subset hash for '%s': %w", h.UniqueName, err)
 		}
 	}
@@ -260,7 +259,7 @@ func (h *DeploymentHandler) setStatus(status riskifiedv1alpha1.LifeCycleStatus)
 		Namespace: h.Subset.Namespace,
 		Status:    status,
 	}
-	if err := h.StatusHandler.AddDeploymentStatusEntry(h.UniqueName, currentState, h.DeploymentType); err != nil {
+	if err := h.StatusHandler.AddDeploymentStatusEntry(h.SubsetName, currentState, h.DeploymentType); err != nil {
 		return err
 	}
 	return nil

diff --git a/pkg/handlers/deployment_handler_test.go b/pkg/handlers/deployment_handler_test.go
@@ -203,6 +203,7 @@ var _ = Describe("DeploymentHandler", func() {
 			}
 			mc := struct{ MockClient }{}
 			handler := mkDeploymentHandler("details-default-dynamicenv-sample", de, mc)
+			handler.SubsetName = fmt.Sprintf("%s/%s", de.Spec.Subsets[0].Namespace, de.Spec.Subsets[0].Name)
 			subset := de.Spec.Subsets[0].DeepCopy()
 			handler.Subset = *subset
 			errorResult := handler.UpdateIfRequired()