Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dynamic cert file ca bundle #83579

Merged
merged 2 commits into from Oct 23, 2019

Conversation

@deads2k
Copy link
Contributor

deads2k commented Oct 7, 2019

Dynamically reloads client certificates for both authentication and serving hints.

client-ca bundles for the all generic-apiserver based servers will dynamically reload from disk on content changes
return nil
}
if servingInfo.ClientCA == nil {
servingInfo.ClientCA = clientCA

This comment has been minimized.

Copy link
@jackkleeman

jackkleeman Oct 7, 2019

Member

Another option would be to get NewUnionCAContentProvider to simply return value A if value B is nil

go wait.Until(c.runWorker, time.Second, stopCh)

// start timer that rechecks every minute, just in case. this also serves to prime the controller quickly.
_ = wait.PollImmediateUntil(1*time.Minute, func() (bool, error) {

This comment has been minimized.

Copy link
@jackkleeman

jackkleeman Oct 7, 2019

Member

If we are going to poll inside of the dynamic providers, do we need to poll in the main controller in tlsconfig.go?

This comment has been minimized.

Copy link
@deads2k

deads2k Oct 7, 2019

Author Contributor

If we are going to poll inside of the dynamic providers, do we need to poll in the main controller in tlsconfig.go?

I think we should. They control two different things. The individual files can trigger the overall, but we also want a backstop on these. It's more about implementations of the interface you did not create.

}

// check to see if we have a change. If the values are the same, do nothing.
existing, ok := c.caBundle.Load().(*caBundleAndVerifier)

This comment has been minimized.

Copy link
@jackkleeman

jackkleeman Oct 7, 2019

Member

It's worth noting that we store pointers in the Value here, but store a non-pointer tls.Config; is there a particular reason either way?

This comment has been minimized.

Copy link
@deads2k

deads2k Oct 7, 2019

Author Contributor

It's worth noting that we store pointers in the Value here, but store a non-pointer tls.Config; is there a particular reason either way?

This felt more natural. I don't have a strong opinion. I guess the pointer for tlsconfig may be more pure. If it shows up in a profile we can adjust.

if err != nil {
return err
}
c.caBundle.Store(caBundleAndVerifier)

This comment has been minimized.

Copy link
@jackkleeman

jackkleeman Oct 7, 2019

Member

It seems to me that we cache the overall tls.Config in an atomic.Value, and cache the consituent dynamic elements also. Why not simply have CurrentCABundleContent always do a read off of disk, and have the only stateful element be the caBundleContent

This comment has been minimized.

Copy link
@jackkleeman

jackkleeman Oct 7, 2019

Member

I feel like if we have 10 SNI certs using a similar approach, thats a lot of controllers polling, or we would need a separate SNIProvider that could unify them. If we always read the file on CurrentCABundleContent, then really we just have one goroutine polling, and maybe some file watches too

I feel like it makes sense for a Provider to know how to build a completely fresh certificate; and callers cache it as desired

This comment has been minimized.

Copy link
@deads2k

deads2k Oct 7, 2019

Author Contributor

Files are only one implementation. I'm actually driving this towards consuming a list/watch from the API. It's comparatively expensive to pull fresh state from that. We're still only talking about o(10) loops/

c.caBundle.Store(caBundleAndVerifier)

for _, listener := range c.listeners {
listener.Enqueue()

This comment has been minimized.

Copy link
@jackkleeman

jackkleeman Oct 7, 2019

Member

It looks like we cause an enqueue irrelevant of whether the bundle changes

This comment has been minimized.

Copy link
@deads2k

deads2k Oct 7, 2019

Author Contributor

It looks like we cause an enqueue irrelevant of whether the bundle changes

we returned early a few lines up if nothing changed.

// AllowedClientNames is a list of common names that may be presented by the authenticating front proxy. Empty means: accept any.
AllowedClientNames []string
}

// CAContentProvider provides ca bundle byte content
type CAContentProvider interface {

This comment has been minimized.

Copy link
@jackkleeman

jackkleeman Oct 7, 2019

Member

How come this needs to be duplicated? Import cycle?

This comment has been minimized.

Copy link
@deads2k

deads2k Oct 7, 2019

Author Contributor

How come this needs to be duplicated? Import cycle?

Yeah, the authenticator shouldn't import an impl.

@deads2k deads2k force-pushed the deads2k:dynamic-cert-file-ca-bundle branch from 4257b7c to 0dbfb66 Oct 7, 2019
@deads2k

This comment has been minimized.

Copy link
Contributor Author

deads2k commented Oct 7, 2019

Updated to the point of being able to create integration tests for reloading the certificates from disk. I still owe the tests, but I think it's wired.


// check to see if we have a change. If the values are the same, do nothing.
existing, ok := c.caBundle.Load().(*caBundleAndVerifier)
if ok && existing != nil && reflect.DeepEqual(existing.caBundle, caBundle) {

This comment has been minimized.

Copy link
@jackkleeman

jackkleeman Oct 8, 2019

Member

bytes.Equal again here

@dims

This comment has been minimized.

Copy link
Member

dims commented Oct 16, 2019

/uncc

@k8s-ci-robot k8s-ci-robot removed the request for review from dims Oct 16, 2019
@deads2k deads2k force-pushed the deads2k:dynamic-cert-file-ca-bundle branch from 0dbfb66 to acd8324 Oct 17, 2019
@k8s-ci-robot k8s-ci-robot added size/L and removed size/XXL labels Oct 17, 2019
@deads2k deads2k changed the title [wip] Dynamic cert file ca bundle Dynamic cert file ca bundle Oct 17, 2019
@deads2k deads2k force-pushed the deads2k:dynamic-cert-file-ca-bundle branch from acd8324 to d3d3200 Oct 17, 2019
@deads2k

This comment has been minimized.

Copy link
Contributor Author

deads2k commented Oct 22, 2019

/retest

}

// check to see if we have a change. If the values are the same, do nothing.
existing, ok := c.caBundle.Load().(*caBundleAndVerifier)

This comment has been minimized.

Copy link
@sttts

sttts Oct 23, 2019

Contributor

The type conversion will panic for nil in the atomic value.

@deads2k deads2k force-pushed the deads2k:dynamic-cert-file-ca-bundle branch from 8395713 to 117a0b2 Oct 23, 2019
@@ -21,10 +21,11 @@ import (
"fmt"
"reflect"

"k8s.io/apiserver/pkg/server/dynamiccertificates"

This comment has been minimized.

Copy link
@sttts

sttts Oct 23, 2019

Contributor

nit: order

@deads2k

This comment has been minimized.

Copy link
Contributor Author

deads2k commented Oct 23, 2019

/retest

@deads2k deads2k force-pushed the deads2k:dynamic-cert-file-ca-bundle branch from 117a0b2 to 6beb962 Oct 23, 2019
@sttts

This comment has been minimized.

Copy link
Contributor

sttts commented Oct 23, 2019

/lgtm

@derekwaynecarr

This comment has been minimized.

Copy link
Member

derekwaynecarr commented Oct 23, 2019

kubelet updates look good.

/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Oct 23, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, derekwaynecarr

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@deads2k

This comment has been minimized.

Copy link
Contributor Author

deads2k commented Oct 23, 2019

/retest

1 similar comment
@jackkleeman

This comment has been minimized.

Copy link
Member

jackkleeman commented Oct 23, 2019

/retest

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Oct 23, 2019

@deads2k: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-node-e2e-containerd 6beb962 link /test pull-kubernetes-node-e2e-containerd

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/test-infra repository. I understand the commands that are listed here.

@deads2k

This comment has been minimized.

Copy link
Contributor Author

deads2k commented Oct 23, 2019

/refresh

@deads2k

This comment has been minimized.

Copy link
Contributor Author

deads2k commented Oct 23, 2019

/kind bug

@k8s-ci-robot k8s-ci-robot merged commit 0f1a805 into kubernetes:master Oct 23, 2019
15 of 16 checks passed
15 of 16 checks passed
pull-kubernetes-node-e2e-containerd Job failed.
Details
cla/linuxfoundation deads2k authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-kind-ipv6 Job succeeded.
Details
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-kind Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
tide In merge pool.
Details
@k8s-ci-robot k8s-ci-robot added this to the v1.17 milestone Oct 23, 2019
@jackkleeman jackkleeman mentioned this pull request Nov 12, 2019
6 of 6 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.