New command for stand-alone GKE certificates controller #41160

pipejakob · 2017-02-08T21:58:41Z

New stand-alone certificates controller for GKE. Rather than requiring the CA's private key on disk, this allows making external calls to GKE in order to sign cluster certificates.

Which issue this PR fixes: fixes #39761

Release note:

New GKE certificates controller.

CC @mikedanese @jcbsmpsn

k8s-reviewable · 2017-02-08T21:59:47Z

This change is

pipejakob · 2017-02-09T01:03:07Z

@k8s-bot cvm gce e2e test this
@k8s-bot cri node e2e test this
@k8s-bot kops aws e2e test this

mikedanese · 2017-02-09T07:06:44Z

/approve

pipejakob · 2017-02-09T18:35:02Z

Hey @lavalamp -- can you approve my hack/verify-flags/known-flags.txt change? Thanks!

mikedanese · 2017-02-09T19:23:41Z

@liggitt

liggitt · 2017-02-09T19:53:04Z

pkg/controller/certificates/webhook_signer_config.go

+	useDefault          = time.Duration(0) // missing/zero value indicates to use default
+)
+
+type webhookSignerConfig struct {


config files need versioning. rather than a new config file format, I'd suggest taking a kubeconfig file and a backoff flag set to a reasonable default

Sounds like a reasonable alternative. In my defense, I was following the example of the ImagePolicyWebhook, which uses an unversioned config file:

https://github.com/kubernetes/kubernetes/blob/master/plugin/pkg/admission/imagepolicy/admission.go#L179

Right, which now has tech debt to switch to versioned config :)

https://github.com/kubernetes/kubernetes/blob/master/plugin/pkg/admission/imagepolicy/admission.go#L217

D'oh. My biggest weakness: TODO comments mere lines away from where I stopped reading :-P.

I like the idea of removing the need for yet another config file anyway, especially since I doubt many users will care enough to override the retryBackoff. Will do.

liggitt · 2017-02-09T19:54:41Z

it seems weird to have a controller call a webhook... why wouldn't we make the existing signing controller individually runnable and you could run it from wherever you felt safe keeping the private signing key?

mikedanese · 2017-02-09T20:45:57Z

Reasons were discussed here #39761 (comment)

liggitt · 2017-02-09T21:03:15Z

#41160 (comment)

it seems weird to have a controller call a webhook

#39761 (comment)

If we wanted to make a controller that can call out to a webhook, that seems ok, if a little weird

heh, glad to know I agree with myself. I didn't expect webhook capabilities to be added to the controller-manager. I envisioned a standalone controller you could run in your network that could call out to a webhook.

mikedanese · 2017-02-10T18:58:31Z

FWIW we were planning on creating a webhook approver as well for 1.7. I agree that the controller-manager is bloated but breaking out the certificate controller seems orthogonal to this feature and should be tackled separately. Do you think these webhooks are generally useful features? What is your concerns with adding to the "bloat"? Do you think adding these features will make it significantly harder to break out the certificates controller later on?

liggitt · 2017-02-10T19:36:58Z

I think adding webhook call-outs from the controller manager crosses the line.

The point of the API is to allow coordination by external controllers. A signer that wants to closely hold their signing cert/key makes sense to me, but I'd expect that to be handled by the signer running a signing controller themselves, pointed at the kube API:

signing-controller --kubeconfig=... --signing-cert=... --signing-key=...

If there is no network visibility from the CA to the kube API, and/or the CA wants to expose an API for signing certs, a signing controller that reads from the kube API, calls out to the CA, and updates the kube API with the results could work as well:

signing-controller --kubeconfig=... --ca-kubeconfig=...

But if network visibility is a concern, it is equally likely the kubernetes installation would not have visibility to the CA, so lumping a glue controller (k8s <-> controller <-> ca) into the controller manager monolith seems like a bad idea.

smarterclayton · 2017-02-10T19:57:19Z

The whole design of kube APIs is predicated that multiple controllers can coexist by talking to fundamental API resources on a dumb server. I don't know what @bgrant0607's opinion is but I am very strongly against this sort of extension model (we have admission, or controllers, but not both together)

mikedanese · 2017-02-10T20:07:29Z

Ok, talked to liggit on slack. We are going to explore some other options. I think I misunderstood his comment on the original issue.

pipejakob · 2017-02-15T22:35:04Z

I'll update the PR description and release notes, but I've pushed a new commit that:

Adds a new command for running just the certificates controller stand-alone
- Copies the OWNERS file from kube-controller-manager to bootstrap the new command's ownership (suggestions welcome)
Removes the kube-controller-manager integration with the new webhook signer, instead only adding support to the stand-alone certificates-controller
Removes the custom configuration file for the webhook signer, instead taking only the kubeconfig and retry backoff via CLI options

Care to take another look, @liggitt?

mikedanese · 2017-02-15T23:15:57Z

