gep: refine CACertificateRefs description for frontend TLS #4183
Conversation
Signed-off-by: Norwin Schnyder <norwin.schnyder+github@gmail.com>
k8s-ci-robot
added
release-note-none
k8s-ci-robot
added
do-not-merge/hold
|
/retest |
Signed-off-by: Norwin Schnyder <norwin.schnyder+github@gmail.com>
kl52752
left a comment
kl52752
left a comment
There was a problem hiding this comment.
LGTM!
Thanks for the PR :)
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: kl52752, snorwin The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/assign @shaneutt |
| This condition is removed as soon as `FrontendValidationModeType` is changed back to `AllowValidOnly`. | ||
|
|
||
| * Introduce a `ObjectReference` structure that can be used to specify `caCertificateRefs` references. | ||
| * Invalid `caCertificateRefs` directly affect the `ResolvedRefs` and `Accepted` conditions of the targeted listeners. |
| // Reason must be set to `InvalidCACertificateRef` and the Message of | ||
| // the Condition must indicate which reference is invalid and why. |
| // * It refers to a resource in another namespace UNLESS there is a | ||
| // ReferenceGrant in the target namespace that allows the CA | ||
| // certificate to be attached. If a ReferenceGrant does not allow this | ||
| // reference, the `ResolvedRefs` condition MUST be set with | ||
| // the Reason `RefNotPermitted`. |
| // Implementations MAY choose to support attaching multiple CA certificates | ||
| // to a listener, but this behavior is implementation-specific. |
There was a problem hiding this comment.
🤔 can we talk through this one a bit more?
There was a problem hiding this comment.
I think this duplicates the intent of the following:
// Support: Implementation-specific - More than one reference, other kinds
// of resources, or a single reference that includes multiple certificates.
It was inspired by the description of the CACertificateRefs in the BackendTLSPolicy:
https://github.com/kubernetes-sigs/gateway-api/blob/main/apis/v1/backendtlspolicy_types.go#L164-L165
There was a problem hiding this comment.
Can you walk through some scenarios where this is going to be needed, and share your experience and thoughts on why the behavior is "implementation-specific" and is not standardizable?
There was a problem hiding this comment.
I think that multiple references is a common case for rotating the certificates. User might want to attach a new CA cert to gateway and be in the state where gateway will check both CA certificates to ensure smooth transition to a new certificate for all clients. Then old CA certificate will be deleted. We can achieve that with a new ConfigMap but having multiple certificates in one map is convenient
There was a problem hiding this comment.
I think there are use cases where clients connect from different environments and therefore rely on different identity providers (CAs) to sign their client certificates. To support this, we need the ability to configure multiple CACertificateRefs.
The second question is whether this should be implementation-specific. During the refinement of the BackendTLSPolicy, we weren’t able to find a common approach that worked for all. To keep things flexible, we defined only a single CA certificate bundle (specified as ConfigMap) as "Core" and left anything beyond that to implementations. For consistency, I followed the same approach here.
7 participants
What type of PR is this?
/kind gep
What this PR does / why we need it:
In the current version of GEP-91, the semantics of an invalid
CACertificateRefsare not fully specified. In addition, it is unclear whether theInsecureFrontendValidationModecondition should ever be removed or set to False.This PR clarifies the expected behavior by aligning the semantics of invalid references in frontend validation with the patterns already established in BackendTLSPolicy and the approach proposed in #4123 for backend TLS configuration on the Gateway.
Which issue(s) this PR fixes:
N/A
Does this PR introduce a user-facing change?: