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

[WIP] Initial implementation of ACM certificate for API server ELB #5414

Merged
merged 7 commits into from Jul 19, 2018

Conversation

Projects
None yet
6 participants
@Raffo
Copy link
Contributor

Raffo commented Jul 6, 2018

This is a WIP implementation fo the support for ACM certificates for the API ELB. This PR allows to specify the ARN of the SSL certificate to use for the Kubernetes API server. It makes the certificate-authority-data) unneeded when specifying the certificate and simplify the content of the kubeconfig file (no more need to ship the certificate-authority-data to the user, super handy especially in combination with the AWS authentication).

I'm not fully sure of the consequences of this PR, so I am really looking forward for feedback in this regards. I'd be happy to make changes especially in the way the ARN of the certificate needs to be passed (currently it's just a CLI flag).

This PR fixes #834 which I promised many weeks ago, but finally took the time to tackle.

Also, if anyone can guide me on how to add tests for this, that'd be great!

@Raffo

This comment has been minimized.

Copy link
Contributor Author

Raffo commented Jul 8, 2018

It looks like one travis job is failing for Go 1.9 on osx, but I don't really understand what could be the difference with the other jobs 🤔 If someone has power to restart the build, that'd be great 💯

@sstarcher

This comment has been minimized.

Copy link
Contributor

sstarcher commented Jul 9, 2018

When trying to add this to an existing cluster I'm getting W0709 07:54:57.344052 32945 executor.go:118] error running task "LoadBalancer/api.cluster.com" (8m55s remaining to succeed): error creating LoadBalancerListeners: InvalidConfigurationRequest: Listeners can't talk to InstancePort 443 with secure and insecure protocols at the same time

Not sure if this is your change or something else on master.

@Raffo

This comment has been minimized.

Copy link
Contributor Author

Raffo commented Jul 9, 2018

Thanks for the comment! Well it looks like my change given the area of error, but I am not sure. I tested it on my machine and I didn't see this issue... I create the cluster with the following command:

kops create cluster --zones=${ZONES} --api-loadbalancer-type public --name ${CLUSTER_NAME} --api-ssl-certificate ${ARN} --yes

Can you post the command that you used?

@sstarcher

This comment has been minimized.

Copy link
Contributor

sstarcher commented Jul 9, 2018

Yes, this is attempting to take an existing cluster and add the sslCertificate to it. I was able to create a new cluster just fine. We certainly don't want people to only be able to do this on a new cluster.

@Raffo

This comment has been minimized.

Copy link
Contributor Author

Raffo commented Jul 9, 2018

You are right, the feature should support upgrading existing clusters. I will try to have a look to understand how it could work.

@Raffo

This comment has been minimized.

Copy link
Contributor Author

Raffo commented Jul 9, 2018

@sstarcher can you post the command anyways? I'd love to fully understand what you are trying to make sure I cover all the cases.

@sstarcher

This comment has been minimized.

Copy link
Contributor

sstarcher commented Jul 9, 2018

I'm editing the yaml to add sslCertificate and doing a update cluster

@Raffo

This comment has been minimized.

Copy link
Contributor Author

Raffo commented Jul 9, 2018

Ok, I managed to reproduce the issue. The commands I used are:

kops create cluster --zones=${ZONES} --api-loadbalancer-type public --name ${CLUSTER_NAME} --v=9 --yes
kops edit cluster ${CLUSTER_NAME}
kops update cluster ${CLUSTER_NAME} --yes

The last one fails with the following output:

W0709 17:48:16.416765   80224 executor.go:118] error running task "LoadBalancer/foo.com" (9m37s remaining to succeed): error creating LoadBalancerListeners: InvalidConfigurationRequest: Listeners can't talk to InstancePort 443 with secure and insecure protocols at the same time

I will dig into it in the next days to try to figure out how to fix it as I would need it as well.

@Raffo

This comment has been minimized.

Copy link
Contributor Author

Raffo commented Jul 13, 2018

@sstarcher To be able to add the SSL certificate, we have to first delete and then add the listener. I think this is a fine operation to do and it shouldn't be too much of a big deal. The change I implemented should be okay to satisfy your use case. TL;DR: please take another look 😄

@justinsb

This comment has been minimized.

Copy link
Member

justinsb commented Jul 15, 2018

This looks great - thanks @Raffo

I had a little debate about sslCertificate vs tlsCertificate, but sslCertificate seems to be more widely used :-)

/approve
/lgtm

@justinsb

This comment has been minimized.

Copy link
Member

justinsb commented Jul 16, 2018

/ok-to-test

@k8s-ci-robot k8s-ci-robot removed the lgtm label Jul 16, 2018

@Raffo

This comment has been minimized.

Copy link
Contributor Author

Raffo commented Jul 16, 2018

/retest

@@ -85,7 +85,8 @@ func BuildKubecfg(cluster *kops.Cluster, keyStore fi.Keystore, secretStore fi.Se

b.Context = clusterName

{
// add the CA Cert to the kubeconfig only if we didn't specify a SSL cert for the LB
if cluster.Spec.API.LoadBalancer.SSLCertificate == "" {

This comment has been minimized.

@justinsb

justinsb Jul 18, 2018

Member

Ah - this is giving us a panic in e2e... (e.g. https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/pr-logs/pull/kops/5414/pull-kops-e2e-kubernetes-aws/6969/ )

I think you need:

if cluster.Spec.API == nil || cluster.Spec.API.LoadBalancer == nil || cluster.SPec.API.LoadBalancer.SSLCertificate == "" {

This comment has been minimized.

@Raffo

Raffo Jul 18, 2018

Author Contributor

Yes, I've seen the test a couple of days ago and yes I think that should be enough to fix, haven't had the time to do it do even if it's super quick, I was pretty busy these days... but the good news is: I will probably get it done tomorrow 😄

@justinsb

This comment has been minimized.

Copy link
Member

justinsb commented Jul 19, 2018

Thanks for fixing @Raffo :-)

/approve
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Jul 19, 2018

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jul 19, 2018

[APPROVALNOTIFIER] This PR is APPROVED

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

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

@Raffo

This comment has been minimized.

Copy link
Contributor Author

Raffo commented Jul 19, 2018

/retest

@k8s-ci-robot k8s-ci-robot merged commit 54cbe49 into kubernetes:master Jul 19, 2018

11 checks passed

cla/linuxfoundation Raffo authorized
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
pull-kops-bazel-build Job succeeded.
Details
pull-kops-bazel-test Job succeeded.
Details
pull-kops-e2e-kubernetes-aws Job succeeded.
Details
pull-kops-verify-bazel Job succeeded.
Details
pull-kops-verify-boilerplate Job succeeded.
Details
pull-kops-verify-gofmt Job succeeded.
Details
pull-kops-verify-govet Job succeeded.
Details
pull-kops-verify-packages Job succeeded.
Details
tide In merge pool.
Details

fernandocarletti added a commit to fernandocarletti/kops that referenced this pull request Sep 16, 2018

Added documentation for Api server LB Certificate
This feature was added by the PR kubernetes#5414 and it is available at release 1.10.0 but there's no related documentation yet available.
@teozkr

This comment has been minimized.

Copy link

teozkr commented Oct 4, 2018

Looks like this doesn't work with the Terraform exporter, unless I'm doing something very wrong.

...
spec:
  api:
    loadBalancer:
      sslCertificate: arn:aws:acm:blah:blah:certificate/blah
      type: Public
  ...

Applying this generates a "dumb" TCP ELB. I'm migrating an existing cluster from directly exposing the API to using a loadBalancer.

I'm using kops 1.10.0.

@Raffo

This comment has been minimized.

Copy link
Contributor Author

Raffo commented Oct 19, 2018

I'm not familiar with the Terraform exporter part, probably need some additional coding?

@abhyuditjain

This comment has been minimized.

Copy link

abhyuditjain commented Jan 3, 2019

Hi,

I created a cluster like:

kops create cluster $NAME \
--api-loadbalancer-type public \
--topology private \
--encrypt-etcd-storage=true \
--api-ssl-certificate "CERTIFICATE ARN" \
--zones "ap-south-1b,ap-south-1a" \
--ssh-public-key ~/.ssh/id_rsa \
--networking calico \
--master-count 1 \
--master-size t2.medium \
--master-volume-size 30 \
--node-count 2 \
--node-size t2.small \
--authorization "RBAC" \
--kubernetes-version "1.13.1" \
--image ami-1780a878 \
--node-volume-size 30 \
--yes

And when I try to validate or do anything that communicates with the Master API, I get the following error:

Using cluster from kubectl context: cluster.k8s.local
​
Validating cluster cluster.k8s.local
​
​
unexpected error during validation: error listing nodes: Get https://cluster-k8s-local-<something>.<region>.elb.amazonaws.com/api/v1/nodes: x509: certificate is valid for *.domain.com, not cluster-k8s-local-<something>.<region>.elb.amazonaws.com

There is a mismatch between ELB address given by AWS and my own domain for which I issued the certificate, hence it cannot communicate with Master API.

Any workaround for this?

@Raffo

This comment has been minimized.

Copy link
Contributor Author

Raffo commented Jan 7, 2019

Hi @abhyuditjain , can you add another issue and quote me adding some details like the version of kops that you are running? It's better for tracking. Also, I think I'm running in a very similar setup from the one that you are specifying and I don't see such issue. I would recommend to output the cluster yaml such that we can see the generated settings like that.

@abhyuditjain

This comment has been minimized.

Copy link

abhyuditjain commented Jan 8, 2019

@Raffo Will do.

@abhyuditjain

This comment has been minimized.

Copy link

abhyuditjain commented Jan 8, 2019

@Raffo Mentioned you here: #6290

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.