Support deletion of ClusterImagePolicy

config/200-clusterrole.yaml

@@ -49,9 +49,10 @@ rules:
   #   resourceNames: ["clusterimagepolicies.cosigned.sigstore.dev"]
 
   # Allow reconciliation of the ClusterImagePolic CRDs.
+  # We need patch to update finalizers
   - apiGroups: ["cosigned.sigstore.dev"]
     resources: ["clusterimagepolicies"]
-    verbs: ["get", "list", "update", "watch"]
+    verbs: ["get", "list", "update", "watch", "patch"]
 
   # This is needed by k8schain to support fetching pull secrets attached to pod specs
   # or their service accounts.  If pull secrets aren't used, the "secrets" below can

pkg/reconciler/clusterimagepolicy/clusterimagepolicy.go

@@ -47,8 +47,9 @@ type Reconciler struct {
 	kubeclient      kubernetes.Interface
 }
 
-// Check that our Reconciler implements Interface
+// Check that our Reconciler implements Interface as well as finalizer
 var _ clusterimagepolicyreconciler.Interface = (*Reconciler)(nil)
+var _ clusterimagepolicyreconciler.Finalizer = (*Reconciler)(nil)
 
 // ReconcileKind implements Interface.ReconcileKind.
 func (r *Reconciler) ReconcileKind(ctx context.Context, cip *v1alpha1.ClusterImagePolicy) reconciler.Event {
@@ -85,6 +86,36 @@ func (r *Reconciler) ReconcileKind(ctx context.Context, cip *v1alpha1.ClusterIma
 	return nil
 }
 
+// FinalizeKind implements Interface.ReconcileKind.
+func (r *Reconciler) FinalizeKind(ctx context.Context, cip *v1alpha1.ClusterImagePolicy) reconciler.Event {
+	// See if the CM holding configs even exists
+	existing, err := r.configmaplister.ConfigMaps(system.Namespace()).Get(config.ImagePoliciesConfigName)
+	if err != nil {
+		if !apierrs.IsNotFound(err) {
+			// There's very little we can do here. This could happen if it's
+			// intermittent error, which is fine when we retry. But if something
+			// goofy happens like we lost access to it, then it's a bit of a
+			// pickle since the entry will exist there and we can't remove it.
+			// So keep trying. Other option would be just to bail.
+			logging.FromContext(ctx).Errorf("Failed to get configmap: %v", err)
+			return err
+		}
+		// Since the CM doesn't exist, there's nothing for us to clean up.
+		return nil
+	}
+	// CM exists, so remove our entry from it.
+	patchBytes, err := resources.CreateRemovePatch(system.Namespace(), config.ImagePoliciesConfigName, existing.DeepCopy(), cip)
+	if err != nil {
+		logging.FromContext(ctx).Errorf("Failed to create remove patch: %v", err)
+		return err
+	}
+	if len(patchBytes) > 0 {
+		_, err = r.kubeclient.CoreV1().ConfigMaps(system.Namespace()).Patch(ctx, config.ImagePoliciesConfigName, types.JSONPatchType, patchBytes, metav1.PatchOptions{})
+		return err
+	}
+	return nil
+}
+
 // Checks to see if we can deal with format yet. This is missing support
 // for things like Secret resolution, so we can't do those yet. As more things
 // are supported, remove them from here.

pkg/reconciler/clusterimagepolicy/clusterimagepolicy_test.go

@@ -50,10 +50,20 @@ const (
 MFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAExB6+H6054/W1SJgs5JR6AJr6J35J
 RCTfQ5s1kD+hGMSE1rH7s46hmXEeyhnlRnaGF8eMU/SBJE/2NKPnxE7WzQ==
 -----END PUBLIC KEY-----`
+
 	// This is the patch for replacing a single entry in the ConfigMap
 	replaceCIPPatch = `[{"op":"replace","path":"/data/test-cip","value":"{\"images\":[{\"glob\":\"ghcr.io/example/*\",\"regex\":\"\",\"authorities\":[{\"key\":{\"data\":\"-----BEGIN PUBLIC KEY-----\\nMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAExB6+H6054/W1SJgs5JR6AJr6J35J\\nRCTfQ5s1kD+hGMSE1rH7s46hmXEeyhnlRnaGF8eMU/SBJE/2NKPnxE7WzQ==\\n-----END PUBLIC KEY-----\"}}]}]}"}]`
+
 	// This is the patch for adding an entry for non-existing KMS for cipName2
 	addCIP2Patch = `[{"op":"add","path":"/data/test-cip-2","value":"{\"images\":[{\"glob\":\"ghcr.io/example/*\",\"regex\":\"\",\"authorities\":[{\"key\":{\"data\":\"azure-kms://foo/bar\"}}]}]}"}]`
+
+	// This is the patch for removing the last entry, leaving just the
+	// configmap objectmeta, no data.
+	removeDataPatch = `[{"op":"remove","path":"/data"}]`
+
+	// THis is the patch for removing only a single entry from a map that has
+	// two entries but only one is being removed.
+	removeSingleEntryPatch = `[{"op":"remove","path":"/data/test-cip-2"}]`
 )
 
 func TestReconcile(t *testing.T) {
@@ -69,14 +79,14 @@ func TestReconcile(t *testing.T) {
 		Name: "ClusterImagePolicy not found",
 		Key:  testKey,
 	}, {
-		Name: "ClusterImagePolicy is being deleted",
+		Name: "ClusterImagePolicy is being deleted, doesn't exist, no changes",
 		Key:  testKey,
 		Objects: []runtime.Object{
 			NewClusterImagePolicy(cipName,
 				WithClusterImagePolicyDeletionTimestamp),
 		},
 	}, {
-		Name: "ClusterImagePolicy with glob and inline key data",
+		Name: "ClusterImagePolicy with glob and inline key data, added to cm and finalizer",
 		Key:  testKey,
 
 		SkipNamespaceValidation: true, // Cluster scoped
@@ -94,7 +104,13 @@ func TestReconcile(t *testing.T) {
 				})),
 		},
 		WantCreates: []runtime.Object{
-			makeConfigMap(cipName),
+			makeConfigMap(),
+		},
+		WantPatches: []clientgotesting.PatchActionImpl{
+			patchFinalizers(system.Namespace(), cipName),
+		},
+		WantEvents: []string{
+			Eventf(corev1.EventTypeNormal, "FinalizerUpdate", `Updated "test-cip" finalizers`),
 		},
 	}, {
 		Name: "ClusterImagePolicy with glob and inline key data, already exists, no patch",
@@ -103,6 +119,7 @@ func TestReconcile(t *testing.T) {
 		SkipNamespaceValidation: true, // Cluster scoped
 		Objects: []runtime.Object{
 			NewClusterImagePolicy(cipName,
+				WithFinalizer,
 				WithImagePattern(v1alpha1.ImagePattern{
 					Glob: glob,
 					Authorities: []v1alpha1.Authority{
@@ -113,7 +130,7 @@ func TestReconcile(t *testing.T) {
 						},
 					},
 				})),
-			makeConfigMap(cipName),
+			makeConfigMap(),
 		},
 	}, {
 		Name: "ClusterImagePolicy with glob and inline key data, needs a patch",
@@ -122,6 +139,7 @@ func TestReconcile(t *testing.T) {
 		SkipNamespaceValidation: true, // Cluster scoped
 		Objects: []runtime.Object{
 			NewClusterImagePolicy(cipName,
+				WithFinalizer,
 				WithImagePattern(v1alpha1.ImagePattern{
 					Glob: glob,
 					Authorities: []v1alpha1.Authority{
@@ -132,10 +150,10 @@ func TestReconcile(t *testing.T) {
 						},
 					},
 				})),
-			makeDifferentConfigMap(cipName),
+			makeDifferentConfigMap(),
 		},
 		WantPatches: []clientgotesting.PatchActionImpl{
-			makePatch(system.Namespace(), config.ImagePoliciesConfigName, replaceCIPPatch),
+			makePatch(replaceCIPPatch),
 		},
 	}, {
 		Name: "ClusterImagePolicy with glob and KMS key data, added as a patch",
@@ -144,6 +162,7 @@ func TestReconcile(t *testing.T) {
 		SkipNamespaceValidation: true, // Cluster scoped
 		Objects: []runtime.Object{
 			NewClusterImagePolicy(cipName2,
+				WithFinalizer,
 				WithImagePattern(v1alpha1.ImagePattern{
 					Glob: glob,
 					Authorities: []v1alpha1.Authority{
@@ -154,10 +173,66 @@ func TestReconcile(t *testing.T) {
 						},
 					},
 				})),
-			makeConfigMap(cipName), // Make the existing configmap
+			makeConfigMap(), // Make the existing configmap
+		},
+		WantPatches: []clientgotesting.PatchActionImpl{
+			makePatch(addCIP2Patch),
+		},
+	}, {
+		Name: "ClusterImagePolicy with glob and inline key data, already exists, deleted",
+		Key:  testKey,
+
+		SkipNamespaceValidation: true, // Cluster scoped
+		Objects: []runtime.Object{
+			NewClusterImagePolicy(cipName,
+				WithFinalizer,
+				WithImagePattern(v1alpha1.ImagePattern{
+					Glob: glob,
+					Authorities: []v1alpha1.Authority{
+						{
+							Key: &v1alpha1.KeyRef{
+								Data: inlineKeyData,
+							},
+						},
+					},
+				}),
+				WithClusterImagePolicyDeletionTimestamp),
+			makeConfigMap(),
+		},
+		WantPatches: []clientgotesting.PatchActionImpl{
+			patchRemoveFinalizers(system.Namespace(), cipName),
+			makePatch(removeDataPatch),
+		},
+		WantEvents: []string{
+			Eventf(corev1.EventTypeNormal, "FinalizerUpdate", `Updated "test-cip" finalizers`),
+		},
+	}, {
+		Name: "Two entries, remove only one",
+		Key:  testKey2,
+
+		SkipNamespaceValidation: true, // Cluster scoped
+		Objects: []runtime.Object{
+			NewClusterImagePolicy(cipName2,
+				WithFinalizer,
+				WithImagePattern(v1alpha1.ImagePattern{
+					Glob: glob,
+					Authorities: []v1alpha1.Authority{
+						{
+							Key: &v1alpha1.KeyRef{
+								Data: inlineKeyData,
+							},
+						},
+					},
+				}),
+				WithClusterImagePolicyDeletionTimestamp),
+			makeConfigMapWithTwoEntries(),
 		},
 		WantPatches: []clientgotesting.PatchActionImpl{
-			makePatch(system.Namespace(), config.ImagePoliciesConfigName, addCIP2Patch),
+			patchRemoveFinalizers(system.Namespace(), cipName2),
+			makePatch(removeSingleEntryPatch),
+		},
+		WantEvents: []string{
+			Eventf(corev1.EventTypeNormal, "FinalizerUpdate", `Updated "test-cip-2" finalizers`),
 		},
 	}, {}}
 
@@ -178,7 +253,7 @@ func TestReconcile(t *testing.T) {
 	))
 }
 
-func makeConfigMap(cipName string) *corev1.ConfigMap {
+func makeConfigMap() *corev1.ConfigMap {
 	return &corev1.ConfigMap{
 		ObjectMeta: metav1.ObjectMeta{
 			Namespace: system.Namespace(),
@@ -191,7 +266,7 @@ func makeConfigMap(cipName string) *corev1.ConfigMap {
 }
 
 // Same as above, just forcing an update by changing PUBLIC => NOTPUBLIC
-func makeDifferentConfigMap(cipName string) *corev1.ConfigMap {
+func makeDifferentConfigMap() *corev1.ConfigMap {
 	return &corev1.ConfigMap{
 		ObjectMeta: metav1.ObjectMeta{
 			Namespace: system.Namespace(),
@@ -203,12 +278,44 @@ func makeDifferentConfigMap(cipName string) *corev1.ConfigMap {
 	}
 }
 
-func makePatch(namespace, name, patch string) clientgotesting.PatchActionImpl {
+// Same as MakeConfigMap but a placeholder for secont entry so we can remove it.
+func makeConfigMapWithTwoEntries() *corev1.ConfigMap {
+	return &corev1.ConfigMap{
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace: system.Namespace(),
+			Name:      config.ImagePoliciesConfigName,
+		},
+		Data: map[string]string{
+			cipName:  `{"images":[{"glob":"ghcr.io/example/*","regex":"","authorities":[{"key":{"data":"-----BEGIN PUBLIC KEY-----\nMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAExB6+H6054/W1SJgs5JR6AJr6J35J\nRCTfQ5s1kD+hGMSE1rH7s46hmXEeyhnlRnaGF8eMU/SBJE/2NKPnxE7WzQ==\n-----END PUBLIC KEY-----"}}]}]}`,
+			cipName2: "remove me please",
+		},
+	}
+}
+
+func makePatch(patch string) clientgotesting.PatchActionImpl {
 	return clientgotesting.PatchActionImpl{
 		ActionImpl: clientgotesting.ActionImpl{
-			Namespace: namespace,
+			Namespace: system.Namespace(),
 		},
-		Name:  name,
+		Name:  config.ImagePoliciesConfigName,
 		Patch: []byte(patch),
 	}
 }
+
+func patchFinalizers(namespace, name string) clientgotesting.PatchActionImpl {
+	action := clientgotesting.PatchActionImpl{}
+	action.Name = name
+	action.Namespace = namespace
+	patch := `{"metadata":{"finalizers":["` + finalizerName + `"],"resourceVersion":""}}`
+	action.Patch = []byte(patch)
+	return action
+}
+
+func patchRemoveFinalizers(namespace, name string) clientgotesting.PatchActionImpl {
+	action := clientgotesting.PatchActionImpl{}
+	action.Name = name
+	action.Namespace = namespace
+	patch := `{"metadata":{"finalizers":[],"resourceVersion":""}}`
+	action.Patch = []byte(patch)
+	return action
+}

pkg/reconciler/clusterimagepolicy/controller.go

@@ -36,6 +36,10 @@ import (
 	clusterimagepolicyreconciler "github.com/sigstore/cosign/pkg/client/injection/reconciler/cosigned/v1alpha1/clusterimagepolicy"
 )
 
+// This is what the default finalizer name is, but make it explicit so we can
+// use it in tests as well.
+const finalizerName = "clusterimagepolicies.cosigned.sigstore.dev"
+
 // NewController creates a Reconciler and returns the result of NewImpl.
 func NewController(
 	ctx context.Context,
@@ -57,7 +61,9 @@ func NewController(
 		configmaplister: nsConfigMapInformer.Lister(),
 		kubeclient:      kubeclient.Get(ctx),
 	}
-	impl := clusterimagepolicyreconciler.NewImpl(ctx, r)
+	impl := clusterimagepolicyreconciler.NewImpl(ctx, r, func(impl *controller.Impl) controller.Options {
+		return controller.Options{FinalizerName: finalizerName}
+	})
 	r.tracker = impl.Tracker
 
 	clusterimagepolicyInformer.Informer().AddEventHandler(controller.HandleAll(impl.Enqueue))

pkg/reconciler/clusterimagepolicy/resources/configmap.go

@@ -54,6 +54,9 @@ func CreatePatch(ns, name string, cm *corev1.ConfigMap, cip *v1alpha1.ClusterIma
 		return nil, err
 	}
 	after := cm.DeepCopy()
+	if after.Data == nil {
+		after.Data = make(map[string]string)
+	}
 	after.Data[cip.Name] = entry
 	jsonPatch, err := duck.CreatePatch(cm, after)
 	if err != nil {
@@ -65,6 +68,23 @@ func CreatePatch(ns, name string, cm *corev1.ConfigMap, cip *v1alpha1.ClusterIma
 	return jsonPatch.MarshalJSON()
 }
 
+// CreateRemovePatch removes an entry from the ConfigMap and returns the patch
+// bytes for it that's suitable for calling ConfigMap.Patch with.
+func CreateRemovePatch(ns, name string, cm *corev1.ConfigMap, cip *v1alpha1.ClusterImagePolicy) ([]byte, error) {
+	after := cm.DeepCopy()
+	// Just remove it without checking if it exists. If it doesn't, then no
+	// patch bytes are created.
+	delete(after.Data, cip.Name)
+	jsonPatch, err := duck.CreatePatch(cm, after)
+	if err != nil {
+		return nil, fmt.Errorf("creating JSON patch: %w", err)
+	}
+	if len(jsonPatch) == 0 {
+		return nil, nil
+	}
+	return jsonPatch.MarshalJSON()
+}
+
 func marshal(spec v1alpha1.ClusterImagePolicySpec) (string, error) {
 	bytes, err := json.Marshal(&spec)
 	if err != nil {

pkg/reconciler/testing/v1alpha1/clusterimagepolicy.go

@@ -22,6 +22,8 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
+const finalizerName = "clusterimagepolicies.cosigned.sigstore.dev"
+
 // ClusterImagePolicyOption enables further configuration of a ClusterImagePolicy.
 type ClusterImagePolicyOption func(*v1alpha1.ClusterImagePolicy)
 
@@ -49,3 +51,7 @@ func WithImagePattern(ip v1alpha1.ImagePattern) ClusterImagePolicyOption {
 		cip.Spec.Images = append(cip.Spec.Images, ip)
 	}
 }
+
+func WithFinalizer(cip *v1alpha1.ClusterImagePolicy) {
+	cip.Finalizers = []string{finalizerName}
+}