diff --git a/config/200-clusterrole.yaml b/config/200-clusterrole.yaml index caebeb1c413..644d49578b5 100644 --- a/config/200-clusterrole.yaml +++ b/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 diff --git a/pkg/reconciler/clusterimagepolicy/clusterimagepolicy.go b/pkg/reconciler/clusterimagepolicy/clusterimagepolicy.go index cb24f6582bb..6ec9c7f96e1 100644 --- a/pkg/reconciler/clusterimagepolicy/clusterimagepolicy.go +++ b/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. diff --git a/pkg/reconciler/clusterimagepolicy/clusterimagepolicy_test.go b/pkg/reconciler/clusterimagepolicy/clusterimagepolicy_test.go index 51b7cfd4a9e..29ca3d0abe4 100644 --- a/pkg/reconciler/clusterimagepolicy/clusterimagepolicy_test.go +++ b/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 +} diff --git a/pkg/reconciler/clusterimagepolicy/controller.go b/pkg/reconciler/clusterimagepolicy/controller.go index 4ab1a4c117b..e46480202db 100644 --- a/pkg/reconciler/clusterimagepolicy/controller.go +++ b/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)) diff --git a/pkg/reconciler/clusterimagepolicy/resources/configmap.go b/pkg/reconciler/clusterimagepolicy/resources/configmap.go index 7061c37608a..404dbcbac16 100644 --- a/pkg/reconciler/clusterimagepolicy/resources/configmap.go +++ b/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 { diff --git a/pkg/reconciler/testing/v1alpha1/clusterimagepolicy.go b/pkg/reconciler/testing/v1alpha1/clusterimagepolicy.go index e5fe036d202..ea33555399d 100644 --- a/pkg/reconciler/testing/v1alpha1/clusterimagepolicy.go +++ b/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} +}