Update CSR controllers & kubelet to respect signerName field #88246
Conversation
k8s-ci-robot
added
the
release-note-none
k8s-ci-robot
added
kind/feature
|
I set release note to NONE as the first PR already adds:
And the admission PR has:
which seems sufficient to me 😄 Is there some way to collect release notes specific to breaking changes in |
munnerz
force-pushed
the
csr-signername-controllers
branch
2 times, most recently
from
5f4e078
to
6791c48
Compare
|
@liggitt I've updated this to address your comments 😄 |
munnerz
force-pushed
the
csr-signername-controllers
branch
from
6791c48
to
453413c
Compare
| case capi.KubeletServingSignerName: | ||
| return capihelper.IsKubeletServingCSR(req, usages) | ||
| case capi.KubeAPIServerClientKubeletSignerName: | ||
| return capihelper.IsKubeletServingCSR(req, usages) |
There was a problem hiding this comment.
@liggitt if the CSR does not exactly match, what should the signing controller do? It could either:
- override the values
- reject the request/mark it as invalid somehow (we don't have this option yet)
- display a warning to the user
- do nothing at all
Right now with this change we do (4), and simply return nil.
There was a problem hiding this comment.
Adding (3) by simply firing an Event and returning nil could work, but has the potential to cause a lot of events as we have no way to recognise the terminal state of the CR.
(2) is the best solution long-term for external signers IMO, but probably needs consideration in the KEP due to the awkward design of the CSR conditions structure.
(1) is an option too - it'd require duplicating/shuffling around some of this checking logic to allow for usages to be different to what's requested by the user. It may be non-obvious though, and it isn't something that's been done in the past (old logic requires usages to be specified as-per policy). Given it's a net-addition and goes beyond what was described in the KEP changes however, I feel like it might be a bit brash to add it in here.
There was a problem hiding this comment.
Do 4 for now. I'm looking into how to let signers indicate permanent failure in the CSR status
|
This PR may require API review. If so, when the changes are ready, complete the pre-review checklist and request an API review. Status of requested reviews is tracked in the API Review project. |
|
thanks for the heads up. i've opened: i think i digested some of the details, but have one question regarding the timeline of the kubeadm change (1.18 vs next):
if true this means we need to apply this change to kubeadm before CF next week and for 1.18. disclaimer: i wouldn't exclude that i'm misunderstanding something. |
|
CSR objects that don't have a signerName get defaulted based on the format of the request. Requests shaped like node client or serving certificates get those signerNames assigned, so auto approval flows for node client certificates continue working uninterrupted.
|
ok, thanks for the clarification. |
|
I'll take #88246 (comment) as a follow up, would like to get this soaking ahead of freeze next week /lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: liggitt, munnerz 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 |
k8s-ci-robot
added
the
approved
|
/hold cancel |
k8s-ci-robot
removed
the
do-not-merge/hold
| l.Insert(string(s)) | ||
| } | ||
| r := sets.NewString() | ||
| for _, s := range right { |
There was a problem hiding this comment.
It seems there is no need to create the second Set.
Checking sizes of the slices and whether each of the second slice is contained in the first Set should suffice.
There was a problem hiding this comment.
that would return true for [a, b, c] vs [a, a, b]
There was a problem hiding this comment.
The following would achieve same semantics with one map.
func equalUnsorted(left, right []certificatesv1beta1.KeyUsage) bool {
var l map[string]int
l = make(map[string]int)
for _, s := range left {
k := string(s)
cnt := l[k]
l[k] = cnt + 1
}
for _, s := range right {
k := string(s)
cnt, ok := l[k]
if !ok {
return false
}
cnt--
if cnt == 0 {
delete(l, k)
} else {
l[k] = cnt
}
}
return len(l) == 0
}
There was a problem hiding this comment.
if we really want to be cheap:
func equalUnsorted(left, right []certificatesv1beta1.KeyUsage) bool {
if len(left) != len(right) {
return false
}
l := make(map[string]int, len(left))
for _, s := range left {
k := string(s)
l[k] = l[k] + 1
}
for _, s := range right {
k := string(s)
c := l[k]
if c == 0 {
return false
}
l[k] = (c - 1)
}
return true
}but we do use the sets API all over, I'm not sure if this change is desirable.
There was a problem hiding this comment.
if len(left) != len(right) {
return false
}
We shouldn't use the above check since there may be duplicate in either side.
c := l[k]
if c == 0 {
The decrement should be done before checking c against 0.
I prefer the formation I presented above.
Set is not expressive in this use case. We don't have to use it.
There was a problem hiding this comment.
We shouldn't use the above check since there may be duplicate in either side.
If we're not checking for identical counts in each side, why even count in the map ...?
The decrement should be done before checking c against 0.
No, if we decrement to zero then we've had an exact match. If we find an entry with zero on the next check we did not. I did test this code 🙃
There was a problem hiding this comment.
Thanks @BenTheElder for looking at my code.
Before we dig deeper into the code, can we get agreement on whether the following are considered equal:
[a, b] vs [a, a, b]
Along the current code, they're equal.
That was why I said we shouldn't check the lengths of two sides.
There was a problem hiding this comment.
@BenTheElder was right in that the value of the map shouldn't be the number of times we see the same key from the left.
Here is revised code which keeps the current semantics and only uses one map where value of 1 represents 'seen from the left' and value of -1 means 'seen from the right' (regardless the number of times seen):
func equalUnsorted(left, right []certificatesv1beta1.KeyUsage) bool {
var l map[string]int
l = make(map[string]int)
for _, s := range left {
k := string(s)
l[k] = 1
}
count := 0
for _, s := range right {
k := string(s)
val, ok := l[k]
if !ok {
// not equal if we haven't seen this key from the left
return false
}
if val == 1 {
count++
// we don't want to delete the key since there may be more occurrence of this key in right
l[k] = -1
}
}
// if we have seen all the keys from the right, they're equal
return len(l) == count
}
I can refine the comment in the code if there is question on how it works or whether it is correct.
Thanks
There was a problem hiding this comment.
We can keep this on the back burner and look out for frequent calls to similar equality checks.
| } | ||
| if !requestValidForSignerName(x509cr, csr.Spec.Usages, *csr.Spec.SignerName) { | ||
| // TODO: mark the CertificateRequest as being in a terminal state and | ||
| // communicate to the user why the request has been refused. |
There was a problem hiding this comment.
Is an error return suitable for conveying that the CertificateRequest is in terminal state ?
There was a problem hiding this comment.
We need a place to record it on the API object for that to be effective. I'm working on a well-defined condition to communicate that.
|
I don't think this change is necessary, the current code is very readable
and AIUI this will not be heavily called.
…On Sat, Feb 29, 2020, 13:14 Ted Yu ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In pkg/apis/certificates/v1beta1/defaults.go
<#88246 (comment)>
:
> + if !equalUnsorted(requiredUsages, usages) {
+ return false
+ }
+
+ return true
+}
+
+// equalUnsorted compares two []string for equality of contents regardless of
+// the order of the elements
+func equalUnsorted(left, right []certificatesv1beta1.KeyUsage) bool {
+ l := sets.NewString()
+ for _, s := range left {
+ l.Insert(string(s))
+ }
+ r := sets.NewString()
+ for _, s := range right {
@BenTheElder <https://github.com/BenTheElder> was right in that the value
of the map shouldn't be the number of times we see the same key from the
left.
Here is revised code which keeps the current semantics and only uses one
map:
func equalUnsorted(left, right []certificatesv1beta1.KeyUsage) bool {
var l map[string]int
l = make(map[string]int)
for _, s := range left {
k := string(s)
l[k] = 1
}
count := 0
for _, s := range right {
k := string(s)
val, ok := l[k]
if !ok {
// not equal if we haven't seen this key from the left
return false
}
if val == 1 {
count++
// we don't want to delete the key since there may be more occurrence of this key in right
l[k] = -1
}
}
// if we have seen all the keys from the right, they're equal
return len(l) == count
}
I can refine the comment in the code if there is question on how it works
or whether it is correct.
Thanks
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#88246?email_source=notifications&email_token=AAHADKZZN4JEAEYLLNAMUPTRFF5ELA5CNFSM4KWVOJD2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCXOTYVY#discussion_r386056037>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAHADK2Z7V3EBBSKQRKF47TRFF5ELANCNFSM4KWVOJDQ>
.
|
|
+1 to @BenTheElder's comment. We should avoid premature optimization that negatively impact readability and maintainability. |
|
Amazing, thanks a lot @munnerz! |
k8s-ci-robot
added
release-note-action-required
liggitt
added
the
release-note-none
k8s-ci-robot
removed
the
release-note-none
New permissions are required to approve CSRs in 1.18, as per [1]. 1: kubernetes/kubernetes#88246
…ollers Update CSR controllers & kubelet to respect signerName field Kubernetes-commit: 03b7f27
What type of PR is this?
/kind feature
What this PR does / why we need it:
Adds support for the
signerNamefield added to the CertificateSigningRequest resource as part of the changes described in kubernetes/enhancements#1400 to:RequestCertificatehelper function.Special notes for your reviewer:
The top commit of this branch is the one relevant to this PR 😄
This PR depends on:
Does this PR introduce a user-facing change?:
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:
/cc @enj @liggitt @deads2k