fix: removing a key in the MergeTarget cleans up (#23)

Closes #19

api/v1beta1/mergetarget_types.go

@@ -174,16 +174,39 @@ func (m *MergeTarget) UpdateDataStatus(configMapData map[string]string) {
 	}
 }
 
+// ReduceDataState mutates configMapData, accumulating the MergeSourceList into the
+// respective keys.
+// nolint:cyclop
 func (m *MergeTarget) ReduceDataState(
-	mergeSources MergeSourceList,
-	configMapData *map[string]string,
-) (updatedKeys int, fieldsErrors []string) {
+	mergeSources MergeSourceList, configMapData *map[string]string,
+) (statusKeysToRemove []string, updatedKeys int, fieldsErrors []string) {
 	configMap := *configMapData
 
 	for k, v := range m.Status.Data {
+		//
+		// If the Spec for the MergeTarget no longer has the key
+		// specified, we revert the configMap to its original state,
+		// either the initial data for that key, or removing it entirely.
+		//
+		// This will end up keeping the status key, which we want to do
+		// until we are confident that the CM has been reverted successfully.
+		if _, ok := m.Spec.Data[k]; !ok {
+			existingValue, exists := configMap[k]
+			if !exists && v.IsStatusNewlyCreated() {
+				// do nothing, this is all good, it doesn't exist
+				// and it was supposed to be newly created/managed by the MergeTarget
+			} else if existingValue != v.Init {
+				configMap[k] = v.Init
+				updatedKeys++
+			}
+
+			statusKeysToRemove = append(statusKeysToRemove, k)
+			continue
+		}
+
+		//
 		// create & aggregate the data from the mergeSources
 		data := v.Init
-
 		for _, source := range mergeSources.Items {
 			if source.Spec.Target.Data == k {
 				data += source.Status.Output
@@ -212,7 +235,13 @@ func (m *MergeTarget) ReduceDataState(
 
 	*configMapData = configMap
 
-	return updatedKeys, fieldsErrors
+	return statusKeysToRemove, updatedKeys, fieldsErrors
+}
+
+func (m *MergeTarget) RemoveDataStatusKeys(keys []string) {
+	for _, k := range keys {
+		delete(m.Status.Data, k)
+	}
 }
 
 func NewMergeTarget(name types.NamespacedName, spec MergeTargetSpec) *MergeTarget {

controllers/mergetarget_controller.go

@@ -118,7 +118,9 @@ func (r *MergeTargetReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 
 // reconcileMergeTarget is the main function that ensures the target ConfigMap
 // has the state it needs to have given the MergeTarget resource.
-func (r *MergeTargetReconciler) reconcileMergeTarget(ctx context.Context, mtName string, mt *MergeTarget, cm *corev1.ConfigMap) (ctrl.Result, error) {
+func (r *MergeTargetReconciler) reconcileMergeTarget(
+	ctx context.Context, mtName string, mt *MergeTarget, cm *corev1.ConfigMap,
+) (ctrl.Result, error) {
 	var (
 		log                = log.FromContext(ctx)
 		setStatusCondition = func(c metav1.Condition) error {
@@ -132,26 +134,33 @@ func (r *MergeTargetReconciler) reconcileMergeTarget(ctx context.Context, mtName
 
 	log.Info("initialized/updated initial state of the target")
 
-	stats, err := r.reduceTargetState(ctx, mtName, mt, cm)
+	// N.B. We don't initially remove the keys to make sure the udpate
+	// goes through successfully before we cleanup the status.
+	keysToRemove, stats, err := r.reduceTargetState(ctx, mtName, mt, cm)
 	if err != nil {
 		return ctrl.Result{RequeueAfter: time.Minute}, errors.WithStack(err)
 	}
 
+	// record the status/condition of the things we are going to attempt to store.
 	r.Recorder.RecordNumSources(mt, stats.NumMergeSources)
 	stats.LogWithValues(log).Info("found and merged sources")
 	err = setStatusCondition(cmmcv1beta1.MergeTargetConditionValidation(stats.FieldsErrorMsgs, stats.NumMergeSources))
 	if err != nil {
 		return ctrl.Result{Requeue: true}, errors.WithStack(err)
 	}
 
-	if stats.NumUpdatedKeys > 0 {
+	if stats.NumUpdatedKeys > 0 { // if we should be doing an update, let's do it
 		if err := r.Update(ctx, cm); err != nil {
 			_ = setStatusCondition(cmmcv1beta1.MergeTargetConditionErrorUpdating(err, stats.NumUpdatedKeys))
 			return ctrl.Result{RequeueAfter: time.Minute}, errors.Wrap(err, "failed updating target configMap")
 		}
 	}
 
-	err = setStatusCondition(cmmcv1beta1.MergeTargetConditionReady(len(stats.FieldsErrorMsgs) > 0))
+	// do Status cleanup, and set the right condition
+	mt.RemoveDataStatusKeys(keysToRemove)
+	mt.SetStatusCondition(cmmcv1beta1.MergeTargetConditionReady(len(stats.FieldsErrorMsgs) > 0))
+	err = r.Status().Update(ctx, mt)
+
 	return ctrl.Result{RequeueAfter: time.Minute}, errors.WithStack(err)
 }
 
@@ -169,40 +178,38 @@ func (m *mergeStats) LogWithValues(l logr.Logger) logr.Logger {
 	)
 }
 
-func (r *MergeTargetReconciler) updateDataStatus(ctx context.Context, mt *MergeTarget, cm *corev1.ConfigMap) error {
+func (r *MergeTargetReconciler) updateDataStatus(
+	ctx context.Context, mt *MergeTarget, cm *corev1.ConfigMap,
+) error {
 	mt.UpdateDataStatus(cm.Data)
 	return errors.Wrap(r.Status().Update(ctx, mt), "failed updating initial status")
 }
 
-func (r *MergeTargetReconciler) setStatusCondition(ctx context.Context, mt *MergeTarget, c metav1.Condition) error {
+func (r *MergeTargetReconciler) setStatusCondition(
+	ctx context.Context, mt *MergeTarget, c metav1.Condition,
+) error {
 	mt.SetStatusCondition(c)
 	return errors.Wrapf(r.Status().Update(ctx, mt), "error updating condition type %s", c.Type)
 }
 
 func (r *MergeTargetReconciler) reduceTargetState(
-	ctx context.Context,
-	name string,
-	mergeTarget *cmmcv1beta1.MergeTarget,
-	targetConfigMap *corev1.ConfigMap,
-) (*mergeStats, error) {
+	ctx context.Context, name string, mt *MergeTarget, targetConfigMap *corev1.ConfigMap,
+) ([]string, *mergeStats, error) {
 	var mergeSources cmmcv1beta1.MergeSourceList
-	if err := r.List(ctx, &mergeSources, client.MatchingFields{"status.target": name}); err != nil {
-		return nil, errors.Wrapf(err, "failed fetching MergeSource list for %s", name)
+	if err := r.List(ctx, &mergeSources, client.MatchingFields{fieldIndexStatusTarget: name}); err != nil {
+		return nil, nil, errors.Wrapf(err, "failed fetching MergeSource list for %s", name)
 	}
 
-	numUpdatedKeys, errorsOnFields := mergeTarget.ReduceDataState(mergeSources, &targetConfigMap.Data)
-	return &mergeStats{
+	statusKeysToRemove, numUpdatedKeys, errorsOnFields := mt.ReduceDataState(mergeSources, &targetConfigMap.Data)
+	return statusKeysToRemove, &mergeStats{
 		NumUpdatedKeys:  numUpdatedKeys,
 		NumMergeSources: len(mergeSources.Items),
 		FieldsErrorMsgs: errorsOnFields,
 	}, nil
 }
 
 func (r *MergeTargetReconciler) targetConfigMap(
-	ctx context.Context,
-	mergeTarget *cmmcv1beta1.MergeTarget,
-	name types.NamespacedName,
-	managedByName string,
+	ctx context.Context, mergeTarget *MergeTarget, name types.NamespacedName, managedByName string,
 ) (*corev1.ConfigMap, bool, error) {
 	var cm corev1.ConfigMap
 	if err := r.Get(ctx, name, &cm); err != nil {

controllers/suite_test.go

@@ -232,6 +232,24 @@ var _ = Describe("cmmc", func() {
 			})
 		})
 
+		When("removing a key in a MergeTarget", func() {
+			It("can successfully delete a key", func() {
+				Expect(k8sClient.Get(ctx, names.target, mergeTarget)).Should(Succeed())
+				delete(mergeTarget.Spec.Data, "mapUsers")
+				Expect(k8sClient.Update(ctx, mergeTarget)).Should(Succeed())
+			})
+
+			It("should remove the key in the target", func() {
+				assertConfigMapState(names.targetCM, MapUsers(""))
+			})
+
+			It("can add the key back", func() { // for the rest of the tests
+				Expect(k8sClient.Get(ctx, names.target, mergeTarget)).Should(Succeed())
+				mergeTarget.Spec.Data["mapUsers"] = cmmcv1beta1.MergeTargetDataSpec{}
+				Expect(k8sClient.Update(ctx, mergeTarget)).Should(Succeed())
+			})
+		})
+
 		Context("cleanup", func() {
 			When("removing roles MergeSource", func() {
 				It("can be deleted", func() {