Add proxy client support #3165

tsandall · 2017-08-08T23:14:59Z

This PR adds support for the --proxy-client-cert-file and --proxy-client-key-file cmd line args that the apiserver accepts now.

/cc @chrislovecnm @blakebarnett

k8s-ci-robot · 2017-08-08T23:15:01Z

@tsandall: GitHub didn't allow me to request PR reviews from the following users: blakebarnett.

Note that only kubernetes members can review this PR, and authors cannot review their own PRs.

In response to this:

This PR adds support for the --proxy-client-cert-file and --proxy-client-key-file cmd line args that the apiserver accepts now.

/cc @chrislovecnm @blakebarnett

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.

k8s-ci-robot · 2017-08-08T23:15:07Z

Hi @tsandall. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

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.

blakebarnett · 2017-08-08T23:25:40Z

cmd/kops/update_cluster.go

@@ -65,6 +65,7 @@ type UpdateClusterOptions struct {
 	SSHPublicKey    string
 	MaxTaskDuration time.Duration
 	CreateKubecfg   bool
+	NodeUpSource    string


It looks like this isn't used, maybe accidentally included? Also would this override KOPS_BASE_URL?

It's used down below when ApplyClusterCmd is instantiated. I wasn't aware of KOPS_BASE_URL. Perhaps this is redundant. I can drop the commit from the PR if that's better.

I think it could be quite confusing if both were used, yeah.

I see now. You can set the same value using the NODEUP_URL env var. OK, will remove the 2nd commit then.

tsandall · 2017-08-09T00:46:10Z

@blakebarnett removed 2nd commit that was adding a new command line flag.

blakebarnett · 2017-08-09T03:49:19Z

👍 LGTM, still need someone with the holy powers of /lgtm to sign-off.

chrislovecnm · 2017-08-10T20:29:58Z

/ok-to-test

chrislovecnm · 2017-08-10T20:30:06Z

/assign

chrislovecnm

Thanks for the PR!

Few questions and tweaks. We need some documentation as well, please. Also either add validation for the values, or file an issue and push another PR. See https://github.com/kubernetes/kops/blob/master/pkg/apis/kops/validation/validation.go for validation example.

chrislovecnm · 2017-08-10T20:31:30Z

nodeup/pkg/model/secrets.go

@@ -116,6 +116,44 @@ func (b *SecretBuilder) Build(c *fi.ModelBuilderContext) error {
 		c.AddTask(t)
 	}

+	if b.Cluster.Spec.KubeAPIServer.ProxyClientKeyFile != nil {


Should this be b.Cluster.Spec.KubeAPIServer.ProxyClientCertFile?

Whoops. Yes, thanks for catching this. FWIW, the validation should catch if only one is specified.

chrislovecnm · 2017-08-10T20:32:04Z

nodeup/pkg/model/secrets.go

+	if b.Cluster.Spec.KubeAPIServer.ProxyClientKeyFile != nil {
+		cert, err := b.KeyStore.Cert("kube-proxy-client")
+		if err != nil {
+			return err


please add ftm.Error with an error message.

chrislovecnm · 2017-08-10T20:34:04Z

pkg/apis/kops/v1alpha1/componentconfig.go

@@ -228,6 +228,10 @@ type KubeAPIServerConfig struct {
 	OIDCClientID *string `json:"oidcClientID,omitempty" flag:"oidc-client-id"`
 	// If set, the OpenID server's certificate will be verified by one of the authorities in the oidc-ca-file
 	OIDCCAFile *string `json:"oidcCAFile,omitempty" flag:"oidc-ca-file"`
+	// The apiserver's client certificate used for outbound requests.
+	ProxyClientCertFile *string `json:"proxyClientCertFile,omitempty" flag:"proxy-client-cert-file"`


Are any of these secrets? Or are these just public certs and key?

tsandall · 2017-08-14T22:40:04Z

Updated error messages, added validation, and covered validation with unit test.

As far as documentation goes, these are basically passthrough to apiserver command line arguments. There doesn't appear to be good documentation from Kubernetes on them yet so I suspect that if someone comes searching for support in kops, the existing GoDoc comments will be sufficient.

Edit: Let me know if there's anything else I can add to make this land.

justinsb · 2017-08-17T12:43:06Z

nodeup/pkg/model/secrets.go

@@ -21,6 +21,8 @@ import (
 	"path/filepath"
 	"strings"

+	"github.com/pkg/errors"


I think this would be a new import, and although it's handy, I don't think we want to add another dependency. This does mean spelling out the error wrapping explicitly, I'm afraid.

justinsb · 2017-08-17T12:53:44Z

nodeup/pkg/model/secrets.go

@@ -116,6 +118,44 @@ func (b *SecretBuilder) Build(c *fi.ModelBuilderContext) error {
 		c.AddTask(t)
 	}

+	if b.Cluster.Spec.KubeAPIServer.ProxyClientCertFile != nil {
+		cert, err := b.KeyStore.Cert("kube-proxy-client")


I think we want to write the cert/key always if the kube-proxy-client cert/key exists. I think we should always create the cert/key if the k8s version >= 1.8 (or maybe >= 1.7). I don't think we should require the ProxyClientCertFile field to be set, in that we can also set it in nodeup/pkg/model/kubeapiserver.go (we set e.g. TLSCertFile there)

Helper function for k8s version in this bit of the code is IsKubernetesGTE

OK, this sounds like the right approach.

justinsb · 2017-08-17T12:54:53Z

pkg/model/pki.go

@@ -120,6 +120,16 @@ func (b *PKIModelBuilder) Build(c *fi.ModelBuilderContext) error {
 		c.AddTask(t)
 	}

+	if b.Cluster.Spec.KubeAPIServer.ProxyClientCertFile != nil && b.Cluster.Spec.KubeAPIServer.ProxyClientKeyFile != nil {


I actually think we should create this based on the kubernetes version i.e. for versions >= 1.8 (or maybe >= 1.7), we create the keys. Or we just always create the key, but only set it on >= 1.7. There is a VersionGTE helper which you can git grep for similar usage.

Same as above. Sounds good.

justinsb · 2017-08-17T13:02:07Z

Looks good in general. I think we should head towards https://github.com/kubernetes/kubernetes/pull/47094/files , which was the equivalent for GKE.

There's not typically a lot of value in letting users change the path here (particularly as kube-apiserver then runs in a container), so I'm wary of requiring the path to be set to be used as the signal for when we should generate the key. Instead I think it's reasonable to default to always generating and setting this keypair when we're using k8s >= 1.7 or 1.8. Does that make sense @tsandall ?

tsandall · 2017-08-18T01:24:56Z

@justinsb Thanks for the review. I'll update the PR to:

Always generate the proxy client cert/key
Set the cert/key flags when k8s >= 1.7

I might not be able to get to this until early next week.

k8s-github-robot · 2017-08-21T20:48:44Z

@tsandall PR needs rebase

tsandall · 2017-08-21T21:06:31Z

/cc @justinsb @chrislovecnm updated so that proxy client cert pair is generated automatically and command line args are set if Kube version >= 1.7.

justinsb · 2017-08-22T06:57:21Z

pkg/model/pki.go

@@ -133,6 +133,16 @@ func (b *PKIModelBuilder) Build(c *fi.ModelBuilderContext) error {

 	{
 		t := &fitasks.Keypair{
+			Name:      fi.String("kube-proxy-client"),


Actually, this is going to be a confusing name I've realized... When I look at it, I'm going to assume something to do with kube-proxy :-)

How about apiserver-proxy-client ?

justinsb · 2017-08-22T06:59:10Z

Looks great, except that looking at it now I've realized that the kube-proxy-client name is going to imply something to do with kube-proxy. Sorry to not have spotted it earlier, but would you mind changing it to something like apiserver-proxy-client ?

This enables external admission controller webhooks, api aggregation, and anything else that relies on the --proxy-client-cert-file/--proxy-client-key-file apiserver args.

tsandall · 2017-08-22T17:12:28Z

@justinsb amended to rename the secret to apiserver-proxy-client. There are no other outstanding changes AFAIK.

justinsb · 2017-08-22T22:01:39Z

Thanks @tsandall - sorry for not spotting it previously.

/lgtm

k8s-github-robot · 2017-08-22T22:01:46Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: justinsb, tsandall

The full list of commands accepted by this bot can be found here.

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

~~OWNERS~~ [justinsb]

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

justinsb · 2017-08-22T22:02:02Z

PS thank you for an awesome contribution @tsandall !

k8s-github-robot · 2017-08-22T22:41:29Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2017-08-22T23:13:06Z

Automatic merge from submit-queue

Automatic merge from submit-queue. Map horizontal-pod-autoscaler-use-rest-clients flag Maps `--horizontal-pod-autoscaler-use-rest-clients` flag which is required for [Horizontal Pod Autoscaling][1]. See Kubernetes code in [kubernetes/kubernetes/blob/v1.7.11/cmd/kube-controller-manager/app/autoscaling.go#L36-L39][2]. Seems this is the missing piece for fulfilment of HPA pre-requisites, which are: * ✅ Enable the [Aggregation Layer][4] via the following kube-apiserver flags * ✅ `--requestheader-client-ca-file=<path to aggregator CA cert>` (see #3679) * ✅ `--requestheader-allowed-names=aggregator` (see #3679) * ✅ `--requestheader-extra-headers-prefix=X-Remote-Extra-` (see #3679) * ✅ `--requestheader-group-headers=X-Remote-Group` (see #3679) * ✅ `--requestheader-username-headers=X-Remote-User` (see #3679) * ✅ `--proxy-client-cert-file=<path to aggregator proxy cert>` (see #3165) * ✅ `--proxy-client-key-file=<path to aggregator proxy key>` (see #3165) * ❓ [Horizontal Pod Scaling][3] ... set the appropriate flags for `kube-controller-manager`: * ❎ `--horizontal-pod-autoscaler-use-rest-clients` should be `true`. * ✅ `--kubeconfig <path-to-kubeconfig>` (already set) **Relevant Documentation:** * https://v1-7.docs.kubernetes.io/docs/tasks/run-application/horizontal-pod-autoscale/ * https://v1-7.docs.kubernetes.io/docs/tasks/access-kubernetes-api/configure-aggregation-layer/ **Relevant Issues & PRs:** * #3679 * #3152 * #2691 * #2652 * #3165 [1]: https://v1-7.docs.kubernetes.io/docs/tasks/run-application/horizontal-pod-autoscale/ [2]: https://github.com/kubernetes/kubernetes/blob/v1.7.11/cmd/kube-controller-manager/app/autoscaling.go#L36-L39 [3]: https://v1-7.docs.kubernetes.io/docs/tasks/run-application/horizontal-pod-autoscale/ [4]: https://v1-7.docs.kubernetes.io/docs/tasks/access-kubernetes-api/configure-aggregation-layer/

k8s-ci-robot requested a review from chrislovecnm August 8, 2017 23:15

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 8, 2017

blakebarnett reviewed Aug 8, 2017

View reviewed changes

tsandall force-pushed the add-proxy-client-support branch from 8c72227 to a8796af Compare August 8, 2017 23:39

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 10, 2017

k8s-ci-robot assigned chrislovecnm Aug 10, 2017

chrislovecnm suggested changes Aug 10, 2017

View reviewed changes

chrislovecnm mentioned this pull request Aug 11, 2017

Support Aggregation Layer #3152

Closed

tsandall force-pushed the add-proxy-client-support branch from a8796af to 0a7e6f0 Compare August 14, 2017 22:38

justinsb reviewed Aug 17, 2017

View reviewed changes

tsandall force-pushed the add-proxy-client-support branch from 0a7e6f0 to b431520 Compare August 21, 2017 20:48

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

tsandall force-pushed the add-proxy-client-support branch from b431520 to 9df4af2 Compare August 21, 2017 21:05

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

tsandall force-pushed the add-proxy-client-support branch from 9df4af2 to c708a4a Compare August 21, 2017 21:30

justinsb reviewed Aug 22, 2017

View reviewed changes

Add support for proxy client certs

7cf6e10

This enables external admission controller webhooks, api aggregation, and anything else that relies on the --proxy-client-cert-file/--proxy-client-key-file apiserver args.

tsandall force-pushed the add-proxy-client-support branch from c708a4a to 7cf6e10 Compare August 22, 2017 17:11

k8s-ci-robot assigned justinsb Aug 22, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 22, 2017

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

k8s-github-robot merged commit eb80658 into kubernetes:master Aug 22, 2017

itskingori mentioned this pull request Nov 27, 2017

Map horizontal-pod-autoscaler-use-rest-clients flag #3939

Merged

Add proxy client support #3165

Add proxy client support #3165

Conversation

tsandall commented Aug 8, 2017

k8s-ci-robot commented Aug 8, 2017

k8s-ci-robot commented Aug 8, 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

tsandall commented Aug 9, 2017

blakebarnett commented Aug 9, 2017

chrislovecnm commented Aug 10, 2017

chrislovecnm commented Aug 10, 2017

chrislovecnm left a 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

tsandall commented Aug 14, 2017 • edited Loading

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

tsandall Aug 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinsb commented Aug 17, 2017

tsandall commented Aug 18, 2017

k8s-github-robot commented Aug 21, 2017

tsandall commented Aug 21, 2017

Choose a reason for hiding this comment

justinsb commented Aug 22, 2017

tsandall commented Aug 22, 2017

justinsb commented Aug 22, 2017

k8s-github-robot commented Aug 22, 2017

justinsb commented Aug 22, 2017

k8s-github-robot commented Aug 22, 2017

k8s-github-robot commented Aug 22, 2017

tsandall commented Aug 14, 2017 •

edited

Loading

tsandall Aug 18, 2017 •

edited

Loading