feat: set GWC accepted condition properly #1021
Conversation
mlavacca
force-pushed
the
validate-parametersref
branch
3 times, most recently
from
fed00d1 to
5f2b74d
Compare
| return ctrl.Result{}, fmt.Errorf("failed patching GatewayClass: %w", err) | ||
| condition, err := getAcceptedCondition(ctx, r.Client, gwc.GatewayClass) | ||
| if err != nil { | ||
| return ctrl.Result{}, fmt.Errorf("failed to get accepted condition: %w", err) |
There was a problem hiding this comment.
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.
The AcceptedCondition message is properly set in the getAcceptedCondition function. Isn't this what you are referring to?
There was a problem hiding this comment.
I was specifically alluding here to lines 42-44 https://github.com/Kong/gateway-operator/pull/1021/files/31ff623213f0fed16947060e8a0eccec5e02f4ff#diff-f4e83572e27854d40d56ef54e8f982cdca3ccaaa3f17900c3ab6e3f4fb04e971R42-R44 where the err is simply returned. Then we seem to return it here as well. Not setting anything on the GatewayClass. Or am I missing something here?
There was a problem hiding this comment.
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.
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.
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.
If that's an error related to permissions, for example, that isn't related to the resource, but to the operator itself.
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 Get from 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.
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.
But here we are discussing about putting information that has nothing to do with the resource itself.
Agreed. Nevertheless this information is beneficial to the user to quickly resolve an issue with this object not being accepted.
I think we should instead try to educate users about what kind of information can be found where.
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 👍
The GWC accepted condition is now properly set to False with reason InvalidParameters in case the ParametersRef field of the GatewayClass spec is not a valid GatewayConfiguration. Signed-off-by: Mattia Lavacca <[email protected]>
Co-authored-by: Patryk Małek <[email protected]>
mlavacca
force-pushed
the
validate-parametersref
branch
from
7c44310 to
5bee6d2
Compare
Co-authored-by: Jakub Warczarek <[email protected]>
Signed-off-by: Mattia Lavacca <[email protected]>
mlavacca
force-pushed
the
validate-parametersref
branch
from
dec6613 to
6f71bcc
Compare
Signed-off-by: Mattia Lavacca <[email protected]>
mlavacca
force-pushed
the
validate-parametersref
branch
from
6f71bcc to
5af207e
Compare
| condition metav1.Condition | ||
| } | ||
|
|
||
| func NewErrNotAcceptedGatewayClass(gatewayClass string, condition metav1.Condition) ErrNotAcceptedGatewayClass { |
| return ErrNotAcceptedGatewayClass{gatewayClass: gatewayClass, condition: condition} | ||
| } | ||
|
|
||
| func (e ErrNotAcceptedGatewayClass) Error() string { |
What this PR does / why we need it:
The GWC accepted condition is now properly set to False with reason InvalidParameters in case the ParametersRef field of the GatewayClass spec is not a valid GatewayConfiguration.
Which issue this PR fixes
Fixes #982
Special notes for your reviewer:
PR Readiness Checklist:
Complete these before marking the PR as
ready to review:CHANGELOG.mdrelease notes have been updated to reflect significant changes