From 7e67ebc19390b20152c1556ae20170e7a6653205 Mon Sep 17 00:00:00 2001 From: "Per G. da Silva" Date: Tue, 20 Oct 2026 20:00:32 +0200 Subject: [PATCH] =?UTF-8?q?=E2=9C=A8=20add=20ClusterObjectDeployment=20con?= =?UTF-8?q?troller=20MVP?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Give the ClusterObjectDeployment (COD) controller its first behavior, in the object-controller binary (experimental-only, //go:build !standard). It reconciles ClusterObjectDeployment objects and owns their ClusterObjectSet (COS) revisions. - Detect template changes via a template-hash label (shared hashutil.DeepHashObject, base36 of sha224 — label-safe, no truncation) and create a new Active COS revision (named -) owned by the COD when the template changes or no active revision exists. - Report rollout progress on the COD's Progressing condition, keyed off the latest matching revision's completedAt. - SetupWithManager: For(COD).Owns(COS).Watches(COS by spec.group); the shared .spec.group index setup moves to a dedicated indexes.go. - Surface AlreadyExists on revision create rather than tolerating it (matches the orb-operator reference; stale-cache retries self-heal on requeue). - Generated revisions carry only the template-hash label; ownership is expressed through the controller owner reference and spec.group. - Add a cod_rollout e2e feature and a Godog step for ClusterObjectDeployment conditions. Co-Authored-By: Claude Opus 4.8 Signed-off-by: Per G. da Silva --- cmd/object-controller/main.go | 1 + .../clusterobjectdeployment_controller.go | 169 ++++++++++++- ...jectdeployment_controller_internal_test.go | 100 ++++++++ ...clusterobjectdeployment_controller_test.go | 225 ++++++++++++++++-- .../clusterobjectset_controller.go | 13 +- .../object-controller/controllers/indexes.go | 30 +++ internal/shared/labels/labels.go | 7 + test/e2e/features/cod_rollout.feature | 97 ++++++++ test/e2e/steps/steps.go | 8 + 9 files changed, 605 insertions(+), 45 deletions(-) create mode 100644 internal/object-controller/controllers/indexes.go create mode 100644 test/e2e/features/cod_rollout.feature diff --git a/cmd/object-controller/main.go b/cmd/object-controller/main.go index 7f9e09e8ea..cea9223910 100644 --- a/cmd/object-controller/main.go +++ b/cmd/object-controller/main.go @@ -202,6 +202,7 @@ func newManager(cfg *config, restConfig *rest.Config) (manager.Manager, error) { } if err := (&controllers.ClusterObjectDeploymentReconciler{ Client: mgr.GetClient(), + Scheme: mgr.GetScheme(), }).SetupWithManager(mgr); err != nil { return nil, fmt.Errorf("setting up ClusterObjectDeployment controller: %w", err) } diff --git a/internal/object-controller/controllers/clusterobjectdeployment_controller.go b/internal/object-controller/controllers/clusterobjectdeployment_controller.go index 220f74fb4f..ab68c273f3 100644 --- a/internal/object-controller/controllers/clusterobjectdeployment_controller.go +++ b/internal/object-controller/controllers/clusterobjectdeployment_controller.go @@ -3,25 +3,35 @@ package controllers import ( + "cmp" "context" + "errors" + "fmt" + "slices" + "k8s.io/apimachinery/pkg/api/equality" + apimeta "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/handler" "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/predicate" ocv1 "github.com/operator-framework/operator-controller/api/v1" + "github.com/operator-framework/operator-controller/internal/shared/labels" + hashutil "github.com/operator-framework/operator-controller/internal/shared/util/hash" ) // ClusterObjectDeploymentReconciler reconciles ClusterObjectDeployment objects by // managing their owned ClusterObjectSet revisions. -// -// This is a skeleton: it does not yet implement any reconciliation behavior. type ClusterObjectDeploymentReconciler struct { Client client.Client + Scheme *runtime.Scheme } //+kubebuilder:rbac:groups=olm.operatorframework.io,resources=clusterobjectdeployments,verbs=get;list;watch;update;patch;create;delete @@ -33,18 +43,99 @@ func (c *ClusterObjectDeploymentReconciler) Reconcile(ctx context.Context, req c l := log.FromContext(ctx).WithName("cluster-object-deployment") ctx = log.IntoContext(ctx, l) - existingDeployment := &ocv1.ClusterObjectDeployment{} - if err := c.Client.Get(ctx, req.NamespacedName, existingDeployment); err != nil { + existing := &ocv1.ClusterObjectDeployment{} + if err := c.Client.Get(ctx, req.NamespacedName, existing); err != nil { return ctrl.Result{}, client.IgnoreNotFound(err) } + if !existing.DeletionTimestamp.IsZero() { + // Owned ClusterObjectSets are removed by Kubernetes cascading deletion via + // their controller owner reference; nothing to do here. + return ctrl.Result{}, nil + } l.Info("reconcile starting") defer l.Info("reconcile ending") - // No reconciliation behavior yet. This skeleton establishes the controller, - // its watches, and its RBAC so that behavior can be layered on in follow-ups. + reconciled := existing.DeepCopy() + reconcileErr := c.reconcile(ctx, reconciled) + + if !equality.Semantic.DeepEqual(existing.Status, reconciled.Status) { + if err := c.Client.Status().Update(ctx, reconciled); err != nil { + reconcileErr = errors.Join(reconcileErr, fmt.Errorf("updating status: %w", err)) + } + } + + return ctrl.Result{}, reconcileErr +} + +// reconcile creates a new ClusterObjectSet revision when the template has changed +// (or no active revision exists) and reflects the latest revision's rollout +// progress in the deployment's Progressing condition. Requeues are driven by the +// controller's watch on owned ClusterObjectSets. +func (c *ClusterObjectDeploymentReconciler) reconcile(ctx context.Context, cod *ocv1.ClusterObjectDeployment) error { + revisions, err := c.listRevisions(ctx, cod.Name) + if err != nil { + return err + } + + hash := templateHash(cod.Spec.Template) + + latest := latestActiveRevision(revisions) + if latest == nil || latest.Labels[labels.TemplateHashKey] != hash { + if err := c.createRevision(ctx, cod, revisions, hash); err != nil { + setProgressing(cod, metav1.ConditionTrue, ocv1.ClusterObjectDeploymentReasonNewClusterObjectSetProgressing, "waiting for new revision to be created") + return err + } + setProgressing(cod, metav1.ConditionTrue, ocv1.ClusterObjectDeploymentReasonNewClusterObjectSetProgressing, "waiting for new revision to be created") + return nil + } + + if latest.Status.CompletedAt.IsZero() { + setProgressing(cod, metav1.ConditionTrue, ocv1.ClusterObjectDeploymentReasonNewClusterObjectSetProgressing, "latest revision is progressing") + } else { + setProgressing(cod, metav1.ConditionFalse, ocv1.ClusterObjectDeploymentReasonNewClusterObjectSetProgressed, "latest revision has progressed") + } + return nil +} + +// listRevisions returns all ClusterObjectSets in the deployment's group, sorted +// ascending by revision number. +func (c *ClusterObjectDeploymentReconciler) listRevisions(ctx context.Context, group string) ([]ocv1.ClusterObjectSet, error) { + var list ocv1.ClusterObjectSetList + if err := c.Client.List(ctx, &list, client.MatchingFields{clusterObjectSetGroupIndex: group}); err != nil { + return nil, fmt.Errorf("listing revisions for group %q: %w", group, err) + } + slices.SortFunc(list.Items, func(a, b ocv1.ClusterObjectSet) int { + return cmp.Compare(a.Spec.Revision, b.Spec.Revision) + }) + return list.Items, nil +} + +// createRevision builds and creates the next ClusterObjectSet revision owned by +// the deployment. Any create error, including AlreadyExists, is returned. +// +// TODO: revisit AlreadyExists handling under the adoption use-case. +func (c *ClusterObjectDeploymentReconciler) createRevision(ctx context.Context, cod *ocv1.ClusterObjectDeployment, revisions []ocv1.ClusterObjectSet, hash string) error { + revision := nextRevisionNumber(revisions) + cos := buildClusterObjectSet(cod, revision, hash) + if err := controllerutil.SetControllerReference(cod, cos, c.Scheme); err != nil { + return fmt.Errorf("setting controller reference: %w", err) + } + if err := c.Client.Create(ctx, cos); err != nil { + return fmt.Errorf("creating revision %d: %w", revision, err) + } + return nil +} - return ctrl.Result{}, nil +// setProgressing sets the deployment's Progressing condition. +func setProgressing(cod *ocv1.ClusterObjectDeployment, status metav1.ConditionStatus, reason, message string) { + apimeta.SetStatusCondition(&cod.Status.Conditions, metav1.Condition{ + Type: ocv1.ClusterObjectDeploymentTypeProgressing, + Status: status, + Reason: reason, + Message: message, + ObservedGeneration: cod.Generation, + }) } func (c *ClusterObjectDeploymentReconciler) SetupWithManager(mgr ctrl.Manager) error { @@ -80,3 +171,67 @@ func mapClusterObjectSetToClusterObjectDeployment(_ context.Context, obj client. } return []ctrl.Request{{NamespacedName: types.NamespacedName{Name: cos.Spec.Group}}} } + +// templateHash returns a stable digest of the deployment template. Any change to +// the template (including changes to fields that are mutable on a ClusterObjectSet) +// produces a different digest, which is how the controller decides a new revision +// is required. The shared DeepHashObject helper base36-encodes a sha224 of the +// template's JSON, which is label-safe and short enough to use as a label value +// without truncation. +func templateHash(template ocv1.ClusterObjectDeploymentTemplate) string { + return hashutil.DeepHashObject(template) +} + +// nextRevisionNumber returns the revision number for a new revision: one greater +// than the highest revision across all revisions in the group (including archived +// ones, so revision numbers are never reused). +func nextRevisionNumber(revisions []ocv1.ClusterObjectSet) int64 { + var max int64 + for i := range revisions { + if revisions[i].Spec.Revision > max { + max = revisions[i].Spec.Revision + } + } + return max + 1 +} + +// latestActiveRevision returns the highest-revision ClusterObjectSet that is +// neither archived nor being deleted, or nil if there is none. +func latestActiveRevision(revisions []ocv1.ClusterObjectSet) *ocv1.ClusterObjectSet { + var latest *ocv1.ClusterObjectSet + for i := range revisions { + rev := &revisions[i] + if !rev.DeletionTimestamp.IsZero() || rev.Spec.LifecycleState == ocv1.ClusterObjectSetLifecycleStateArchived { + continue + } + if latest == nil || rev.Spec.Revision > latest.Spec.Revision { + latest = rev + } + } + return latest +} + +// buildClusterObjectSet constructs the ClusterObjectSet for a revision from the +// deployment template. It does not set the owner reference; the caller does. +func buildClusterObjectSet(cod *ocv1.ClusterObjectDeployment, revision int64, hash string) *ocv1.ClusterObjectSet { + return &ocv1.ClusterObjectSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: fmt.Sprintf("%s-%d", cod.Name, revision), + Labels: map[string]string{ + // TemplateHashKey records which template produced this revision so the + // controller can detect template changes. Ownership is expressed through + // the controller owner reference and the revision's spec.group, so no + // owner-kind/owner-name labels are set. + labels.TemplateHashKey: hash, + }, + }, + Spec: ocv1.ClusterObjectSetSpec{ + Group: cod.Name, + Revision: revision, + LifecycleState: ocv1.ClusterObjectSetLifecycleStateActive, + Phases: cod.Spec.Template.Spec.Phases, + ProgressionProbes: cod.Spec.Template.Spec.ProgressionProbes, + CollisionProtection: cod.Spec.Template.Spec.CollisionProtection, + }, + } +} diff --git a/internal/object-controller/controllers/clusterobjectdeployment_controller_internal_test.go b/internal/object-controller/controllers/clusterobjectdeployment_controller_internal_test.go index 6727f7ba79..88ad87501f 100644 --- a/internal/object-controller/controllers/clusterobjectdeployment_controller_internal_test.go +++ b/internal/object-controller/controllers/clusterobjectdeployment_controller_internal_test.go @@ -4,16 +4,116 @@ package controllers import ( "testing" + "time" "github.com/stretchr/testify/require" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/types" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" ocv1 "github.com/operator-framework/operator-controller/api/v1" + "github.com/operator-framework/operator-controller/internal/shared/labels" ) +func testTemplate() ocv1.ClusterObjectDeploymentTemplate { + return ocv1.ClusterObjectDeploymentTemplate{ + Spec: ocv1.ClusterObjectDeploymentTemplateSpec{ + CollisionProtection: ocv1.CollisionProtectionNone, + Phases: []ocv1.ClusterObjectSetPhase{ + { + Name: "resources", + Objects: []ocv1.ClusterObjectSetObject{ + {Object: unstructured.Unstructured{Object: map[string]interface{}{ + "apiVersion": "v1", + "kind": "ConfigMap", + "metadata": map[string]interface{}{"name": "cm", "namespace": "ns"}, + }}}, + }, + }, + }, + }, + } +} + +func Test_templateHash_isStableAndSensitive(t *testing.T) { + tmpl := testTemplate() + + h1 := templateHash(tmpl) + require.NotEmpty(t, h1) + require.LessOrEqual(t, len(h1), 63, "digest must fit within the label-value length limit") + require.Regexp(t, "^[0-9a-z]+$", h1, "digest must be label-value safe") + + h2 := templateHash(tmpl) + require.Equal(t, h1, h2, "same template must hash the same") + + // A change to a phase object changes the hash. + changed := testTemplate() + changed.Spec.Phases[0].Objects[0].Object.Object["data"] = map[string]interface{}{"k": "v"} + require.NotEqual(t, h1, templateHash(changed)) + + // A change to ONLY progressionProbes (a mutable COS field) still changes the hash. + probesChanged := testTemplate() + probesChanged.Spec.ProgressionProbes = []ocv1.ProgressionProbe{{ + Selector: ocv1.ObjectSelector{Type: ocv1.SelectorTypeGroupKind, GroupKind: metav1.GroupKind{Kind: "ConfigMap"}}, + Assertions: []ocv1.Assertion{{Type: ocv1.ProbeTypeFieldValue}}, + }} + require.NotEqual(t, h1, templateHash(probesChanged), "a progressionProbes-only change must create a new revision") +} + +func Test_nextRevisionNumber(t *testing.T) { + require.Equal(t, int64(1), nextRevisionNumber(nil)) + revs := []ocv1.ClusterObjectSet{ + {Spec: ocv1.ClusterObjectSetSpec{Revision: 1}}, + {Spec: ocv1.ClusterObjectSetSpec{Revision: 3, LifecycleState: ocv1.ClusterObjectSetLifecycleStateArchived}}, + } + require.Equal(t, int64(4), nextRevisionNumber(revs), "must count archived revisions so numbers never repeat") +} + +func Test_latestActiveRevision(t *testing.T) { + require.Nil(t, latestActiveRevision(nil)) + + // Highest-revision active revision wins. + revs := []ocv1.ClusterObjectSet{ + {ObjectMeta: metav1.ObjectMeta{Name: "cod-1"}, Spec: ocv1.ClusterObjectSetSpec{Revision: 1, LifecycleState: ocv1.ClusterObjectSetLifecycleStateActive}}, + {ObjectMeta: metav1.ObjectMeta{Name: "cod-2"}, Spec: ocv1.ClusterObjectSetSpec{Revision: 2, LifecycleState: ocv1.ClusterObjectSetLifecycleStateActive}}, + } + require.Equal(t, "cod-2", latestActiveRevision(revs).Name) + + // All archived -> nil. + archived := []ocv1.ClusterObjectSet{ + {Spec: ocv1.ClusterObjectSetSpec{Revision: 1, LifecycleState: ocv1.ClusterObjectSetLifecycleStateArchived}}, + } + require.Nil(t, latestActiveRevision(archived)) + + // A higher-revision revision being deleted is skipped. + deleting := metav1.NewTime(time.Now()) + mixed := []ocv1.ClusterObjectSet{ + {ObjectMeta: metav1.ObjectMeta{Name: "cod-1"}, Spec: ocv1.ClusterObjectSetSpec{Revision: 1, LifecycleState: ocv1.ClusterObjectSetLifecycleStateActive}}, + {ObjectMeta: metav1.ObjectMeta{Name: "cod-2", DeletionTimestamp: &deleting, Finalizers: []string{"x"}}, Spec: ocv1.ClusterObjectSetSpec{Revision: 2, LifecycleState: ocv1.ClusterObjectSetLifecycleStateActive}}, + } + require.Equal(t, "cod-1", latestActiveRevision(mixed).Name) +} + +func Test_buildClusterObjectSet(t *testing.T) { + cod := &ocv1.ClusterObjectDeployment{ + ObjectMeta: metav1.ObjectMeta{Name: "cod"}, + Spec: ocv1.ClusterObjectDeploymentSpec{Template: testTemplate()}, + } + + cos := buildClusterObjectSet(cod, 1, "deadbeef") + + require.Equal(t, "cod-1", cos.Name) + require.Equal(t, "deadbeef", cos.Labels[labels.TemplateHashKey]) + require.NotContains(t, cos.Labels, labels.OwnerNameKey) + require.Equal(t, "cod", cos.Spec.Group) + require.Equal(t, int64(1), cos.Spec.Revision) + require.Equal(t, ocv1.ClusterObjectSetLifecycleStateActive, cos.Spec.LifecycleState) + require.Equal(t, cod.Spec.Template.Spec.Phases, cos.Spec.Phases) + require.Equal(t, ocv1.CollisionProtectionNone, cos.Spec.CollisionProtection, "template collisionProtection must be copied through") +} + func Test_mapClusterObjectSetToClusterObjectDeployment(t *testing.T) { for _, tc := range []struct { name string diff --git a/internal/object-controller/controllers/clusterobjectdeployment_controller_test.go b/internal/object-controller/controllers/clusterobjectdeployment_controller_test.go index 0ac2b18198..39e0d02461 100644 --- a/internal/object-controller/controllers/clusterobjectdeployment_controller_test.go +++ b/internal/object-controller/controllers/clusterobjectdeployment_controller_test.go @@ -3,58 +3,231 @@ package controllers_test import ( + "context" "testing" "github.com/stretchr/testify/require" + apierrors "k8s.io/apimachinery/pkg/api/errors" + apimeta "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/types" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" ocv1 "github.com/operator-framework/operator-controller/api/v1" "github.com/operator-framework/operator-controller/internal/object-controller/controllers" + "github.com/operator-framework/operator-controller/internal/shared/labels" + "github.com/operator-framework/operator-controller/internal/testutil" ) const clusterObjectDeploymentName = "test-cod-1" -func Test_ClusterObjectDeploymentReconciler_Reconcile(t *testing.T) { - testScheme := newScheme(t) +func codFixture() *ocv1.ClusterObjectDeployment { + return &ocv1.ClusterObjectDeployment{ + ObjectMeta: metav1.ObjectMeta{Name: clusterObjectDeploymentName}, + Spec: ocv1.ClusterObjectDeploymentSpec{ + Template: ocv1.ClusterObjectDeploymentTemplate{ + Spec: ocv1.ClusterObjectDeploymentTemplateSpec{ + CollisionProtection: ocv1.CollisionProtectionPrevent, + Phases: []ocv1.ClusterObjectSetPhase{{ + Name: "resources", + Objects: []ocv1.ClusterObjectSetObject{{ + Object: unstructured.Unstructured{Object: map[string]interface{}{ + "apiVersion": "v1", + "kind": "ConfigMap", + "metadata": map[string]interface{}{"name": "cm", "namespace": "ns"}, + }}, + }}, + }}, + }, + }, + }, + } +} + +func newCODClient(t *testing.T, objs ...client.Object) client.Client { + t.Helper() + return testutil.WithIndexes(t, fake.NewClientBuilder().WithScheme(newScheme(t)), controllers.SetupIndexes). + WithStatusSubresource(&ocv1.ClusterObjectDeployment{}, &ocv1.ClusterObjectSet{}). + WithObjects(objs...). + Build() +} + +func reconcileCOD(t *testing.T, c client.Client) (ctrl.Result, error) { + t.Helper() + return (&controllers.ClusterObjectDeploymentReconciler{Client: c, Scheme: newScheme(t)}). + Reconcile(t.Context(), ctrl.Request{NamespacedName: types.NamespacedName{Name: clusterObjectDeploymentName}}) +} + +func getCOS(t *testing.T, c client.Client, name string) (*ocv1.ClusterObjectSet, bool) { + t.Helper() + cos := &ocv1.ClusterObjectSet{} + err := c.Get(t.Context(), types.NamespacedName{Name: name}, cos) + if apierrors.IsNotFound(err) { + return nil, false + } + require.NoError(t, err) + return cos, true +} + +func progressing(t *testing.T, c client.Client) *metav1.Condition { + t.Helper() + cod := &ocv1.ClusterObjectDeployment{} + require.NoError(t, c.Get(t.Context(), types.NamespacedName{Name: clusterObjectDeploymentName}, cod)) + return apimeta.FindStatusCondition(cod.Status.Conditions, ocv1.ClusterObjectDeploymentTypeProgressing) +} + +func Test_ClusterObjectDeploymentReconciler_CreatesFirstRevision(t *testing.T) { + c := newCODClient(t, codFixture()) + + _, err := reconcileCOD(t, c) + require.NoError(t, err) + + cos, ok := getCOS(t, c, clusterObjectDeploymentName+"-1") + require.True(t, ok, "revision 1 should be created") + require.Equal(t, clusterObjectDeploymentName, cos.Spec.Group) + require.Equal(t, int64(1), cos.Spec.Revision) + require.Equal(t, ocv1.ClusterObjectSetLifecycleStateActive, cos.Spec.LifecycleState) + require.NotEmpty(t, cos.Labels[labels.TemplateHashKey]) + + ref := metav1.GetControllerOf(cos) + require.NotNil(t, ref) + require.Equal(t, ocv1.ClusterObjectDeploymentKind, ref.Kind) + require.Equal(t, clusterObjectDeploymentName, ref.Name) + + cond := progressing(t, c) + require.NotNil(t, cond) + require.Equal(t, metav1.ConditionTrue, cond.Status) + require.Equal(t, ocv1.ClusterObjectDeploymentReasonNewClusterObjectSetProgressing, cond.Reason) +} +func Test_ClusterObjectDeploymentReconciler_NoNewRevisionWhenTemplateUnchanged(t *testing.T) { for _, tc := range []struct { - name string - existingObjs []client.Object + name string + completed bool + wantStatus metav1.ConditionStatus + wantReason string }{ { - name: "returns no error when the ClusterObjectDeployment does not exist", - existingObjs: nil, + name: "completed latest revision reports Progressed", + completed: true, + wantStatus: metav1.ConditionFalse, + wantReason: ocv1.ClusterObjectDeploymentReasonNewClusterObjectSetProgressed, }, { - name: "returns no error for an existing ClusterObjectDeployment", - existingObjs: []client.Object{ - &ocv1.ClusterObjectDeployment{ - ObjectMeta: metav1.ObjectMeta{Name: clusterObjectDeploymentName}, - }, - }, + name: "not-yet-completed latest revision reports Progressing", + completed: false, + wantStatus: metav1.ConditionTrue, + wantReason: ocv1.ClusterObjectDeploymentReasonNewClusterObjectSetProgressing, }, } { t.Run(tc.name, func(t *testing.T) { - testClient := fake.NewClientBuilder(). - WithScheme(testScheme). - WithStatusSubresource(&ocv1.ClusterObjectDeployment{}). - WithObjects(tc.existingObjs...). - Build() - - result, err := (&controllers.ClusterObjectDeploymentReconciler{ - Client: testClient, - }).Reconcile(t.Context(), ctrl.Request{ - NamespacedName: types.NamespacedName{ - Name: clusterObjectDeploymentName, - }, - }) + c := newCODClient(t, codFixture()) + + // First reconcile creates revision 1 with the correct template-hash label. + _, err := reconcileCOD(t, c) + require.NoError(t, err) + + if tc.completed { + cos, ok := getCOS(t, c, clusterObjectDeploymentName+"-1") + require.True(t, ok) + cos.Status.CompletedAt = metav1.Now() + require.NoError(t, c.Status().Update(t.Context(), cos)) + } + // Second reconcile must not create a new revision. + _, err = reconcileCOD(t, c) require.NoError(t, err) - require.Equal(t, ctrl.Result{}, result) + + _, ok := getCOS(t, c, clusterObjectDeploymentName+"-2") + require.False(t, ok, "no second revision should be created when the template is unchanged") + + cond := progressing(t, c) + require.NotNil(t, cond) + require.Equal(t, tc.wantStatus, cond.Status) + require.Equal(t, tc.wantReason, cond.Reason) }) } } + +func Test_ClusterObjectDeploymentReconciler_CreatesNewRevisionOnTemplateChange(t *testing.T) { + c := newCODClient(t, codFixture()) + + _, err := reconcileCOD(t, c) + require.NoError(t, err) + + // Change the template. + cod := &ocv1.ClusterObjectDeployment{} + require.NoError(t, c.Get(t.Context(), types.NamespacedName{Name: clusterObjectDeploymentName}, cod)) + cod.Spec.Template.Spec.Phases[0].Objects[0].Object.Object["data"] = map[string]interface{}{"k": "v"} + require.NoError(t, c.Update(t.Context(), cod)) + + _, err = reconcileCOD(t, c) + require.NoError(t, err) + + cos, ok := getCOS(t, c, clusterObjectDeploymentName+"-2") + require.True(t, ok, "a template change should create revision 2") + require.Equal(t, int64(2), cos.Spec.Revision) + + cond := progressing(t, c) + require.NotNil(t, cond) + require.Equal(t, metav1.ConditionTrue, cond.Status) +} + +func Test_ClusterObjectDeploymentReconciler_CreatesNewRevisionWhenLatestArchived(t *testing.T) { + c := newCODClient(t, codFixture()) + + _, err := reconcileCOD(t, c) + require.NoError(t, err) + + // Archive revision 1. + cos, ok := getCOS(t, c, clusterObjectDeploymentName+"-1") + require.True(t, ok) + cos.Spec.LifecycleState = ocv1.ClusterObjectSetLifecycleStateArchived + require.NoError(t, c.Update(t.Context(), cos)) + + _, err = reconcileCOD(t, c) + require.NoError(t, err) + + rev2, ok := getCOS(t, c, clusterObjectDeploymentName+"-2") + require.True(t, ok, "archiving the only revision should trigger a new revision") + require.Equal(t, int64(2), rev2.Spec.Revision) +} + +func Test_ClusterObjectDeploymentReconciler_DeletionIsNoop(t *testing.T) { + cod := codFixture() + now := metav1.Now() + cod.DeletionTimestamp = &now + cod.Finalizers = []string{"test.only/keep"} + + c := newCODClient(t, cod) + + _, err := reconcileCOD(t, c) + require.NoError(t, err) + + _, ok := getCOS(t, c, clusterObjectDeploymentName+"-1") + require.False(t, ok, "a deleting ClusterObjectDeployment must not create revisions") + + require.Nil(t, progressing(t, c)) +} + +func Test_ClusterObjectDeploymentReconciler_SurfacesAlreadyExists(t *testing.T) { + base := testutil.WithIndexes(t, fake.NewClientBuilder().WithScheme(newScheme(t)), controllers.SetupIndexes). + WithStatusSubresource(&ocv1.ClusterObjectDeployment{}, &ocv1.ClusterObjectSet{}). + WithObjects(codFixture()). + Build() + + c := interceptor.NewClient(base, interceptor.Funcs{ + Create: func(_ context.Context, _ client.WithWatch, obj client.Object, _ ...client.CreateOption) error { + gr := ocv1.GroupVersion.WithResource("clusterobjectsets").GroupResource() + return apierrors.NewAlreadyExists(gr, obj.GetName()) + }, + }) + + _, err := (&controllers.ClusterObjectDeploymentReconciler{Client: c, Scheme: newScheme(t)}). + Reconcile(t.Context(), ctrl.Request{NamespacedName: types.NamespacedName{Name: clusterObjectDeploymentName}}) + require.Error(t, err, "an AlreadyExists on create must surface as a reconcile error, not be silently tolerated") +} diff --git a/internal/object-controller/controllers/clusterobjectset_controller.go b/internal/object-controller/controllers/clusterobjectset_controller.go index 89174880ff..6354f35abe 100644 --- a/internal/object-controller/controllers/clusterobjectset_controller.go +++ b/internal/object-controller/controllers/clusterobjectset_controller.go @@ -441,17 +441,6 @@ type Sourcoser interface { Source(handler handler.EventHandler, predicates ...predicate.Predicate) source.Source } -// SetupIndexes registers the cache indexes used to look up ClusterObjectSet revisions. -func SetupIndexes(ctx context.Context, indexer client.FieldIndexer) error { - return indexer.IndexField(ctx, &ocv1.ClusterObjectSet{}, ".spec.group", func(obj client.Object) []string { - cos, ok := obj.(*ocv1.ClusterObjectSet) - if !ok || cos.Spec.Group == "" { - return nil - } - return []string{cos.Spec.Group} - }) -} - func (c *ClusterObjectSetReconciler) SetupWithManager(mgr ctrl.Manager) error { c.Clock = clock.RealClock{} return ctrl.NewControllerManagedBy(mgr). @@ -556,7 +545,7 @@ func (c *ClusterObjectSetReconciler) listOtherActiveRevisions( predicate func(*ocv1.ClusterObjectSet) bool, ) ([]*ocv1.ClusterObjectSet, error) { revList := &ocv1.ClusterObjectSetList{} - if err := c.Client.List(ctx, revList, client.MatchingFields{".spec.group": cos.Spec.Group}); err != nil { + if err := c.Client.List(ctx, revList, client.MatchingFields{clusterObjectSetGroupIndex: cos.Spec.Group}); err != nil { return nil, fmt.Errorf("listing revisions: %w", err) } diff --git a/internal/object-controller/controllers/indexes.go b/internal/object-controller/controllers/indexes.go new file mode 100644 index 0000000000..45c97a7161 --- /dev/null +++ b/internal/object-controller/controllers/indexes.go @@ -0,0 +1,30 @@ +//go:build !standard + +package controllers + +import ( + "context" + + "sigs.k8s.io/controller-runtime/pkg/client" + + ocv1 "github.com/operator-framework/operator-controller/api/v1" +) + +// clusterObjectSetGroupIndex is the field index on ClusterObjectSet spec.group. It +// is registered once per manager by SetupIndexes and used to look up all revisions +// in a group. +const clusterObjectSetGroupIndex = ".spec.group" + +// SetupIndexes registers the cache indexes used to look up ClusterObjectSet +// revisions by group. Both the ClusterObjectSet and ClusterObjectDeployment +// controllers rely on the spec.group index, so it is registered once per manager +// rather than per controller. +func SetupIndexes(ctx context.Context, indexer client.FieldIndexer) error { + return indexer.IndexField(ctx, &ocv1.ClusterObjectSet{}, clusterObjectSetGroupIndex, func(obj client.Object) []string { + cos, ok := obj.(*ocv1.ClusterObjectSet) + if !ok || cos.Spec.Group == "" { + return nil + } + return []string{cos.Spec.Group} + }) +} diff --git a/internal/shared/labels/labels.go b/internal/shared/labels/labels.go index d3d9a2d89d..05420566ae 100644 --- a/internal/shared/labels/labels.go +++ b/internal/shared/labels/labels.go @@ -17,6 +17,13 @@ const ( // resource responsible for creating or managing a ClusterObjectSet. OwnerNameKey = "olm.operatorframework.io/owner-name" + // TemplateHashKey is the label key used to record a digest of the + // ClusterObjectDeployment template that produced a ClusterObjectSet + // revision. The ClusterObjectDeployment controller compares the latest + // revision's digest to the current template to decide whether a new + // revision is required. + TemplateHashKey = "olm.operatorframework.io/template-hash" + // PackageNameKey is the label key used to record the package name // associated with a ClusterObjectSet. PackageNameKey = "olm.operatorframework.io/package-name" diff --git a/test/e2e/features/cod_rollout.feature b/test/e2e/features/cod_rollout.feature new file mode 100644 index 0000000000..fb3dad53ed --- /dev/null +++ b/test/e2e/features/cod_rollout.feature @@ -0,0 +1,97 @@ +@BoxcutterRuntime +Feature: ClusterObjectDeployment revision rollout + + As an OLM user applying a ClusterObjectDeployment, I want the controller to roll + out my template as an immutable ClusterObjectSet revision, create a new revision + when the template changes, and report rollout progress on the Progressing + condition. + + Background: + Given OLM is available + + Scenario: ClusterObjectDeployment rolls out its initial revision + Given namespace "${TEST_NAMESPACE}" is available + When resource is applied + """ + apiVersion: olm.operatorframework.io/v1 + kind: ClusterObjectDeployment + metadata: + name: ${NAME} + spec: + template: + spec: + collisionProtection: Prevent + phases: + - name: resources + objects: + - object: + apiVersion: v1 + kind: ConfigMap + metadata: + name: cod-cm + namespace: ${TEST_NAMESPACE} + data: + k: v + """ + Then ClusterObjectSet "${NAME}-1" has group "${NAME}" + And ClusterObjectSet "${NAME}-1" reports Ready as True with Reason AllObjectsReady + And ClusterObjectDeployment "${NAME}" reports Progressing as False with Reason NewClusterObjectSetProgressed + + Scenario: Updating the template creates a new revision + Given namespace "${TEST_NAMESPACE}" is available + When resource is applied + """ + apiVersion: olm.operatorframework.io/v1 + kind: ClusterObjectDeployment + metadata: + name: ${NAME} + spec: + template: + spec: + collisionProtection: Prevent + phases: + - name: resources + objects: + - object: + apiVersion: v1 + kind: ConfigMap + metadata: + name: cod-cm + namespace: ${TEST_NAMESPACE} + data: + k: v + """ + And ClusterObjectSet "${NAME}-1" reports Ready as True with Reason AllObjectsReady + And resource is applied + """ + apiVersion: olm.operatorframework.io/v1 + kind: ClusterObjectDeployment + metadata: + name: ${NAME} + spec: + template: + spec: + collisionProtection: Prevent + phases: + - name: resources + objects: + - object: + apiVersion: v1 + kind: ConfigMap + metadata: + name: cod-cm + namespace: ${TEST_NAMESPACE} + data: + k: v + - object: + apiVersion: v1 + kind: ConfigMap + metadata: + name: cod-cm-2 + namespace: ${TEST_NAMESPACE} + data: + k: v + """ + Then ClusterObjectSet "${NAME}-2" has group "${NAME}" + And ClusterObjectSet "${NAME}-2" reports Ready as True with Reason AllObjectsReady + And ClusterObjectDeployment "${NAME}" reports Progressing as False with Reason NewClusterObjectSetProgressed diff --git a/test/e2e/steps/steps.go b/test/e2e/steps/steps.go index 271e88bed7..e3b23f5571 100644 --- a/test/e2e/steps/steps.go +++ b/test/e2e/steps/steps.go @@ -127,6 +127,8 @@ func RegisterSteps(sc *godog.ScenarioContext) { sc.Step(`^(?i)ClusterObjectSet "([^"]+)" reports ([[:alnum:]]+) as ([[:alnum:]]+) with Reason ([[:alnum:]]+)$`, ClusterObjectSetReportsConditionWithoutMsg) sc.Step(`^(?i)ClusterObjectSet "([^"]+)" reports ([[:alnum:]]+) as ([[:alnum:]]+) with Reason ([[:alnum:]]+) and Message:$`, ClusterObjectSetReportsConditionWithMsg) sc.Step(`^(?i)ClusterObjectSet "([^"]+)" reports ([[:alnum:]]+) as ([[:alnum:]]+) with Reason ([[:alnum:]]+) and Message includes:$`, ClusterObjectSetReportsConditionWithMessageFragment) + + sc.Step(`^(?i)ClusterObjectDeployment "([^"]+)" reports ([[:alnum:]]+) as ([[:alnum:]]+) with Reason ([[:alnum:]]+)$`, ClusterObjectDeploymentReportsConditionWithoutMsg) sc.Step(`^(?i)ClusterExtension reports ([[:alnum:]]+) transition between (\d+) and (\d+) minutes since its creation$`, ClusterExtensionReportsConditionTransitionTime) sc.Step(`^(?i)ClusterObjectSet is applied(?:\s+.*)?$`, ResourceIsApplied) sc.Step(`^(?i)ClusterObjectSet with Namespace owner "([^"]*)" is applied$`, ClusterObjectSetWithNamespaceOwnerApplied) @@ -1009,6 +1011,12 @@ func ClusterObjectSetReportsConditionWithMessageFragment(ctx context.Context, re return waitForCondition(ctx, "clusterobjectset", substituteScenarioVars(revisionName, scenarioCtx(ctx)), conditionType, conditionStatus, &conditionReason, messageFragmentComparison(ctx, msgFragment)) } +// ClusterObjectDeploymentReportsConditionWithoutMsg waits for the named ClusterObjectDeployment to have a +// condition matching type, status, and reason. Polls with timeout. +func ClusterObjectDeploymentReportsConditionWithoutMsg(ctx context.Context, name, conditionType, conditionStatus, conditionReason string) error { + return waitForCondition(ctx, "clusterobjectdeployment", substituteScenarioVars(name, scenarioCtx(ctx)), conditionType, conditionStatus, &conditionReason, nil) +} + // TriggerClusterObjectSetReconciliation annotates the named ClusterObjectSet // to trigger a new reconciliation cycle. func TriggerClusterObjectSetReconciliation(ctx context.Context, cosName string) error {