diff --git a/changelog/fragments/helm-observed-generation.yaml b/changelog/fragments/helm-observed-generation.yaml new file mode 100644 index 00000000000..8966e3db6b9 --- /dev/null +++ b/changelog/fragments/helm-observed-generation.yaml @@ -0,0 +1,8 @@ +entries: + - description: > + For Helm-based operators, the custom resource status now records + `observedGeneration`, the `metadata.generation` that its conditions and + `deployedRelease` describe, so clients can tell whether the status + reflects the current spec. + kind: addition + breaking: false diff --git a/internal/helm/controller/reconcile.go b/internal/helm/controller/reconcile.go index ec9a77ef90e..cf26f654203 100644 --- a/internal/helm/controller/reconcile.go +++ b/internal/helm/controller/reconcile.go @@ -106,6 +106,8 @@ func (r HelmOperatorReconciler) Reconcile(ctx context.Context, request reconcile status := types.StatusFor(o) originalStatus := types.StatusFor(o.DeepCopy()) + // Set first so every status write records it, even when the release is unchanged. + status.ObservedGeneration = o.GetGeneration() log = log.WithValues("release", manager.ReleaseName()) reconcileResult := reconcile.Result{RequeueAfter: r.ReconcilePeriod} diff --git a/internal/helm/controller/reconcile_test.go b/internal/helm/controller/reconcile_test.go index 4700a2e5924..eddd5d83556 100644 --- a/internal/helm/controller/reconcile_test.go +++ b/internal/helm/controller/reconcile_test.go @@ -15,11 +15,25 @@ package controller import ( + "context" + "errors" "testing" "time" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + rpb "helm.sh/helm/v3/pkg/release" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/tools/record" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" + "sigs.k8s.io/controller-runtime/pkg/reconcile" + + helmtypes "github.com/operator-framework/operator-sdk/internal/helm/internal/types" + "github.com/operator-framework/operator-sdk/internal/helm/release" ) func TestDetermineReconcilePeriod(t *testing.T) { @@ -251,3 +265,165 @@ func Test_readBoolAnnotationWithDefault(t *testing.T) { }) } } + +var testGVK = schema.GroupVersionKind{Group: "example.com", Version: "v1", Kind: "App"} + +func TestReconcileSetsObservedGeneration(t *testing.T) { + deployed := &rpb.Release{Name: "app", Version: 2, Manifest: "kind: ConfigMap\n"} + + tests := []struct { + name string + status map[string]any + manager *fakeManager + wantErr bool + wantCondition helmtypes.HelmAppConditionType + }{ + { + name: "Should record the generation on install", + manager: &fakeManager{installed: &rpb.Release{Name: "app", Version: 1, Manifest: "kind: ConfigMap\n"}}, + wantCondition: helmtypes.ConditionDeployed, + }, + { + name: "Should record the generation on upgrade", + status: deployedStatus(t, 1, deployed), + manager: &fakeManager{isInstalled: true, upgradeRequired: true, upgraded: deployed}, + wantCondition: helmtypes.ConditionDeployed, + }, + { + // The release is unchanged, so only the generation differs from + // the stored status. + name: "Should record the generation when the release is unchanged", + status: deployedStatus(t, 1, deployed), + manager: &fakeManager{isInstalled: true, reconciled: deployed}, + wantCondition: helmtypes.ConditionDeployed, + }, + { + name: "Should record the generation when sync fails", + status: deployedStatus(t, 1, deployed), + manager: &fakeManager{isInstalled: true, syncErr: errors.New("sync failed")}, + wantErr: true, + wantCondition: helmtypes.ConditionIrreconcilable, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + obj := newApp(2, tc.status) + c := fake.NewClientBuilder().WithObjects(obj).WithStatusSubresource(obj). + WithInterceptorFuncs(interceptor.Funcs{SubResourceUpdate: typedStatusToMap}).Build() + r := HelmOperatorReconciler{ + Client: c, + EventRecorder: record.NewFakeRecorder(10), + GVK: testGVK, + ManagerFactory: fakeManagerFactory{tc.manager}, + } + + _, err := r.Reconcile(context.Background(), reconcile.Request{NamespacedName: types.NamespacedName{Namespace: "ns", Name: "app"}}) + if tc.wantErr { + require.Error(t, err) + } else { + require.NoError(t, err) + } + + got := &unstructured.Unstructured{} + got.SetGroupVersionKind(testGVK) + require.NoError(t, c.Get(context.Background(), client.ObjectKeyFromObject(obj), got)) + status := helmtypes.StatusFor(got) + assert.Equal(t, got.GetGeneration(), status.ObservedGeneration) + assert.True(t, hasCondition(status, tc.wantCondition), "missing condition %s", tc.wantCondition) + }) + } +} + +// typedStatusToMap converts the typed status that updateResourceStatus stores +// in the unstructured object into a map, as JSON serialization does for a real +// API server. The fake client cannot deep copy the typed value. +func typedStatusToMap(ctx context.Context, c client.Client, subResource string, obj client.Object, opts ...client.SubResourceUpdateOption) error { + if u, ok := obj.(*unstructured.Unstructured); ok { + if s, ok := u.Object["status"].(*helmtypes.HelmAppStatus); ok { + m, err := s.ToMap() + if err != nil { + return err + } + u.Object["status"] = m + } + } + return c.SubResource(subResource).Update(ctx, obj, opts...) +} + +func newApp(generation int64, status map[string]any) *unstructured.Unstructured { + obj := &unstructured.Unstructured{Object: map[string]any{"spec": map[string]any{"replicas": int64(1)}}} + obj.SetGroupVersionKind(testGVK) + obj.SetNamespace("ns") + obj.SetName("app") + obj.SetGeneration(generation) + obj.SetFinalizers([]string{uninstallFinalizer}) + if status != nil { + obj.Object["status"] = status + } + return obj +} + +func deployedStatus(t *testing.T, generation int64, rel *rpb.Release) map[string]any { + t.Helper() + status := &helmtypes.HelmAppStatus{ + Conditions: []helmtypes.HelmAppCondition{ + {Type: helmtypes.ConditionInitialized, Status: helmtypes.StatusTrue}, + {Type: helmtypes.ConditionDeployed, Status: helmtypes.StatusTrue, Reason: helmtypes.ReasonUpgradeSuccessful}, + }, + DeployedRelease: &helmtypes.HelmAppRelease{Name: rel.Name, Manifest: rel.Manifest}, + ObservedGeneration: generation, + } + m, err := status.ToMap() + require.NoError(t, err) + return m +} + +func hasCondition(status *helmtypes.HelmAppStatus, ct helmtypes.HelmAppConditionType) bool { + for _, c := range status.Conditions { + if c.Type == ct && c.Status == helmtypes.StatusTrue { + return true + } + } + return false +} + +type fakeManagerFactory struct { + manager *fakeManager +} + +func (f fakeManagerFactory) NewManager(*unstructured.Unstructured, map[string]string, string) (release.Manager, error) { + return f.manager, nil +} + +type fakeManager struct { + isInstalled bool + upgradeRequired bool + syncErr error + installed *rpb.Release + upgraded *rpb.Release + reconciled *rpb.Release +} + +func (m *fakeManager) ReleaseName() string { return "app" } +func (m *fakeManager) IsInstalled() bool { return m.isInstalled } +func (m *fakeManager) IsUpgradeRequired() bool { return m.upgradeRequired } +func (m *fakeManager) Sync() error { return m.syncErr } +func (m *fakeManager) RollBack(...release.RollBackOption) error { return nil } +func (m *fakeManager) CleanupRelease(string) (bool, error) { return true, nil } + +func (m *fakeManager) InstallRelease(...release.InstallOption) (*rpb.Release, error) { + return m.installed, nil +} + +func (m *fakeManager) UpgradeRelease(...release.UpgradeOption) (*rpb.Release, *rpb.Release, error) { + return m.upgraded, m.upgraded, nil +} + +func (m *fakeManager) ReconcileRelease(context.Context) (*rpb.Release, error) { + return m.reconciled, nil +} + +func (m *fakeManager) UninstallRelease(...release.UninstallOption) (*rpb.Release, error) { + return nil, nil +} diff --git a/internal/helm/internal/types/types.go b/internal/helm/internal/types/types.go index a1ac6d63ba6..4cf6a585200 100644 --- a/internal/helm/internal/types/types.go +++ b/internal/helm/internal/types/types.go @@ -75,8 +75,9 @@ const ( ) type HelmAppStatus struct { - Conditions []HelmAppCondition `json:"conditions"` - DeployedRelease *HelmAppRelease `json:"deployedRelease,omitempty"` + Conditions []HelmAppCondition `json:"conditions"` + DeployedRelease *HelmAppRelease `json:"deployedRelease,omitempty"` + ObservedGeneration int64 `json:"observedGeneration,omitempty"` } func (s *HelmAppStatus) ToMap() (map[string]any, error) {