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
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,8 @@ const (
type HostedControlPlaneReconciler struct {
client.Client

GVKAccessChecker component.GVKAccessChecker

components []component.ControlPlaneComponent

// ManagementClusterCapabilities can be asked for support of optional management cluster capabilities
Expand Down Expand Up @@ -1172,6 +1174,7 @@ func (r *HostedControlPlaneReconciler) reconcileCPOV2(ctx context.Context, hcp *
cpContext := component.ControlPlaneContext{
Context: ctx,
Client: r.Client,
GVKAccessChecker: r.GVKAccessChecker,
HCP: hcp,
ApplyProvider: upsert.NewApplyProvider(r.EnableCIDebugOutput),
InfraStatus: infraStatus,
Expand Down
2 changes: 2 additions & 0 deletions control-plane-operator/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import (
hyperapi "github.com/openshift/hypershift/support/api"
"github.com/openshift/hypershift/support/capabilities"
"github.com/openshift/hypershift/support/config"
component "github.com/openshift/hypershift/support/controlplane-component"
"github.com/openshift/hypershift/support/events"
"github.com/openshift/hypershift/support/metrics"
"github.com/openshift/hypershift/support/releaseinfo"
Expand Down Expand Up @@ -469,6 +470,7 @@ func NewStartCommand() *cobra.Command {

if err := (&hostedcontrolplane.HostedControlPlaneReconciler{
Client: mgr.GetClient(),
GVKAccessChecker: component.NewGVKAccessCache(mgr.GetAPIReader()),
ManagementClusterCapabilities: mgmtClusterCaps,
ReleaseProvider: cpReleaseProvider,
UserReleaseProvider: userReleaseProvider,
Expand Down
14 changes: 14 additions & 0 deletions support/controlplane-component/controlplane-component.go
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,10 @@ type ControlPlaneContext struct {
// This is useful when the component is not managed by the same HostedControlPlane controller like capi and the CPO itself.
OmitOwnerReference bool

// GVKAccessChecker caches GVK accessibility to avoid repeated probes.
// It uses an uncached reader internally to probe without creating informers.
GVKAccessChecker GVKAccessChecker

// SkipPredicate is used for the generic unit test, so we can always generate a fixture for the components deployment/statefulset.
SkipPredicate bool
// SkipCertificateSigning is used for the generic unit test to skip the signing of certificates and maintain a stable output.
Expand Down Expand Up @@ -209,6 +213,16 @@ func (c *controlPlaneWorkload[T]) delete(cpContext ControlPlaneContext) error {
}
obj.SetNamespace(cpContext.HCP.Namespace)

if cpContext.GVKAccessChecker != nil {
accessible, err := cpContext.GVKAccessChecker.GetOrProbe(cpContext, obj)
if err != nil {
return err
}
if !accessible {
return nil
}
}

_, err = util.DeleteIfNeeded(cpContext, cpContext.Client, obj)
return err
}); err != nil {
Expand Down
10 changes: 10 additions & 0 deletions support/controlplane-component/generic-adapter.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,16 @@ func (ga *genericAdapter) reconcile(cpContext ControlPlaneContext, obj client.Ob
workloadContext := cpContext.workloadContext()

if ga.predicate != nil && !ga.predicate(workloadContext) {
if cpContext.GVKAccessChecker != nil {
accessible, err := cpContext.GVKAccessChecker.GetOrProbe(cpContext, obj)
if err != nil {
return err
}
if !accessible {
return nil
}
}

// get the existing object to read its ownerRefs
existing := obj.DeepCopyObject().(client.Object)
err := cpContext.Client.Get(cpContext, client.ObjectKeyFromObject(obj), existing)
Expand Down
260 changes: 260 additions & 0 deletions support/controlplane-component/generic-adapter_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,260 @@
package controlplanecomponent

import (
"context"
"fmt"
"testing"

. "github.com/onsi/gomega"

hyperv1 "github.com/openshift/hypershift/api/hypershift/v1beta1"
"github.com/openshift/hypershift/support/upsert"

corev1 "k8s.io/api/core/v1"
apierrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/api/meta"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/runtime/schema"

"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/client/fake"
)

func testScheme() *runtime.Scheme {
s := runtime.NewScheme()
_ = corev1.AddToScheme(s)
_ = hyperv1.AddToScheme(s)
return s
}

func testCPContext(t *testing.T, checker GVKAccessChecker, objects ...client.Object) ControlPlaneContext {
t.Helper()
return ControlPlaneContext{
Context: t.Context(),
ApplyProvider: upsert.NewApplyProvider(false),
HCP: &hyperv1.HostedControlPlane{
ObjectMeta: metav1.ObjectMeta{
Name: "test-hcp",
Namespace: "test-ns",
},
},
Client: fake.NewClientBuilder().WithScheme(testScheme()).
WithObjects(objects...).Build(),
GVKAccessChecker: checker,
}
}

func testObjWithGVK(gvk schema.GroupVersionKind) client.Object {
obj := &corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Name: "test-resource",
Namespace: "test-ns",
},
}
obj.SetGroupVersionKind(gvk)
return obj
}

var inaccessibleGVK = schema.GroupVersionKind{Group: "secrets-store.csi.x-k8s.io", Version: "v1", Kind: "SecretProviderClass"}

func TestGenericAdapterReconcile(t *testing.T) {
t.Run("When predicate is false and GVK is inaccessible it should skip without error", func(t *testing.T) {
g := NewWithT(t)

reader := &fakeReader{err: apierrors.NewForbidden(
schema.GroupResource{Group: inaccessibleGVK.Group, Resource: "secretproviderclasses"},
"test-resource", fmt.Errorf("forbidden"),
)}
checker := NewGVKAccessCache(reader)

cpCtx := testCPContext(t, checker)

ga := &genericAdapter{
predicate: func(_ WorkloadContext) bool { return false },
}

obj := testObjWithGVK(inaccessibleGVK)
err := ga.reconcile(cpCtx, obj)
g.Expect(err).ToNot(HaveOccurred())
// Probe should have been called exactly once.
g.Expect(reader.callCount.Load()).To(Equal(int32(1)))
})

t.Run("When predicate is false and GVK is accessible it should attempt cleanup", func(t *testing.T) {
g := NewWithT(t)

// NotFound means GVK is accessible (CRD exists, resource just doesn't exist yet).
reader := &fakeReader{err: apierrors.NewNotFound(
schema.GroupResource{Group: "apps", Resource: "deployments"},
"test-resource",
)}
checker := NewGVKAccessCache(reader)

cpCtx := testCPContext(t, checker)

ga := &genericAdapter{
predicate: func(_ WorkloadContext) bool { return false },
}

// Use a ConfigMap with a known GVK (accessible). The object doesn't exist
// on the fake client so the Get in the predicate-false path returns NotFound → returns nil.
obj := testObjWithGVK(schema.GroupVersionKind{Group: "", Version: "v1", Kind: "ConfigMap"})
err := ga.reconcile(cpCtx, obj)
g.Expect(err).ToNot(HaveOccurred())
g.Expect(reader.callCount.Load()).To(Equal(int32(1)))
})

t.Run("When predicate is false and GVK checker is nil it should proceed with existing logic", func(t *testing.T) {
g := NewWithT(t)

// No checker — backward compatibility.
cpCtx := testCPContext(t, nil)

ga := &genericAdapter{
predicate: func(_ WorkloadContext) bool { return false },
}

obj := testObjWithGVK(inaccessibleGVK)
// Without a checker the code falls through to Client.Get which will
// return an error or NotFound depending on the fake client setup.
// Since the object doesn't exist and the GVK is registered (ConfigMap used in fake),
// we use a ConfigMap to avoid scheme issues.
cmObj := &corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Name: "test-resource",
Namespace: "test-ns",
},
}
_ = obj // unused in this path
err := ga.reconcile(cpCtx, cmObj)
// Should succeed (object not found → no deletion needed).
g.Expect(err).ToNot(HaveOccurred())
})

t.Run("When predicate is false and GVK probe returns transient error it should propagate error", func(t *testing.T) {
g := NewWithT(t)

reader := &fakeReader{err: fmt.Errorf("connection refused")}
checker := NewGVKAccessCache(reader)

cpCtx := testCPContext(t, checker)

ga := &genericAdapter{
predicate: func(_ WorkloadContext) bool { return false },
}

obj := testObjWithGVK(inaccessibleGVK)
err := ga.reconcile(cpCtx, obj)
g.Expect(err).To(HaveOccurred())
g.Expect(err.Error()).To(ContainSubstring("connection refused"))
})

t.Run("When predicate is false and GVK is NoMatch it should skip without error", func(t *testing.T) {
g := NewWithT(t)

reader := &fakeReader{err: &meta.NoKindMatchError{
GroupKind: schema.GroupKind{Group: inaccessibleGVK.Group, Kind: inaccessibleGVK.Kind},
}}
checker := NewGVKAccessCache(reader)

cpCtx := testCPContext(t, checker)

ga := &genericAdapter{
predicate: func(_ WorkloadContext) bool { return false },
}

obj := testObjWithGVK(inaccessibleGVK)
err := ga.reconcile(cpCtx, obj)
g.Expect(err).ToNot(HaveOccurred())
})

t.Run("When predicate is true it should not check GVK access", func(t *testing.T) {
g := NewWithT(t)

reader := &fakeReader{err: fmt.Errorf("should not be called")}
checker := NewGVKAccessCache(reader)

cpCtx := testCPContext(t, checker)

adapted := false
ga := &genericAdapter{
predicate: func(_ WorkloadContext) bool { return true },
adapt: func(_ WorkloadContext, _ client.Object) error {
adapted = true
return nil
},
}

obj := &corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Name: "test-resource",
Namespace: "test-ns",
},
}
err := ga.reconcile(cpCtx, obj)
g.Expect(err).ToNot(HaveOccurred())
g.Expect(adapted).To(BeTrue())
// Reader should not have been called since predicate was true.
g.Expect(reader.callCount.Load()).To(Equal(int32(0)))
})

t.Run("When predicate is false and accessible resource has HCP owner ref it should delete it", func(t *testing.T) {
g := NewWithT(t)

reader := &fakeReader{err: nil} // OK → accessible
checker := NewGVKAccessCache(reader)

hcp := &hyperv1.HostedControlPlane{
ObjectMeta: metav1.ObjectMeta{
Name: "test-hcp",
Namespace: "test-ns",
UID: "test-uid",
},
}

// Create an existing resource with HCP owner reference.
existingCM := &corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Name: "test-resource",
Namespace: "test-ns",
OwnerReferences: []metav1.OwnerReference{
{
APIVersion: hyperv1.GroupVersion.String(),
Kind: "HostedControlPlane",
Name: hcp.Name,
UID: hcp.UID,
},
},
},
}

cpCtx := ControlPlaneContext{
Context: t.Context(),
ApplyProvider: upsert.NewApplyProvider(false),
HCP: hcp,
Client: fake.NewClientBuilder().WithScheme(testScheme()).WithObjects(existingCM).Build(),
GVKAccessChecker: checker,
}

ga := &genericAdapter{
predicate: func(_ WorkloadContext) bool { return false },
}

obj := &corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Name: "test-resource",
Namespace: "test-ns",
},
}
obj.SetGroupVersionKind(schema.GroupVersionKind{Version: "v1", Kind: "ConfigMap"})

err := ga.reconcile(cpCtx, obj)
g.Expect(err).ToNot(HaveOccurred())

// Verify the resource was deleted.
got := &corev1.ConfigMap{}
err = cpCtx.Client.Get(context.Background(), client.ObjectKeyFromObject(obj), got)
g.Expect(apierrors.IsNotFound(err)).To(BeTrue())
})
}
Loading