Skip to content

NE-1969: Set Degraded=True if unmanaged Gateway API CRDs exist #1205

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 15, 2025
Merged

NE-1969: Set Degraded=True if unmanaged Gateway API CRDs exist #1205

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 7 additions & 1 deletion pkg/operator/controller/gateway-service-dns/controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,7 +254,13 @@ func Test_Reconcile(t *testing.T) {
WithScheme(scheme).
WithRuntimeObjects(tc.existingObjects...).
Build()
cl := &testutil.FakeClientRecorder{fakeClient, t, []client.Object{}, []client.Object{}, []client.Object{}}
cl := &testutil.FakeClientRecorder{
Client: fakeClient,
T: t,
Added: []client.Object{},
Updated: []client.Object{},
Deleted: []client.Object{},
}
informer := informertest.FakeInformers{Scheme: scheme}
cache := testutil.FakeCache{Informers: &informer, Reader: cl}
reconciler := &reconciler{
Expand Down
43 changes: 38 additions & 5 deletions pkg/operator/controller/gatewayapi/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package gatewayapi

import (
"context"
"fmt"
"sync"

logf "github.com/openshift/cluster-ingress-operator/pkg/log"
Expand All @@ -14,6 +15,7 @@ import (
apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
"k8s.io/apimachinery/pkg/types"

"sigs.k8s.io/controller-runtime/pkg/cache"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/controller"
"sigs.k8s.io/controller-runtime/pkg/handler"
Expand All @@ -25,7 +27,10 @@ import (
)

const (
controllerName = "gatewayapi_controller"
controllerName = "gatewayapi_controller"
experimentalGatewayAPIGroupName = "gateway.networking.x-k8s.io"
Copy link
Contributor

@candita candita Apr 3, 2025

Choose a reason for hiding this comment

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

nit: it would be nice to define this in names.go so it could be used by other packages.

Copy link
Contributor Author

@alebedev87 alebedev87 Apr 4, 2025

Choose a reason for hiding this comment

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

Currently it's only used in this package. In the unit tests, I deliberately abstain from using the same constants as in the code to avoid duplicating errors.

Copy link
Contributor

@Miciah Miciah Apr 4, 2025

Choose a reason for hiding this comment

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

It always seems wrong to me when we define a name that doesn't belong to the operator in names.go. Maybe we could import sigs.k8s.io/gateway-api/apisx/v1alpha1 and then use gatewayapixv1alpha1.GroupName? Alternatively, we could put something in manifests.go.

gatewayAPICRDIndexFieldName = "gatewayAPICRD"
unmanagedGatewayAPICRDIndexFieldValue = "unmanaged"
)

var log = logf.Logger.WithName(controllerName)
Expand All @@ -36,6 +41,7 @@ func New(mgr manager.Manager, config Config) (controller.Controller, error) {
operatorCache := mgr.GetCache()
reconciler := &reconciler{
client: mgr.GetClient(),
cache: operatorCache,
config: config,
}
c, err := controller.New(controllerName, mgr, controller.Options{Reconciler: reconciler})
Expand All @@ -60,14 +66,34 @@ func New(mgr manager.Manager, config Config) (controller.Controller, error) {
}}
}

// watch for CRDs
crdPredicate := predicate.NewPredicateFuncs(func(o client.Object) bool {
return o.(*apiextensionsv1.CustomResourceDefinition).Spec.Group == gatewayapiv1.GroupName
})
isGatewayAPICRD := func(o client.Object) bool {
crd := o.(*apiextensionsv1.CustomResourceDefinition)
return crd.Spec.Group == gatewayapiv1.GroupName || crd.Spec.Group == experimentalGatewayAPIGroupName
}
crdPredicate := predicate.NewPredicateFuncs(isGatewayAPICRD)

// watch for CRDs
if err := c.Watch(source.Kind[client.Object](operatorCache, &apiextensionsv1.CustomResourceDefinition{}, handler.EnqueueRequestsFromMapFunc(toFeatureGate), crdPredicate)); err != nil {
return nil, err
}

// Index unmanaged Gateway API CRDs to enable efficient filtering
// during list operations.
if err := mgr.GetFieldIndexer().IndexField(
Copy link
Contributor

@candita candita Mar 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Could you create two indexers, one for managed and one for unmanaged CRDs? See https://github.com/openshift/cluster-ingress-operator/pull/1205/files?diff=split&w=1#r2019309675

Copy link
Contributor

@Miciah Miciah Mar 28, 2025

Choose a reason for hiding this comment

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

What would be the purpose of having two indexers?

Copy link
Contributor

@Miciah Miciah Mar 28, 2025

Choose a reason for hiding this comment

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

Ah, I see your comment on listUnmanagedGatewayAPICRDs now.

Wouldn't it make more sense to do this?

		client.IndexerFunc(func(o client.Object) []string {
			if isGatewayAPICRD(o) {
				return []string{"managed"}
			}
			return []string{"unmanaged"}
		})

and then

r.cache.List(ctx, gatewayAPICRDs, client.MatchingFields{crdAPIGroupIndexFieldName: "managed"});

or

r.cache.List(ctx, gatewayAPICRDs, client.MatchingFields{crdAPIGroupIndexFieldName: "unmanaged"});

Copy link
Contributor

@candita candita Mar 28, 2025

Choose a reason for hiding this comment

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

I don't quite know why we need this indexer, which picks up managed and unmanaged CRDs together, but there is code around https://github.com/openshift/cluster-ingress-operator/pull/1205/files?diff=split&w=1#r2019309675 that gets all the CRDs from this index and then iterates through to get the unmanaged ones. One indexer for unmanaged CRDs should be enough.

Copy link
Contributor

@Miciah Miciah Mar 28, 2025

Choose a reason for hiding this comment

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

No, I guess my last comment doesn't make sense. The index is used in order to get all Gateway API CRDs, be they managed or be they unmanaged, and then the loop in listUnmanagedGatewayAPICRDs checks each of those Gateway API CRDs to determine whether any unmanaged one was indexed. This makes sense to me: The cluster could have many CRDs, and we only index the ~half dozen (at most) that belong to Gateway API. It's probably cheaper to iterate over a half dozen CRDs than it would be to maintain another index to try to optimize the loop away.

Copy link
Contributor Author

@alebedev87 alebedev87 Mar 31, 2025

Choose a reason for hiding this comment

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

I think that 2 indexes do not make sense as "managed" index would be of no use in this case.

One indexer for unmanaged CRDs should be enough.

This may be another approach to the problem indeed. I was thinking more of the field name which matches the CRD field spec.group (similar to what Miciah described here). I didn't respect the naming convention though.

Copy link
Contributor

@Miciah Miciah Mar 31, 2025

Choose a reason for hiding this comment

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

One indexer for unmanaged CRDs should be enough.

My understanding from our call today is that we agreed on this approach: using one index, which has only CRDs that are both in any of the API groups that belong to Gateway API and not managed by cluster-ingress-operator.

Copy link
Contributor Author

@alebedev87 alebedev87 Apr 1, 2025

Choose a reason for hiding this comment

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

Changed the index to have a field for all unmanaged gateway API CRDs.

context.Background(),
&apiextensionsv1.CustomResourceDefinition{},
gatewayAPICRDIndexFieldName,
client.IndexerFunc(func(o client.Object) []string {
if isGatewayAPICRD(o) {
if _, found := managedCRDMap[o.GetName()]; !found {
return []string{unmanagedGatewayAPICRDIndexFieldValue}
}
}
return []string{}
})); err != nil {
return nil, fmt.Errorf("failed to create index for custom resource definitions: %w", err)
}

return c, nil
}

Expand All @@ -90,6 +116,7 @@ type reconciler struct {
config Config

client client.Client
cache cache.Cache
Copy link
Contributor

@candita candita Apr 3, 2025

Choose a reason for hiding this comment

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

cache is unused now, isn't it?

Copy link
Contributor Author

@alebedev87 alebedev87 Apr 4, 2025

Choose a reason for hiding this comment

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

No, it is still used in status.go, we still have to do List() call from it.

recorder record.EventRecorder
startControllers sync.Once
}
Expand All @@ -111,6 +138,12 @@ func (r *reconciler) Reconcile(ctx context.Context, request reconcile.Request) (
return reconcile.Result{}, err
}

if crdNames, err := r.listUnmanagedGatewayAPICRDs(ctx); err != nil {
return reconcile.Result{}, fmt.Errorf("failed to list unmanaged gateway CRDs: %w", err)
} else if err = r.setUnmanagedGatewayAPICRDNamesStatus(ctx, crdNames); err != nil {
return reconcile.Result{}, fmt.Errorf("failed to update the ingress cluster operator status: %w", err)
}

if !r.config.GatewayAPIControllerEnabled {
return reconcile.Result{}, nil
}
Expand Down
179 changes: 168 additions & 11 deletions pkg/operator/controller/gatewayapi/controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package gatewayapi

import (
"context"
"strings"
"testing"
"time"

Expand All @@ -17,6 +18,7 @@ import (
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/types"

"sigs.k8s.io/controller-runtime/pkg/cache/informertest"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/client/fake"
"sigs.k8s.io/controller-runtime/pkg/controller"
Expand All @@ -36,28 +38,57 @@ func Test_Reconcile(t *testing.T) {
ObjectMeta: metav1.ObjectMeta{Name: name},
}
}
co := func(name string) *configv1.ClusterOperator {
return &configv1.ClusterOperator{
ObjectMeta: metav1.ObjectMeta{Name: name},
}
}
coWithExtension := func(name, extension string) *configv1.ClusterOperator {
co := co(name)
co.Status = configv1.ClusterOperatorStatus{
Extension: runtime.RawExtension{
Raw: []byte(extension),
},
}
return co
}

tests := []struct {
name string
gatewayAPIEnabled bool
gatewayAPIControllerEnabled bool
existingObjects []runtime.Object
expectCreate []client.Object
expectUpdate []client.Object
expectDelete []client.Object
expectStartCtrl bool
// existingStatusSubresource contains the original version of objects
// whose status will updated by Reconcile function.
// This field is similar to `existingObjects` but is specifically used
// for objects where status updates are performed using `Status().Update()` call.
existingStatusSubresource []client.Object
expectCreate []client.Object
expectUpdate []client.Object
expectDelete []client.Object
// expectStatusUpdate contains the updated versions of objects
// whose status is expected to be updated by the test.
expectStatusUpdate []client.Object
expectStartCtrl bool
}{
{
name: "gateway API disabled",
gatewayAPIEnabled: false,
expectCreate: []client.Object{},
expectUpdate: []client.Object{},
expectDelete: []client.Object{},
expectStartCtrl: false,
existingObjects: []runtime.Object{
co("ingress"),
},
expectCreate: []client.Object{},
expectUpdate: []client.Object{},
expectDelete: []client.Object{},
expectStartCtrl: false,
},
{
name: "gateway API enabled",
gatewayAPIEnabled: true,
gatewayAPIControllerEnabled: true,
existingObjects: []runtime.Object{
co("ingress"),
},
expectCreate: []client.Object{
crd("gatewayclasses.gateway.networking.k8s.io"),
crd("gateways.gateway.networking.k8s.io"),
Expand All @@ -75,6 +106,9 @@ func Test_Reconcile(t *testing.T) {
name: "gateway API enabled, gateway API controller disabled",
gatewayAPIEnabled: true,
gatewayAPIControllerEnabled: false,
existingObjects: []runtime.Object{
co("ingress"),
},
expectCreate: []client.Object{
crd("gatewayclasses.gateway.networking.k8s.io"),
crd("gateways.gateway.networking.k8s.io"),
Expand All @@ -88,6 +122,87 @@ func Test_Reconcile(t *testing.T) {
expectDelete: []client.Object{},
expectStartCtrl: false,
},
{
name: "unmanaged gateway API CRDs created",
gatewayAPIEnabled: true,
gatewayAPIControllerEnabled: true,
existingObjects: []runtime.Object{
co("ingress"),
crd("listenersets.gateway.networking.x-k8s.io"),
crd("backendtrafficpolicies.gateway.networking.x-k8s.io"),
},
existingStatusSubresource: []client.Object{
co("ingress"),
Copy link
Contributor

@candita candita Apr 3, 2025

Choose a reason for hiding this comment

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

Can you add a comment about why we need an ingress co in both existingObjects and existingStatusSubresource? Do they represent the same ingress co?

Copy link
Contributor Author

@alebedev87 alebedev87 Apr 4, 2025

Choose a reason for hiding this comment

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

Do they represent the same ingress co?

Yes, they do. I have a comment for existingStatusSubresource above which explains that this field is needed if Reconcile uses Status() calls, which Reconcile is doing now. The reason why it's a dedicated field is because it mimics the fake controller-runtime's mechanics which have a dedicated method for the status subresource: WithStatusSubresource (see below).

},
expectCreate: []client.Object{
crd("gatewayclasses.gateway.networking.k8s.io"),
crd("gateways.gateway.networking.k8s.io"),
crd("grpcroutes.gateway.networking.k8s.io"),
crd("httproutes.gateway.networking.k8s.io"),
crd("referencegrants.gateway.networking.k8s.io"),
clusterRole("system:openshift:gateway-api:aggregate-to-admin"),
clusterRole("system:openshift:gateway-api:aggregate-to-view"),
},
expectUpdate: []client.Object{},
expectDelete: []client.Object{},
expectStatusUpdate: []client.Object{
coWithExtension("ingress", `{"unmanagedGatewayAPICRDNames":"backendtrafficpolicies.gateway.networking.x-k8s.io,listenersets.gateway.networking.x-k8s.io"}`),
},
expectStartCtrl: true,
},
{
name: "unmanaged gateway API CRDs removed",
Copy link
Contributor

@candita candita Apr 3, 2025

Choose a reason for hiding this comment

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

I'm not sure what's happening here. Is this simulating the removal of unmanaged CRDs?

Copy link
Contributor Author

@alebedev87 alebedev87 Apr 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yes. We have the ingress cluster operator with unmanagedGatewayAPICRDNames extension but no unmanaged CRD exists (== added to existingObjects). So, we have to reset unmanagedGatewayAPICRDNames extension (see expectStatusUpdate).

gatewayAPIEnabled: true,
gatewayAPIControllerEnabled: true,
existingObjects: []runtime.Object{
coWithExtension("ingress", `{"unmanagedGatewayAPICRDNames":"listenersets.gateway.networking.x-k8s.io"}`),
Copy link
Contributor

@Miciah Miciah Apr 7, 2025

Choose a reason for hiding this comment

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

Consider adding something like "unrelatedKey":"unrelatedValue" to make sure that the logic doesn't stomp unrelated data in status.extension.

Copy link
Contributor Author

@alebedev87 alebedev87 Apr 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Keeping unexpected fields in status.extension can be more work than already done. The current implementation assumes the status extension raw data can be unmarshalled into IngressOperatorStatusExtension struct which doesn't handle "unexpected fields" (using something like Extra map[string]interface{} `json:"-"` ). It only assumes that other fields of IngressOperatorStatusExtension can be updated by other controllers.

So the question is - do we want to allow unsolicited status extension data in ingress cluster operator.

Copy link
Contributor

@candita candita Apr 14, 2025

Choose a reason for hiding this comment

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

@alebedev87 can you follow this up later. I understand we don't need to account for other uses of extension now, but we should be prepared for the future. Or at least post a warning that more work needs to be done to share the extension.

Copy link
Contributor Author

@alebedev87 alebedev87 Apr 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

So the question is - do we want to allow unsolicited status extension data in ingress cluster operator.

Here is what the description of the extension field says:

extension contains any additional status information specific to the operator which owns this status object.

So, I prefer to answer my own question with "no" because the cluster operator extension should not be updated by any third party (note that I covered the sharing of the extension between different controllers of the operator). And if it is, we should know about in a form of a bug (which we will have rights to challenge).

},
existingStatusSubresource: []client.Object{
co("ingress"),
},
expectCreate: []client.Object{
crd("gatewayclasses.gateway.networking.k8s.io"),
crd("gateways.gateway.networking.k8s.io"),
crd("grpcroutes.gateway.networking.k8s.io"),
crd("httproutes.gateway.networking.k8s.io"),
crd("referencegrants.gateway.networking.k8s.io"),
clusterRole("system:openshift:gateway-api:aggregate-to-admin"),
clusterRole("system:openshift:gateway-api:aggregate-to-view"),
},
expectUpdate: []client.Object{},
expectDelete: []client.Object{},
expectStatusUpdate: []client.Object{
coWithExtension("ingress", `{}`),
},
expectStartCtrl: true,
},
{
name: "third party CRDs",
gatewayAPIEnabled: true,
gatewayAPIControllerEnabled: true,
existingObjects: []runtime.Object{
co("ingress"),
crd("thirdpartycrd1.openshift.io"),
crd("thirdpartycrd2.openshift.io"),
},
existingStatusSubresource: []client.Object{
co("ingress"),
},
expectCreate: []client.Object{
crd("gatewayclasses.gateway.networking.k8s.io"),
crd("gateways.gateway.networking.k8s.io"),
crd("grpcroutes.gateway.networking.k8s.io"),
crd("httproutes.gateway.networking.k8s.io"),
crd("referencegrants.gateway.networking.k8s.io"),
clusterRole("system:openshift:gateway-api:aggregate-to-admin"),
clusterRole("system:openshift:gateway-api:aggregate-to-view"),
},
expectUpdate: []client.Object{},
expectDelete: []client.Object{},
// Third party CRDs have no impact on cluster operator status.
expectStatusUpdate: []client.Object{},
expectStartCtrl: true,
},
}
Copy link
Contributor

@candita candita Mar 28, 2025

Choose a reason for hiding this comment

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

Maybe add another test for experimental gateway CRDs.

Copy link
Contributor Author

@alebedev87 alebedev87 Mar 31, 2025

Choose a reason for hiding this comment

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

I have a test case called unmanaged gateway API CRDs just above this one, it has a CRD ListenerSet and verifies that the status is updated.

Copy link
Contributor

@candita candita Mar 31, 2025

Choose a reason for hiding this comment

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

That ListenerSet CRD is already existing in existingObjects. I'm asking for test case where it tries to add an experimental CRD and shows that it makes the operator Degraded=true. This could be in pkg/operator/controller/status/controller_test.go.

Copy link
Contributor Author

@alebedev87 alebedev87 Apr 1, 2025

Choose a reason for hiding this comment

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

I'm asking for test case where it tries to add an experimental CRD and shows that it makes the operator Degraded=true.

This is tested in the e2e test I added, because it involves 2 different controllers (gatewayapi and status).


scheme := runtime.NewScheme()
Expand All @@ -100,11 +215,31 @@ func Test_Reconcile(t *testing.T) {
fakeClient := fake.NewClientBuilder().
WithScheme(scheme).
WithRuntimeObjects(tc.existingObjects...).
WithStatusSubresource(tc.existingStatusSubresource...).
WithIndex(&apiextensionsv1.CustomResourceDefinition{}, "gatewayAPICRD", client.IndexerFunc(func(o client.Object) []string {
// Assume that all experimental CRDs are unmanaged.
if strings.Contains(o.GetName(), "gateway.networking.x-k8s.io") {
return []string{"unmanaged"}
}
return []string{}
})).
Build()
cl := &testutil.FakeClientRecorder{fakeClient, t, []client.Object{}, []client.Object{}, []client.Object{}}
cl := &testutil.FakeClientRecorder{
Client: fakeClient,
T: t,
Added: []client.Object{},
Updated: []client.Object{},
Deleted: []client.Object{},
StatusWriter: &testutil.FakeStatusWriter{
StatusWriter: fakeClient.Status(),
},
}
ctrl := &testutil.FakeController{t, false, nil}
informer := informertest.FakeInformers{Scheme: scheme}
cache := &testutil.FakeCache{Informers: &informer, Reader: fakeClient}
reconciler := &reconciler{
client: cl,
cache: cache,
config: Config{
GatewayAPIEnabled: tc.gatewayAPIEnabled,
GatewayAPIControllerEnabled: tc.gatewayAPIControllerEnabled,
Expand Down Expand Up @@ -144,6 +279,9 @@ func Test_Reconcile(t *testing.T) {
if diff := cmp.Diff(tc.expectDelete, cl.Deleted, cmpOpts...); diff != "" {
t.Fatalf("found diff between expected and actual deletes: %s", diff)
}
if diff := cmp.Diff(tc.expectStatusUpdate, cl.StatusWriter.Updated, cmpOpts...); diff != "" {
t.Fatalf("found diff between expected and actual status updates: %s", diff)
}
})
}
}
Expand All @@ -153,11 +291,30 @@ func TestReconcileOnlyStartsControllerOnce(t *testing.T) {
configv1.Install(scheme)
apiextensionsv1.AddToScheme(scheme)
rbacv1.AddToScheme(scheme)
fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithRuntimeObjects().Build()
cl := &testutil.FakeClientRecorder{fakeClient, t, []client.Object{}, []client.Object{}, []client.Object{}}
fakeClient := fake.NewClientBuilder().
WithScheme(scheme).
WithRuntimeObjects(
&configv1.ClusterOperator{
ObjectMeta: metav1.ObjectMeta{Name: "ingress"},
}).
WithIndex(&apiextensionsv1.CustomResourceDefinition{}, "gatewayAPICRD", client.IndexerFunc(func(o client.Object) []string {
// Assume that there are no unmanaged CRDs.
return []string{}
})).
Build()
cl := &testutil.FakeClientRecorder{
Client: fakeClient,
T: t,
Added: []client.Object{},
Updated: []client.Object{},
Deleted: []client.Object{},
}
ctrl := &testutil.FakeController{t, false, make(chan struct{})}
informer := informertest.FakeInformers{Scheme: scheme}
cache := &testutil.FakeCache{Informers: &informer, Reader: fakeClient}
reconciler := &reconciler{
client: cl,
cache: cache,
config: Config{
GatewayAPIEnabled: true,
GatewayAPIControllerEnabled: true,
Expand Down
Loading