Skip to content
Closed
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions pkg/controllers/installer/probes.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@ func allProbes() []*probing.GroupKindSelector {
return []*probing.GroupKindSelector{
crdEstablishedProbe(),
deploymentAvailableProbe(),
compatibilityRequirementAdmittedProbe(),
compatibilityRequirementCompatibleProbe(),
Comment thread
theobarberbany marked this conversation as resolved.
}
}

Expand Down Expand Up @@ -88,6 +90,26 @@ func probeSucceededPredicate(probes ...*probing.GroupKindSelector) predicate.Pre
}
}

// compatibilityRequirementAdmittedProbe checks that a CompatibilityRequirement
// has the Admitted condition set to True. This confirms the validating webhook
// is in place to guard against future incompatible CRD updates.
func compatibilityRequirementAdmittedProbe() *probing.GroupKindSelector {
return &probing.GroupKindSelector{
GroupKind: schema.GroupKind{Group: "apiextensions.openshift.io", Kind: "CompatibilityRequirement"},
Prober: &probing.ConditionProbe{Type: "Admitted", Status: "True"},
}
}

// compatibilityRequirementCompatibleProbe checks that a CompatibilityRequirement
// has the Compatible condition set to True. This confirms the current CRD
// satisfies the compatibility contract.
func compatibilityRequirementCompatibleProbe() *probing.GroupKindSelector {
return &probing.GroupKindSelector{
GroupKind: schema.GroupKind{Group: "apiextensions.openshift.io", Kind: "CompatibilityRequirement"},
Prober: &probing.ConditionProbe{Type: "Compatible", Status: "True"},
}
}

// noGenerationPredicate returns a predicate that passes all update events for
// objects that don't have generation tracking (e.g., ConfigMaps, Secrets).
// Objects with generation tracking have it initialised to 1 on creation, so
Expand Down
5 changes: 4 additions & 1 deletion pkg/controllers/installer/revision_reconciler.go
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,10 @@ func (r *revisionReconciler) reconcile(ctx context.Context, revisions []operator
// 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))
rev, err := revisiongenerator.NewInstallerRevisionFromAPI(apiRev, r.providerProfiles,
revisiongenerator.WithObjectCollectors(r.collectObjects),
revisiongenerator.WithUnmanagedCRDs(apiRev.UnmanagedCustomResourceDefinitions),
)
if err != nil {
err = fmt.Errorf("error creating installer revision from API revision %s: %w", apiRev.Name, reconcile.TerminalError(err))
}
Expand Down
25 changes: 16 additions & 9 deletions pkg/controllers/revision/revision_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -89,13 +89,7 @@ func (r *RevisionController) Reconcile(ctx context.Context, _ ctrl.Request) (ctr
}

func (r *RevisionController) reconcile(ctx context.Context, log logr.Logger) operatorstatus.ReconcileResult {
// Generate a desired revision from the current state
desiredRevision, result := r.generateDesiredRevision(ctx)
if result != nil {
return *result
}

// Get ClusterAPI singleton
// Get ClusterAPI singleton first — generateDesiredRevision needs spec fields.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is such a clanker comment 😅

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup - welcome to the future ..

clusterAPI := &operatorv1alpha1.ClusterAPI{}
if err := r.Get(ctx, client.ObjectKey{Name: clusterAPIName}, clusterAPI); err != nil {
if apierrors.IsNotFound(err) {
Expand All @@ -105,6 +99,16 @@ func (r *RevisionController) reconcile(ctx context.Context, log logr.Logger) ope
return opresult.Error(fmt.Errorf("fetching ClusterAPI: %w", err))
}

var unmanagedCRDs []string
if clusterAPI.Spec != nil {
unmanagedCRDs = clusterAPI.Spec.UnmanagedCustomResourceDefinitions
}

desiredRevision, result := r.generateDesiredRevision(ctx, unmanagedCRDs)
if result != nil {
return *result
}

// Create a reverse sorted, merged list of revisions. It will prepend the
// new revision if necessary. Note that the latest revision is always
// first, and there is guaranteed to be at least one revision.
Expand Down Expand Up @@ -134,7 +138,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, unmanagedCRDs []string) (revisiongenerator.RenderedRevision, *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))
Expand All @@ -147,7 +151,10 @@ 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.NewRenderedRevision(providerComponents,
revisiongenerator.WithManifestSubstitutions(r.manifestSubstitutions),
revisiongenerator.WithUnmanagedCRDs(unmanagedCRDs),
)
if err != nil {
return nil, opresult.ErrorP(fmt.Errorf("error creating rendered revision: %w", err))
}
Expand Down
36 changes: 36 additions & 0 deletions pkg/controllers/revision/revision_controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -379,6 +379,42 @@ var _ = Describe("RevisionController", Serial, func() {
Expect(updatedClusterAPI.Status.Revisions).To(HaveLen(16))
}, defaultNodeTimeout)

Context("when unmanagedCustomResourceDefinitions is set on the spec", func() {
It("should include them in the revision status", func(ctx context.Context) {
By("setting unmanagedCustomResourceDefinitions on the ClusterAPI spec")
Eventually(kWithCtx(ctx).Update(clusterAPI, func() {
clusterAPI.Spec.UnmanagedCustomResourceDefinitions = []string{"widgets.example.com"}
})).WithContext(ctx).Should(Succeed())

Eventually(kWithCtx(ctx).Object(clusterAPI)).
WithContext(ctx).
Should(HaveField("Status.Revisions", ContainElement(
HaveField("UnmanagedCustomResourceDefinitions", Equal([]string{"widgets.example.com"})),
)))
}, defaultNodeTimeout)

It("should produce a different content ID", func(ctx context.Context) {
By("capturing the original content ID")
Eventually(kWithCtx(ctx).Object(clusterAPI)).
WithContext(ctx).
Should(HaveField("Status.Revisions", HaveLen(1)))
originalContentID := clusterAPI.Status.Revisions[0].ContentID

By("setting unmanagedCustomResourceDefinitions on the ClusterAPI spec")
Eventually(kWithCtx(ctx).Update(clusterAPI, func() {
clusterAPI.Spec.UnmanagedCustomResourceDefinitions = []string{"widgets.example.com"}
})).WithContext(ctx).Should(Succeed())

By("waiting for a new revision")
Eventually(kWithCtx(ctx).Object(clusterAPI)).
WithContext(ctx).
Should(HaveField("Status.Revisions", HaveLen(2)))

newRev := latestRevision(clusterAPI.Status.Revisions)
Expect(newRev.ContentID).NotTo(Equal(originalContentID))
}, defaultNodeTimeout)
})

It("sets Available=False with NonRetryableError when manifest has invalid adopt-existing annotation", func(ctx context.Context) {
// Stop first manager (created by BeforeEach with valid providers)
mgr.stop()
Expand Down
5 changes: 4 additions & 1 deletion pkg/controllers/revision/suite_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (
. "github.com/onsi/gomega"
configv1 "github.com/openshift/api/config/v1"
"github.com/openshift/cluster-capi-operator/pkg/providerimages"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/client-go/rest"
"k8s.io/klog/v2"
"sigs.k8s.io/controller-runtime/pkg/client"
Expand Down Expand Up @@ -73,10 +74,12 @@ var _ = BeforeSuite(func() {
func setupProviderFixtures() {
tb := GinkgoTB()

widgetCRD := test.GenerateCRD(schema.GroupVersionKind{Group: "example.com", Version: "v1", Kind: "Widget"})

defaultProviderImgs = []providerimages.ProviderImageManifests{
test.NewProviderImageManifests(tb, "core").
WithImageRef("registry.example.com/core@sha256:0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef").
WithManifests(test.ConfigMapYAML("core-cm")).
WithManifests(test.ConfigMapYAML("core-cm"), test.CRDToYAML(widgetCRD)).
Build(),
test.NewProviderImageManifests(tb, "infra-aws").
WithInstallOrder(20).
Expand Down
78 changes: 78 additions & 0 deletions pkg/revisiongenerator/compatibility.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
/*
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"

apiextensionsv1alpha1 "github.com/openshift/api/apiextensions/v1alpha1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/runtime"
k8syaml "sigs.k8s.io/yaml"
)

const (
compatibilityRequirementNamePrefix = "ccapio-"
capiNamespace = "openshift-cluster-api"
)

// buildCompatibilityRequirement constructs a CompatibilityRequirement for the
// given CRD and returns it as an unstructured object for inclusion in a
// renderedComponent.
func buildCompatibilityRequirement(crd unstructured.Unstructured) (unstructured.Unstructured, error) {
crdYAML, err := k8syaml.Marshal(crd.Object)
if err != nil {
return unstructured.Unstructured{}, fmt.Errorf("marshalling CRD %s to YAML: %w", crd.GetName(), err)
}

cr := &apiextensionsv1alpha1.CompatibilityRequirement{
TypeMeta: metav1.TypeMeta{
APIVersion: apiextensionsv1alpha1.GroupVersion.String(),
Kind: "CompatibilityRequirement",
},
ObjectMeta: metav1.ObjectMeta{
Name: compatibilityRequirementNamePrefix + crd.GetName(),
},
Comment on lines +49 to +50

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Guard against invalid CompatibilityRequirement names for long CRD names.

Line 49 can generate a metadata.name longer than Kubernetes’ max DNS-subdomain length when a CRD name is already near the limit. That will make the synthetic object invalid and break reconciliation for those unmanaged CRDs.

Suggested fix
+const maxK8sNameLen = 253
+
+func compatibilityRequirementName(crdName string) string {
+	name := compatibilityRequirementNamePrefix + crdName
+	if len(name) <= maxK8sNameLen {
+		return name
+	}
+
+	// keep deterministic name while preserving uniqueness
+	sum := sha256.Sum256([]byte(crdName))
+	suffix := "-" + hex.EncodeToString(sum[:8])
+	keep := maxK8sNameLen - len(suffix)
+	return name[:keep] + suffix
+}
+
 func buildCompatibilityRequirement(crd unstructured.Unstructured) (unstructured.Unstructured, error) {
@@
 		ObjectMeta: metav1.ObjectMeta{
-			Name: compatibilityRequirementNamePrefix + crd.GetName(),
+			Name: compatibilityRequirementName(crd.GetName()),
 		},
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/revisiongenerator/compatibility.go` around lines 49 - 50, The Name field
construction at line 49 concatenates compatibilityRequirementNamePrefix with
crd.GetName() without validating the total length, which can exceed Kubernetes'
253-character DNS-subdomain limit for long CRD names. Add length validation to
ensure the resulting name stays within the limit. If the concatenated name would
be too long, either truncate the CRD name proportionally or use a hash-based
suffix of the CRD name to keep the overall name length valid and deterministic
while preventing reconciliation failures for long CRD names.

Spec: apiextensionsv1alpha1.CompatibilityRequirementSpec{
CompatibilitySchema: apiextensionsv1alpha1.CompatibilitySchema{
CustomResourceDefinition: apiextensionsv1alpha1.CRDData{
Type: apiextensionsv1alpha1.CRDDataTypeYAML,
Data: string(crdYAML),
},
},
CustomResourceDefinitionSchemaValidation: apiextensionsv1alpha1.CustomResourceDefinitionSchemaValidation{
Action: apiextensionsv1alpha1.CRDAdmitActionDeny,
},
ObjectSchemaValidation: apiextensionsv1alpha1.ObjectSchemaValidation{
Action: apiextensionsv1alpha1.CRDAdmitActionDeny,
NamespaceSelector: metav1.LabelSelector{
MatchLabels: map[string]string{
"kubernetes.io/metadata.name": capiNamespace,
},
},
},
},
}

data, err := runtime.DefaultUnstructuredConverter.ToUnstructured(cr)
if err != nil {
return unstructured.Unstructured{}, fmt.Errorf("converting CompatibilityRequirement to unstructured: %w", err)
}

return unstructured.Unstructured{Object: data}, nil
}
Loading