TLS bootstrap API group (alpha) #25562
Conversation
k8s-github-robot
added
kind/api-change
|
@k8s-bot ok to test: #IGNORE |
|
@kubernetes/sig-api-machinery @bgrant0607 this change contains an alpha apigroup addition and exposure. I will do my best to review the csi changes but would appreciate if someone skimmed. |
| Fingerprint string `json:"fingerprint,omitempty"` | ||
|
|
||
| // Subject fields from the request | ||
| Subject Subject `json:"subject,omitempty"` |
There was a problem hiding this comment.
I think subject should be moved into this file and have json struct tags so it serializes with the api-convention lower camecase.
There was a problem hiding this comment.
Sounds good to me. I don't want it to be processed as a top-level API object though, will inclusion in types.go but not in register.go be fine for that?
There was a problem hiding this comment.
Yes that is ok. Most types in types files are not top level api objects. e.g. CertificateSignedRequestStatus
|
placement of fields in Spec vs Status looks very good. Thanks! |
| optional string message = 3; | ||
|
|
||
| // timestamp for the last update to this condition | ||
| optional k8s.io.kubernetes.pkg.api.unversioned.Time lastupdatetime = 4; |
There was a problem hiding this comment.
@smarterclayton have you considered converting field names from lowerCamelCase to snake_case during proto gen?
There was a problem hiding this comment.
I guess this is dicated from struct tags. What is the convention?
|
cc @liggitt |
| func NewREST(opts generic.RESTOptions) (*REST, *StatusREST, *ApprovalREST) { | ||
| prefix := "/certificatesigningrequests" | ||
|
|
||
| newListFunc := func() runtime.Object { return &certificates.CertificateSigningRequestList{} } |
There was a problem hiding this comment.
nit: why is this extracted? why not inline like the others
There was a problem hiding this comment.
no particular reason. i'll inline it.
There was a problem hiding this comment.
oh, right- this is used in the the opts.Decorator initialization as well as the registry store.
|
@gtank If you rebase and fixup commit histroy, I will LGTM this. |
gtank
force-pushed
the
certificates-api-v9
branch
from
9f144f4 to
bcc7bdd
Compare
k8s-github-robot
removed
needs-rebase
|
GCE e2e build/test failed for commit bcc7bdd9e8e0ed5f4ce74439882cfe9ccfd6c3f0. Please reference the list of currently known flakes when examining this failure. If you request a re-test, you must reference the issue describing the flake. |
gtank
force-pushed
the
certificates-api-v9
branch
from
bcc7bdd to
d74278e
Compare
|
GCE e2e build/test passed for commit d74278e761399ee297b54b7a88bd5e22f8c8a838. |
gtank
force-pushed
the
certificates-api-v9
branch
from
d74278e to
c9c6fff
Compare
|
GCE e2e build/test passed for commit c9c6fff. |
mikedanese
added
lgtm
|
p0 to get out of rebase hell |
|
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
|
GCE e2e build/test passed for commit c9c6fff. |
|
Automatic merge from submit-queue |
|
This is awesome! Can you make sure to get a proposal in the features repo for this? I know it all derives from an existing proposal, but we should definitely highlight this work. |
Automatic merge from submit-queue Certificate signing controller for TLS bootstrap (alpha) The controller handles generating and signing certificates when a CertificateSigningRequest has the "Approved" condition. Uses cfssl to support a wide set of possible keys and algorithms. Depends on PR #25562, only the last two commits are relevant to this PR. cc @mikedanese []()
This PR only covers the new types and related client/storage code- the vast majority of the line count is codegen. The implementation differs slightly from the current proposal document based on discussions in design thread (#20439). The controller logic and kubelet support mentioned in the proposal are forthcoming in separate requests.
I submit that #18762 ("Creating a new API group is really hard") is, if anything, understating it. I've tried to structure the commits to illustrate the process.
@mikedanese @erictune @smarterclayton @deads2k