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

Add option to control SSL chain completion #63845

Merged
merged 1 commit into from
Jun 22, 2018

Conversation

paulgear
Copy link
Contributor

What this PR does / why we need it:

This adds templated support to the kubernetes-worker juju charm for the --enable-ssl-chain-completion option on the ingress proxy. It defaults to false, to ensure that production sites are not reliant on OCSP or DNS in order to function.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Release note:

kubernetes-worker juju charm: Added support for setting the --enable-ssl-chain-completion option on the ingress proxy.  "action required": if your installation relies on supplying incomplete certificate chains and using OCSP to fill them in, you must set "ingress-ssl-chain-completion" to "true" in your juju configuration.

@k8s-ci-robot k8s-ci-robot added release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 15, 2018
@paulgear
Copy link
Contributor Author

/assign @marcoceppi

@marcoceppi
Copy link
Contributor

/ok-to-test

@k8s-ci-robot 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 May 15, 2018
@marcoceppi
Copy link
Contributor

Thanks for the contribution! This change seems straight forward, my only concern is kubernetes/ingress-nginx#1977 which points that this might not work in 0.10.1 (though auto in the charm defaults to 0.9.0). That's something in general we should take care of outside of this PR, unless setting to false fails for 0.9.0.

controller. Set this to true if you would like the ingress controller
to attempt auto-retrieval of intermediate certificates. The default
(false) is recommended for all production kubernetes installations, and
any environment which does not have outbound Internet access.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm gonna need some context for the default: false here. The nginx-ingress-controller docs say this defaults to true. What's our justification for deviating from that? Who is recommending false for all production Kubernetes installations?

Copy link

Choose a reason for hiding this comment

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

@Cynerva auto-retrieval of intermediate certificates is a fragile process. It can fail for many reasons, for example firewalls blocking traffic or CA website outages. In a secure production environment it requires egress firewall access rules and they might stop working if CAs move IPs.

At the same time we saw nginx-ingress controller pods crash when chain completion fails.

Because of that the Canonical IS decided that disabling completion is the safest setting for production environments. We may end up with incomplete chain but we prefer that to possible outage.

If however you think that charms should have same default as upstream it would still work for us. What we need is a way to disable this functionality. Making it disabled by default is, in our opinion, the best but we would be happy with "true" being default, we can always just set it to "true" in our environments.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @jacekn. I'm content to ship default: false - you guys know production concerns better than I do. I just wanted to make sure the decision was made deliberately and with careful thought, which it sounds like it was. 👍

@Cynerva
Copy link
Contributor

Cynerva commented May 16, 2018

/assign @Cynerva

@Cynerva
Copy link
Contributor

Cynerva commented May 17, 2018

This looks good. I'm just gonna test real quick to make sure it doesn't break on 0.9.0 (thanks @marcoceppi for bringing this up)

@Cynerva
Copy link
Contributor

Cynerva commented May 17, 2018

Dang. It does break with the default version we ship (0.9.0-beta.15):

$ kubectl get po
NAME                                               READY     STATUS             RESTARTS   AGE
default-http-backend-sl6k6                         1/1       Running            0          13m
nginx-ingress-kubernetes-worker-controller-c5msb   0/1       CrashLoopBackOff   15         13m
nginx-ingress-kubernetes-worker-controller-km2vx   0/1       CrashLoopBackOff   15         13m
nginx-ingress-kubernetes-worker-controller-nqm5z   0/1       CrashLoopBackOff   15         13m

$ kubectl logs nginx-ingress-kubernetes-worker-controller-c5msb
unknown flag: --enable-ssl-chain-completion

We will need to catch up before this can be merged. It looks like the first version that includes --enable-ssl-chain-completion is 0.9.0-beta.18.

@Cynerva
Copy link
Contributor

Cynerva commented Jun 8, 2018

FYI, we have a pull request open to bump the default nginx-ingress-controller version to 0.15.0: #64285

It is currently blocked by code freeze, which should end roughly around Jun 19. Once that lands, we'll follow up on this PR.

@Cynerva
Copy link
Contributor

Cynerva commented Jun 21, 2018

This is unblocked - code freeze is over, and the default nginx-ingress-controller version has been bumped to 0.15.0. I'm doing a quick test now.

@Cynerva
Copy link
Contributor

Cynerva commented Jun 21, 2018

/lgtm

This looks good now on top of the latest charm code. A bot should merge it within the next couple of days once it gets through the submit queue. Thanks for your patience 👍

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 21, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Cynerva, paulgear

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 21, 2018
@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 65301, 65291, 65307, 63845, 65313). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 5bde5a5 into kubernetes:master Jun 22, 2018
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. release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants