diff --git a/cmd/capi-installer/main.go b/cmd/capi-installer/main.go index a9cf024ca..47fb3731f 100644 --- a/cmd/capi-installer/main.go +++ b/cmd/capi-installer/main.go @@ -42,6 +42,7 @@ import ( "github.com/openshift/cluster-capi-operator/pkg/controllers" "github.com/openshift/cluster-capi-operator/pkg/controllers/installer" "github.com/openshift/cluster-capi-operator/pkg/controllers/revision" + "github.com/openshift/cluster-capi-operator/pkg/manifesttransformer" "github.com/openshift/cluster-capi-operator/pkg/providerimages" "github.com/openshift/cluster-capi-operator/pkg/util" ) @@ -130,16 +131,23 @@ func setupControllers(ctx context.Context, mgr ctrl.Manager, operatorConfig comm log.Info("loaded provider profile", "name", profile.Name, "imageRef", profile.ImageRef, "profile", profile.Profile) } + transformers := []manifesttransformer.ManifestTransformer{ + manifesttransformer.NewEnvsubstTransformer(nil), + manifesttransformer.NewManagedByTransformer(), + &manifesttransformer.AdoptExistingTransformer{}, + } + if err := (&revision.RevisionController{ Client: mgr.GetClient(), ProviderProfiles: currentReleaseProfiles, ReleaseVersion: util.GetReleaseVersion(), + Transformers: transformers, }).SetupWithManager(mgr, operatorConfig.TLSOptions); err != nil { log.Error(err, "unable to create revision controller", "controller", "RevisionController") return fmt.Errorf("unable to create revision controller: %w", err) } - if err := installer.SetupWithManager(mgr, allProviderProfiles); err != nil { + if err := installer.SetupWithManager(mgr, allProviderProfiles, transformers); err != nil { return fmt.Errorf("unable to create installer controller: %w", err) } diff --git a/pkg/controllers/installer/boxcutter.go b/pkg/controllers/installer/boxcutter.go index 0e2693902..94f55c8b2 100644 --- a/pkg/controllers/installer/boxcutter.go +++ b/pkg/controllers/installer/boxcutter.go @@ -17,136 +17,146 @@ limitations under the License. package installer import ( + "context" + "errors" + "fmt" + "slices" + + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime/schema" "pkg.package-operator.run/boxcutter" "pkg.package-operator.run/boxcutter/probing" "sigs.k8s.io/controller-runtime/pkg/client" + "github.com/openshift/cluster-capi-operator/pkg/manifesttransformer" "github.com/openshift/cluster-capi-operator/pkg/revisiongenerator" "github.com/openshift/cluster-capi-operator/pkg/util" ) -func toBoxcutterRevision(installerRevision revisiongenerator.InstallerRevision) boxcutter.Revision { - return boxcutterRevision{revision: installerRevision} -} - -// boxcutterRevision wraps an InstallerRevision and provides a boxcutter.Revision implementation. -type boxcutterRevision struct { - revision revisiongenerator.InstallerRevision -} - -var _ boxcutter.Revision = boxcutterRevision{} - -// GetName returns the name of the revision. -func (r boxcutterRevision) GetName() string { - return string(r.revision.RevisionName()) -} - -// GetRevisionNumber returns the revision number of the revision. -func (r boxcutterRevision) GetRevisionNumber() int64 { - return r.revision.RevisionIndex() +func toClientObject(obj *unstructured.Unstructured) client.Object { + return obj } -// GetPhases returns the phases of the revision. -func (r boxcutterRevision) GetPhases() []boxcutter.Phase { +// toBoxcutterRevision converts an InstallerRevision to a boxcutter.Revision. +// Each ManifestTransformer is called for every object before phase construction. +func toBoxcutterRevision(ctx context.Context, installerRevision revisiongenerator.InstallerRevision, transformers []manifesttransformer.ManifestTransformer, collectObjects func(obj *unstructured.Unstructured)) (boxcutter.Revision, error) { probeOpts := util.SliceMap(allProbes(), func(p *probing.GroupKindSelector) boxcutter.PhaseReconcileOption { return boxcutter.WithProbe(boxcutter.ProgressProbeType, p) }) var phases []boxcutter.Phase - for _, component := range r.revision.Components() { - if crds := component.CRDs(); len(crds) > 0 { - objects, adoptOpts := processAdoptExistingAnnotations(crds) - phases = append(phases, boxcutterPhase{ - name: component.Name() + "-crds", - objects: objects, - reconcileOptions: append(probeOpts, adoptOpts...), - }) + withRevision := func(t manifesttransformer.ManifestTransformer) manifesttransformer.ManifestTransformer { + return t.WithRevision(ctx, installerRevision) + } + revisionTransformers := util.SliceMap(transformers, withRevision) + + var allErrs []error + + for _, component := range installerRevision.Components() { + withComponent := func(t manifesttransformer.ManifestTransformer) manifesttransformer.ManifestTransformer { + return t.WithComponent(ctx, component) + } + componentTransformers := util.SliceMap(revisionTransformers, withComponent) + + var crds, objects []*unstructured.Unstructured + + for _, obj := range component.Objects() { + if collectObjects != nil { + collectObjects(obj) + } + + gvk := obj.GetObjectKind().GroupVersionKind() + if gvk.GroupKind() == (schema.GroupKind{Group: "apiextensions.k8s.io", Kind: "CustomResourceDefinition"}) { + crds = append(crds, obj) + } else { + objects = append(objects, obj) + } + } + + var err error + + if phases, err = addPhase(ctx, phases, probeOpts, component.Name()+"-crds", crds, componentTransformers); err != nil { + allErrs = append(allErrs, err) } - if objects := component.Objects(); len(objects) > 0 { - objects, adoptOpts := processAdoptExistingAnnotations(objects) - phases = append(phases, boxcutterPhase{ - name: component.Name(), - objects: objects, - reconcileOptions: append(probeOpts, adoptOpts...), - }) + if phases, err = addPhase(ctx, phases, probeOpts, component.Name(), objects, componentTransformers); err != nil { + allErrs = append(allErrs, err) } } - return phases + if len(allErrs) > 0 { + return nil, errors.Join(allErrs...) + } + + return boxcutter.NewRevision( + string(installerRevision.RevisionName()), + installerRevision.RevisionIndex(), + phases, + ), nil } -// processAdoptExistingAnnotations processes the adopt-existing annotation on -// each object. Objects with the annotation are deep copied and the annotation -// is stripped from the copy. Objects with "always" get a per-object -// CollisionProtectionIfNoController option. Objects without the annotation are -// returned unchanged. -// -// This function assumes that annotation values have already been validated -// during revision creation. -func processAdoptExistingAnnotations(objects []client.Object) ([]client.Object, []boxcutter.PhaseReconcileOption) { - var reconcileOpts []boxcutter.PhaseReconcileOption - - return util.SliceMap(objects, func(obj client.Object) client.Object { - annotations := obj.GetAnnotations() - value, hasAnnotation := annotations[revisiongenerator.AdoptExistingAnnotation] - - if hasAnnotation { - // Disable collision protection if the annotation is set to "always" - if value == revisiongenerator.AdoptExistingAlways { - reconcileOpts = append(reconcileOpts, - boxcutter.WithObjectReconcileOptions(obj, - boxcutter.WithCollisionProtection(boxcutter.CollisionProtectionNone), - ), - ) - } +func addPhase(ctx context.Context, phases []boxcutter.Phase, probeOpts []boxcutter.PhaseReconcileOption, name string, objects []*unstructured.Unstructured, ctxTransformers []manifesttransformer.ManifestTransformer) ([]boxcutter.Phase, error) { + if len(objects) == 0 { + return phases, nil + } + + var ( + xfmrOpts []boxcutter.PhaseReconcileOption + allErrs []error + ) - // Strip the annotation from the object before returning it - obj = obj.DeepCopyObject().(client.Object) //nolint:forcetypeassert // This is guaranteed to be client.Object because obj is client.Object - annotationsCopy := obj.GetAnnotations() - delete(annotationsCopy, revisiongenerator.AdoptExistingAnnotation) - obj.SetAnnotations(annotationsCopy) + transformedObjects := make([]*unstructured.Unstructured, 0, len(objects)) + + for _, obj := range objects { + transformedObj, objOpts, objErrs := applyTransformers(ctx, ctxTransformers, obj) + if len(objErrs) > 0 { + allErrs = append(allErrs, fmt.Errorf("transforming %s %s: %w", obj.GroupVersionKind(), client.ObjectKeyFromObject(obj), errors.Join(objErrs...))) + continue } - return obj - }), reconcileOpts -} + // A nil object means a transformer chose to skip it; it must not appear in any phase. + if transformedObj == nil { + continue + } -// GetReconcileOptions returns the reconcile options of the revision. -func (r boxcutterRevision) GetReconcileOptions() []boxcutter.RevisionReconcileOption { - return nil -} + if len(objOpts) > 0 { + xfmrOpts = append(xfmrOpts, boxcutter.WithObjectReconcileOptions(transformedObj, objOpts...)) + } -// GetTeardownOptions returns the teardown options of the revision. -func (r boxcutterRevision) GetTeardownOptions() []boxcutter.RevisionTeardownOption { - return nil -} + transformedObjects = append(transformedObjects, transformedObj) + } + + allOpts := slices.Concat(probeOpts, xfmrOpts) + bcPhase := boxcutter.NewPhase(name, util.SliceMap(transformedObjects, toClientObject)).WithReconcileOptions(allOpts...) -type boxcutterPhase struct { - name string - objects []client.Object - reconcileOptions []boxcutter.PhaseReconcileOption + return append(phases, bcPhase), errors.Join(allErrs...) } -var _ boxcutter.Phase = boxcutterPhase{} +// applyTransformers applies all transformers to an object in order, accumulating +// all boxcutter reconcile options and errors they return. +func applyTransformers(ctx context.Context, transformers []manifesttransformer.ManifestTransformer, obj *unstructured.Unstructured) (*unstructured.Unstructured, []boxcutter.ObjectReconcileOption, []error) { + var ( + errs []error + allOpts []boxcutter.ObjectReconcileOption + ) + + for _, t := range transformers { + transformedObj, opts, err := t.TransformObject(ctx, obj) + if err != nil { + errs = append(errs, err) + continue + } -// GetName returns the name of the phase. -func (p boxcutterPhase) GetName() string { - return p.name -} + // If the transformer returns a nil object, it means the object should be skipped. + if transformedObj == nil { + return nil, opts, errs + } -// GetObjects returns the objects of the phase. -func (p boxcutterPhase) GetObjects() []client.Object { - return p.objects -} + allOpts = append(allOpts, opts...) -// GetReconcileOptions returns the reconcile options of the phase. -func (p boxcutterPhase) GetReconcileOptions() []boxcutter.PhaseReconcileOption { - return p.reconcileOptions -} + obj = transformedObj + } -// GetTeardownOptions returns the teardown options of the phase. -func (p boxcutterPhase) GetTeardownOptions() []boxcutter.PhaseTeardownOption { - return nil + return obj, allOpts, errs } diff --git a/pkg/controllers/installer/boxcutter_test.go b/pkg/controllers/installer/boxcutter_test.go new file mode 100644 index 000000000..a28e075ca --- /dev/null +++ b/pkg/controllers/installer/boxcutter_test.go @@ -0,0 +1,475 @@ +/* +Copyright 2026 Red Hat, Inc. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package installer + +import ( + "context" + "errors" + "fmt" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "pkg.package-operator.run/boxcutter" + "sigs.k8s.io/controller-runtime/pkg/client" + + "github.com/openshift/cluster-capi-operator/pkg/manifesttransformer" + "github.com/openshift/cluster-capi-operator/pkg/revisiongenerator" + "github.com/openshift/cluster-capi-operator/pkg/util" +) + +// noopCollector is a collectObjects callback that does nothing, for tests that +// don't care about collected objects. +func noopCollector(*unstructured.Unstructured) {} + +// objectRef returns a stable identifier for an object, for asserting object +// identity without depending on object ordering. +func objectRef(kind, name string) string { + return kind + "/" + name +} + +// findPhase returns the phase with the given name, failing the test if none is found. +func findPhase(phases []boxcutter.Phase, name string) boxcutter.Phase { + GinkgoHelper() + + for _, phase := range phases { + if phase.GetName() == name { + return phase + } + } + + Fail(fmt.Sprintf("phase %q not found", name)) + + return nil +} + +// objectKinds returns the Kind of each object, for asserting phase contents +// without depending on object ordering. +func objectKinds(objs []client.Object) []string { + return util.SliceMap(objs, func(obj client.Object) string { + return obj.GetObjectKind().GroupVersionKind().Kind + }) +} + +// stubTransformer is a test double for manifesttransformer.ManifestTransformer. +type stubTransformer struct { + opts []boxcutter.ObjectReconcileOption + err error + validateErr error +} + +func (s *stubTransformer) TransformObject(_ context.Context, obj *unstructured.Unstructured) (*unstructured.Unstructured, []boxcutter.ObjectReconcileOption, error) { + return obj, s.opts, s.err +} + +func (s *stubTransformer) Validate(_ *unstructured.Unstructured) error { + return s.validateErr +} + +func (s *stubTransformer) WithRevision(_ context.Context, _ revisiongenerator.ParsedRevision) manifesttransformer.ManifestTransformer { + return s +} + +func (s *stubTransformer) WithComponent(_ context.Context, _ revisiongenerator.ParsedComponent) manifesttransformer.ManifestTransformer { + return s +} + +var _ manifesttransformer.ManifestTransformer = &stubTransformer{} + +// fnTransformer adapts a plain function to the manifesttransformer.ManifestTransformer +// interface, letting tests express skip/mutate/observe behaviour inline. +type fnTransformer struct { + fn func(obj *unstructured.Unstructured) (*unstructured.Unstructured, []boxcutter.ObjectReconcileOption, error) +} + +func (f *fnTransformer) TransformObject(_ context.Context, obj *unstructured.Unstructured) (*unstructured.Unstructured, []boxcutter.ObjectReconcileOption, error) { + return f.fn(obj) +} + +func (f *fnTransformer) Validate(_ *unstructured.Unstructured) error { + return nil +} + +func (f *fnTransformer) WithRevision(_ context.Context, _ revisiongenerator.ParsedRevision) manifesttransformer.ManifestTransformer { + return f +} + +func (f *fnTransformer) WithComponent(_ context.Context, _ revisiongenerator.ParsedComponent) manifesttransformer.ManifestTransformer { + return f +} + +var _ manifesttransformer.ManifestTransformer = &fnTransformer{} + +// installerRevisionFromProfiles builds a bare InstallerRevision from the named +// provider profiles without writing anything to the cluster. +func installerRevisionFromProfiles(names ...string) revisiongenerator.InstallerRevision { + GinkgoHelper() + + profiles := lookupProfiles(names...) + parsed, err := revisiongenerator.NewParsedRevision(profiles) + Expect(err).NotTo(HaveOccurred(), "NewParsedRevision should not fail for valid profiles") + + rev, err := parsed.ForInstall("4.18.0-test", 1) + Expect(err).NotTo(HaveOccurred(), "ForInstall should not fail for a valid parsed revision") + + return rev +} + +var _ = Describe("toBoxcutterRevision", func() { + Describe("construction", func() { + It("should return a Revision with the name of the InstallerRevision", func() { + rev := installerRevisionFromProfiles(providerCore) + + bcRev, err := toBoxcutterRevision(context.Background(), rev, nil, noopCollector) + Expect(err).NotTo(HaveOccurred()) + + Expect(bcRev.GetName()).To(Equal(string(rev.RevisionName())), + "returned Revision should carry the same name as the InstallerRevision") + }) + }) + + Describe("GetPhases idempotency", func() { + DescribeTable("should return stable phases on every call", + func(providerName string, wantPhaseCount int) { + rev := installerRevisionFromProfiles(providerName) + + bcRev, err := toBoxcutterRevision(context.Background(), rev, nil, noopCollector) + Expect(err).NotTo(HaveOccurred()) + + first := bcRev.GetPhases() + second := bcRev.GetPhases() + + Expect(second).To(HaveLen(wantPhaseCount), + "expected %d phase(s) for provider %q", wantPhaseCount, providerName) + + for i := range first { + Expect(second[i].GetName()).To(Equal(first[i].GetName()), + "phase[%d] name must be stable across GetPhases calls", i) + Expect(second[i].GetObjects()).To(HaveLen(len(first[i].GetObjects())), + "phase[%d] object count must be stable across GetPhases calls", i) + } + }, + Entry("objects only — one objects phase", providerCore, 1), + Entry("CRDs only — one CRD phase", providerCRD, 1), + Entry("CRDs and objects — two phases", providerMixed, 2), + Entry("adopt-existing annotation is stable across calls", providerAdoptExisting, 1), + ) + }) + + Describe("CRD splitting", func() { + It("splits a component with CRDs and objects into a '-crds' phase and an objects phase", func() { + rev := installerRevisionFromProfiles(providerMixed) + + bcRev, err := toBoxcutterRevision(context.Background(), rev, nil, noopCollector) + Expect(err).NotTo(HaveOccurred()) + + phases := bcRev.GetPhases() + Expect(phases).To(HaveLen(2)) + + crdPhase := findPhase(phases, providerMixed+"-crds") + Expect(objectKinds(crdPhase.GetObjects())).To(ConsistOf("CustomResourceDefinition"), + "the '-crds' phase should contain only CRDs") + + objectsPhase := findPhase(phases, providerMixed) + Expect(objectKinds(objectsPhase.GetObjects())).To(ConsistOf("ConfigMap"), + "the base phase should contain only non-CRD objects") + }) + + It("does not create a '-crds' phase for a component with no CRDs", func() { + rev := installerRevisionFromProfiles(providerCore) + + bcRev, err := toBoxcutterRevision(context.Background(), rev, nil, noopCollector) + Expect(err).NotTo(HaveOccurred()) + + phases := bcRev.GetPhases() + Expect(phases).To(HaveLen(1)) + Expect(phases[0].GetName()).To(Equal(providerCore)) + Expect(objectKinds(phases[0].GetObjects())).To(ConsistOf("ConfigMap")) + }) + + It("does not create a plain objects phase for a component with only CRDs", func() { + rev := installerRevisionFromProfiles(providerCRD) + + bcRev, err := toBoxcutterRevision(context.Background(), rev, nil, noopCollector) + Expect(err).NotTo(HaveOccurred()) + + phases := bcRev.GetPhases() + Expect(phases).To(HaveLen(1)) + Expect(phases[0].GetName()).To(Equal(providerCRD + "-crds")) + Expect(objectKinds(phases[0].GetObjects())).To(ConsistOf("CustomResourceDefinition")) + }) + }) + + Describe("collectObjects callback", func() { + testWidgetCRDName := fmt.Sprintf("testwidgets.%s", testCRDGVK.Group) + testGadgetCRDName := fmt.Sprintf("testgadgets.%s", mixedCRDGVK.Group) + + DescribeTable("is called once for every object in every component", + func(wantRefs []string, providerNames ...string) { + rev := installerRevisionFromProfiles(providerNames...) + + var collectedRefs []string + + collectObjects := func(obj *unstructured.Unstructured) { + collectedRefs = append(collectedRefs, objectRef(obj.GetKind(), obj.GetName())) + } + _, err := toBoxcutterRevision(context.Background(), rev, nil, collectObjects) + Expect(err).NotTo(HaveOccurred()) + + Expect(collectedRefs).To(ConsistOf(wantRefs), + "collectObjects should be called exactly once for every object that ends up in a phase") + }, + Entry("objects only", + []string{objectRef("ConfigMap", coreCMName)}, + providerCore), + Entry("CRDs only", + []string{objectRef("CustomResourceDefinition", testWidgetCRDName)}, + providerCRD), + Entry("CRDs and objects in the same component", + []string{ + objectRef("CustomResourceDefinition", testGadgetCRDName), + objectRef("ConfigMap", mixedCMName), + }, + providerMixed), + Entry("multiple components", + []string{ + objectRef("ConfigMap", coreCMName), + objectRef("CustomResourceDefinition", testGadgetCRDName), + objectRef("ConfigMap", mixedCMName), + objectRef("CustomResourceDefinition", testWidgetCRDName), + }, + providerCore, providerMixed, providerCRD), + ) + }) + + Describe("transformer integration", func() { + It("should return an error when a transformer fails", func() { + stub := &stubTransformer{err: errors.New("transform failed")} + rev := installerRevisionFromProfiles(providerCore) + + _, err := toBoxcutterRevision(context.Background(), rev, []manifesttransformer.ManifestTransformer{stub}, nil) + + Expect(err).To(MatchError(ContainSubstring("transform failed"))) + }) + + It("should include options returned by transformers in phase reconcile options", func() { + rev := installerRevisionFromProfiles(providerCore) + + base, err := toBoxcutterRevision(context.Background(), rev, nil, nil) + Expect(err).NotTo(HaveOccurred()) + + baseOptCount := len(base.GetPhases()[0].GetReconcileOptions()) + + stub := &stubTransformer{opts: []boxcutter.ObjectReconcileOption{ + boxcutter.WithCollisionProtection(boxcutter.CollisionProtectionNone), + }} + withTfm, err := toBoxcutterRevision(context.Background(), rev, []manifesttransformer.ManifestTransformer{stub}, nil) + Expect(err).NotTo(HaveOccurred()) + + Expect(len(withTfm.GetPhases()[0].GetReconcileOptions())).To( + BeNumerically(">", baseOptCount), + "transformer options should augment the phase reconcile options", + ) + }) + + It("should omit an object from its phase when a transformer returns a nil object", func() { + rev := installerRevisionFromProfiles(providerMixed) + + skipConfigMaps := &fnTransformer{fn: func(obj *unstructured.Unstructured) (*unstructured.Unstructured, []boxcutter.ObjectReconcileOption, error) { + if obj.GetKind() == "ConfigMap" { + return nil, nil, nil + } + + return obj, nil, nil + }} + + bcRev, err := toBoxcutterRevision(context.Background(), rev, []manifesttransformer.ManifestTransformer{skipConfigMaps}, nil) + Expect(err).NotTo(HaveOccurred()) + + phases := bcRev.GetPhases() + + crdPhase := findPhase(phases, providerMixed+"-crds") + Expect(objectKinds(crdPhase.GetObjects())).To(ConsistOf("CustomResourceDefinition"), + "objects not matched by the skip transformer should be unaffected") + + objectsPhase := findPhase(phases, providerMixed) + Expect(objectsPhase.GetObjects()).To(BeEmpty(), + "an object skipped by a transformer (nil return) must not appear in any phase") + }) + + It("should not invoke later transformers for an object a prior transformer already skipped", func() { + rev := installerRevisionFromProfiles(providerCore) + + skip := &fnTransformer{fn: func(*unstructured.Unstructured) (*unstructured.Unstructured, []boxcutter.ObjectReconcileOption, error) { + return nil, nil, nil + }} + + var secondCalled bool + + recordCall := &fnTransformer{fn: func(obj *unstructured.Unstructured) (*unstructured.Unstructured, []boxcutter.ObjectReconcileOption, error) { + secondCalled = true + return obj, nil, nil + }} + + _, err := toBoxcutterRevision(context.Background(), rev, []manifesttransformer.ManifestTransformer{skip, recordCall}, nil) + Expect(err).NotTo(HaveOccurred()) + + Expect(secondCalled).To(BeFalse(), + "a transformer must not run on an object a prior transformer already skipped") + }) + + It("should pass the output of one transformer as the input to the next", func() { + rev := installerRevisionFromProfiles(providerCore) + + const renamedTo = "renamed-by-first-transformer" + + rename := &fnTransformer{fn: func(obj *unstructured.Unstructured) (*unstructured.Unstructured, []boxcutter.ObjectReconcileOption, error) { + renamed := obj.DeepCopy() + renamed.SetName(renamedTo) + + return renamed, nil, nil + }} + + var sawName string + + record := &fnTransformer{fn: func(obj *unstructured.Unstructured) (*unstructured.Unstructured, []boxcutter.ObjectReconcileOption, error) { + sawName = obj.GetName() + return obj, nil, nil + }} + + bcRev, err := toBoxcutterRevision(context.Background(), rev, []manifesttransformer.ManifestTransformer{rename, record}, nil) + Expect(err).NotTo(HaveOccurred()) + + Expect(sawName).To(Equal(renamedTo), + "the second transformer should observe the first transformer's output, not the original object") + Expect(bcRev.GetPhases()[0].GetObjects()[0].GetName()).To(Equal(renamedTo), + "the final phase should contain the transformed object, not the original") + }) + + It("should apply transformers to CRDs as well as plain objects", func() { + rev := installerRevisionFromProfiles(providerCRD) + + var sawKind string + + record := &fnTransformer{fn: func(obj *unstructured.Unstructured) (*unstructured.Unstructured, []boxcutter.ObjectReconcileOption, error) { + sawKind = obj.GetKind() + return obj, nil, nil + }} + + _, err := toBoxcutterRevision(context.Background(), rev, []manifesttransformer.ManifestTransformer{record}, nil) + Expect(err).NotTo(HaveOccurred()) + + Expect(sawKind).To(Equal("CustomResourceDefinition"), "transformers must also run against CRD objects") + }) + + It("should include the failing object's identity in the returned error", func() { + rev := installerRevisionFromProfiles(providerCore) + stub := &stubTransformer{err: errors.New("boom")} + + _, err := toBoxcutterRevision(context.Background(), rev, []manifesttransformer.ManifestTransformer{stub}, nil) + + Expect(err).To(MatchError(ContainSubstring(coreCMName)), + "the error should identify which object failed transformation") + }) + + It("should invoke transformers exactly once per object, even if GetPhases is called multiple times", func() { + rev := installerRevisionFromProfiles(providerCore) + + var callCount int + + counting := &fnTransformer{fn: func(obj *unstructured.Unstructured) (*unstructured.Unstructured, []boxcutter.ObjectReconcileOption, error) { + callCount++ + return obj, nil, nil + }} + + bcRev, err := toBoxcutterRevision(context.Background(), rev, []manifesttransformer.ManifestTransformer{counting}, nil) + Expect(err).NotTo(HaveOccurred()) + + _ = bcRev.GetPhases() + _ = bcRev.GetPhases() + + Expect(callCount).To(Equal(1), + "transformation happens once during construction, not on every GetPhases call") + }) + }) + + Describe("error aggregation", func() { + It("aggregates errors from multiple objects in the same phase", func() { + // providerManyClusterScoped has ten ClusterRoles and no CRDs, so all + // of them are built into a single objects phase for the component. + rev := installerRevisionFromProfiles(providerManyClusterScoped) + stub := &stubTransformer{err: errors.New("boom")} + + _, err := toBoxcutterRevision(context.Background(), rev, []manifesttransformer.ManifestTransformer{stub}, nil) + + Expect(err).To(MatchError(SatisfyAll( + ContainSubstring("/test-cr-1"), + ContainSubstring("/test-cr-2"), + )), "expected failures from multiple objects within the same phase") + }) + + It("aggregates errors from multiple transformers on the same object", func() { + rev := installerRevisionFromProfiles(providerCore) + stubA := &stubTransformer{err: errors.New("first failure")} + stubB := &stubTransformer{err: errors.New("second failure")} + + _, err := toBoxcutterRevision(context.Background(), rev, + []manifesttransformer.ManifestTransformer{stubA, stubB}, nil) + + Expect(err).To(MatchError(SatisfyAll( + ContainSubstring("first failure"), + ContainSubstring("second failure"), + )), "expected both transformer failures for the object in a single joined error") + }) + + It("aggregates errors across the CRD and non-CRD phases of the same component", func() { + // providerMixed has one CRD and one ConfigMap, in the same component, + // but built into two separate phases (crds, objects). + rev := installerRevisionFromProfiles(providerMixed) + stub := &stubTransformer{err: errors.New("boom")} + + _, err := toBoxcutterRevision(context.Background(), rev, []manifesttransformer.ManifestTransformer{stub}, nil) + + Expect(err).To(MatchError(SatisfyAll( + ContainSubstring("/testgadgets.test.example.com"), + ContainSubstring("default/test-cm-mixed"), + )), "expected failures from both the CRD phase and the objects phase") + }) + + It("aggregates errors across multiple components rather than stopping at the first", func() { + rev := installerRevisionFromProfiles(providerCore, providerInfra) + stub := &stubTransformer{err: errors.New("boom")} + + _, err := toBoxcutterRevision(context.Background(), rev, []manifesttransformer.ManifestTransformer{stub}, nil) + + Expect(err).To(MatchError(SatisfyAll( + ContainSubstring("default/test-cm-core"), + ContainSubstring("default/test-cm-infra"), + )), "expected failures from both components, not just the first one processed") + }) + + It("does not build a Revision when any object fails transformation", func() { + rev := installerRevisionFromProfiles(providerCore, providerInfra) + stub := &stubTransformer{err: errors.New("boom")} + + bcRev, err := toBoxcutterRevision(context.Background(), rev, []manifesttransformer.ManifestTransformer{stub}, nil) + + Expect(err).To(HaveOccurred()) + Expect(bcRev).To(BeNil()) + }) + }) +}) diff --git a/pkg/controllers/installer/helpers_test.go b/pkg/controllers/installer/helpers_test.go index 53cac9265..7874ad8a2 100644 --- a/pkg/controllers/installer/helpers_test.go +++ b/pkg/controllers/installer/helpers_test.go @@ -262,8 +262,8 @@ func addRevision(ctx context.Context, providerNames ...string) operatorv1alpha1. By("Rendering new revision", func() { profiles := lookupProfiles(providerNames...) - // Render the revision to compute the correct content ID. - rendered, err := revisiongenerator.NewRenderedRevision(profiles) + // Parse the revision to compute the correct content ID. + parsed, err := revisiongenerator.NewParsedRevision(profiles) Expect(err).NotTo(HaveOccurred()) var revisionIndex int64 @@ -273,7 +273,7 @@ func addRevision(ctx context.Context, providerNames ...string) operatorv1alpha1. revisionIndex = latestRevision(clusterAPI.Status.Revisions).Revision + 1 } - installerRev, err := rendered.ForInstall("4.18.0-test", revisionIndex) + installerRev, err := parsed.ForInstall("4.18.0-test", revisionIndex) Expect(err).NotTo(HaveOccurred()) apiRev, err = installerRev.ToAPIRevision() diff --git a/pkg/controllers/installer/installer_controller.go b/pkg/controllers/installer/installer_controller.go index 047e5ca5a..60a3461ac 100644 --- a/pkg/controllers/installer/installer_controller.go +++ b/pkg/controllers/installer/installer_controller.go @@ -44,9 +44,9 @@ import ( "pkg.package-operator.run/boxcutter" "pkg.package-operator.run/boxcutter/managedcache" + "github.com/openshift/cluster-capi-operator/pkg/manifesttransformer" "github.com/openshift/cluster-capi-operator/pkg/operatorstatus" "github.com/openshift/cluster-capi-operator/pkg/providerimages" - "github.com/openshift/cluster-capi-operator/pkg/revisiongenerator" "github.com/openshift/cluster-capi-operator/pkg/util" ) @@ -65,12 +65,13 @@ type InstallerController struct { revisionEngine *boxcutter.RevisionEngine providerProfiles []providerimages.ProviderImageManifests restMapper meta.RESTMapper + transformers []manifesttransformer.ManifestTransformer } // SetupWithManager creates the boxcutter dependencies and sets up the installer // controller with the Manager. Additional sources may be provided to trigger // reconciliation from external events (e.g. a channel source for testing). -func SetupWithManager(mgr ctrl.Manager, providerProfiles []providerimages.ProviderImageManifests, additionalSources ...source.Source) error { +func SetupWithManager(mgr ctrl.Manager, providerProfiles []providerimages.ProviderImageManifests, transformers []manifesttransformer.ManifestTransformer, additionalSources ...source.Source) error { trackingCache, err := setupTrackingCache(mgr) if err != nil { return fmt.Errorf("unable to setup tracking cache: %w", err) @@ -87,6 +88,7 @@ func SetupWithManager(mgr ctrl.Manager, providerProfiles []providerimages.Provid revisionEngine: revisionEngine, providerProfiles: providerProfiles, restMapper: mgr.GetRESTMapper(), + transformers: transformers, } toClusterAPI := func(_ context.Context, _ client.Object) []reconcile.Request { @@ -137,7 +139,7 @@ func SetupWithManager(mgr ctrl.Manager, providerProfiles []providerimages.Provid func setupTrackingCache(mgr ctrl.Manager) (managedcache.TrackingCache, error) { // Configure cache to watch only objects with our label. The label is // applied by the revision generator. - managedByReq, err := labels.NewRequirement(revisiongenerator.ManagedLabelKey, selection.Exists, nil) + managedByReq, err := labels.NewRequirement(manifesttransformer.ManagedLabelKey, selection.Exists, nil) if err != nil { return nil, fmt.Errorf("creating managed-by label requirement: %w", err) } diff --git a/pkg/controllers/installer/installer_controller_test.go b/pkg/controllers/installer/installer_controller_test.go index 78bcfd9ec..b18dd55c0 100644 --- a/pkg/controllers/installer/installer_controller_test.go +++ b/pkg/controllers/installer/installer_controller_test.go @@ -30,6 +30,7 @@ import ( appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" rbacv1 "k8s.io/api/rbac/v1" + "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/reconcile" @@ -194,6 +195,51 @@ var _ = Describe("InstallerController", Serial, func() { }, defaultNodeTimeout) }) + Context("ManifestTransformer Drift", func() { + AfterEach(func() { + testAnnotationValue.Store(nil) + }) + + It("re-applies an updated transformer output without a new revision", func(ctx context.Context) { + testAnnotationValue.Store(ptr.To("v1")) + addRevisionAndWaitForSuccess(ctx, providerCore) + + cm, err := getConfigMap(ctx, coreCMName) + Expect(err).NotTo(HaveOccurred()) + Expect(cm.Annotations).To(HaveKeyWithValue(testAnnotationKey, "v1")) + + // Simulate an updated transformation -- no revision change at all. + testAnnotationValue.Store(ptr.To("v2")) + triggerReconcile() + + Eventually(kWithCtx(ctx).Object(cm)). + WithContext(ctx). + WithTimeout(defaultEventuallyTimeout). + Should(HaveField("Annotations", HaveKeyWithValue(testAnnotationKey, "v2"))) + }, defaultNodeTimeout) + + It("restores a transformer-added annotation after it is manually removed", func(ctx context.Context) { + testAnnotationValue.Store(ptr.To("v1")) + addRevisionAndWaitForSuccess(ctx, providerCore) + + cm, err := getConfigMap(ctx, coreCMName) + Expect(err).NotTo(HaveOccurred()) + Expect(cm.Annotations).To(HaveKeyWithValue(testAnnotationKey, "v1")) + + Eventually(kWithCtx(ctx).Update(cm, func() { + delete(cm.Annotations, testAnnotationKey) + })). + WithContext(ctx). + WithTimeout(defaultEventuallyTimeout). + Should(Succeed()) + + Eventually(kWithCtx(ctx).Object(cm)). + WithContext(ctx). + WithTimeout(defaultEventuallyTimeout). + Should(HaveField("Annotations", HaveKeyWithValue(testAnnotationKey, "v1"))) + }, defaultNodeTimeout) + }) + Context("Waiting States", func() { It("reports WaitingOnExternal when ClusterAPI has no revisions", func(ctx context.Context) { // ClusterAPI exists but has no revisions (created by createFixtures). diff --git a/pkg/controllers/installer/revision_reconciler.go b/pkg/controllers/installer/revision_reconciler.go index 4cb4a6ff2..581c54dd5 100644 --- a/pkg/controllers/installer/revision_reconciler.go +++ b/pkg/controllers/installer/revision_reconciler.go @@ -36,6 +36,7 @@ import ( machinerytypes "pkg.package-operator.run/boxcutter/machinery/types" "sigs.k8s.io/controller-runtime/pkg/reconcile" + "github.com/openshift/cluster-capi-operator/pkg/manifesttransformer" "github.com/openshift/cluster-capi-operator/pkg/revisiongenerator" "github.com/openshift/cluster-capi-operator/pkg/util" ) @@ -85,16 +86,6 @@ import ( // a collision with an existing object on the cluster. var errCollision = errors.New("collision with existing objects") -// convertedRevision holds a pre-converted InstallerRevision. -// If conversion fails, revision is nil and conversionErr is set. -// We need this because conversion errors are handled differently depending on -// whether the revision is being reconciled or torn down, and we don't know -// which in advance. -type convertedRevision struct { - revision revisiongenerator.InstallerRevision - conversionErr error -} - // collectedObjectRef holds intermediate object reference data collected during // revision rendering. The resource name is resolved later. type collectedObjectRef struct { @@ -122,53 +113,43 @@ func newRevisionReconciler(installerController *InstallerController, log logr.Lo } } -func (r *revisionReconciler) reconcile(ctx context.Context, revisions []operatorv1alpha1.ClusterAPIInstallerRevision) (*operatorv1alpha1.RevisionName, []string, []error) { +func (r *revisionReconciler) reconcile(ctx context.Context, apiRevisions []operatorv1alpha1.ClusterAPIInstallerRevision) (*operatorv1alpha1.RevisionName, []string, []error) { // Sort revisions descending by revision number (newest first) - revisions = slices.Clone(revisions) - slices.SortFunc(revisions, func(a, b operatorv1alpha1.ClusterAPIInstallerRevision) int { + apiRevisions = slices.Clone(apiRevisions) + cmpRevisions := func(a, b operatorv1alpha1.ClusterAPIInstallerRevision) int { return cmp.Compare(b.Revision, a.Revision) - }) + } + slices.SortFunc(apiRevisions, cmpRevisions) - revisionNames := util.SliceMap(revisions, func(rev operatorv1alpha1.ClusterAPIInstallerRevision) string { + getRevisionName := func(rev operatorv1alpha1.ClusterAPIInstallerRevision) string { return string(rev.Name) - }) - r.log.Info("Reconciling revisions", "revisions", strings.Join(revisionNames, ", ")) + } + revisionNames := util.SliceMap(apiRevisions, getRevisionName) - // Convert all API revisions upfront so that collectObjects (and thus - // relatedObjects) is fully populated before reconciliation begins. - converted := util.SliceMap(revisions, func(apiRev operatorv1alpha1.ClusterAPIInstallerRevision) convertedRevision { - rev, err := revisiongenerator.NewInstallerRevisionFromAPI(apiRev, r.providerProfiles, revisiongenerator.WithObjectCollectors(r.collectObjects)) - if err != nil { - err = fmt.Errorf("error creating installer revision from API revision %s: %w", apiRev.Name, reconcile.TerminalError(err)) - } + r.log.Info("Reconciling revisions", "revisions", strings.Join(revisionNames, ", ")) - return convertedRevision{ - revision: rev, - conversionErr: err, - } - }) + isComplete, messages, errs := r.reconcileRevisions(ctx, apiRevisions) // Resolve collected objects to final ObjectReferences with correct plurals if err := r.resolveCollectedObjects(); err != nil { - return nil, nil, []error{err} + errs = append(errs, err) } - isComplete, messages, errs := r.reconcileRevisions(ctx, converted) if isComplete { - name := converted[0].revision.RevisionName() + name := apiRevisions[0].Name return &name, messages, errs } return nil, messages, errs } -func (r *revisionReconciler) reconcileRevisions(ctx context.Context, revisions []convertedRevision) (bool, []string, []error) { - if len(revisions) == 0 { +func (r *revisionReconciler) reconcileRevisions(ctx context.Context, apiRevisions []operatorv1alpha1.ClusterAPIInstallerRevision) (bool, []string, []error) { + if len(apiRevisions) == 0 { return true, nil, nil } - head := revisions[0] - tail := revisions[1:] + head := apiRevisions[0] + tail := apiRevisions[1:] isComplete, messages, err := r.reconcileRevision(ctx, head) @@ -187,14 +168,25 @@ func (r *revisionReconciler) reconcileRevisions(ctx context.Context, revisions [ // * a summary message // * a boolean indicating if the revision was reconciled completely // * an error if any occurred. -func (r *revisionReconciler) reconcileRevision(ctx context.Context, conv convertedRevision) (bool, string, error) { - if conv.conversionErr != nil { - return false, "", conv.conversionErr +func (r *revisionReconciler) reconcileRevision(ctx context.Context, apiRevision operatorv1alpha1.ClusterAPIInstallerRevision) (bool, string, error) { + revision, err := revisiongenerator.NewInstallerRevisionFromAPI(apiRevision, r.providerProfiles) + if err != nil { + return false, "", fmt.Errorf("error creating installer revision from API revision %s: %w", apiRevision.Name, reconcile.TerminalError(err)) + } + + // Defence in depth: the revision controller validates transformers before + // writing a new revision, but a revision read back from the API could in + // principle be malformed (e.g. hand-edited), so validate again here. + if err := manifesttransformer.ValidateTransformers(r.transformers, revision); err != nil { + err = fmt.Errorf("validating revision %s: %w", revision.RevisionName(), reconcile.TerminalError(err)) + return false, err.Error(), err } - revision := conv.revision + bcRevision, err := toBoxcutterRevision(ctx, revision, r.transformers, r.collectObjects) + if err != nil { + return false, err.Error(), reconcile.TerminalError(fmt.Errorf("building boxcutter revision %s: %w", revision.RevisionName(), err)) + } - bcRevision := toBoxcutterRevision(revision) phases := bcRevision.GetPhases() totalObjects := 0 @@ -233,21 +225,19 @@ func (r *revisionReconciler) reconcileRevision(ctx context.Context, conv convert return true, fmt.Sprintf("Revision %s: complete", revision.RevisionName()), nil } - var message string + return false, waitingRevisionMessage(revision.RevisionName(), result), nil +} +// waitingRevisionMessage returns the message to use when a revision is still in progress. +func waitingRevisionMessage(revisionName operatorv1alpha1.RevisionName, result machinery.RevisionResult) string { for _, phase := range result.GetPhases() { if !phase.IsComplete() { - message = fmt.Sprintf("Revision %s: waiting on phase %s", revision.RevisionName(), phase.GetName()) - break + return fmt.Sprintf("Revision %s: waiting on phase %s", revisionName, phase.GetName()) } } - if message == "" { - // Probably shouldn't happen? - message = fmt.Sprintf("Revision %s: waiting for reconciliation", revision.RevisionName()) - } - - return false, message, nil + // Probably shouldn't happen? + return fmt.Sprintf("Revision %s: waiting for reconciliation", revisionName) } func (r *revisionReconciler) handlePhaseResults(revisionName operatorv1alpha1.RevisionName, result machinery.RevisionResult) error { @@ -337,13 +327,13 @@ func handlePhaseObject(log logr.Logger, obj machinery.ObjectResult, actionCounts return result } -func (r *revisionReconciler) teardownRevisions(ctx context.Context, revisions []convertedRevision) (bool, []string, []error) { - if len(revisions) == 0 { +func (r *revisionReconciler) teardownRevisions(ctx context.Context, apiRevisions []operatorv1alpha1.ClusterAPIInstallerRevision) (bool, []string, []error) { + if len(apiRevisions) == 0 { return true, nil, nil } - head := revisions[0] - tail := revisions[1:] + head := apiRevisions[0] + tail := apiRevisions[1:] return mergeWithTail(ctx, r.teardownRevisions, tail)(r.teardownRevision(ctx, head)) } @@ -352,16 +342,21 @@ func (r *revisionReconciler) teardownRevisions(ctx context.Context, revisions [] // * a summary message // * a boolean indicating if the revision was torn down completely // * an error if any occurred. -func (r *revisionReconciler) teardownRevision(ctx context.Context, conv convertedRevision) (bool, string, error) { - if conv.conversionErr != nil { +func (r *revisionReconciler) teardownRevision(ctx context.Context, apiRevision operatorv1alpha1.ClusterAPIInstallerRevision) (bool, string, error) { + revision, err := revisiongenerator.NewInstallerRevisionFromAPI(apiRevision, r.providerProfiles) + if err != nil { // We can't teardown this revision if we can't create it, so we consider it complete. - return true, "", conv.conversionErr + return true, "", fmt.Errorf("error creating installer revision from API revision %s: %w", apiRevision.Name, reconcile.TerminalError(err)) } - revision := conv.revision revisionName := revision.RevisionName() - bcRevision := toBoxcutterRevision(revision) + bcRevision, err := toBoxcutterRevision(ctx, revision, r.transformers, r.collectObjects) + if err != nil { + // Cannot tear down a revision that cannot be constructed — treat as complete. + return true, "", err + } + phases := bcRevision.GetPhases() totalObjects := 0 @@ -432,10 +427,10 @@ func (r *revisionReconciler) logTeardownPhaseResults(revisionName operatorv1alph } } -type revisionHandler func(context.Context, []convertedRevision) (bool, []string, []error) +type revisionHandler func(context.Context, []operatorv1alpha1.ClusterAPIInstallerRevision) (bool, []string, []error) // mergeWithTail merges the results of the head revision with the results of calling the tail handler on the tail revisions. -func mergeWithTail(ctx context.Context, tailHandler revisionHandler, tailRevisions []convertedRevision) func(bool, string, error) (bool, []string, []error) { +func mergeWithTail(ctx context.Context, tailHandler revisionHandler, tailRevisions []operatorv1alpha1.ClusterAPIInstallerRevision) func(bool, string, error) (bool, []string, []error) { return func(headComplete bool, headMessage string, headErr error) (bool, []string, []error) { tailComplete, tailMessages, tailErrs := tailHandler(ctx, tailRevisions) @@ -451,7 +446,7 @@ func mergeWithTail(ctx context.Context, tailHandler revisionHandler, tailRevisio } } -func (r *revisionReconciler) collectObjects(obj unstructured.Unstructured) { +func (r *revisionReconciler) collectObjects(obj *unstructured.Unstructured) { gvk := obj.GroupVersionKind() r.gvks.Insert(gvk) diff --git a/pkg/controllers/installer/revision_reconciler_test.go b/pkg/controllers/installer/revision_reconciler_test.go index 5560e950b..2ce13b5d0 100644 --- a/pkg/controllers/installer/revision_reconciler_test.go +++ b/pkg/controllers/installer/revision_reconciler_test.go @@ -17,20 +17,41 @@ limitations under the License. package installer import ( + "context" "errors" "fmt" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" configv1 "github.com/openshift/api/config/v1" + operatorv1alpha1 "github.com/openshift/api/operator/v1alpha1" "k8s.io/apimachinery/pkg/api/meta" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/util/sets" "sigs.k8s.io/controller-runtime/pkg/reconcile" + "github.com/openshift/cluster-capi-operator/pkg/manifesttransformer" + "github.com/openshift/cluster-capi-operator/pkg/revisiongenerator" "github.com/openshift/cluster-capi-operator/pkg/test" ) +// apiRevisionFromProfiles parses the named provider profiles into an API +// revision, without requiring an envtest client. +func apiRevisionFromProfiles(names ...string) operatorv1alpha1.ClusterAPIInstallerRevision { + GinkgoHelper() + + parsed, err := revisiongenerator.NewParsedRevision(lookupProfiles(names...)) + Expect(err).NotTo(HaveOccurred()) + + installerRev, err := parsed.ForInstall("4.18.0-test", 1) + Expect(err).NotTo(HaveOccurred()) + + apiRev, err := installerRev.ToAPIRevision() + Expect(err).NotTo(HaveOccurred()) + + return apiRev +} + // errorInjectingRESTMapper wraps a real RESTMapper and injects errors for specific GVKs. // This allows testing error handling while using a real RESTMapper for normal cases. type errorInjectingRESTMapper struct { @@ -176,3 +197,24 @@ var _ = Describe("revisionReconciler.resolveCollectedObjects", func() { })).To(BeTrue()) }) }) + +var _ = Describe("revisionReconciler.reconcileRevision", func() { + It("rejects the revision with a terminal error when a transformer fails validation", func() { + apiRev := apiRevisionFromProfiles(providerCore) + + validateErr := errors.New("boom") + failingTransformer := &stubTransformer{validateErr: validateErr} + + r := newRevisionReconciler(&InstallerController{ + providerProfiles: lookupProfiles(providerCore), + transformers: []manifesttransformer.ManifestTransformer{failingTransformer}, + }, test.NewVerboseGinkgoLogger(4)) + + isComplete, _, err := r.reconcileRevision(context.Background(), apiRev) + + Expect(isComplete).To(BeFalse()) + Expect(err).To(HaveOccurred()) + Expect(errors.Is(err, validateErr)).To(BeTrue()) + Expect(errors.Is(err, reconcile.TerminalError(nil))).To(BeTrue(), "expected terminal error") + }) +}) diff --git a/pkg/controllers/installer/suite_test.go b/pkg/controllers/installer/suite_test.go index fffaebaab..f5ef7d852 100644 --- a/pkg/controllers/installer/suite_test.go +++ b/pkg/controllers/installer/suite_test.go @@ -37,6 +37,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/reconcile" "sigs.k8s.io/controller-runtime/pkg/source" + "github.com/openshift/cluster-capi-operator/pkg/manifesttransformer" "github.com/openshift/cluster-capi-operator/pkg/test" ) @@ -106,7 +107,13 @@ var _ = BeforeSuite(func() { handler.EnqueueRequestsFromMapFunc(toClusterAPI), ) - Expect(SetupWithManager(mgr, allProviderProfiles, triggerSource)).To(Succeed()) + transformers := []manifesttransformer.ManifestTransformer{ + manifesttransformer.NewEnvsubstTransformer(nil), + manifesttransformer.NewManagedByTransformer(), + &manifesttransformer.AdoptExistingTransformer{}, + testValueTransformer{}, + } + Expect(SetupWithManager(mgr, allProviderProfiles, transformers, triggerSource)).To(Succeed()) Expect(test.AddNamespaceFinalizerCleanup(mgr)).To(Succeed()) // Start manager in background. diff --git a/pkg/controllers/installer/testvalue_transformer_test.go b/pkg/controllers/installer/testvalue_transformer_test.go new file mode 100644 index 000000000..d7086fbef --- /dev/null +++ b/pkg/controllers/installer/testvalue_transformer_test.go @@ -0,0 +1,80 @@ +/* +Copyright 2026 Red Hat, Inc. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package installer + +import ( + "context" + "sync/atomic" + + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "pkg.package-operator.run/boxcutter" + + "github.com/openshift/cluster-capi-operator/pkg/manifesttransformer" + "github.com/openshift/cluster-capi-operator/pkg/revisiongenerator" +) + +// testAnnotationKey is the annotation testValueTransformer writes. +const testAnnotationKey = "test.openshift.io/transformer-value" + +// testAnnotationValue is set/cleared directly by tests to control what +// testValueTransformer stamps onto objects on the next reconcile. nil means +// "do nothing" -- the no-op path already exercised by every other test in +// the suite, which never touches this variable. +var testAnnotationValue atomic.Pointer[string] + +// testValueTransformer stamps obj with whatever testAnnotationValue +// currently holds. It exists purely to let tests simulate a ManifestTransformer +// whose output changes between reconciles without a new revision, so drift +// correction of transformer-derived state can be verified directly. +type testValueTransformer struct{} + +var _ manifesttransformer.ManifestTransformer = testValueTransformer{} + +// TransformObject implements manifesttransformer.ManifestTransformer. +func (testValueTransformer) TransformObject(_ context.Context, obj *unstructured.Unstructured) (*unstructured.Unstructured, []boxcutter.ObjectReconcileOption, error) { + val := testAnnotationValue.Load() + if val == nil { + return obj, nil, nil + } + + obj = obj.DeepCopy() + + annotations := obj.GetAnnotations() + if annotations == nil { + annotations = map[string]string{} + } + + annotations[testAnnotationKey] = *val + obj.SetAnnotations(annotations) + + return obj, nil, nil +} + +// Validate implements manifesttransformer.ManifestTransformer. +func (testValueTransformer) Validate(_ *unstructured.Unstructured) error { return nil } + +// WithRevision implements manifesttransformer.ManifestTransformer. It is a +// no-op; testValueTransformer does not need revision context. +func (t testValueTransformer) WithRevision(_ context.Context, _ revisiongenerator.RenderedRevision) manifesttransformer.ManifestTransformer { + return t +} + +// WithComponent implements manifesttransformer.ManifestTransformer. It is a +// no-op; testValueTransformer does not need component context. +func (t testValueTransformer) WithComponent(_ context.Context, _ revisiongenerator.RenderedComponent) manifesttransformer.ManifestTransformer { + return t +} diff --git a/pkg/controllers/revision/helpers_test.go b/pkg/controllers/revision/helpers_test.go index fc8c3e6e3..bd0c0dd03 100644 --- a/pkg/controllers/revision/helpers_test.go +++ b/pkg/controllers/revision/helpers_test.go @@ -19,20 +19,25 @@ package revision import ( "context" "crypto/tls" + "errors" "slices" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" "github.com/onsi/gomega/types" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/utils/ptr" + "pkg.package-operator.run/boxcutter" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" ctrlconfig "sigs.k8s.io/controller-runtime/pkg/config" configv1 "github.com/openshift/api/config/v1" operatorv1alpha1 "github.com/openshift/api/operator/v1alpha1" + "github.com/openshift/cluster-capi-operator/pkg/manifesttransformer" "github.com/openshift/cluster-capi-operator/pkg/providerimages" + "github.com/openshift/cluster-capi-operator/pkg/revisiongenerator" "github.com/openshift/cluster-capi-operator/pkg/test" ) @@ -69,10 +74,12 @@ func newManagerWrapper(providerImgs []providerimages.ProviderImageManifests, tls } } + adoptExisting := &manifesttransformer.AdoptExistingTransformer{} err = (&RevisionController{ Client: mgr.GetClient(), ProviderProfiles: imgs, ReleaseVersion: "4.18.0", + Transformers: []manifesttransformer.ManifestTransformer{adoptExisting}, }).SetupWithManager(mgr, tlsOptions) Expect(err).NotTo(HaveOccurred()) @@ -192,3 +199,42 @@ func latestRevision(revisions []operatorv1alpha1.ClusterAPIInstallerRevision) op return latest } + +// stubTransformer is a test double for manifesttransformer.ManifestTransformer. +type stubTransformer struct { + validateErr error +} + +func (s *stubTransformer) TransformObject(_ context.Context, obj *unstructured.Unstructured) (*unstructured.Unstructured, []boxcutter.ObjectReconcileOption, error) { + return obj, nil, nil +} + +func (s *stubTransformer) Validate(_ *unstructured.Unstructured) error { + return s.validateErr +} + +func (s *stubTransformer) WithRevision(_ context.Context, _ revisiongenerator.ParsedRevision) manifesttransformer.ManifestTransformer { + return s +} + +func (s *stubTransformer) WithComponent(_ context.Context, _ revisiongenerator.ParsedComponent) manifesttransformer.ManifestTransformer { + return s +} + +var _ manifesttransformer.ManifestTransformer = &stubTransformer{} + +// fakeRevision implements revisiongenerator.ParsedRevision for unit tests. +type fakeRevision struct { + components []revisiongenerator.ParsedComponent +} + +func (f *fakeRevision) ContentID() (string, error) { return "fake-content-id", nil } +func (f *fakeRevision) Components() []revisiongenerator.ParsedComponent { + return f.components +} +func (f *fakeRevision) ForInstall(string, int64) (revisiongenerator.InstallerRevision, error) { + return nil, errors.New("not implemented") +} +func (f *fakeRevision) ManifestSubstitutions() map[string]string { return nil } + +var _ revisiongenerator.ParsedRevision = &fakeRevision{} diff --git a/pkg/controllers/revision/revision_controller.go b/pkg/controllers/revision/revision_controller.go index a2afbe5b2..9a41985ee 100644 --- a/pkg/controllers/revision/revision_controller.go +++ b/pkg/controllers/revision/revision_controller.go @@ -39,6 +39,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/predicate" "sigs.k8s.io/controller-runtime/pkg/reconcile" + "github.com/openshift/cluster-capi-operator/pkg/manifesttransformer" "github.com/openshift/cluster-capi-operator/pkg/operatorstatus" "github.com/openshift/cluster-capi-operator/pkg/providerimages" "github.com/openshift/cluster-capi-operator/pkg/revisiongenerator" @@ -65,6 +66,7 @@ type RevisionController struct { client.Client ProviderProfiles []providerimages.ProviderImageManifests ReleaseVersion string + Transformers []manifesttransformer.ManifestTransformer // manifestSubstitutions is derived from TLSProfileSpec during SetupWithManager. manifestSubstitutions map[string]string @@ -134,7 +136,7 @@ func (r *RevisionController) reconcile(ctx context.Context, log logr.Logger) ope return opresult.Success() } -func (r *RevisionController) generateDesiredRevision(ctx context.Context) (revisiongenerator.RenderedRevision, *operatorstatus.ReconcileResult) { +func (r *RevisionController) generateDesiredRevision(ctx context.Context) (revisiongenerator.ParsedRevision, *operatorstatus.ReconcileResult) { infra := &configv1.Infrastructure{} if err := r.Get(ctx, client.ObjectKey{Name: infrastructureName}, infra); err != nil { return nil, opresult.ErrorP(fmt.Errorf("fetching infrastructure: %w", err)) @@ -147,15 +149,19 @@ func (r *RevisionController) generateDesiredRevision(ctx context.Context) (revis // Build ordered component list from provider metadata providerComponents := r.buildComponentList(infra.Status.PlatformStatus.Type) - revision, err := revisiongenerator.NewRenderedRevision(providerComponents, revisiongenerator.WithManifestSubstitutions(r.manifestSubstitutions)) + revision, err := revisiongenerator.NewParsedRevision(providerComponents, revisiongenerator.WithManifestSubstitutions(r.manifestSubstitutions)) if err != nil { - return nil, opresult.ErrorP(fmt.Errorf("error creating rendered revision: %w", err)) + return nil, opresult.ErrorP(fmt.Errorf("error creating parsed revision: %w", err)) + } + + if err := manifesttransformer.ValidateTransformers(r.Transformers, revision); err != nil { + return nil, opresult.NonRetryableErrorP(fmt.Errorf("transformer validation failed: %w", err)) } return revision, nil } -func (r *RevisionController) mergeRevisions(log logr.Logger, apiRevisions []operatorv1alpha1.ClusterAPIInstallerRevision, desiredRevision revisiongenerator.RenderedRevision) ([]operatorv1alpha1.ClusterAPIInstallerRevision, error) { +func (r *RevisionController) mergeRevisions(log logr.Logger, apiRevisions []operatorv1alpha1.ClusterAPIInstallerRevision, desiredRevision revisiongenerator.ParsedRevision) ([]operatorv1alpha1.ClusterAPIInstallerRevision, error) { // If there's no current revision we have nothing to merge if desiredRevision == nil { return apiRevisions, nil @@ -275,8 +281,9 @@ func (r *RevisionController) SetupWithManager(mgr ctrl.Manager, tlsOptions []fun } r.manifestSubstitutions = map[string]string{ - "TLS_MIN_VERSION": libgocrypto.TLSVersionToNameOrDie(tlsCfg.MinVersion), - "TLS_CIPHER_SUITES": strings.Join(util.SliceMap(tlsCfg.CipherSuites, tls.CipherSuiteName), ","), + "EXP_BOOTSTRAP_FORMAT_IGNITION": "true", + "TLS_MIN_VERSION": libgocrypto.TLSVersionToNameOrDie(tlsCfg.MinVersion), + "TLS_CIPHER_SUITES": strings.Join(util.SliceMap(tlsCfg.CipherSuites, tls.CipherSuiteName), ","), } isInfrastructureReady := func(obj client.Object) bool { diff --git a/pkg/controllers/revision/revision_controller_test.go b/pkg/controllers/revision/revision_controller_test.go index c303e0e19..9345a0319 100644 --- a/pkg/controllers/revision/revision_controller_test.go +++ b/pkg/controllers/revision/revision_controller_test.go @@ -31,6 +31,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client/interceptor" "sigs.k8s.io/controller-runtime/pkg/reconcile" + "github.com/openshift/cluster-capi-operator/pkg/manifesttransformer" "github.com/openshift/cluster-capi-operator/pkg/operatorstatus" "github.com/openshift/cluster-capi-operator/pkg/providerimages" "github.com/openshift/cluster-capi-operator/pkg/test" @@ -520,11 +521,13 @@ var _ = Describe("RevisionController manifest substitutions", Serial, func() { Expect(updatedClusterAPI.Status.Revisions).To(HaveLen(1)) rev := updatedClusterAPI.Status.Revisions[0] - Expect(rev.ManifestSubstitutions).To(HaveLen(2)) - Expect(rev.ManifestSubstitutions[0].Key).To(Equal("TLS_CIPHER_SUITES")) - Expect(*rev.ManifestSubstitutions[0].Value).To(Equal("TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256")) - Expect(rev.ManifestSubstitutions[1].Key).To(Equal("TLS_MIN_VERSION")) - Expect(*rev.ManifestSubstitutions[1].Value).To(Equal("VersionTLS12")) + Expect(rev.ManifestSubstitutions).To(HaveLen(3)) + Expect(rev.ManifestSubstitutions[0].Key).To(Equal("EXP_BOOTSTRAP_FORMAT_IGNITION")) + Expect(*rev.ManifestSubstitutions[0].Value).To(Equal("true")) + Expect(rev.ManifestSubstitutions[1].Key).To(Equal("TLS_CIPHER_SUITES")) + Expect(*rev.ManifestSubstitutions[1].Value).To(Equal("TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256")) + Expect(rev.ManifestSubstitutions[2].Key).To(Equal("TLS_MIN_VERSION")) + Expect(*rev.ManifestSubstitutions[2].Value).To(Equal("VersionTLS12")) }, defaultNodeTimeout) }) @@ -574,4 +577,28 @@ var _ = Describe("RevisionController error handling", Serial, func() { WithReason(operatorstatus.ReasonEphemeralError). WithMessage(ContainSubstring(testErr.Error()))) }, defaultNodeTimeout) + + It("sets NonRetryableError when a transformer Validate fails", func(ctx context.Context) { + stub := &stubTransformer{validateErr: errors.New("invalid manifest")} + r := &RevisionController{ + Client: cl, + ProviderProfiles: defaultProviderImgs, + ReleaseVersion: "4.18.0", + Transformers: []manifesttransformer.ManifestTransformer{stub}, + } + + _, _ = r.Reconcile(ctx, reconcile.Request{NamespacedName: client.ObjectKey{Name: "cluster"}}) + + co := &configv1.ClusterOperator{} + Expect(cl.Get(ctx, client.ObjectKey{Name: "cluster-api"}, co)).To(Succeed()) + Expect(co.Status.Conditions).To(SatisfyAll( + test.HaveCondition(conditionTypeProgressing). + WithStatus(configv1.ConditionFalse). + WithReason(operatorstatus.ReasonNonRetryableError), + test.HaveCondition(conditionTypeAvailable). + WithStatus(configv1.ConditionFalse). + WithReason(operatorstatus.ReasonNonRetryableError). + WithMessage(ContainSubstring("invalid manifest")), + )) + }, defaultNodeTimeout) }) diff --git a/pkg/manifesttransformer/adopt_existing_transformer.go b/pkg/manifesttransformer/adopt_existing_transformer.go new file mode 100644 index 000000000..c90280419 --- /dev/null +++ b/pkg/manifesttransformer/adopt_existing_transformer.go @@ -0,0 +1,109 @@ +/* +Copyright 2026 Red Hat, Inc. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package manifesttransformer + +import ( + "context" + "errors" + "fmt" + + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "pkg.package-operator.run/boxcutter" + "sigs.k8s.io/controller-runtime/pkg/reconcile" + + "github.com/openshift/cluster-capi-operator/pkg/revisiongenerator" +) + +// ErrInvalidAdoptExistingAnnotation is returned by Validate when an object +// carries an adopt-existing annotation with an unrecognised value. +var ErrInvalidAdoptExistingAnnotation = errors.New("invalid annotation value") + +// AdoptExistingTransformer implements ManifestTransformer for the adopt-existing +// annotation. It strips the annotation from each object and, for objects +// annotated with "always", returns an object-level CollisionProtectionNone +// option so that boxcutter adopts pre-existing cluster resources instead of +// reporting a collision. It also validates that any annotation value is +// recognised, returning an error that wraps ErrInvalidAdoptExistingAnnotation. +type AdoptExistingTransformer struct{} + +var _ ManifestTransformer = &AdoptExistingTransformer{} + +// TransformObject returns a copy of obj with the adopt-existing annotation +// stripped, along with a CollisionProtectionNone option when the annotation +// value is "always". Objects without the annotation are returned unchanged. +func (a *AdoptExistingTransformer) TransformObject(_ context.Context, obj *unstructured.Unstructured) (*unstructured.Unstructured, []boxcutter.ObjectReconcileOption, error) { + annotations := obj.GetAnnotations() + + value, hasAnnotation := annotations[revisiongenerator.AdoptExistingAnnotation] + if !hasAnnotation { + return obj, nil, nil + } + + // Strip the annotation from a copy before the object is applied to the cluster. + obj = obj.DeepCopy() + annotations = obj.GetAnnotations() + delete(annotations, revisiongenerator.AdoptExistingAnnotation) + obj.SetAnnotations(annotations) + + if value == revisiongenerator.AdoptExistingAlways { + return obj, []boxcutter.ObjectReconcileOption{ + boxcutter.WithCollisionProtection(boxcutter.CollisionProtectionNone), + }, nil + } + + return obj, nil, nil +} + +// Validate returns an error wrapping ErrInvalidAdoptExistingAnnotation when +// the object carries an adopt-existing annotation with an unrecognised value. +func (a *AdoptExistingTransformer) Validate(obj *unstructured.Unstructured) error { + annotations := obj.GetAnnotations() + if len(annotations) == 0 { + return nil + } + + value, exists := annotations[revisiongenerator.AdoptExistingAnnotation] + if !exists { + return nil + } + + switch value { + case revisiongenerator.AdoptExistingAlways, revisiongenerator.AdoptExistingNever: + return nil + default: + return fmt.Errorf("%w: %s=%q on %s %s/%s", + reconcile.TerminalError(ErrInvalidAdoptExistingAnnotation), + revisiongenerator.AdoptExistingAnnotation, + value, + obj.GetObjectKind().GroupVersionKind().Kind, + obj.GetNamespace(), + obj.GetName(), + ) + } +} + +// WithRevision implements ManifestTransformer. AdoptExistingTransformer does +// not need revision context. +func (a *AdoptExistingTransformer) WithRevision(_ context.Context, _ revisiongenerator.ParsedRevision) ManifestTransformer { + return a +} + +// WithComponent implements ManifestTransformer. AdoptExistingTransformer does +// not need component context. +func (a *AdoptExistingTransformer) WithComponent(_ context.Context, _ revisiongenerator.ParsedComponent) ManifestTransformer { + return a +} diff --git a/pkg/manifesttransformer/adopt_existing_transformer_test.go b/pkg/manifesttransformer/adopt_existing_transformer_test.go new file mode 100644 index 000000000..c83a4f497 --- /dev/null +++ b/pkg/manifesttransformer/adopt_existing_transformer_test.go @@ -0,0 +1,120 @@ +/* +Copyright 2026 Red Hat, Inc. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package manifesttransformer + +import ( + "context" + "errors" + "testing" + + . "github.com/onsi/gomega" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + + "github.com/openshift/cluster-capi-operator/pkg/revisiongenerator" +) + +func adoptTestObject(name string, annotations map[string]string) *unstructured.Unstructured { + obj := &unstructured.Unstructured{} + obj.SetName(name) + + if len(annotations) > 0 { + obj.SetAnnotations(annotations) + } + + return obj +} + +func TestAdoptExistingTransformer_TransformObject(t *testing.T) { + ctx := context.Background() + transformer := &AdoptExistingTransformer{} + + t.Run("object without annotation returns unchanged with nil options", func(t *testing.T) { + g := NewWithT(t) + obj := adoptTestObject("no-annotation", nil) + + transformed, opts, err := transformer.TransformObject(ctx, obj) + + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(opts).To(BeNil()) + g.Expect(transformed).To(BeIdenticalTo(obj)) + }) + + t.Run("object with always: annotation stripped and CollisionProtectionNone option returned", func(t *testing.T) { + g := NewWithT(t) + obj := adoptTestObject("adopt-always", map[string]string{ + revisiongenerator.AdoptExistingAnnotation: revisiongenerator.AdoptExistingAlways, + }) + + transformed, opts, err := transformer.TransformObject(ctx, obj) + + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(opts).To(HaveLen(1)) + g.Expect(transformed.GetAnnotations()).NotTo(HaveKey(revisiongenerator.AdoptExistingAnnotation)) + + // The original object must not be mutated. + g.Expect(obj.GetAnnotations()).To(HaveKey(revisiongenerator.AdoptExistingAnnotation)) + }) + + t.Run("object with never: annotation stripped and nil options returned", func(t *testing.T) { + g := NewWithT(t) + obj := adoptTestObject("adopt-never", map[string]string{ + revisiongenerator.AdoptExistingAnnotation: revisiongenerator.AdoptExistingNever, + }) + + transformed, opts, err := transformer.TransformObject(ctx, obj) + + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(opts).To(BeNil()) + g.Expect(transformed.GetAnnotations()).NotTo(HaveKey(revisiongenerator.AdoptExistingAnnotation)) + }) +} + +func TestAdoptExistingTransformer_Validate(t *testing.T) { + transformer := &AdoptExistingTransformer{} + + t.Run("object without annotation returns nil", func(t *testing.T) { + g := NewWithT(t) + g.Expect(transformer.Validate(adoptTestObject("no-annotation", nil))).To(Succeed()) + }) + + t.Run("object with always returns nil", func(t *testing.T) { + g := NewWithT(t) + obj := adoptTestObject("valid-always", map[string]string{ + revisiongenerator.AdoptExistingAnnotation: revisiongenerator.AdoptExistingAlways, + }) + g.Expect(transformer.Validate(obj)).To(Succeed()) + }) + + t.Run("object with never returns nil", func(t *testing.T) { + g := NewWithT(t) + obj := adoptTestObject("valid-never", map[string]string{ + revisiongenerator.AdoptExistingAnnotation: revisiongenerator.AdoptExistingNever, + }) + g.Expect(transformer.Validate(obj)).To(Succeed()) + }) + + t.Run("object with invalid value returns terminal error wrapping ErrInvalidAdoptExistingAnnotation", func(t *testing.T) { + g := NewWithT(t) + obj := adoptTestObject("bad-annotation", map[string]string{ + revisiongenerator.AdoptExistingAnnotation: "bogus", + }) + err := transformer.Validate(obj) + g.Expect(err).To(HaveOccurred()) + g.Expect(errors.Is(err, ErrInvalidAdoptExistingAnnotation)).To(BeTrue()) + g.Expect(err).To(MatchError(ContainSubstring("bogus"))) + }) +} diff --git a/pkg/manifesttransformer/envsubst_transformer.go b/pkg/manifesttransformer/envsubst_transformer.go new file mode 100644 index 000000000..c4782fe4d --- /dev/null +++ b/pkg/manifesttransformer/envsubst_transformer.go @@ -0,0 +1,148 @@ +/* +Copyright 2026 Red Hat, Inc. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package manifesttransformer + +import ( + "context" + "fmt" + "maps" + + "github.com/drone/envsubst/v2" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "pkg.package-operator.run/boxcutter" + + "github.com/openshift/cluster-capi-operator/pkg/revisiongenerator" +) + +// EnvsubstTransformer expands envsubst variables in every string value of an +// object at install time. staticSubs are configured at construction and take +// precedence over substitutions recorded on the revision. +type EnvsubstTransformer struct { + staticSubs map[string]string + mergedSubs map[string]string +} + +var _ ManifestTransformer = &EnvsubstTransformer{} + +// NewEnvsubstTransformer creates an EnvsubstTransformer with the given static +// substitutions. Static substitutions take precedence over revision-level +// substitutions. Pass nil for no static substitutions. +func NewEnvsubstTransformer(staticSubs map[string]string) *EnvsubstTransformer { + return &EnvsubstTransformer{ + staticSubs: maps.Clone(staticSubs), + } +} + +// TransformObject returns a copy of obj with envsubst variables expanded in +// every string value, using the revision's substitutions merged with any +// static substitutions (which take precedence). +func (e *EnvsubstTransformer) TransformObject(_ context.Context, obj *unstructured.Unstructured) (*unstructured.Unstructured, []boxcutter.ObjectReconcileOption, error) { + obj = obj.DeepCopy() + + if err := expandMapStrings(obj.Object, e.mergedSubs); err != nil { + return nil, nil, fmt.Errorf("expanding envsubst variables in %s: %w", obj.GetName(), err) + } + + return obj, nil, nil +} + +// Validate is a no-op for EnvsubstTransformer. +func (e *EnvsubstTransformer) Validate(_ *unstructured.Unstructured) error { + return nil +} + +// WithRevision returns a new EnvsubstTransformer that merges the revision's +// ManifestSubstitutions with the static substitutions. Static substitutions +// take precedence. +func (e *EnvsubstTransformer) WithRevision(_ context.Context, revision revisiongenerator.ParsedRevision) ManifestTransformer { + merged := revision.ManifestSubstitutions() + if merged == nil { + merged = make(map[string]string, len(e.staticSubs)) + } + + maps.Copy(merged, e.staticSubs) + + return &EnvsubstTransformer{ + staticSubs: e.staticSubs, + mergedSubs: merged, + } +} + +// WithComponent is a no-op; envsubst expansion does not need component context. +func (e *EnvsubstTransformer) WithComponent(_ context.Context, _ revisiongenerator.ParsedComponent) ManifestTransformer { + return e +} + +// expandMapStrings recursively walks a map[string]interface{} and calls +// envsubst.Eval on every string leaf value. +func expandMapStrings(m map[string]interface{}, subs map[string]string) error { + for k, val := range m { + expanded, err := expandValue(val, subs) + if err != nil { + return err + } + + m[k] = expanded + } + + return nil +} + +// expandSliceStrings recursively walks a []interface{} and calls envsubst.Eval +// on every string element. +func expandSliceStrings(s []interface{}, subs map[string]string) error { + for i, elem := range s { + expanded, err := expandValue(elem, subs) + if err != nil { + return err + } + + s[i] = expanded + } + + return nil +} + +// expandValue expands a single value: strings are envsubst-expanded, maps and +// slices are walked recursively, and all other types are returned unchanged. +func expandValue(val interface{}, subs map[string]string) (interface{}, error) { + switch t := val.(type) { + case string: + expanded, err := envsubst.Eval(t, func(key string) string { + return subs[key] + }) + if err != nil { + return nil, fmt.Errorf("envsubst.Eval: %w", err) + } + + return expanded, nil + case map[string]interface{}: + if err := expandMapStrings(t, subs); err != nil { + return nil, err + } + + return t, nil + case []interface{}: + if err := expandSliceStrings(t, subs); err != nil { + return nil, err + } + + return t, nil + default: + return val, nil + } +} diff --git a/pkg/manifesttransformer/envsubst_transformer_test.go b/pkg/manifesttransformer/envsubst_transformer_test.go new file mode 100644 index 000000000..56c56438c --- /dev/null +++ b/pkg/manifesttransformer/envsubst_transformer_test.go @@ -0,0 +1,237 @@ +/* +Copyright 2026 Red Hat, Inc. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package manifesttransformer + +import ( + "context" + "errors" + "maps" + "testing" + + . "github.com/onsi/gomega" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + + "github.com/openshift/cluster-capi-operator/pkg/revisiongenerator" +) + +// fakeRevisionWithSubs is a minimal ParsedRevision for EnvsubstTransformer tests. +type fakeRevisionWithSubs struct { + subs map[string]string +} + +func (f *fakeRevisionWithSubs) ContentID() (string, error) { return "fake", nil } +func (f *fakeRevisionWithSubs) Components() []revisiongenerator.ParsedComponent { return nil } +func (f *fakeRevisionWithSubs) ForInstall(string, int64) (revisiongenerator.InstallerRevision, error) { + return nil, errors.New("not implemented") +} + +func (f *fakeRevisionWithSubs) ManifestSubstitutions() map[string]string { + out := make(map[string]string, len(f.subs)) + maps.Copy(out, f.subs) + + return out +} + +var _ revisiongenerator.ParsedRevision = &fakeRevisionWithSubs{} + +type anyMap = map[string]interface{} + +func envsubstTestObject(data anyMap) *unstructured.Unstructured { + obj := &unstructured.Unstructured{} + obj.Object = data + + return obj +} + +func TestEnvsubstTransformer_TransformObject(t *testing.T) { + ctx := context.Background() + + tests := []struct { + name string + staticSubs map[string]string + revisionSubs map[string]string + input anyMap + want anyMap + }{ + { + name: "expands string values using merged substitutions", + revisionSubs: map[string]string{"FOO": "bar"}, + input: anyMap{ + "spec": anyMap{ + "value": "${FOO}", + }, + }, + want: anyMap{ + "spec": anyMap{ + "value": "bar", + }, + }, + }, + { + name: "expands strings in nested maps", + revisionSubs: map[string]string{"K": "v"}, + input: anyMap{ + "a": anyMap{ + "b": anyMap{ + "c": "${K}", + }, + }, + }, + want: anyMap{ + "a": anyMap{ + "b": anyMap{ + "c": "v", + }, + }, + }, + }, + { + name: "expands strings inside slices", + revisionSubs: map[string]string{"X": "hello"}, + input: anyMap{ + "items": []interface{}{"${X}", "literal"}, + }, + want: anyMap{ + "items": []interface{}{"hello", "literal"}, + }, + }, + { + name: "expands strings in maps nested inside slices", + revisionSubs: map[string]string{"Y": "world"}, + input: anyMap{ + "containers": []interface{}{anyMap{"name": "${Y}"}}, + }, + want: anyMap{ + "containers": []interface{}{anyMap{"name": "world"}}, + }, + }, + { + name: "leaves non-string values unchanged", + revisionSubs: map[string]string{"X": "x"}, + input: anyMap{ + "replicas": int64(3), + "enabled": true, + }, + want: anyMap{ + "replicas": int64(3), + "enabled": true, + }, + }, + { + name: "unknown variable replaced with empty string", + input: anyMap{"val": "${UNKNOWN}"}, + want: anyMap{"val": ""}, + }, + { + name: "default value syntax works when variable is unset", + input: anyMap{"val": "${MY_VAR:-fallback}"}, + want: anyMap{"val": "fallback"}, + }, + { + name: "static subs take precedence over revision subs", + staticSubs: map[string]string{"VAR": "static"}, + revisionSubs: map[string]string{"VAR": "revision"}, + input: anyMap{"val": "${VAR}"}, + want: anyMap{"val": "static"}, + }, + { + name: "revision subs used when no static sub for key", + staticSubs: map[string]string{"A": "from-static"}, + revisionSubs: map[string]string{"B": "from-revision"}, + input: anyMap{ + "a": "${A}", + "b": "${B}", + }, + want: anyMap{ + "a": "from-static", + "b": "from-revision", + }, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + g := NewWithT(t) + + tfm := NewEnvsubstTransformer(tc.staticSubs). + WithRevision(ctx, &fakeRevisionWithSubs{subs: tc.revisionSubs}) + + obj := envsubstTestObject(tc.input) + + transformed, opts, err := tfm.TransformObject(ctx, obj) + + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(opts).To(BeNil()) + g.Expect(transformed.Object).To(Equal(tc.want)) + }) + } + + t.Run("does not mutate the original object", func(t *testing.T) { + g := NewWithT(t) + + tfm := NewEnvsubstTransformer(nil). + WithRevision(ctx, &fakeRevisionWithSubs{subs: map[string]string{"FOO": "bar"}}) + + obj := envsubstTestObject(anyMap{"val": "${FOO}"}) + + _, _, err := tfm.TransformObject(ctx, obj) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(obj.Object["val"]).To(Equal("${FOO}")) + }) + + t.Run("does not panic when revision has no substitutions and static subs are set", func(t *testing.T) { + g := NewWithT(t) + + tfm := NewEnvsubstTransformer(map[string]string{"A": "static"}). + WithRevision(ctx, &fakeRevisionWithSubs{subs: nil}) + + obj := envsubstTestObject(anyMap{"a": "${A}"}) + + transformed, _, err := tfm.TransformObject(ctx, obj) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(transformed.Object["a"]).To(Equal("static")) + }) + + t.Run("object with no substitutions configured expands to empty string", func(t *testing.T) { + g := NewWithT(t) + + tfm := NewEnvsubstTransformer(nil) + + obj := envsubstTestObject(anyMap{"val": "${UNSET}"}) + + transformed, _, err := tfm.TransformObject(ctx, obj) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(transformed.Object["val"]).To(Equal("")) + }) +} + +func TestEnvsubstTransformer_WithComponent(t *testing.T) { + g := NewWithT(t) + + ctx := context.Background() + tfm := NewEnvsubstTransformer(map[string]string{"V": "x"}). + WithRevision(ctx, &fakeRevisionWithSubs{subs: nil}) + + g.Expect(tfm.WithComponent(ctx, nil)).To(BeIdenticalTo(tfm)) +} + +func TestEnvsubstTransformer_Validate(t *testing.T) { + g := NewWithT(t) + + tfm := NewEnvsubstTransformer(nil) + g.Expect(tfm.Validate(&unstructured.Unstructured{})).To(Succeed()) +} diff --git a/pkg/manifesttransformer/managed_by_transformer.go b/pkg/manifesttransformer/managed_by_transformer.go new file mode 100644 index 000000000..7a4a53637 --- /dev/null +++ b/pkg/manifesttransformer/managed_by_transformer.go @@ -0,0 +1,74 @@ +/* +Copyright 2026 Red Hat, Inc. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package manifesttransformer + +import ( + "context" + + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "pkg.package-operator.run/boxcutter" + + "github.com/openshift/cluster-capi-operator/pkg/operatorstatus" + "github.com/openshift/cluster-capi-operator/pkg/revisiongenerator" +) + +// ManagedLabelKey is a label key used to identify objects managed by the CAPI operator. +const ManagedLabelKey = operatorstatus.CAPIOperatorIdentifierDomain + "/managed-by" + +// ManagedByTransformer adds a managed-by label to every object at install time. +// The label value is set to the component name via WithComponent. +type ManagedByTransformer struct { + componentName string +} + +var _ ManifestTransformer = &ManagedByTransformer{} + +// NewManagedByTransformer creates a ManagedByTransformer. The component name +// is populated per-component via WithComponent. +func NewManagedByTransformer() *ManagedByTransformer { + return &ManagedByTransformer{} +} + +// TransformObject returns a copy of obj with ManagedLabelKey=componentName added. +func (m *ManagedByTransformer) TransformObject(_ context.Context, obj *unstructured.Unstructured) (*unstructured.Unstructured, []boxcutter.ObjectReconcileOption, error) { + obj = obj.DeepCopy() + + labels := obj.GetLabels() + if labels == nil { + labels = map[string]string{} + } + + labels[ManagedLabelKey] = m.componentName + obj.SetLabels(labels) + + return obj, nil, nil +} + +// Validate is a no-op for ManagedByTransformer. +func (m *ManagedByTransformer) Validate(_ *unstructured.Unstructured) error { + return nil +} + +// WithRevision is a no-op; managed-by labelling does not need revision context. +func (m *ManagedByTransformer) WithRevision(_ context.Context, _ revisiongenerator.ParsedRevision) ManifestTransformer { + return m +} + +// WithComponent returns a new ManagedByTransformer with the component name set. +func (m *ManagedByTransformer) WithComponent(_ context.Context, component revisiongenerator.ParsedComponent) ManifestTransformer { + return &ManagedByTransformer{componentName: component.Name()} +} diff --git a/pkg/manifesttransformer/managed_by_transformer_test.go b/pkg/manifesttransformer/managed_by_transformer_test.go new file mode 100644 index 000000000..256fa0c5e --- /dev/null +++ b/pkg/manifesttransformer/managed_by_transformer_test.go @@ -0,0 +1,101 @@ +/* +Copyright 2026 Red Hat, Inc. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package manifesttransformer + +import ( + "context" + "testing" + + . "github.com/onsi/gomega" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" +) + +func TestManagedByTransformer_TransformObject(t *testing.T) { + ctx := context.Background() + + t.Run("adds managed-by label to object with no labels", func(t *testing.T) { + g := NewWithT(t) + + tfm := NewManagedByTransformer(). + WithComponent(ctx, &fakeComponent{name: "my-provider"}) + + obj := &unstructured.Unstructured{} + obj.SetName("test-obj") + + transformed, opts, err := tfm.TransformObject(ctx, obj) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(opts).To(BeNil()) + g.Expect(transformed.GetLabels()).To(HaveKeyWithValue(ManagedLabelKey, "my-provider")) + + // The original object must not be mutated. + g.Expect(obj.GetLabels()).To(BeEmpty()) + }) + + t.Run("preserves existing labels", func(t *testing.T) { + g := NewWithT(t) + + tfm := NewManagedByTransformer(). + WithComponent(ctx, &fakeComponent{name: "my-provider"}) + + obj := &unstructured.Unstructured{} + obj.SetLabels(map[string]string{"existing": "label"}) + + transformed, _, err := tfm.TransformObject(ctx, obj) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(transformed.GetLabels()).To(HaveKeyWithValue("existing", "label")) + g.Expect(transformed.GetLabels()).To(HaveKeyWithValue(ManagedLabelKey, "my-provider")) + }) + + t.Run("uses component name from WithComponent", func(t *testing.T) { + g := NewWithT(t) + + base := NewManagedByTransformer() + + tfm1 := base.WithComponent(ctx, &fakeComponent{name: "component-one"}) + tfm2 := base.WithComponent(ctx, &fakeComponent{name: "component-two"}) + + obj1 := &unstructured.Unstructured{} + obj2 := &unstructured.Unstructured{} + + transformed1, _, err := tfm1.TransformObject(ctx, obj1) + g.Expect(err).NotTo(HaveOccurred()) + + transformed2, _, err := tfm2.TransformObject(ctx, obj2) + g.Expect(err).NotTo(HaveOccurred()) + + g.Expect(transformed1.GetLabels()).To(HaveKeyWithValue(ManagedLabelKey, "component-one")) + g.Expect(transformed2.GetLabels()).To(HaveKeyWithValue(ManagedLabelKey, "component-two")) + }) +} + +func TestManagedByTransformer_WithRevision(t *testing.T) { + g := NewWithT(t) + + ctx := context.Background() + tfm := NewManagedByTransformer() + same := tfm.WithRevision(ctx, nil) + + // WithRevision is a no-op: returns the same receiver. + g.Expect(same).To(BeIdenticalTo(tfm)) +} + +func TestManagedByTransformer_Validate(t *testing.T) { + g := NewWithT(t) + + tfm := NewManagedByTransformer() + g.Expect(tfm.Validate(&unstructured.Unstructured{})).To(Succeed()) +} diff --git a/pkg/manifesttransformer/manifest_transformer.go b/pkg/manifesttransformer/manifest_transformer.go new file mode 100644 index 000000000..fc0bdcaa6 --- /dev/null +++ b/pkg/manifesttransformer/manifest_transformer.go @@ -0,0 +1,45 @@ +/* +Copyright 2026 Red Hat, Inc. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package manifesttransformer + +import ( + "context" + + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "pkg.package-operator.run/boxcutter" + + "github.com/openshift/cluster-capi-operator/pkg/revisiongenerator" +) + +// ManifestTransformer transforms objects at install time while objects are being +// collected into boxcutter phases. +type ManifestTransformer interface { + // TransformObject returns a transformed copy of the object and any + // boxcutter options that should apply to the object. If TransformObject + // returns a nil object, it means the object should be skipped. + TransformObject(ctx context.Context, obj *unstructured.Unstructured) (*unstructured.Unstructured, []boxcutter.ObjectReconcileOption, error) + + // Validate checks that the object is valid for this transformer. An error + // prevents revision creation and is treated as non-retryable. + Validate(obj *unstructured.Unstructured) error + + // WithRevision returns a new transformer that will be used for the given revision. + WithRevision(ctx context.Context, revision revisiongenerator.ParsedRevision) ManifestTransformer + + // WithComponent returns a new transformer that will be used for the given component. + WithComponent(ctx context.Context, component revisiongenerator.ParsedComponent) ManifestTransformer +} diff --git a/pkg/manifesttransformer/validate.go b/pkg/manifesttransformer/validate.go new file mode 100644 index 000000000..26c7ee380 --- /dev/null +++ b/pkg/manifesttransformer/validate.go @@ -0,0 +1,44 @@ +/* +Copyright 2026 Red Hat, Inc. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package manifesttransformer + +import ( + "errors" + "fmt" + + "sigs.k8s.io/controller-runtime/pkg/client" + + "github.com/openshift/cluster-capi-operator/pkg/revisiongenerator" +) + +// ValidateTransformers calls Validate on each transformer for every object in the revision. +// All errors are collected and returned together via errors.Join. +func ValidateTransformers(transformers []ManifestTransformer, rev revisiongenerator.ParsedRevision) error { + var allErrs []error + + for _, component := range rev.Components() { + for _, obj := range component.Objects() { + for _, t := range transformers { + if err := t.Validate(obj); err != nil { + allErrs = append(allErrs, fmt.Errorf("%s %s: %w", component.Name(), client.ObjectKeyFromObject(obj), err)) + } + } + } + } + + return errors.Join(allErrs...) +} diff --git a/pkg/manifesttransformer/validate_test.go b/pkg/manifesttransformer/validate_test.go new file mode 100644 index 000000000..5c3c9a788 --- /dev/null +++ b/pkg/manifesttransformer/validate_test.go @@ -0,0 +1,181 @@ +/* +Copyright 2026 Red Hat, Inc. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package manifesttransformer + +import ( + "context" + "errors" + "testing" + + . "github.com/onsi/gomega" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "pkg.package-operator.run/boxcutter" + + "github.com/openshift/cluster-capi-operator/pkg/revisiongenerator" +) + +func TestValidateTransformers(t *testing.T) { + const componentName = "my-component" + + testObj := unstructured.Unstructured{} + testObj.SetName("my-obj") + + testObj2 := unstructured.Unstructured{} + testObj2.SetName("my-obj2") + + t.Run("nil transformers returns no error", func(t *testing.T) { + g := NewWithT(t) + rev := &fakeRevision{ + components: []revisiongenerator.ParsedComponent{ + &fakeComponent{name: componentName, objects: []*unstructured.Unstructured{&testObj}}, + }, + } + g.Expect(ValidateTransformers(nil, rev)).To(Succeed()) + }) + + t.Run("empty transformers returns no error", func(t *testing.T) { + g := NewWithT(t) + rev := &fakeRevision{ + components: []revisiongenerator.ParsedComponent{ + &fakeComponent{name: componentName, objects: []*unstructured.Unstructured{&testObj}}, + }, + } + g.Expect(ValidateTransformers([]ManifestTransformer{}, rev)).To(Succeed()) + }) + + t.Run("validates Objects and includes component name in error", func(t *testing.T) { + g := NewWithT(t) + rev := &fakeRevision{ + components: []revisiongenerator.ParsedComponent{ + &fakeComponent{name: componentName, objects: []*unstructured.Unstructured{&testObj}}, + }, + } + stub := &stubTransformer{validateErr: errors.New("obj invalid")} + g.Expect(ValidateTransformers([]ManifestTransformer{stub}, rev)). + To(MatchError(SatisfyAll( + ContainSubstring(componentName), + ContainSubstring("my-obj"), + ContainSubstring("obj invalid"), + ))) + }) + + t.Run("collects errors from multiple objects", func(t *testing.T) { + g := NewWithT(t) + rev := &fakeRevision{ + components: []revisiongenerator.ParsedComponent{ + &fakeComponent{name: componentName, objects: []*unstructured.Unstructured{&testObj, &testObj2}}, + }, + } + stub := &stubTransformer{validateErr: errors.New("invalid")} + g.Expect(ValidateTransformers([]ManifestTransformer{stub}, rev)). + To(MatchError(SatisfyAll( + ContainSubstring("my-obj"), + ContainSubstring("my-obj2"), + ))) + }) + + t.Run("aggregates errors across multiple components rather than stopping at the first", func(t *testing.T) { + g := NewWithT(t) + + objA := &unstructured.Unstructured{} + objA.SetName("obj-a") + + objB := &unstructured.Unstructured{} + objB.SetName("obj-b") + + rev := &fakeRevision{ + components: []revisiongenerator.ParsedComponent{ + &fakeComponent{name: "comp-1", objects: []*unstructured.Unstructured{objA}}, + &fakeComponent{name: "comp-2", objects: []*unstructured.Unstructured{objB}}, + }, + } + stub := &stubTransformer{validateErr: errors.New("invalid")} + + err := ValidateTransformers([]ManifestTransformer{stub}, rev) + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring("comp-1")) + g.Expect(err.Error()).To(ContainSubstring("obj-a")) + g.Expect(err.Error()).To(ContainSubstring("comp-2")) + g.Expect(err.Error()).To(ContainSubstring("obj-b")) + }) + + t.Run("aggregates errors from multiple transformers on the same object", func(t *testing.T) { + g := NewWithT(t) + + rev := &fakeRevision{ + components: []revisiongenerator.ParsedComponent{ + &fakeComponent{name: componentName, objects: []*unstructured.Unstructured{&testObj}}, + }, + } + stubA := &stubTransformer{validateErr: errors.New("error-a")} + stubB := &stubTransformer{validateErr: errors.New("error-b")} + + err := ValidateTransformers([]ManifestTransformer{stubA, stubB}, rev) + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring("error-a")) + g.Expect(err.Error()).To(ContainSubstring("error-b")) + }) +} + +// stubTransformer is a test double for ManifestTransformer. +type stubTransformer struct { + validateErr error +} + +func (s *stubTransformer) TransformObject(_ context.Context, obj *unstructured.Unstructured) (*unstructured.Unstructured, []boxcutter.ObjectReconcileOption, error) { + return obj, nil, nil +} + +func (s *stubTransformer) Validate(_ *unstructured.Unstructured) error { + return s.validateErr +} + +func (s *stubTransformer) WithRevision(_ context.Context, _ revisiongenerator.ParsedRevision) ManifestTransformer { + return s +} + +func (s *stubTransformer) WithComponent(_ context.Context, _ revisiongenerator.ParsedComponent) ManifestTransformer { + return s +} + +var _ ManifestTransformer = &stubTransformer{} + +// fakeComponent implements revisiongenerator.ParsedComponent for unit tests +// that need a revision without running the full revision generator. +type fakeComponent struct { + name string + objects []*unstructured.Unstructured +} + +func (f *fakeComponent) Name() string { return f.name } +func (f *fakeComponent) Objects() []*unstructured.Unstructured { return f.objects } + +// fakeRevision implements revisiongenerator.ParsedRevision for unit tests. +type fakeRevision struct { + components []revisiongenerator.ParsedComponent +} + +func (f *fakeRevision) ContentID() (string, error) { return "fake-content-id", nil } +func (f *fakeRevision) Components() []revisiongenerator.ParsedComponent { + return f.components +} +func (f *fakeRevision) ForInstall(string, int64) (revisiongenerator.InstallerRevision, error) { + return nil, errors.New("not implemented") +} +func (f *fakeRevision) ManifestSubstitutions() map[string]string { return nil } + +var _ revisiongenerator.ParsedRevision = &fakeRevision{} diff --git a/pkg/revisiongenerator/helpers_test.go b/pkg/revisiongenerator/helpers_test.go index b90a9bf66..30b0af0d1 100644 --- a/pkg/revisiongenerator/helpers_test.go +++ b/pkg/revisiongenerator/helpers_test.go @@ -49,13 +49,13 @@ func profile(t *testing.T, name, imageRef, profileName, manifestContent string) // contentIDForProfiles computes the contentID for a set of profiles. func contentIDForProfiles(g *WithT, profiles ...providerimages.ProviderImageManifests) string { g.THelper() - rev := must(NewRenderedRevision(profiles))(g) + rev := must(NewParsedRevision(profiles))(g) return must(rev.ContentID())(g) } -// forInstall creates an InstallerRevision from a RenderedRevision, failing the test on error. -func forInstall(g *WithT, rev RenderedRevision, releaseVersion string, revisionIndex int64) InstallerRevision { //nolint:unparam +// forInstall creates an InstallerRevision from a ParsedRevision, failing the test on error. +func forInstall(g *WithT, rev ParsedRevision, releaseVersion string, revisionIndex int64) InstallerRevision { //nolint:unparam g.THelper() return must(rev.ForInstall(releaseVersion, revisionIndex))(g) } diff --git a/pkg/revisiongenerator/revision.go b/pkg/revisiongenerator/revision.go index 70704811e..5d12fd075 100644 --- a/pkg/revisiongenerator/revision.go +++ b/pkg/revisiongenerator/revision.go @@ -25,12 +25,12 @@ import ( "fmt" "maps" "slices" + "strings" operatorv1alpha1 "github.com/openshift/api/operator/v1alpha1" operatorv1alpha1ac "github.com/openshift/client-go/operator/applyconfigurations/operator/v1alpha1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" - "k8s.io/apimachinery/pkg/runtime/schema" - "sigs.k8s.io/controller-runtime/pkg/client" + "k8s.io/utils/ptr" k8syaml "sigs.k8s.io/yaml" "github.com/openshift/cluster-capi-operator/pkg/providerimages" @@ -48,25 +48,30 @@ var ( errContentIDMismatch = errors.New("content ID mismatch") ) -// RenderedRevision represents a set of components whose manifests have been -// fully rendered and are ready to be applied to a cluster. -type RenderedRevision interface { +// ParsedRevision represents a set of components whose manifests have been +// parsed from provider image manifests and are ready to be installed. +type ParsedRevision interface { // ContentID returns a unique identifier for the revision's content. ContentID() (string, error) - // Components returns the rendered components for this revision. - Components() []RenderedComponent + // Components returns the parsed components for this revision. + Components() []ParsedComponent // ForInstall creates an InstallerRevision by assigning a release version - // and revision index to this rendered content. + // and revision index to this parsed content. ForInstall(releaseVersion string, revisionIndex int64) (InstallerRevision, error) + + // ManifestSubstitutions returns a copy of the substitutions stored in + // this revision. These are used by install-time transformers to expand + // envsubst variables in manifests. + ManifestSubstitutions() map[string]string } -// InstallerRevision is a RenderedRevision that has been assigned a revision +// InstallerRevision is a ParsedRevision that has been assigned a revision // identity (name and index), making it ready for installation or conversion // to an API revision. type InstallerRevision interface { - RenderedRevision + ParsedRevision // RevisionName returns the name of this revision. RevisionName() operatorv1alpha1.RevisionName @@ -78,42 +83,40 @@ type InstallerRevision interface { ToAPIRevision() (operatorv1alpha1.ClusterAPIInstallerRevision, error) } -// RenderedComponent represents a single provider component with manifests -// separated into CRDs and other objects. -type RenderedComponent interface { +// ParsedComponent represents a single provider component with its manifests +// parsed and ready to be applied. +type ParsedComponent interface { // Name returns the component name. Name() string - // CRDs returns the CRD objects for this component. - CRDs() []client.Object - // Objects returns the non-CRD objects for this component. - Objects() []client.Object + // Objects returns all objects for this component, including CRDs. + Objects() []*unstructured.Unstructured } -type renderedRevision struct { - components []*renderedComponent +type parsedRevision struct { + components []*parsedComponent contentID string - substitutions []operatorv1alpha1.ClusterAPIInstallerRevisionManifestSubstitution + substitutions map[string]string } -var _ RenderedRevision = &renderedRevision{} +var _ ParsedRevision = &parsedRevision{} -// NewRenderedRevision creates a new RenderedRevision from a list of provider image manifests. -func NewRenderedRevision(profiles []providerimages.ProviderImageManifests, opts ...revisionRenderOption) (RenderedRevision, error) { - return newRenderedRevision(profiles, opts...) +// NewParsedRevision creates a new ParsedRevision from a list of provider image manifests. +func NewParsedRevision(profiles []providerimages.ProviderImageManifests, opts ...revisionRenderOption) (ParsedRevision, error) { + return newParsedRevision(profiles, opts...) } -// newRenderedRevision implements NewRenderedRevision. It exists to return a +// newParsedRevision implements NewParsedRevision. It exists to return a // concrete type for internal use. -func newRenderedRevision(profiles []providerimages.ProviderImageManifests, opts ...revisionRenderOption) (*renderedRevision, error) { +func newParsedRevision(profiles []providerimages.ProviderImageManifests, opts ...revisionRenderOption) (*parsedRevision, error) { cfg := &revisionRenderConfig{} for _, opt := range opts { opt(cfg) } - components := make([]*renderedComponent, len(profiles)) + components := make([]*parsedComponent, len(profiles)) for i, profile := range profiles { - component, err := newRenderedComponent(&profile, cfg) + component, err := newParsedComponent(&profile) if err != nil { return nil, err } @@ -121,13 +124,9 @@ func newRenderedRevision(profiles []providerimages.ProviderImageManifests, opts components[i] = component } - rev := &renderedRevision{ + rev := &parsedRevision{ components: components, - substitutions: substitutionsFromMap(cfg.substitutions), - } - - if err := validateRenderedRevision(rev); err != nil { - return nil, err + substitutions: maps.Clone(cfg.substitutions), } return rev, nil @@ -154,10 +153,15 @@ func substitutionsFromMap(m map[string]string) []operatorv1alpha1.ClusterAPIInst return subs } +// ManifestSubstitutions returns a copy of the substitutions stored in this revision. +func (r *parsedRevision) ManifestSubstitutions() map[string]string { + return maps.Clone(r.substitutions) +} + // ContentID returns a unique identifier for the revision's content. // Specifically it returns a SHA256 over all manifests and substitutions, // but callers MUST NOT assume this. -func (r *renderedRevision) ContentID() (string, error) { +func (r *parsedRevision) ContentID() (string, error) { if r.contentID == "" { h := sha256.New() @@ -175,7 +179,16 @@ func (r *renderedRevision) ContentID() (string, error) { // revision, even if they were not used. This is not strictly necessary, // but it should reduce operator confusion if old but unused // substitutions continued to be listed in the current revision. - if data, err := json.Marshal(r.substitutions); err == nil { + // + // Normalise nil to an empty map before marshalling so that a nil + // substitutions map and an empty one produce the same hash, regardless + // of how the revision was constructed. + subs := r.substitutions + if subs == nil { + subs = map[string]string{} + } + + if data, err := json.Marshal(subs); err == nil { h.Write(data) } else { return "", fmt.Errorf("error marshalling substitutions: %w", err) @@ -187,32 +200,32 @@ func (r *renderedRevision) ContentID() (string, error) { return r.contentID, nil } -// Components returns the rendered components for this revision. -func (r *renderedRevision) Components() []RenderedComponent { - return util.SliceMap(r.components, func(c *renderedComponent) RenderedComponent { +// Components returns the parsed components for this revision. +func (r *parsedRevision) Components() []ParsedComponent { + return util.SliceMap(r.components, func(c *parsedComponent) ParsedComponent { return c }) } // ForInstall creates an InstallerRevision by assigning a release version and -// revision index to this rendered content. -func (r *renderedRevision) ForInstall(releaseVersion string, revisionIndex int64) (InstallerRevision, error) { +// revision index to this parsed content. +func (r *parsedRevision) ForInstall(releaseVersion string, revisionIndex int64) (InstallerRevision, error) { contentID, err := r.ContentID() if err != nil { return nil, fmt.Errorf("error calculating contentID: %w", err) } return &installerRevision{ - renderedRevision: r, - revisionName: buildRevisionName(releaseVersion, contentID, revisionIndex), - revisionIndex: revisionIndex, + parsedRevision: r, + revisionName: buildRevisionName(releaseVersion, contentID, revisionIndex), + revisionIndex: revisionIndex, }, nil } -// installerRevision is a renderedRevision that has been assigned a revision +// installerRevision is a parsedRevision that has been assigned a revision // identity (name and index). type installerRevision struct { - *renderedRevision + *parsedRevision revisionName operatorv1alpha1.RevisionName revisionIndex int64 } @@ -259,7 +272,7 @@ func (r *installerRevision) ToAPIRevision() (operatorv1alpha1.ClusterAPIInstalle Name: r.revisionName, Revision: r.revisionIndex, ContentID: contentID, - ManifestSubstitutions: slices.Clone(r.substitutions), + ManifestSubstitutions: substitutionsFromMap(r.substitutions), Components: apiComponents, }, nil } @@ -287,26 +300,14 @@ func buildRevisionName(releaseVersion, contentID string, index int64) operatorv1 } type revisionRenderConfig struct { - objectCollectors []RevisionObjectCollector - substitutions map[string]string + substitutions map[string]string } type revisionRenderOption func(*revisionRenderConfig) -// RevisionObjectCollector is a function that will be called for each object in -// the rendered revision. -type RevisionObjectCollector func(obj unstructured.Unstructured) - -// WithObjectCollectors adds object collectors to the revision render config. -func WithObjectCollectors(collectors ...RevisionObjectCollector) revisionRenderOption { - return func(opts *revisionRenderConfig) { - opts.objectCollectors = append(opts.objectCollectors, collectors...) - } -} - // WithManifestSubstitutions adds envsubst-style substitutions that will be -// applied to manifests during rendering and recorded on the revision. When -// called multiple times, later values merge with and override earlier ones. +// recorded on the revision and applied at install time. When called multiple +// times, later values merge with and override earlier ones. func WithManifestSubstitutions(subs map[string]string) revisionRenderOption { return func(opts *revisionRenderConfig) { if opts.substitutions == nil { @@ -322,7 +323,7 @@ func WithManifestSubstitutions(subs map[string]string) revisionRenderOption { // rendering the matched manifests. The revision name and index are taken // directly from the API revision. Components are matched by Image.Ref and // Image.Profile. An error is returned if any component cannot be found in the -// provided profiles, or if the rendered content ID does not match the content +// provided profiles, or if the parsed content ID does not match the content // ID recorded in the API revision. func NewInstallerRevisionFromAPI( apiRev operatorv1alpha1.ClusterAPIInstallerRevision, @@ -350,49 +351,43 @@ func NewInstallerRevisionFromAPI( } } - // Prepend substitutions from the API revision so they are applied during - // rendering and included in the content ID for validation. Later options - // merge with and override these values. apiSubs := make(map[string]string, len(apiRev.ManifestSubstitutions)) for _, s := range apiRev.ManifestSubstitutions { - if s.Value != nil { - apiSubs[s.Key] = *s.Value - } + apiSubs[s.Key] = ptr.Deref(s.Value, "") } - opts = append([]revisionRenderOption{WithManifestSubstitutions(apiSubs)}, opts...) + opts = append(opts, WithManifestSubstitutions(apiSubs)) - rendered, err := newRenderedRevision(matched, opts...) + parsed, err := newParsedRevision(matched, opts...) if err != nil { return nil, err } - // Validate that the rendered content ID matches the API revision content ID. - if contentID, err := rendered.ContentID(); err != nil { + // Validate that the parsed content ID matches the API revision content ID. + if contentID, err := parsed.ContentID(); err != nil { return nil, fmt.Errorf("error computing content ID: %w", err) } else if contentID != apiRev.ContentID { - return nil, fmt.Errorf("%w: rendered revision has content ID %s, but API revision specifies %s", + return nil, fmt.Errorf("%w: parsed revision has content ID %s, but API revision specifies %s", errContentIDMismatch, contentID, apiRev.ContentID) } return &installerRevision{ - renderedRevision: rendered, - revisionName: apiRev.Name, - revisionIndex: apiRev.Revision, + parsedRevision: parsed, + revisionName: apiRev.Name, + revisionIndex: apiRev.Revision, }, nil } -type renderedComponent struct { +type parsedComponent struct { name string imageRef string profile string - crds []unstructured.Unstructured - objects []unstructured.Unstructured + objects []*unstructured.Unstructured } -func newRenderedComponent(providerProfile *providerimages.ProviderImageManifests, cfg *revisionRenderConfig) (*renderedComponent, error) { - component := &renderedComponent{ +func newParsedComponent(providerProfile *providerimages.ProviderImageManifests) (*parsedComponent, error) { + component := &parsedComponent{ name: providerProfile.Name, imageRef: providerProfile.ImageRef, profile: providerProfile.Profile, @@ -403,60 +398,38 @@ func newRenderedComponent(providerProfile *providerimages.ProviderImageManifests return nil, fmt.Errorf("error reading manifests: %w", err) } - yaml, err = transformYaml(providerProfile, yaml, cfg.substitutions) - if err != nil { - return nil, fmt.Errorf("error transforming manifest yaml: %w", err) - } - - var unstructured unstructured.Unstructured - if err := k8syaml.Unmarshal([]byte(yaml), &unstructured); err != nil { - return nil, fmt.Errorf("error unmarshalling transformed manifest: %w", err) + // Replace SelfImageRef with the actual image ref before unmarshalling. + if providerProfile.SelfImageRef != "" { + yaml = strings.ReplaceAll(yaml, providerProfile.SelfImageRef, providerProfile.ImageRef) } - unstructured = transformObject(unstructured, component.name) - - for _, collector := range cfg.objectCollectors { - collector(unstructured) + var obj unstructured.Unstructured + if err := k8syaml.Unmarshal([]byte(yaml), &obj.Object); err != nil { + return nil, fmt.Errorf("error unmarshalling manifest: %w", err) } - gvk := unstructured.GroupVersionKind() - - switch gvk.GroupKind() { - case schema.GroupKind{Group: "apiextensions.k8s.io", Kind: "CustomResourceDefinition"}: - component.crds = append(component.crds, unstructured) - default: - component.objects = append(component.objects, unstructured) - } + component.objects = append(component.objects, &obj) } return component, nil } -var _ RenderedComponent = &renderedComponent{} +var _ ParsedComponent = &parsedComponent{} // Name returns the component name. -func (c *renderedComponent) Name() string { +func (c *parsedComponent) Name() string { return c.name } -// CRDs returns the CRD objects for this component. -func (c *renderedComponent) CRDs() []client.Object { - return util.SliceMap(c.crds, func(crd unstructured.Unstructured) client.Object { - return &crd - }) -} - -// Objects returns the non-CRD objects for this component. -func (c *renderedComponent) Objects() []client.Object { - return util.SliceMap(c.objects, func(obj unstructured.Unstructured) client.Object { - return &obj - }) +// Objects returns all objects for this component, including CRDs. +func (c *parsedComponent) Objects() []*unstructured.Unstructured { + return c.objects } -func (c *renderedComponent) contentID() (string, error) { +func (c *parsedComponent) contentID() (string, error) { h := sha256.New() - for _, obj := range slices.Concat(c.crds, c.objects) { + for _, obj := range c.objects { data, err := json.Marshal(obj.Object) if err != nil { return "", fmt.Errorf("error marshalling object: %w", err) diff --git a/pkg/revisiongenerator/revision_test.go b/pkg/revisiongenerator/revision_test.go index ef17dd5f9..5c0b68b85 100644 --- a/pkg/revisiongenerator/revision_test.go +++ b/pkg/revisiongenerator/revision_test.go @@ -207,7 +207,7 @@ func TestContentID(t *testing.T) { t.Run("contentID is deterministic across calls", func(t *testing.T) { g := NewWithT(t) - rev := must(NewRenderedRevision([]providerimages.ProviderImageManifests{ + rev := must(NewParsedRevision([]providerimages.ProviderImageManifests{ profile(t, "p1", "img1", "default", configMapA), }))(g) @@ -220,7 +220,7 @@ func TestContentID(t *testing.T) { t.Run("empty manifests", func(t *testing.T) { g := NewWithT(t) - rev := must(NewRenderedRevision([]providerimages.ProviderImageManifests{ + rev := must(NewParsedRevision([]providerimages.ProviderImageManifests{ profile(t, "p1", "img1", "default", ""), }))(g) @@ -242,8 +242,8 @@ data: Build() profiles := []providerimages.ProviderImageManifests{prof} - rev1 := must(NewRenderedRevision(profiles, WithManifestSubstitutions(map[string]string{"VAR1": "value1"})))(g) - rev2 := must(NewRenderedRevision(profiles, WithManifestSubstitutions(map[string]string{"VAR1": "value2"})))(g) + rev1 := must(NewParsedRevision(profiles, WithManifestSubstitutions(map[string]string{"VAR1": "value1"})))(g) + rev2 := must(NewParsedRevision(profiles, WithManifestSubstitutions(map[string]string{"VAR1": "value2"})))(g) id1 := must(rev1.ContentID())(g) id2 := must(rev2.ContentID())(g) @@ -258,8 +258,8 @@ data: subs := map[string]string{"VAR1": "value1"} - rev1 := must(NewRenderedRevision(profiles, WithManifestSubstitutions(subs)))(g) - rev2 := must(NewRenderedRevision(profiles, WithManifestSubstitutions(subs)))(g) + rev1 := must(NewParsedRevision(profiles, WithManifestSubstitutions(subs)))(g) + rev2 := must(NewParsedRevision(profiles, WithManifestSubstitutions(subs)))(g) id1 := must(rev1.ContentID())(g) id2 := must(rev2.ContentID())(g) @@ -272,8 +272,8 @@ data: profiles := []providerimages.ProviderImageManifests{profile(t, "p1", "img1", "default", configMapA)} - rev1 := must(NewRenderedRevision(profiles))(g) - rev2 := must(NewRenderedRevision(profiles, WithManifestSubstitutions(map[string]string{"UNUSED_VAR": "unused_value"})))(g) + rev1 := must(NewParsedRevision(profiles))(g) + rev2 := must(NewParsedRevision(profiles, WithManifestSubstitutions(map[string]string{"UNUSED_VAR": "unused_value"})))(g) id1 := must(rev1.ContentID())(g) id2 := must(rev2.ContentID())(g) @@ -286,7 +286,7 @@ func TestForInstall(t *testing.T) { t.Run("returns installer revision with correct name and index", func(t *testing.T) { g := NewWithT(t) - rev := must(NewRenderedRevision([]providerimages.ProviderImageManifests{ + rev := must(NewParsedRevision([]providerimages.ProviderImageManifests{ profile(t, "core", "img1", "default", configMapA), }))(g) @@ -302,7 +302,7 @@ func TestForInstall(t *testing.T) { t.Run("components accessible through installer revision", func(t *testing.T) { g := NewWithT(t) - rev := must(NewRenderedRevision([]providerimages.ProviderImageManifests{ + rev := must(NewParsedRevision([]providerimages.ProviderImageManifests{ profile(t, "core", "img1", "default", multiDoc(crdA, configMapA)), }))(g) @@ -310,14 +310,13 @@ func TestForInstall(t *testing.T) { components := installer.Components() g.Expect(components).To(HaveLen(1)) - g.Expect(components[0].CRDs()).To(HaveLen(1)) - g.Expect(components[0].Objects()).To(HaveLen(1)) + g.Expect(components[0].Objects()).To(HaveLen(2)) }) t.Run("name truncation with long version", func(t *testing.T) { g := NewWithT(t) - rev := must(NewRenderedRevision([]providerimages.ProviderImageManifests{ + rev := must(NewParsedRevision([]providerimages.ProviderImageManifests{ profile(t, "core", "img1", "default", configMapA), }))(g) @@ -332,7 +331,7 @@ func TestToAPIRevision(t *testing.T) { t.Run("single component fields", func(t *testing.T) { g := NewWithT(t) - rev := must(NewRenderedRevision([]providerimages.ProviderImageManifests{ + rev := must(NewParsedRevision([]providerimages.ProviderImageManifests{ profile(t, "core", "quay.io/openshift/core@sha256:abcd", "default", configMapA), }))(g) @@ -350,7 +349,7 @@ func TestToAPIRevision(t *testing.T) { t.Run("multiple components preserve order", func(t *testing.T) { g := NewWithT(t) - rev := must(NewRenderedRevision([]providerimages.ProviderImageManifests{ + rev := must(NewParsedRevision([]providerimages.ProviderImageManifests{ profile(t, "core", "img-core", "default", configMapA), profile(t, "infra", "img-infra", "aws", configMapB), }))(g) @@ -372,7 +371,7 @@ func TestToAPIRevision(t *testing.T) { unnamed := profile(t, "placeholder", "img1", "default", configMapA) unnamed.Name = "" - rev := must(NewRenderedRevision([]providerimages.ProviderImageManifests{unnamed}))(g) + rev := must(NewParsedRevision([]providerimages.ProviderImageManifests{unnamed}))(g) apiRev := must(forInstall(g, rev, "4.18.0", 1).ToAPIRevision())(g) g.Expect(apiRev.Components).To(HaveLen(1)) @@ -382,7 +381,7 @@ func TestToAPIRevision(t *testing.T) { t.Run("name format", func(t *testing.T) { g := NewWithT(t) - rev := must(NewRenderedRevision([]providerimages.ProviderImageManifests{ + rev := must(NewParsedRevision([]providerimages.ProviderImageManifests{ profile(t, "core", "img1", "default", configMapA), }))(g) @@ -396,7 +395,7 @@ func TestToAPIRevision(t *testing.T) { t.Run("contentID matches standalone call", func(t *testing.T) { g := NewWithT(t) - rev := must(NewRenderedRevision([]providerimages.ProviderImageManifests{ + rev := must(NewParsedRevision([]providerimages.ProviderImageManifests{ profile(t, "core", "img1", "default", configMapA), }))(g) @@ -410,7 +409,7 @@ func TestToAPIRevision(t *testing.T) { t.Run("zero components", func(t *testing.T) { g := NewWithT(t) - rev := must(NewRenderedRevision([]providerimages.ProviderImageManifests{}))(g) + rev := must(NewParsedRevision([]providerimages.ProviderImageManifests{}))(g) apiRev := must(forInstall(g, rev, "4.18.0", 1).ToAPIRevision())(g) @@ -426,7 +425,7 @@ func TestToAPIRevision(t *testing.T) { "TLS_MIN_VERSION": "VersionTLS12", } - rev := must(NewRenderedRevision( + rev := must(NewParsedRevision( []providerimages.ProviderImageManifests{profile(t, "core", "img1", "default", configMapA)}, WithManifestSubstitutions(subs), ))(g) @@ -441,11 +440,11 @@ func TestToAPIRevision(t *testing.T) { }) } -func TestNewRenderedRevision(t *testing.T) { +func TestNewParsedRevision(t *testing.T) { t.Run("error from nonexistent manifest path", func(t *testing.T) { g := NewWithT(t) - _, err := NewRenderedRevision([]providerimages.ProviderImageManifests{ + _, err := NewParsedRevision([]providerimages.ProviderImageManifests{ {ProviderMetadata: providerimages.ProviderMetadata{Name: "p1"}, ImageRef: "img1", Profile: "default", ManifestsPath: "/nonexistent/path/manifests.yaml"}, }) g.Expect(err).To(HaveOccurred()) @@ -454,17 +453,20 @@ func TestNewRenderedRevision(t *testing.T) { t.Run("error from invalid yaml", func(t *testing.T) { g := NewWithT(t) - _, err := NewRenderedRevision([]providerimages.ProviderImageManifests{ + _, err := NewParsedRevision([]providerimages.ProviderImageManifests{ profile(t, "p1", "img1", "default", "not: valid: yaml: ["), }) g.Expect(err).To(HaveOccurred()) }) - t.Run("yaml transformation applied during construction", func(t *testing.T) { + t.Run("objects stored with unexpanded envsubst variables", func(t *testing.T) { g := NewWithT(t) - // Manifest with envsubst variable; revision built from this should - // produce the same contentID as one built from the expanded form. + // Envsubst expansion now happens at install time via EnvsubstTransformer. + // The revision generator stores the raw unexpanded YAML, so a revision + // built from a manifest with ${VAR} has a different contentID than one + // built from the pre-expanded form — even if the same substitutions are + // provided. provWithVar := test.NewProviderImageManifests(t, "p1"). WithImageRef("img1"). WithManifests(`apiVersion: v1 @@ -472,7 +474,7 @@ kind: ConfigMap metadata: name: cm data: - v: "${EXP_BOOTSTRAP_FORMAT_IGNITION}"`). + v: "${MY_VAR}"`). Build() provExpanded := test.NewProviderImageManifests(t, "p1"). WithImageRef("img1"). @@ -481,52 +483,42 @@ kind: ConfigMap metadata: name: cm data: - v: "true"`). + v: "hello"`). Build() - rev1 := must(NewRenderedRevision([]providerimages.ProviderImageManifests{provWithVar}))(g) - rev2 := must(NewRenderedRevision([]providerimages.ProviderImageManifests{provExpanded}))(g) + subs := map[string]string{"MY_VAR": "hello"} + + rev1 := must(NewParsedRevision([]providerimages.ProviderImageManifests{provWithVar}, WithManifestSubstitutions(subs)))(g) + rev2 := must(NewParsedRevision([]providerimages.ProviderImageManifests{provExpanded}, WithManifestSubstitutions(subs)))(g) id1 := must(rev1.ContentID())(g) id2 := must(rev2.ContentID())(g) - g.Expect(id1).To(Equal(id2)) + // Different raw content → different contentIDs even with the same subs. + g.Expect(id1).NotTo(Equal(id2)) }) - t.Run("user substitutions applied during construction", func(t *testing.T) { + t.Run("ManifestSubstitutions returns stored substitutions", func(t *testing.T) { g := NewWithT(t) - // Manifest with a user-provided envsubst variable - provWithVar := test.NewProviderImageManifests(t, "p1"). - WithImageRef("img1"). - WithManifests(`apiVersion: v1 -kind: ConfigMap -metadata: - name: cm -data: - v: "${MY_VAR}"`). - Build() - provExpanded := test.NewProviderImageManifests(t, "p1"). - WithImageRef("img1"). - WithManifests(`apiVersion: v1 -kind: ConfigMap -metadata: - name: cm -data: - v: "hello"`). - Build() + subs := map[string]string{"TLS_MIN_VERSION": "VersionTLS12", "EXP_BOOTSTRAP_FORMAT_IGNITION": "true"} - subs := map[string]string{"MY_VAR": "hello"} + rev := must(NewParsedRevision( + []providerimages.ProviderImageManifests{profile(t, "p1", "img1", "default", configMapA)}, + WithManifestSubstitutions(subs), + ))(g) - // Both use the same substitutions, so the hash of substitutions is identical. - // The manifest content after rendering is also identical. - rev1 := must(NewRenderedRevision([]providerimages.ProviderImageManifests{provWithVar}, WithManifestSubstitutions(subs)))(g) - rev2 := must(NewRenderedRevision([]providerimages.ProviderImageManifests{provExpanded}, WithManifestSubstitutions(subs)))(g) + g.Expect(rev.ManifestSubstitutions()).To(Equal(subs)) + }) - id1 := must(rev1.ContentID())(g) - id2 := must(rev2.ContentID())(g) + t.Run("ManifestSubstitutions returns nil when no substitutions", func(t *testing.T) { + g := NewWithT(t) - g.Expect(id1).To(Equal(id2)) + rev := must(NewParsedRevision( + []providerimages.ProviderImageManifests{profile(t, "p1", "img1", "default", configMapA)}, + ))(g) + + g.Expect(rev.ManifestSubstitutions()).To(BeNil()) }) } @@ -534,7 +526,7 @@ func TestComponents(t *testing.T) { t.Run("returns correct component count and names", func(t *testing.T) { g := NewWithT(t) - rev := must(NewRenderedRevision([]providerimages.ProviderImageManifests{ + rev := must(NewParsedRevision([]providerimages.ProviderImageManifests{ profile(t, "core", "img-core", "default", configMapA), profile(t, "infra", "img-infra", "aws", configMapB), }))(g) @@ -548,59 +540,60 @@ func TestComponents(t *testing.T) { for _, tc := range []struct { name string manifests string - wantCRDs int wantObjects int }{ { - name: "separates CRDs from other objects", + name: "returns all objects including CRDs", manifests: multiDoc(crdA, configMapA, crdB, configMapB), - wantCRDs: 2, - wantObjects: 2, + wantObjects: 4, }, { - name: "component with only CRDs has empty Objects", + name: "CRDs returned by Objects", manifests: crdA, - wantCRDs: 1, - wantObjects: 0, + wantObjects: 1, }, { - name: "component with only objects has empty CRDs", + name: "non-CRD objects returned by Objects", manifests: configMapA, - wantCRDs: 0, wantObjects: 1, }, + { + name: "returns no objects", + manifests: "", + wantObjects: 0, + }, } { t.Run(tc.name, func(t *testing.T) { g := NewWithT(t) - rev := must(NewRenderedRevision([]providerimages.ProviderImageManifests{ + rev := must(NewParsedRevision([]providerimages.ProviderImageManifests{ profile(t, "core", "img1", "default", tc.manifests), }))(g) c := rev.Components()[0] - g.Expect(c.CRDs()).To(HaveLen(tc.wantCRDs)) g.Expect(c.Objects()).To(HaveLen(tc.wantObjects)) }) } - t.Run("CRDs returns client.Object slices matching underlying objects", func(t *testing.T) { + t.Run("CRD stored as unstructured.Unstructured with correct GVK", func(t *testing.T) { g := NewWithT(t) - rev := must(NewRenderedRevision([]providerimages.ProviderImageManifests{ + rev := must(NewParsedRevision([]providerimages.ProviderImageManifests{ profile(t, "core", "img1", "default", crdA), }))(g) - crds := rev.Components()[0].CRDs() - g.Expect(crds).To(HaveLen(1)) - g.Expect(crds[0].GetName()).To(Equal("widgets.example.com")) - g.Expect(crds[0].GetObjectKind().GroupVersionKind().Kind).To(Equal("CustomResourceDefinition")) - g.Expect(crds[0].GetLabels()).To(HaveKeyWithValue(ManagedLabelKey, "core")) + objs := rev.Components()[0].Objects() + g.Expect(objs).To(HaveLen(1)) + g.Expect(objs[0].GetName()).To(Equal("widgets.example.com")) + g.Expect(objs[0].GetObjectKind().GroupVersionKind().Kind).To(Equal("CustomResourceDefinition")) + // Managed-by label is no longer added at render time (Phase 2: moved to ManagedByTransformer). + g.Expect(objs[0].GetLabels()).NotTo(HaveKey("capi-operator.openshift.io/managed-by")) }) - t.Run("Objects returns client.Object slices matching underlying objects", func(t *testing.T) { + t.Run("Objects returns unstructured.Unstructured slices matching underlying objects", func(t *testing.T) { g := NewWithT(t) - rev := must(NewRenderedRevision([]providerimages.ProviderImageManifests{ + rev := must(NewParsedRevision([]providerimages.ProviderImageManifests{ profile(t, "core", "img1", "default", configMapA), }))(g) @@ -608,13 +601,14 @@ func TestComponents(t *testing.T) { g.Expect(objs).To(HaveLen(1)) g.Expect(objs[0].GetName()).To(Equal("config-a")) g.Expect(objs[0].GetObjectKind().GroupVersionKind().Kind).To(Equal("ConfigMap")) - g.Expect(objs[0].GetLabels()).To(HaveKeyWithValue(ManagedLabelKey, "core")) + // Managed-by label is no longer added at render time (Phase 2: moved to ManagedByTransformer). + g.Expect(objs[0].GetLabels()).NotTo(HaveKey("capi-operator.openshift.io/managed-by")) }) t.Run("zero components returns empty slice", func(t *testing.T) { g := NewWithT(t) - rev := must(NewRenderedRevision([]providerimages.ProviderImageManifests{}))(g) + rev := must(NewParsedRevision([]providerimages.ProviderImageManifests{}))(g) g.Expect(rev.Components()).To(BeEmpty()) }) @@ -710,7 +704,7 @@ func TestNewInstallerRevisionFromAPI(t *testing.T) { g.Expect(components[1].Name()).To(Equal("core")) }) - t.Run("renders CRDs and objects from matched profiles", func(t *testing.T) { + t.Run("renders objects from matched profiles", func(t *testing.T) { g := NewWithT(t) profiles := makeProfiles(t) @@ -728,8 +722,8 @@ func TestNewInstallerRevisionFromAPI(t *testing.T) { rev := must(NewInstallerRevisionFromAPI(apiRev, profiles))(g) c := rev.Components()[0] - g.Expect(c.CRDs()).To(HaveLen(1)) - g.Expect(c.Objects()).To(HaveLen(1)) + // azure profile has crdA + configMapA: both returned by Objects() since Phase 2. + g.Expect(c.Objects()).To(HaveLen(2)) }) t.Run("returns error for missing component", func(t *testing.T) { @@ -780,7 +774,7 @@ func TestNewInstallerRevisionFromAPI(t *testing.T) { g.Expect(rev.Components()).To(BeEmpty()) }) - t.Run("succeeds when contentID matches rendered content", func(t *testing.T) { + t.Run("succeeds when contentID matches parsed content", func(t *testing.T) { g := NewWithT(t) profiles := makeProfiles(t) @@ -806,11 +800,11 @@ func TestNewInstallerRevisionFromAPI(t *testing.T) { g.Expect(rev.Components()).To(HaveLen(2)) }) - t.Run("returns error when contentID does not match rendered content", func(t *testing.T) { + t.Run("returns error when contentID does not match parsed content", func(t *testing.T) { g := NewWithT(t) apiRev := operatorv1alpha1.ClusterAPIInstallerRevision{ - ContentID: "does-not-match-any-rendered-content", + ContentID: "does-not-match-any-parsed-content", Components: []operatorv1alpha1.ClusterAPIInstallerComponent{ {ClusterAPIInstallerComponentSource: operatorv1alpha1.ClusterAPIInstallerComponentSource{ Type: operatorv1alpha1.InstallerComponentTypeImage, Image: operatorv1alpha1.ClusterAPIInstallerComponentImage{ @@ -825,10 +819,14 @@ func TestNewInstallerRevisionFromAPI(t *testing.T) { g.Expect(err.Error()).To(ContainSubstring("content ID mismatch")) }) - t.Run("substitutions from API revision applied during rendering", func(t *testing.T) { + t.Run("substitutions from API revision included in content ID", func(t *testing.T) { g := NewWithT(t) - // Create a profile with a variable that needs substitution + // Create a profile with an envsubst variable. Envsubst is no longer + // applied at render time (Phase 2); it happens at install time via + // EnvsubstTransformer. Substitutions are still recorded on the revision + // and included in the content ID so that different substitutions produce + // different revisions. prof := test.NewProviderImageManifests(t, "core"). WithImageRef("quay.io/openshift/core@sha256:aaaa"). WithProfile("default"). @@ -842,18 +840,18 @@ data: subs := map[string]string{"TLS_MIN_VERSION": "VersionTLS12"} - // Create a revision with substitutions and convert to API revision - rev := must(NewRenderedRevision( + // Create a revision with substitutions and convert to API revision. + rev := must(NewParsedRevision( []providerimages.ProviderImageManifests{prof}, WithManifestSubstitutions(subs), ))(g) apiRev := must(forInstall(g, rev, "4.18.0", 1).ToAPIRevision())(g) - // Round-trip: reconstruct from API revision with same profiles + // Round-trip: reconstruct from API revision with same profiles. + // Content ID validation must pass, confirming substitutions are included. reconstructed := must(NewInstallerRevisionFromAPI(apiRev, []providerimages.ProviderImageManifests{prof}))(g) - // Content ID validation passes, confirming substitutions were applied correctly reconstructedContentID := must(reconstructed.ContentID())(g) g.Expect(reconstructedContentID).To(Equal(apiRev.ContentID)) g.Expect(reconstructed.Components()).To(HaveLen(1)) diff --git a/pkg/revisiongenerator/transform.go b/pkg/revisiongenerator/transform.go deleted file mode 100644 index 6e93a606c..000000000 --- a/pkg/revisiongenerator/transform.go +++ /dev/null @@ -1,84 +0,0 @@ -/* -Copyright 2026 Red Hat, Inc. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package revisiongenerator - -import ( - "fmt" - "strings" - - "github.com/drone/envsubst/v2" - "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" - - "github.com/openshift/cluster-capi-operator/pkg/operatorstatus" - "github.com/openshift/cluster-capi-operator/pkg/providerimages" -) - -// ManagedLabelKey is a label key used to identify objects managed by the CAPI operator. -const ManagedLabelKey = operatorstatus.CAPIOperatorIdentifierDomain + "/managed-by" - -func envSubstSubstitutions(key string) string { - switch key { - // Used only in the AWS provider. Eventually, we intend to move this into - // provider metadata. - case "EXP_BOOTSTRAP_FORMAT_IGNITION": - return "true" - default: - return "" - } -} - -// IMPORTANT NOTE: changes to transformYaml or transformObject which are not -// dependent on a change in the API revision are breaking changes: it will -// update the revision's content ID without the revision having been updated. -// The controller will recover when a new revision is created, but it will no -// longer be able to reconcile the old revision. This should be done with care. - -// transformYaml applies transformations to an object's YAML before it is unmarshalled. -func transformYaml(providerProfile *providerimages.ProviderImageManifests, yaml string, substitutions map[string]string) (string, error) { - // Expand envsubst variables, checking user-provided substitutions first. - yaml, err := envsubst.Eval(yaml, func(key string) string { - if v, ok := substitutions[key]; ok { - return v - } - - return envSubstSubstitutions(key) - }) - if err != nil { - return "", fmt.Errorf("failed to substitute variables: %w", err) - } - - // Replace self-image-ref with actual image ref. - if providerProfile.SelfImageRef != "" { - yaml = strings.ReplaceAll(yaml, providerProfile.SelfImageRef, providerProfile.ImageRef) - } - - return yaml, nil -} - -// transformObject applies transformations to an object after it is -// unmarshalled. -func transformObject(obj unstructured.Unstructured, componentName string) unstructured.Unstructured { - labels := obj.GetLabels() - if labels == nil { - labels = map[string]string{} - } - - labels[ManagedLabelKey] = componentName - obj.SetLabels(labels) - - return obj -} diff --git a/pkg/revisiongenerator/transform_test.go b/pkg/revisiongenerator/transform_test.go deleted file mode 100644 index 14410d523..000000000 --- a/pkg/revisiongenerator/transform_test.go +++ /dev/null @@ -1,206 +0,0 @@ -/* -Copyright 2026 Red Hat, Inc. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package revisiongenerator - -import ( - "testing" - - . "github.com/onsi/gomega" - "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" - - "github.com/openshift/cluster-capi-operator/pkg/providerimages" -) - -func TestTransformYaml(t *testing.T) { - tests := []struct { - name string - yaml string - profile providerimages.ProviderImageManifests - substitutions map[string]string - expected string - }{ - { - name: "known envsubst variable replaced", - yaml: "bootstrap: ${EXP_BOOTSTRAP_FORMAT_IGNITION}", - profile: providerimages.ProviderImageManifests{ - ImageRef: "example.com/img@sha256:abc", - }, - expected: "bootstrap: true", - }, - { - name: "unknown variable replaced with empty", - yaml: "value: ${UNKNOWN_VAR}", - profile: providerimages.ProviderImageManifests{ - ImageRef: "example.com/img@sha256:abc", - }, - expected: "value: ", - }, - { - name: "no variables passes through unchanged", - yaml: "apiVersion: v1\nkind: ConfigMap", - profile: providerimages.ProviderImageManifests{ - ImageRef: "example.com/img@sha256:abc", - }, - expected: "apiVersion: v1\nkind: ConfigMap", - }, - { - name: "self image ref replaced", - yaml: "image: placeholder-ref", - profile: providerimages.ProviderImageManifests{ - ProviderMetadata: providerimages.ProviderMetadata{ - SelfImageRef: "placeholder-ref", - }, - ImageRef: "real-ref", - }, - expected: "image: real-ref", - }, - { - name: "self image ref all occurrences", - yaml: "a: old-ref\nb: old-ref", - profile: providerimages.ProviderImageManifests{ - ProviderMetadata: providerimages.ProviderMetadata{ - SelfImageRef: "old-ref", - }, - ImageRef: "new-ref", - }, - expected: "a: new-ref\nb: new-ref", - }, - { - name: "empty self image ref skips replacement", - yaml: "image: something", - profile: providerimages.ProviderImageManifests{ - ImageRef: "new-ref", - }, - expected: "image: something", - }, - { - name: "both transformations applied", - yaml: "format: ${EXP_BOOTSTRAP_FORMAT_IGNITION}\nimage: old-ref", - profile: providerimages.ProviderImageManifests{ - ProviderMetadata: providerimages.ProviderMetadata{ - SelfImageRef: "old-ref", - }, - ImageRef: "new-ref", - }, - expected: "format: true\nimage: new-ref", - }, - { - name: "envsubst applied before image replacement", - yaml: "image: ${EXP_BOOTSTRAP_FORMAT_IGNITION}", - profile: providerimages.ProviderImageManifests{ - ProviderMetadata: providerimages.ProviderMetadata{ - SelfImageRef: "true", - }, - ImageRef: "replaced", - }, - expected: "image: replaced", - }, - { - name: "empty yaml", - yaml: "", - profile: providerimages.ProviderImageManifests{ - ImageRef: "example.com/img@sha256:abc", - }, - expected: "", - }, - { - name: "user substitution applied", - yaml: "version: ${TLS_MIN_VERSION}", - profile: providerimages.ProviderImageManifests{ - ImageRef: "example.com/img@sha256:abc", - }, - substitutions: map[string]string{"TLS_MIN_VERSION": "VersionTLS12"}, - expected: "version: VersionTLS12", - }, - { - name: "user substitution overrides hardcoded", - yaml: "bootstrap: ${EXP_BOOTSTRAP_FORMAT_IGNITION}", - profile: providerimages.ProviderImageManifests{ - ImageRef: "example.com/img@sha256:abc", - }, - substitutions: map[string]string{"EXP_BOOTSTRAP_FORMAT_IGNITION": "false"}, - expected: "bootstrap: false", - }, - { - name: "unknown var with no user substitution still empty", - yaml: "value: ${UNKNOWN_VAR}", - profile: providerimages.ProviderImageManifests{ - ImageRef: "example.com/img@sha256:abc", - }, - substitutions: map[string]string{"OTHER_VAR": "something"}, - expected: "value: ", - }, - { - name: "multiple substitutions applied", - yaml: "version: ${TLS_MIN_VERSION}\nciphers: ${TLS_CIPHER_SUITES}", - profile: providerimages.ProviderImageManifests{ - ImageRef: "example.com/img@sha256:abc", - }, - substitutions: map[string]string{ - "TLS_MIN_VERSION": "VersionTLS12", - "TLS_CIPHER_SUITES": "TLS_AES_128_GCM_SHA256,TLS_AES_256_GCM_SHA384", - }, - expected: "version: VersionTLS12\nciphers: TLS_AES_128_GCM_SHA256,TLS_AES_256_GCM_SHA384", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - g := NewWithT(t) - - result, err := transformYaml(&tt.profile, tt.yaml, tt.substitutions) - g.Expect(err).NotTo(HaveOccurred()) - g.Expect(result).To(Equal(tt.expected)) - }) - } -} - -func TestTransformObject(t *testing.T) { - tests := []struct { - name string - labels map[string]string - componentName string - }{ - { - name: "adds managed label to object with no labels", - labels: nil, - componentName: "core", - }, - { - name: "preserves existing labels", - labels: map[string]string{"existing-key": "existing-value"}, - componentName: "infra", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - g := NewWithT(t) - - obj := unstructured.Unstructured{} - obj.SetLabels(tt.labels) - - result := transformObject(obj, tt.componentName) - - g.Expect(result.GetLabels()).To(HaveKeyWithValue(ManagedLabelKey, tt.componentName)) - - for k, v := range tt.labels { - g.Expect(result.GetLabels()).To(HaveKeyWithValue(k, v)) - } - }) - } -} diff --git a/pkg/revisiongenerator/validate.go b/pkg/revisiongenerator/validate.go index b49f0c47a..fdad0ad2d 100644 --- a/pkg/revisiongenerator/validate.go +++ b/pkg/revisiongenerator/validate.go @@ -16,20 +16,12 @@ limitations under the License. package revisiongenerator -import ( - "errors" - "fmt" - - "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/reconcile" - - "github.com/openshift/cluster-capi-operator/pkg/operatorstatus" -) +import "github.com/openshift/cluster-capi-operator/pkg/operatorstatus" const ( // AdoptExistingAnnotation controls whether collision protection is disabled // for a specific object, allowing us to adopt an object which already - // exists on the cluster but is not managed by the CAPI operator. Permitted + // exists on the cluster but is not managed by the CAPI operator. Permitted // values are AdoptExistingAlways and AdoptExistingNever. The annotation is // stripped from the object before it is applied to the cluster, but is // included in the content hash so that adding or removing the annotation @@ -46,54 +38,3 @@ const ( // setting the annotation at all. AdoptExistingNever = "never" ) - -// ErrInvalidAdoptExistingAnnotation is returned when an object has an -// adopt-existing annotation with an unrecognised value. -var ErrInvalidAdoptExistingAnnotation = errors.New("invalid " + AdoptExistingAnnotation + " annotation value") - -// ValidateAdoptExistingAnnotation returns an error if the object has an -// adopt-existing annotation with an unrecognised value. -func ValidateAdoptExistingAnnotation(obj client.Object) error { - annotations := obj.GetAnnotations() - if annotations == nil { - return nil - } - - value, exists := annotations[AdoptExistingAnnotation] - if !exists { - return nil - } - - switch value { - case AdoptExistingAlways, AdoptExistingNever: - return nil - default: - return fmt.Errorf( - "%w: %q on %s %s/%s", - reconcile.TerminalError(ErrInvalidAdoptExistingAnnotation), - value, - obj.GetObjectKind().GroupVersionKind().Kind, - obj.GetNamespace(), - obj.GetName(), - ) - } -} - -// validateRenderedRevision validates all objects in a rendered revision. -func validateRenderedRevision(rev *renderedRevision) error { - for _, component := range rev.components { - for _, obj := range component.CRDs() { - if err := ValidateAdoptExistingAnnotation(obj); err != nil { - return err - } - } - - for _, obj := range component.Objects() { - if err := ValidateAdoptExistingAnnotation(obj); err != nil { - return err - } - } - } - - return nil -} diff --git a/pkg/revisiongenerator/validate_test.go b/pkg/revisiongenerator/validate_test.go deleted file mode 100644 index bcb659c97..000000000 --- a/pkg/revisiongenerator/validate_test.go +++ /dev/null @@ -1,151 +0,0 @@ -/* -Copyright 2026 Red Hat, Inc. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package revisiongenerator - -import ( - "errors" - "testing" - - "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" - "sigs.k8s.io/controller-runtime/pkg/reconcile" -) - -func TestValidateAdoptExistingAnnotation(t *testing.T) { - tests := []struct { - name string - annotations map[string]string - wantErr bool - wantTerminal bool - }{ - { - name: "nil annotations", - annotations: nil, - wantErr: false, - }, - { - name: "no adopt-existing annotation", - annotations: map[string]string{"other": "value"}, - wantErr: false, - }, - { - name: "adopt-existing always", - annotations: map[string]string{AdoptExistingAnnotation: AdoptExistingAlways}, - wantErr: false, - }, - { - name: "adopt-existing never", - annotations: map[string]string{AdoptExistingAnnotation: AdoptExistingNever}, - wantErr: false, - }, - { - name: "adopt-existing invalid value", - annotations: map[string]string{AdoptExistingAnnotation: "invalid"}, - wantErr: true, - wantTerminal: true, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - obj := &unstructured.Unstructured{} - obj.SetAnnotations(tt.annotations) - - err := ValidateAdoptExistingAnnotation(obj) - if (err != nil) != tt.wantErr { - t.Errorf("ValidateAdoptExistingAnnotation() error = %v, wantErr %v", err, tt.wantErr) - } - - if tt.wantErr { - if !errors.Is(err, ErrInvalidAdoptExistingAnnotation) { - t.Errorf("expected error to wrap ErrInvalidAdoptExistingAnnotation, got %v", err) - } - - if tt.wantTerminal && !errors.Is(err, reconcile.TerminalError(nil)) { - t.Errorf("expected terminal error, got %v", err) - } - } - }) - } -} - -func TestValidateRenderedRevision(t *testing.T) { - t.Run("valid revision", func(t *testing.T) { - rev := &renderedRevision{ - components: []*renderedComponent{ - { - objects: []unstructured.Unstructured{ - makeUnstructuredWithAnnotations(nil), - makeUnstructuredWithAnnotations(map[string]string{AdoptExistingAnnotation: AdoptExistingAlways}), - }, - }, - }, - } - - if err := validateRenderedRevision(rev); err != nil { - t.Errorf("expected no error, got %v", err) - } - }) - - t.Run("invalid annotation in objects", func(t *testing.T) { - rev := &renderedRevision{ - components: []*renderedComponent{ - { - objects: []unstructured.Unstructured{ - makeUnstructuredWithAnnotations(map[string]string{AdoptExistingAnnotation: "bad"}), - }, - }, - }, - } - - err := validateRenderedRevision(rev) - if err == nil { - t.Fatal("expected error, got nil") - } - - if !errors.Is(err, ErrInvalidAdoptExistingAnnotation) { - t.Errorf("expected error to wrap ErrInvalidAdoptExistingAnnotation, got %v", err) - } - }) - - t.Run("invalid annotation in CRDs", func(t *testing.T) { - rev := &renderedRevision{ - components: []*renderedComponent{ - { - crds: []unstructured.Unstructured{ - makeUnstructuredWithAnnotations(map[string]string{AdoptExistingAnnotation: "bad"}), - }, - }, - }, - } - - err := validateRenderedRevision(rev) - if err == nil { - t.Fatal("expected error, got nil") - } - - if !errors.Is(err, ErrInvalidAdoptExistingAnnotation) { - t.Errorf("expected error to wrap ErrInvalidAdoptExistingAnnotation, got %v", err) - } - }) -} - -func makeUnstructuredWithAnnotations(annotations map[string]string) unstructured.Unstructured { - obj := unstructured.Unstructured{} - obj.SetAnnotations(annotations) - - return obj -}