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

sync the secret if the certificate isn't present #1201

Closed
wants to merge 1 commit into from

Conversation

ibawt
Copy link
Contributor

@ibawt ibawt commented Aug 21, 2017

Problem

When the controller is being started, there's a period where various ingress resources will have their TLS certificate not present. This is shown by showing the "default certificate", for awhile until the controller syncs that secret.

If you have many ingresses ( think like >200 ), it can take minutes for the entire thing to converge. You can see the error message in the logs:
"ssl certificate \"some-cert\" does not exist in local store"

Reproduce

  1. nginx ingress controller with 1 replica
  2. create many ingresses and many updates, the idea is to get the the syncQueue as full as possible.
  3. Wait for steady state
  4. do while true ; do curl https://someplace.with.valid.cer.com/ -m 1s ; done
  5. delete the ingress controller
  6. observe many invalid cert errors

Fix

Since the queue is rate limited, and the only time that certificate syncing is done when the specific ingress of the event is processed. This leaves a big window of invalid certs as NGINX needs to know the entire state of the system (and indeed it makes sites for them, just no certificates).

When the cert isn't found, I added a syncSecret call so that they can be loaded. The result is how I would expect now, instead of invalid cert errors we get timeouts until the site is loaded. This is much more preferable than serving the invalid cert. And it also converges in about 20s instead of minutes.

Other ways to fix

I believe this is this way because for the GCE controller, one ingress == one load balancer, but NGINX is many ingress -> one load balancer instance.

  1. We could sync all the certificates on start.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 21, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 44.364% when pulling a23073d on ibawt:fix-cert-loading-on-load into e0b3fcb on kubernetes:master.

@aledbf
Copy link
Member

aledbf commented Aug 21, 2017

@ibawt I understand your issue but I prefer to add a delay in the start of the controller that doing this change.
We need to embrace the asynchronous behavior of the controller

@aledbf
Copy link
Member

aledbf commented Aug 21, 2017

@ibawt 1205 syncs all the certificates on start.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants