Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
104 changes: 104 additions & 0 deletions pkg/controllers/capiinstaller/apiextensions.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
/*
Copyright 2025 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 capiinstaller

import (
"context"
"fmt"

"github.com/openshift/library-go/pkg/operator/events"
"github.com/openshift/library-go/pkg/operator/resource/resourceapply"
"github.com/openshift/library-go/pkg/operator/resource/resourcehelper"
"github.com/openshift/library-go/pkg/operator/resource/resourcemerge"
apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
apiextclientv1 "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset/typed/apiextensions/v1"
apierrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/klog/v2"
)

// applyCustomResourceDefinitionV1Improved applies the required CustomResourceDefinition to the cluster.
//
//nolint:forcetypeassert
func applyCustomResourceDefinitionV1Improved(ctx context.Context, client apiextclientv1.CustomResourceDefinitionsGetter, recorder events.Recorder, required *apiextensionsv1.CustomResourceDefinition) (*apiextensionsv1.CustomResourceDefinition, bool, error) {
existing, err := client.CustomResourceDefinitions().Get(ctx, required.Name, metav1.GetOptions{})
if apierrors.IsNotFound(err) {
requiredCopy := required.DeepCopy()
actual, err := client.CustomResourceDefinitions().Create(
ctx, resourcemerge.WithCleanLabelsAndAnnotations(requiredCopy).(*apiextensionsv1.CustomResourceDefinition), metav1.CreateOptions{})
resourcehelper.ReportCreateEvent(recorder, required, err)

return actual, true, fmt.Errorf("error creating CustomResourceDefinition %q: %w", required.Name, err)
}

if err != nil {
return nil, false, fmt.Errorf("error getting CustomResourceDefinition %q: %w", required.Name, err)
}

modified := false
existingCopy := existing.DeepCopy()

ensureCustomResourceDefinitionV1CaBundle(required, *existing)

resourcemerge.EnsureCustomResourceDefinitionV1(&modified, existingCopy, *required)

if !modified {
return existing, false, nil
}
Comment on lines 51 to 60
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Input mutation: required is modified as a side effect

ensureCustomResourceDefinitionV1CaBundle mutates the required parameter directly (line 49), which could cause unexpected behavior if the caller reuses the object. Consider making a deep copy of required before mutation, similar to how existingCopy is handled.

 	modified := false
 	existingCopy := existing.DeepCopy()
+	requiredCopy := required.DeepCopy()

-	ensureCustomResourceDefinitionV1CaBundle(required, *existing)
+	ensureCustomResourceDefinitionV1CaBundle(requiredCopy, *existing)

-	resourcemerge.EnsureCustomResourceDefinitionV1(&modified, existingCopy, *required)
+	resourcemerge.EnsureCustomResourceDefinitionV1(&modified, existingCopy, *requiredCopy)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
modified := false
existingCopy := existing.DeepCopy()
ensureCustomResourceDefinitionV1CaBundle(required, *existing)
resourcemerge.EnsureCustomResourceDefinitionV1(&modified, existingCopy, *required)
if !modified {
return existing, false, nil
}
modified := false
existingCopy := existing.DeepCopy()
requiredCopy := required.DeepCopy()
ensureCustomResourceDefinitionV1CaBundle(requiredCopy, *existing)
resourcemerge.EnsureCustomResourceDefinitionV1(&modified, existingCopy, *requiredCopy)
if !modified {
return existing, false, nil
}
🤖 Prompt for AI Agents
In pkg/controllers/capiinstaller/apiextensions.go around lines 46-54,
ensureCustomResourceDefinitionV1CaBundle mutates the required object passed in;
to avoid side effects, make a deep copy of required (e.g., reqCopy :=
required.DeepCopy()) and use that copy when calling
ensureCustomResourceDefinitionV1CaBundle and the subsequent
resourcemerge.EnsureCustomResourceDefinitionV1 call so the original required
remains unchanged.


if klog.V(2).Enabled() {
klog.Infof("CustomResourceDefinition %q changes: %s", existing.Name, resourceapply.JSONPatchNoError(existing, existingCopy))
}

actual, err := client.CustomResourceDefinitions().Update(ctx, existingCopy, metav1.UpdateOptions{})
resourcehelper.ReportUpdateEvent(recorder, required, err)

return actual, true, fmt.Errorf("error updating CustomResourceDefinition %q: %w", required.Name, err)
}

// injectCABundleAnnotation is the annotation used to indicate into which resources
// the service-ca controller should inject the CA bundle.
const injectCABundleAnnotation = "service.beta.openshift.io/inject-cabundle"

// ensureCustomResourceDefinitionV1CaBundle ensures that the field
// spec.Conversion.Webhook.ClientConfig.CABundle of a CRD is not managed by the CVO when
// the service-ca controller is responsible for the field.
// Note: this is the same way as CVO does it https://github.com/openshift/cluster-version-operator/blob/0e6c916f99e05983190202575bb530200560acb9/lib/resourcemerge/apiext.go#L34
func ensureCustomResourceDefinitionV1CaBundle(required *apiextensionsv1.CustomResourceDefinition, existing apiextensionsv1.CustomResourceDefinition) {
if val, ok := existing.ObjectMeta.Annotations[injectCABundleAnnotation]; !ok || val != "true" {
return
}

req := required.Spec.Conversion
if req == nil ||
req.Webhook == nil ||
req.Webhook.ClientConfig == nil {
return
}

if req.Strategy != apiextensionsv1.WebhookConverter {
// The service CA bundle is only injected by the service-ca controller into
// the CRD if the CRD is configured to use a webhook for conversion
return
}

exc := existing.Spec.Conversion
if exc != nil &&
exc.Webhook != nil &&
exc.Webhook.ClientConfig != nil {
req.Webhook.ClientConfig.CABundle = exc.Webhook.ClientConfig.CABundle
}
}
45 changes: 38 additions & 7 deletions pkg/controllers/capiinstaller/capi_installer_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -176,12 +176,36 @@ func (r *CapiInstallerController) reconcile(ctx context.Context, log logr.Logger

// applyProviderComponents applies the provider components to the cluster.
// It does so by differentiating between static components and dynamic components (i.e. Deployments).
//
//nolint:funlen
func (r *CapiInstallerController) applyProviderComponents(ctx context.Context, components []string) error {
componentsFilenames, componentsAssets, deploymentsFilenames, deploymentsAssets, err := getProviderComponents(r.Scheme, components)
componentsFilenames, componentsAssets, deploymentsFilenames, deploymentsAssets, customResourceDefinitionFilenames, customResourceDefinitionAssets, err := getProviderComponents(r.Scheme, components)
if err != nil {
return fmt.Errorf("error getting provider components: %w", err)
}

// For each of the CRD components perform a CRD-specific apply.
for _, c := range customResourceDefinitionFilenames {
customResourceDefinitionManifest, ok := customResourceDefinitionAssets[c]
if !ok {
panic("error finding custom resource definition manifest")
}

obj, err := yamlToRuntimeObject(r.Scheme, customResourceDefinitionManifest)
if err != nil {
return fmt.Errorf("error parsing CAPI provider CRD manifest %q: %w", c, err)
}

crd, ok := obj.(*apiextensionsv1.CustomResourceDefinition)
if !ok {
return fmt.Errorf("error casting object to CustomResourceDefinition: %w", err)
}

if _, _, err := applyCustomResourceDefinitionV1Improved(ctx, r.APIExtensionsClient.ApiextensionsV1(), events.NewInMemoryRecorder("cluster-capi-operator-capi-installer-apply-client", clock.RealClock{}), crd); err != nil {
return fmt.Errorf("error applying CAPI provider CRD %q: %w", crd.Name, err)
}
}

// Perform a Direct apply of the static components.
res := resourceapply.ApplyDirectly(
ctx,
Expand Down Expand Up @@ -234,18 +258,21 @@ func (r *CapiInstallerController) applyProviderComponents(ctx context.Context, c

// getProviderComponents parses the provided list of components into a map of filenames and assets.
// Deployments are handled separately so are returned in a separate map.
func getProviderComponents(scheme *runtime.Scheme, components []string) ([]string, map[string]string, []string, map[string]string, error) {
func getProviderComponents(scheme *runtime.Scheme, components []string) ([]string, map[string]string, []string, map[string]string, []string, map[string]string, error) {
componentsFilenames := []string{}
componentsAssets := make(map[string]string)

deploymentsFilenames := []string{}
deploymentsAssets := make(map[string]string)

customResourceDefinitionFilenames := []string{}
customResourceDefinitionAssets := make(map[string]string)

for i, m := range components {
// Parse the YAML manifests into unstructure objects.
u, err := yamlToUnstructured(scheme, m)
if err != nil {
return nil, nil, nil, nil, fmt.Errorf("error parsing provider component at position %d to unstructured: %w", i, err)
return nil, nil, nil, nil, nil, nil, fmt.Errorf("error parsing provider component at position %d to unstructured: %w", i, err)
}

name := fmt.Sprintf("%s/%s/%s - %s",
Expand All @@ -255,17 +282,21 @@ func getProviderComponents(scheme *runtime.Scheme, components []string) ([]strin
getResourceName(u.GetNamespace(), u.GetName()),
)

// Divide manifests into static vs deployment components.
if u.GroupVersionKind().Kind == "Deployment" {
// Divide manifests into static vs deployment vs crd components.
switch u.GroupVersionKind().Kind {
case "Deployment":
deploymentsFilenames = append(deploymentsFilenames, name)
deploymentsAssets[name] = m
} else {
case "CustomResourceDefinition":
customResourceDefinitionFilenames = append(customResourceDefinitionFilenames, name)
customResourceDefinitionAssets[name] = m
default:
componentsFilenames = append(componentsFilenames, name)
componentsAssets[name] = m
}
}

return componentsFilenames, componentsAssets, deploymentsFilenames, deploymentsAssets, nil
return componentsFilenames, componentsAssets, deploymentsFilenames, deploymentsAssets, customResourceDefinitionFilenames, customResourceDefinitionAssets, nil
}

// setAvailableCondition sets the ClusterOperator status condition to Available.
Expand Down