Repository navigation
PodCertificateRequest v1 support - #1829
Conversation
This adds dual v1beta1 and v1 PCR support, favoring v1. We have seen users on v1.37 without v1beta1 but with v1 seeing incomaptibility. Given our usage of PCR is pretty scoped its not too bad to have the dual version support.
Eitan Yarmush (EItanya)
left a comment
Eitan Yarmush (EItanya)
left a comment
There was a problem hiding this comment.
🤖 AI-generated review.
Non-blocking compatibility note: with a 1.37 control plane and 1.35 kubelets, legacy PCRs contain PKIXPublicKey, which Kubernetes omits when serving them through v1. Preferring v1 therefore prevents these requests from being signed, even when v1beta1 remains available. This combination is allowed by Kubernetes' version-skew policy.
Documenting that nodes using these certificates must run kubelet 1.36+ before upgrading the control plane to 1.37 would be a reasonable resolution; this doesn't need to block the PR on adding fallback code.
|
|
||
| // GetCached reads from the owned informer cache and returns the signers' beta representation. | ||
| // Callers must treat the result as read-only, as with a standard lister. | ||
| func (c *Client) GetCached(namespace, name string) (*certsv1beta1.PodCertificateRequest, error) { |
There was a problem hiding this comment.
nit: Can we just say Get? GetCached seems to just leak the detail
There was a problem hiding this comment.
The distinction is pretty important when dealing with informers
|
This solution makes sense to me, I think Taahir Ahmed (@ahmedtd) should also take a look |
|
Hi, I noticed that |
I think it fits but I didn't want to bloat up this PR |
|
|
||
| // GetCached reads from the owned informer cache and returns the signers' beta representation. | ||
| // Callers must treat the result as read-only, as with a standard lister. | ||
| func (c *Client) GetCached(namespace, name string) (*certsv1beta1.PodCertificateRequest, error) { |
There was a problem hiding this comment.
The distinction is pretty important when dealing with informers
|
|
||
| // toBeta copies the fields shared by the stable and beta APIs. | ||
| func toBeta(pcr *certsv1.PodCertificateRequest) *certsv1beta1.PodCertificateRequest { | ||
| pcr = pcr.DeepCopy() |
There was a problem hiding this comment.
Not sure if the clone is needed, assuming the caller continues to treat the result as read-only. But I don't mind much either way.
Bowei Du (bowei)
added
area/security
Relates to agent-substrate#1819 This is the followup to agent-substrate#1829 which did PCR. > It's a good idea to open an issue first for discussion. - [x] Tests pass - [x] Appropriate changes to documentation are included in the PR
This adds dual v1beta1 and v1 PCR support, favoring v1. We have seen users on v1.37 without v1beta1 but with v1 seeing incomaptibility.
Given our usage of PCR is pretty scoped its not too bad to have the dual version support.
Fixes #<issue_number_goes_here>