cmd/certificates-controller/OWNERS

@@ -0,0 +1,67 @@
+approvers:


where'd this come from?

Copied verbatim from cmd/kube-controller-manager/OWNERS, since the code and responsibility has a lot in common with kube-controller-manager. I could also bootstrap it from pkg/controller/certificates/OWNERS instead, or omit it entirely and fall back on cmd/OWNERS. I'm definitely open to suggestions on the best route to bootstrap this.

With the pending rename, please just put you, me, and mike as owners/approvers of this directory. Other folks can send PRs if they want to be added to the list.

Sounds good.

pipejakob · 2017-02-24T01:51:17Z

I've proactively rebased again to make sure this wasn't broken by upstream commits.

I have more changes after this, but I've broken them apart into two follow-on PRs to keep from too much scope creep here (#42011 and #42030). Will you have time to take a look before the weekend, @liggitt? I'd love to beat the code freeze if I can. Thanks!

ixdy · 2017-02-24T01:52:35Z

I don't fully understand what this is doing, but I'm a little worried about adding something that appears to be GKE-specific to the kubernetes-server tarball. Is there a reason we need to sideload this instead of treating it more like an addon and pulling from a container registry?

liggitt · 2017-02-24T03:24:37Z

Will you have time to take a look before the weekend, @liggitt? I'd love to beat the code freeze if I can.

At a glance it looks like my comments were addressed. @mikedanese can have final lgtm on the controller.

I don't fully understand what this is doing, but I'm a little worried about adding something that appears to be GKE-specific to the kubernetes-server tarball.

The code is intended to be moved out of tree asap. I agree that the packaging is odd. Is there any way to just let GKE package this?

roberthbailey · 2017-02-24T04:28:24Z

I don't see any reason we can't package it separately as long as the code can live in the main repo for a short while longer.

pipejakob · 2017-02-24T16:19:38Z

@ixdy My original intent was to reuse this mechanism because it lets us thoroughly e2e test new changes (even against GKE clusters) in order to prevent regressions before they happen. But given these comments, I agree that the right thing to do is remove the packaging from OSS and have it live outside of this repo.

I'll update the PR again to only include the code and Makefile changes necessary to build the single static binary, and worry about building the image (and other cluster configuration behavior) elsewhere. This should also make my other two chained PRs superfluous.

pipejakob · 2017-02-24T17:15:03Z

Done. My updated commit is just the gke-certificates-controller code and changes necessary to build the binary and pass verification.

pipejakob · 2017-02-24T20:20:04Z

@k8s-bot non-cri e2e test this
@k8s-bot kops aws e2e test this

spxtr · 2017-02-24T21:51:41Z

/approve

ixdy · 2017-02-24T21:58:07Z

hack/lib/golang.sh

@@ -24,6 +24,7 @@ readonly KUBE_GOPATH="${KUBE_OUTPUT}/go"
 # If you update this list, please also update build/release-tars/BUILD.
 kube::golang::server_targets() {
  local targets=(
+    cmd/gke-certificates-controller


this still ends up in kubernetes-server.tar.gz. :( but at least there's not a docker tarfile, too.

Ah. Is it more appropriate to add the target to KUBE_ALL_TARGETS instead? I don't actually need it in the tar, I just want to make sure that it gets built by default when anyone runs make so we'll prevent anyone from breaking the code.

that would probably work! I'm not sure whether we want to make another grouping for this (I don't know what to call it) or just add to the list directly.

ixdy · 2017-02-24T21:59:23Z

hack/lib/golang.sh

@@ -24,6 +24,7 @@ readonly KUBE_GOPATH="${KUBE_OUTPUT}/go"
 # If you update this list, please also update build/release-tars/BUILD.


nobody reads my comment. :(

To be completely honest, I read the comment immediately before it that said:

# Note: if you are adding something here, you might need to add it to # kube::build::source_targets in build/common.sh as well.

and must have stopped reading because source_targets doesn't exist in build/common.sh (or anywhere else in the codebase, says grep -r). :-)

Given that we don't actually want this in the tar, and I just want to ensure that the binary gets built when users run make (or make bazel-build), I'll just leave this alone and move the target to KUBE_ALL_TARGETS.

that's only been gone for 4.5 months (#30787). :)

mind removing that part of the comment while you're here?

Done and done. I could create a new target group like KUBE_EXTRA_TARGETS just for this binary, but it seemed overkill for something that's meant to be temporary. Plus, the fact that this target sticks out like a sore thumb in KUBE_ALL_TARGETS is a good reminder that we should remove it.

This adds a new stand-alone certificates controller for use on GKE. It allows calling GKE to sign certificates instead of requiring the CA private key locally. It does not aim for 100% feature parity with kube-controller-manager yet, so for instance, leader election support is omitted.

ixdy · 2017-02-24T22:58:45Z

/lgtm

Thanks!

k8s-ci-robot · 2017-02-24T22:58:45Z

@ixdy: you can't LGTM a PR unless you are an assignee.

In response to this comment:

/lgtm

Thanks!

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.

k8s-github-robot · 2017-02-24T22:59:26Z

[APPROVALNOTIFIER] This PR is APPROVED

The following people have approved this PR: ixdy, mikedanese, pipejakob, spxtr

Needs approval from an approver in each of these OWNERS Files:

~~cmd/OWNERS~~ [mikedanese]
~~hack/OWNERS~~ [ixdy,spxtr]
~~hack/verify-flags/OWNERS~~ [ixdy,spxtr]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

pipejakob · 2017-02-24T23:23:56Z

@k8s-bot unit test this

pipejakob · 2017-02-25T00:37:24Z

@k8s-bot cvm gce e2e test this
@k8s-bot gci gce e2e test this

k8s-github-robot · 2017-02-27T09:29:58Z

Automatic merge from submit-queue (batch tested with PRs 42058, 41160, 42065, 42076, 39338)

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 8, 2017

k8s-github-robot assigned jessfraz Feb 8, 2017

pipejakob force-pushed the webhooksigner-pr branch from 3008d13 to e6a53da Compare February 8, 2017 22:38

mikedanese assigned mikedanese and unassigned jessfraz Feb 9, 2017

liggitt reviewed Feb 9, 2017

View reviewed changes

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 10, 2017

pipejakob force-pushed the webhooksigner-pr branch from e6a53da to 453c402 Compare February 15, 2017 22:27

k8s-github-robot removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API labels Feb 15, 2017

pipejakob changed the title ~~New webhook-based certificate signer.~~ New command for stand-alone certificates controller (with webhook support) Feb 15, 2017

mikedanese reviewed Feb 15, 2017

View reviewed changes

pipejakob force-pushed the webhooksigner-pr branch 2 times, most recently from 2f909d9 to b11d601 Compare February 16, 2017 00:37

pipejakob mentioned this pull request Feb 23, 2017

Build gke-certificates-controller image #42011

Closed

pipejakob force-pushed the webhooksigner-pr branch from 6b5a4a9 to 1430f63 Compare February 24, 2017 01:22

pipejakob mentioned this pull request Feb 24, 2017

Add GCE support to optionally start gke-certificates-controller #42030

Closed

pipejakob force-pushed the webhooksigner-pr branch from 1430f63 to 7f4eb53 Compare February 24, 2017 01:42

pipejakob force-pushed the webhooksigner-pr branch from 7f4eb53 to 38abf2f Compare February 24, 2017 17:11

pipejakob force-pushed the webhooksigner-pr branch from 38abf2f to 6fb784d Compare February 24, 2017 18:36

mikedanese added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 24, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 24, 2017

ixdy reviewed Feb 24, 2017

View reviewed changes

pipejakob force-pushed the webhooksigner-pr branch from 6fb784d to 0d7a6eb Compare February 24, 2017 22:35

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 24, 2017

mikedanese added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 24, 2017

k8s-github-robot merged commit 7d510f2 into kubernetes:master Feb 27, 2017

deads2k mentioned this pull request Feb 14, 2018

using cobra commands for gke-certificate-controller #59709

Closed

		@@ -24,6 +24,7 @@ readonly KUBE_GOPATH="${KUBE_OUTPUT}/go"
		# If you update this list, please also update build/release-tars/BUILD.

New command for stand-alone GKE certificates controller #41160

New command for stand-alone GKE certificates controller #41160

Conversation

pipejakob commented Feb 8, 2017 • edited

k8s-reviewable commented Feb 8, 2017

pipejakob commented Feb 9, 2017

mikedanese commented Feb 9, 2017

pipejakob commented Feb 9, 2017

mikedanese commented Feb 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt commented Feb 9, 2017

mikedanese commented Feb 9, 2017

liggitt commented Feb 9, 2017 • edited

mikedanese commented Feb 10, 2017

liggitt commented Feb 10, 2017

smarterclayton commented Feb 10, 2017 • edited

mikedanese commented Feb 10, 2017

pipejakob commented Feb 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pipejakob commented Feb 24, 2017

ixdy commented Feb 24, 2017

liggitt commented Feb 24, 2017

roberthbailey commented Feb 24, 2017

pipejakob commented Feb 24, 2017

pipejakob commented Feb 24, 2017

pipejakob commented Feb 24, 2017

spxtr commented Feb 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ixdy commented Feb 24, 2017

k8s-ci-robot commented Feb 24, 2017

k8s-github-robot commented Feb 24, 2017

pipejakob commented Feb 24, 2017

pipejakob commented Feb 25, 2017

k8s-github-robot commented Feb 27, 2017

pipejakob commented Feb 8, 2017 •

edited

liggitt commented Feb 9, 2017 •

edited

smarterclayton commented Feb 10, 2017 •

edited