-
Notifications
You must be signed in to change notification settings - Fork 615
api: experimental ResolvedRefs condition for Gateway #4195
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.
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Norwin Schnyder <norwin.schnyder+github@gmail.com>
11ba606 to
bc1e880
Compare
|
Nice work, thanks @snorwin. /approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: snorwin, youngnick The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
4856b7f to
0818f34
Compare
Signed-off-by: Norwin Schnyder <norwin.schnyder+github@gmail.com>
0818f34 to
425d501
Compare
| // * It refers to a resource that cannot be resolved (e.g., the referenced resource | ||
| // does not exist) or is misconfigured (e.g., a Secret does not contain the keys | ||
| // named `tls.crt` and `tls.key`). In this case, the `ResolvedRefs` condition | ||
| // on the Gateway MUST be set to False with the Reason `InvalidClientCertificateRef` |
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'm worried that Cx might confused this error with other TLS based features like ClientCertValidation, can we be more specifica about the certificate?
InvalidClientCertificateRef -> InvalidGatewayClientCertificateRef
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.
Wouldn't InvalidBackendClientCertificateRef be more accurate?
However, it is quite long, and since a Gateway only has one clientCertificateRef field, it should be clear which one is meant.
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.
InvalidBackendClientCertificateRef is not accurate because this is Gateway's certificate to communicate with Backends.
|
|
||
| // This reason is used with the "ResolvedRefs" condition when one or more | ||
| // Listeners have their "ResolvedRefs" condition set to false in their status. | ||
| GatewayReasonListenersNotResolved GatewayConditionReason = "ListenersNotResolved" |
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 think it will be useful to specify what should be overall Gateway Status when References are not resolved. If if is already defined please point me to the docs.
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.
What are you referring to as the overall Gateway status, the Accepted status condition?
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.
yes
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 think it might be tricky since the setting is on Gateway but its usage depends on BackendTLSPolicy so maybe it should not impact Accepted condition but it is worth to define that.
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.
Good point, I had implicitly assumed that the ResolvedRefs condition wouldn’t affect acceptance (this was the main reason for introducing a new condition). I’ll make this explicit and add a short description.
What type of PR is this?
/kind documentation
/kind feature
What this PR does / why we need it:
This PR follows up on #4123 to update the GoDoc comments and introduce a new GatewayConditionType constant of
ResolvedRefs, with associated Reason as its own GatewayConditionReason.Which issue(s) this PR fixes:
N/A
Does this PR introduce a user-facing change?: