feat: set GWC accepted condition properly #1021
+339
−44
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,65 @@ | ||
| package gatewayclass | ||
|
|
||
| import ( | ||
| "context" | ||
| "strings" | ||
|
|
||
| k8serrors "k8s.io/apimachinery/pkg/api/errors" | ||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| "sigs.k8s.io/controller-runtime/pkg/client" | ||
| gatewayv1 "sigs.k8s.io/gateway-api/apis/v1" | ||
|
|
||
| operatorv1beta1 "github.com/kong/gateway-operator/api/v1beta1" | ||
| "github.com/kong/gateway-operator/pkg/consts" | ||
| k8sutils "github.com/kong/gateway-operator/pkg/utils/kubernetes" | ||
| ) | ||
|
|
||
| // getAcceptedCondition returns the accepted condition for the GatewayClass, with | ||
| // the proper status, reason and message. | ||
| func getAcceptedCondition(ctx context.Context, cl client.Client, gwc *gatewayv1.GatewayClass) (*metav1.Condition, error) { | ||
| reason := string(gatewayv1.GatewayClassReasonAccepted) | ||
mlavacca marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| message := []string{} | ||
mlavacca marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| status := metav1.ConditionFalse | ||
|
|
||
| if gwc.Spec.ParametersRef != nil { | ||
| validRef := true | ||
| if gwc.Spec.ParametersRef.Group != gatewayv1.Group(operatorv1beta1.SchemeGroupVersion.Group) || | ||
| gwc.Spec.ParametersRef.Kind != "GatewayConfiguration" { | ||
| reason = string(gatewayv1.GatewayClassReasonInvalidParameters) | ||
| message = append(message, "ParametersRef must reference a gateway-operator.konghq.com/GatewayConfiguration") | ||
| validRef = false | ||
| } | ||
|
|
||
| if gwc.Spec.ParametersRef.Namespace == nil { | ||
| reason = string(gatewayv1.GatewayClassReasonInvalidParameters) | ||
| message = append(message, "ParametersRef must reference a namespaced resource") | ||
| validRef = false | ||
| } | ||
|
|
||
| if validRef { | ||
| gatewayConfig := operatorv1beta1.GatewayConfiguration{} | ||
| err := cl.Get(ctx, client.ObjectKey{Name: gwc.Spec.ParametersRef.Name, Namespace: string(*gwc.Spec.ParametersRef.Namespace)}, &gatewayConfig) | ||
| if client.IgnoreNotFound(err) != nil { | ||
| return nil, err | ||
| } | ||
| if k8serrors.IsNotFound(err) { | ||
| reason = string(gatewayv1.GatewayClassReasonInvalidParameters) | ||
| message = append(message, "The referenced GatewayConfiguration does not exist") | ||
| } | ||
programmer04 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
| } | ||
| if reason == string(gatewayv1.GatewayClassReasonAccepted) { | ||
| status = metav1.ConditionTrue | ||
| message = []string{"GatewayClass is accepted"} | ||
| } | ||
|
|
||
| acceptedCondition := k8sutils.NewConditionWithGeneration( | ||
| consts.ConditionType(gatewayv1.GatewayClassConditionStatusAccepted), | ||
| status, | ||
| consts.ConditionReason(reason), | ||
| strings.Join(message, ". "), | ||
| gwc.GetGeneration(), | ||
| ) | ||
|
|
||
| return &acceptedCondition, nil | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,131 @@ | ||
| package gatewayclass | ||
|
|
||
| import ( | ||
| "context" | ||
| "testing" | ||
|
|
||
| "github.com/samber/lo" | ||
| "github.com/stretchr/testify/assert" | ||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| "k8s.io/apimachinery/pkg/runtime" | ||
| "sigs.k8s.io/controller-runtime/pkg/client/fake" | ||
| gatewayv1 "sigs.k8s.io/gateway-api/apis/v1" | ||
|
|
||
| operatorv1beta1 "github.com/kong/gateway-operator/api/v1beta1" | ||
| ) | ||
|
|
||
| func TestGetAcceptedCondition(t *testing.T) { | ||
| scheme := runtime.NewScheme() | ||
| assert.NoError(t, gatewayv1.Install(scheme)) | ||
| assert.NoError(t, operatorv1beta1.AddToScheme(scheme)) | ||
|
|
||
| tests := []struct { | ||
| name string | ||
| gwc *gatewayv1.GatewayClass | ||
| existingObjs []runtime.Object | ||
| expectedStatus metav1.ConditionStatus | ||
| expectedReason string | ||
| expectedMsg string | ||
| }{ | ||
| { | ||
| name: "ParametersRef is nil", | ||
| gwc: &gatewayv1.GatewayClass{ | ||
| Spec: gatewayv1.GatewayClassSpec{ | ||
| ParametersRef: nil, | ||
| }, | ||
| }, | ||
| expectedStatus: metav1.ConditionTrue, | ||
| expectedReason: string(gatewayv1.GatewayClassReasonAccepted), | ||
| expectedMsg: "GatewayClass is accepted", | ||
| }, | ||
| { | ||
| name: "Invalid ParametersRef Group and kind", | ||
| gwc: &gatewayv1.GatewayClass{ | ||
| Spec: gatewayv1.GatewayClassSpec{ | ||
|
|
||
| ParametersRef: &gatewayv1.ParametersReference{ | ||
| Group: "invalid.group", | ||
| Kind: "InvalidKind", | ||
| Namespace: lo.ToPtr(gatewayv1.Namespace("default")), | ||
| Name: "invalid", | ||
| }, | ||
| }, | ||
| }, | ||
| expectedStatus: metav1.ConditionFalse, | ||
| expectedReason: string(gatewayv1.GatewayClassReasonInvalidParameters), | ||
| expectedMsg: "ParametersRef must reference a gateway-operator.konghq.com/GatewayConfiguration", | ||
| }, | ||
| { | ||
| name: "ParametersRef Namespace is nil", | ||
| gwc: &gatewayv1.GatewayClass{ | ||
| Spec: gatewayv1.GatewayClassSpec{ | ||
| ParametersRef: &gatewayv1.ParametersReference{ | ||
| Group: gatewayv1.Group(operatorv1beta1.SchemeGroupVersion.Group), | ||
| Kind: "GatewayConfiguration", | ||
| Name: "no-namespace", | ||
| }, | ||
| }, | ||
| }, | ||
| expectedStatus: metav1.ConditionFalse, | ||
| expectedReason: string(gatewayv1.GatewayClassReasonInvalidParameters), | ||
| expectedMsg: "ParametersRef must reference a namespaced resource", | ||
| }, | ||
| { | ||
| name: "GatewayConfiguration does not exist", | ||
| gwc: &gatewayv1.GatewayClass{ | ||
| Spec: gatewayv1.GatewayClassSpec{ | ||
| ParametersRef: &gatewayv1.ParametersReference{ | ||
| Group: gatewayv1.Group(operatorv1beta1.SchemeGroupVersion.Group), | ||
| Kind: "GatewayConfiguration", | ||
| Name: "nonexistent", | ||
| Namespace: lo.ToPtr(gatewayv1.Namespace("default")), | ||
| }, | ||
| }, | ||
| }, | ||
| expectedStatus: metav1.ConditionFalse, | ||
| expectedReason: string(gatewayv1.GatewayClassReasonInvalidParameters), | ||
| expectedMsg: "The referenced GatewayConfiguration does not exist", | ||
| }, | ||
| { | ||
| name: "Valid ParametersRef", | ||
| gwc: &gatewayv1.GatewayClass{ | ||
| Spec: gatewayv1.GatewayClassSpec{ | ||
| ParametersRef: &gatewayv1.ParametersReference{ | ||
| Group: gatewayv1.Group(operatorv1beta1.SchemeGroupVersion.Group), | ||
| Kind: "GatewayConfiguration", | ||
| Name: "valid-config", | ||
| Namespace: lo.ToPtr(gatewayv1.Namespace("default")), | ||
| }, | ||
| }, | ||
| }, | ||
| existingObjs: []runtime.Object{ | ||
| &operatorv1beta1.GatewayConfiguration{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: "valid-config", | ||
| Namespace: "default", | ||
| }, | ||
| }, | ||
| }, | ||
| expectedStatus: metav1.ConditionTrue, | ||
| expectedReason: string(gatewayv1.GatewayClassReasonAccepted), | ||
| expectedMsg: "GatewayClass is accepted", | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| ctx := context.TODO() | ||
mlavacca marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| cl := fake.NewClientBuilder(). | ||
| WithScheme(scheme). | ||
| WithRuntimeObjects(tt.existingObjs...). | ||
| Build() | ||
|
|
||
| condition, err := getAcceptedCondition(ctx, cl, tt.gwc) | ||
| assert.NoError(t, err) | ||
| assert.NotNil(t, condition) | ||
| assert.Equal(t, tt.expectedStatus, condition.Status) | ||
| assert.Equal(t, tt.expectedReason, condition.Reason) | ||
| assert.Equal(t, tt.expectedMsg, condition.Message) | ||
| }) | ||
| } | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we set the condition here with some error message so that users do not have to look into operators logs? Ideally I'd consider adding the error's message into the condition message so that users know what happened. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
AcceptedConditionmessage is properly set in thegetAcceptedConditionfunction. Isn't this what you are referring to?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was specifically alluding here to lines 42-44 https://github.com/Kong/gateway-operator/pull/1021/files/31ff623213f0fed16947060e8a0eccec5e02f4ff#diff-f4e83572e27854d40d56ef54e8f982cdca3ccaaa3f17900c3ab6e3f4fb04e971R42-R44 where the
erris simply returned. Then we seem to return it here as well. Not setting anything on theGatewayClass. Or am I missing something here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That can be only a transient error caused by the get function. I don't think we should change the status of the resource when such an error is raised. Do you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't have to be transient. It can also be a problem with e.g. permissions. I believe it would be better UX to make the error visible in the status rather than forcing users to go through operator's logs. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so honestly. If that's an error related to permissions, for example, that isn't related to the resource, but to the operator itself. An object status should include information related to the resource(s) themselves, not to the operators managing it IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree but the error in object's status is a breadcrumb which can help you as a user debug this problem faster.
The permission might not be directly related to the operator. There might exist policies in the cluster which would prevent the
Getfrom succeeding.I have already seen reports of several issues where users were confused because an object didn't get any status condition set (for different reasons but still). For this reason I do believe that putting information that can help debug a problem in status condition is going to allow people to quickly resolve an issue that caused it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hear you and tend to agree with this general statement of helping users to debug problems. But here we are discussing about putting information that has nothing to do with the resource itself. I think we should instead try to educate users about what kind of information can be found where. I am very opinionated on this matter, and for the reasons above I think this is not the way to go honestly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. Nevertheless this information is beneficial to the user to quickly resolve an issue with this object not being accepted.
Agreed on this as well. Do you have an idea how we could educate users about this one so that they can better debug problems at we discuss here?
I see your view and I understand it yet I do not share it. Feel free to continue with the way this is proposed as of now 👍