migrate pkg/server/dynamiccertificates to contextual logging #126002
Conversation
|
/release-note-none |
|
/retest |
|
/wg structured-logging |
|
there is error as below message, it seems there is no related with PR changes. Do you think I should fix this error in the same PR, or submit a new PR to handle it? @mengjiao-liu , what is your suggestion? staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/tlsconfig.go:214:3: SA1019: newTLSConfigCopy.NameToCertificate has been deprecated since Go 1.14: NameToCertificate only allows associating a single certificate with a given name. Leave this field nil to let the library select the first compatible chain from Certificates. (staticcheck) |
Because the changes are related to this PR, you can commit two different commits in this PR for the static check and log migration sections. If you feel that the content of the change is too much, you can submit a separate PR. |
thanks @mengjiao-liu , will submit a separate PR to handle this error. |
|
After investigation and check previous PR about deprecated "newTLSConfigCopy.NameToCertificate", it is fundamental function, should be very carefully to remove. and I have no idea to fix it til now. I have to get the help/consultant/feedback from @liggitt @deads2k and @jackkleeman , who worked on related PR about certificate before. Do you think it is ok to leave this lint check or I have to remove the deprecated method? Thanks your feedback and any suggestion. |
add the related PR here: /cc @liggitt @deads2k @jackkleeman for your reference. |
just got the feedback from Jack, he might have no idea on my question. thanks Jack. |
|
LGTM label has been added. Git tree hash: 365855ce18d311fca578f324df9ca27238780460
|
|
/assign @jpbetz , @smarterclayton , @tallclair please help to approve, thanks. |
any comment? |
any feedback? |
staging/src/k8s.io/pod-security-admission/cmd/webhook/server/server.go
Outdated
Show resolved
Hide resolved
Signed-off-by: liyuerich <yue.li@daocloud.io>
01bc2e8 to
8d1993f
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: liyuerich 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 |
|
@liyuerich: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/retest-required |
1 similar comment
|
/retest-required |
|
/lgtm |
|
LGTM label has been added. Git tree hash: 0fc9f0e76a0cf6026ba2f4d1c7dd30d9affbb05a
|
|
hi @jpbetz , @smarterclayton , @tallclair is it ok to help to approve and merge now? thanks. |
|
/assign @jpbetz @smarterclayton @tallclair to approve sorry , have to send this message again. |
|
/test pull-kubernetes-cmd |
|
seems all checks are pass, @jpbetz , @smarterclayton , @tallclair is it ok to approve and merge now? also /cc @mengjiao-liu @pohly , any comment or suggestion? |
LGTM from my side. |
| @@ -74,7 +74,7 @@ type caBundleAndVerifier struct { | |||
| } | |||
|
|
|||
| // NewDynamicCAContentFromFile returns a CAContentProvider based on a filename that automatically reloads content | |||
| func NewDynamicCAContentFromFile(purpose, filename string) (*DynamicFileCAContent, error) { | |||
| func NewDynamicCAContentFromFile(ctx context.Context, purpose, filename string) (*DynamicFileCAContent, error) { | |||
There was a problem hiding this comment.
I see usages for this function and NewDynamicServingContentFromFiles outside just apiserver. (kubelet and cloud-provider). Do we need an additional NewDynamicCAContentFromFileWithContext function to provide backwards compatibility?
Though It's possible this comment applies #126002 (comment) for these controllers as well.
There was a problem hiding this comment.
yes, it is used outside, will add new one.
| klog.Warningf("Initial population of dynamic certificates failed: %v", err) | ||
| } | ||
| go dynamicCertificateController.Run(1, stopCh) | ||
| go dynamicCertificateController.Run(ctx, 1, stopCh) |
There was a problem hiding this comment.
it's really confusing to pass ctx and stopCh in, when ctx also cancels when stopCh is closed
There was a problem hiding this comment.
will fix soon. thanks
| @@ -166,7 +166,7 @@ func Run(ctx context.Context, opts options.CompletedOptions) error { | |||
| } | |||
|
|
|||
| // CreateServerChain creates the apiservers connected via delegation. | |||
| func CreateServerChain(config CompletedConfig) (*aggregatorapiserver.APIAggregator, error) { | |||
| func CreateServerChain(ctx context.Context, config CompletedConfig) (*aggregatorapiserver.APIAggregator, error) { | |||
There was a problem hiding this comment.
need to make it clear if the provided context is retained and controls lifecycle of things beyond this function invocation or not (this comment holds for most of the changes)
It's really hard to reason about what context should be passed in here depending on whether it is just used for logging during the function invocation, or if the cancellation of the context after the function returns does something
There was a problem hiding this comment.
by drill down CreateServerChain, and finally in loadCertKeyPair, you could find:
c.certKeyPair.Store(newCertKey)
logger := klog.FromContext(ctx)
logger.V(2).Info("Loaded a new cert/key pair", "name", c.Name())
that is why a ctx is needed.
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
migrate pkg/server/dynamiccertificates to contextual logging
Which issue(s) this PR fixes:
Ref # kubernetes/enhancements#3077
Special notes for your reviewer:
NONE
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: