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

Use cert-manager with Let's Encrypt to enable TLS for gcsweb #182

Merged
merged 4 commits into from
Feb 28, 2019

Conversation

ixdy
Copy link
Member

@ixdy ixdy commented Feb 7, 2019

The first commit is basically following the cert-manager setup guide and installing ClusterIssuers for the staging and prod Let's Encrypt endpoints.

The second commit then uses this setup to obtain a certificate for gcsweb and install it in the new Ingress.

I haven't deployed any of this to the real cluster, but I've tested a version of it on my own deployment and cluster: http://gcsweb.ixdyland.net/gcs/kubernetes-jenkins/

Fixes #38
(and paves the way to using this for spartakus and the k8s.io redirectors)

/assign @thockin @hh
cc @munnerz

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. wg/k8s-infra labels Feb 7, 2019
@k8s-ci-robot k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Feb 7, 2019
@ixdy
Copy link
Member Author

ixdy commented Feb 7, 2019

To bootstrap cert-manager components (per the cert-manager docs):

cd cert-manager/
kubectl create clusterrolebinding cluster-admin-binding --clusterrole=cluster-admin --user=$(gcloud config get-value core/account)
kubectl apply -f cert-manager.yaml --validate=false
kubectl apply -f letsencrypt-staging.yaml
kubectl apply -f letsencrypt-prod.yaml

Then create the Certificate and update the Ingress per normal:

cd gcsweb.k8s.io/
kubectl apply -f certificate.yaml
kubectl apply -f ingress.yaml

Subsequent migrations (spartakus, redirector, etc) only need the latter steps (creating a Certificate and updating the Ingress).

@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 7, 2019
@munnerz
Copy link
Member

munnerz commented Feb 8, 2019

Awesome to see, and this lgtm 😄

One thing to note - we've identified an issue with certificate rotation in the webhook component, which could leave the webhook solving with old certificates when renewal times comes along. More details here: cert-manager/cert-manager#1340

The certificate duration here is configured to be 1y for the serving cert, and 5y for the CA used. Just a heads up to upgrade to v0.7 once that is released some time in the next year, or otherwise {somehow remember} to restart your webhook pod in ~8mths time (we renew 4m before expiry).

If you want to keep an eye on the expiry state of your certificates, we also expose expiry time prometheus metrics. You might want to set up prometheus/alertmanager/stackdriver or the like, to automatically alert you if a certificate is nearing expiry 😄

@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Feb 8, 2019
Copy link
Member

@munnerz munnerz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This all looks good to me (doing a visual review, I've not actually pulled and deployed these manifests) barring the one comment about ordering when it comes to applying these changes 😄

@@ -0,0 +1,12 @@
To bootstrap (per the [cert-manager getting started
guide](https://cert-manager.readthedocs.io/en/latest/getting-started/install.html#installing-with-regular-manifests)):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: may want to pin latest to release-0.6?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


```
kubectl create clusterrolebinding cluster-admin-binding --clusterrole=cluster-admin --user=$(gcloud config get-value core/account)
kubectl apply -f cert-manager.yaml --validate=false
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: possibly add a note mentioning that --validate=false can go away with k8s 1.13+

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -7,6 +7,8 @@ metadata:
annotations:
kubernetes.io/ingress.global-static-ip-name: gcsweb-k8s-io
spec:
tls:
- secretName: gcsweb-k8s-io-tls
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Due to an issue with the way cert-manager obtains certificates from LE (we generate and store a private key before the corresponding certificate has been obtained from Let's Encrypt) (more info: cert-manager/cert-manager#1343) - if you add this field here when no Secret resource already exists, the ingress-gce ingress controller will fail to actually update the GCP API with the load balancer configuration.

Because of this, you must instead create the Certificate resource and obtain the certificate before updating the ingress resource to reference that secret.

We'll be building a workaround into v0.7 which is due in ~1.5wks (most likely, pre-generating a self-signed certificate which can be used in the interim whilst CM is obtaining a publicly trusted certificate).

What this means, is that in order to deploy this the first time, you'll need to apply these changes after creating all the other resources in this PR 😄

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I discovered this, but I appreciate the clarification. I added a note to our README as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've got a fix in progress that will be landing in v0.7 here: cert-manager/cert-manager#1392 😄

@hh
Copy link
Member

hh commented Feb 20, 2019

During our meeting today approval for this PR was requested.
/cc @munnerz @thockin

@spiffxp
Copy link
Member

spiffxp commented Feb 25, 2019

/approve
/hold
I don't have enough context to full review, @munnerz can you take a look?

For followup, I feel like we should have a cert-manager/OWNERS file so this doesn't have to hit root OWNERS

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Feb 25, 2019
@ixdy
Copy link
Member Author

ixdy commented Feb 25, 2019

good point re: OWNERS, though I'm not sure who should be in it. :)

@munnerz
Copy link
Member

munnerz commented Feb 25, 2019

Taking another look, this all looks 👍 to me (although sods law states ...)

I'll add my lgtm, but defer a /hold cancel to someone in a better position to manage the roll out of this change 😄

good point re: OWNERS, though I'm not sure who should be in it. :)

I'd be happy to put my name forward to be pinged for reviews etc. on this 😄

@munnerz
Copy link
Member

munnerz commented Feb 25, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 25, 2019
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 26, 2019
@ixdy
Copy link
Member Author

ixdy commented Feb 26, 2019

added OWNERS with me and @munnerz. any other volunteers? :)

@thockin
Copy link
Member

thockin commented Feb 27, 2019

I am OK with this, but defer actual review to you folks. I will approve but not remove he hold. At your leisure.

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: spiffxp, thockin

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

@ixdy
Copy link
Member Author

ixdy commented Feb 28, 2019

this needs one more /lgtm (for the OWNERS file).

@munnerz
Copy link
Member

munnerz commented Feb 28, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 28, 2019
@ixdy
Copy link
Member Author

ixdy commented Feb 28, 2019

/hold cancel
:shipit:

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 28, 2019
@k8s-ci-robot k8s-ci-robot merged commit e2ae285 into kubernetes:master Feb 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants