apiserver identity : use SHA256 hash in lease names #113649
Conversation
k8s-ci-robot
added
release-note-none
k8s-ci-robot
added
area/apiserver
area/test
sig/api-machinery
|
@andrewsykim looks like the verify is barfing on gofmt. can you please do that? |
andrewsykim
force-pushed
the
apiserver-identity-hash
branch
from
d489510
to
80f29a7
Compare
| h.Write([]byte(hostname)) | ||
| id = "kube-apiserver-" + fmt.Sprint(h.Sum32()) | ||
| hash := sha256.Sum256([]byte(hostname)) | ||
| id = "kube-apiserver-" + hex.EncodeToString(hash[:]) |
There was a problem hiding this comment.
This will be 64 characters long, I'd suggest maybe hash[:16] to get it down to 32 characters. Or use encoding/base32 or 64 (but might need to remove forbidden characters). I thought we had a base58 encoding package in use but I can't seem to find it if so.
There was a problem hiding this comment.
I still do not understand why we feel the need to truncate here. Are we expecting people to interact with these values somehow? That being said normally I use base32:
"kube-apiserver-" + strings.ToLower(base32.StdEncoding.WithPadding(base32.NoPadding).EncodeToString(hash[:]))which gives you something like kube-apiserver-ftze3os7wcrq4jxihmvmlopctynrmhs4d6tuexttaqzwfe4ltasa (67 characters total, 52 for the hash.
There was a problem hiding this comment.
It's mostly just nicer to deal with shorter strings. Your solution works for me. You could call base32.NewEncoding() instead of using ToLower every time, that would make it more obvious that it's not obscuring the difference between upper and lowercase letters.
There was a problem hiding this comment.
sgtm, will update shortly
There was a problem hiding this comment.
Updated to use base32 encoding and only the first 16 bytes of the hash. I ended up leaving the strings.ToLower in there to avoid creating two copies of the encoder string.
There was a problem hiding this comment.
Actually inclined to stick with hex encoding with hash[:16]. It's only a few more characters but it's just more consistent with everything else that uses hex encoding in our codebase.
There was a problem hiding this comment.
I don't think we use hex in object names?
There was a problem hiding this comment.
base 16 is just extremely not dense.
andrewsykim
force-pushed
the
apiserver-identity-hash
branch
from
80f29a7
to
dba66fd
Compare
Signed-off-by: Andrew Sy Kim <andrewsy@google.com>
andrewsykim
force-pushed
the
apiserver-identity-hash
branch
from
dba66fd
to
5b3a9e2
Compare
|
I might add a comment to the effect that the ToLower isn't a problem because there's no mixed case. But not urgent. /lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andrewsykim, lavalamp 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
|
/triage accepted |
k8s-ci-robot
added
the
triage/accepted
k8s-ci-robot
removed
the
needs-triage
Signed-off-by: Andrew Sy Kim andrewsy@google.com
What type of PR is this?
/kind feature
What this PR does / why we need it:
Following up on #113307 (comment) and updating the apiserver identity lease to use a SHA256 hash in the lease name.
Which issue(s) this PR fixes:
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: