Emit event on TLS errors

[MrHohn]

From kubernetes/kubernetes#58643, found that no event is emitted when glbc fails to retrieve TLS cert. It might be helpful to make this visible to users.

/assign @nicksardo

[nicksardo]

Fixes #41

[nicksardo]

I was actually holding off on this fix because of the PR #106 will allow us to just return an error. Then we can raise the event at a high level in a single location.
https://github.com/kubernetes/ingress-gce/pull/106/files#diff-3c862eb54a8e0e161b534e0c67e5379eR288

[MrHohn]

I was actually holding off on this fix because of the PR for syncing a single ingress will allow us to just return an error. Then we can raise the event at a high level in a single location.

Ah that makes sense, I didn't look close to that PR. Closing this.

[MrHohn]

I was actually holding off on this fix because of the PR for syncing a single ingress will allow us to just return an error. Then we can raise the event at a high level in a single location.

Will reopen and modify this to simply return error after #106.

[nicksardo]

/reopen

[MrHohn]

@nicksardo I just realize toRuntimeInfo() is generating L7RuntimeInfo for all ingresses. Returning error on a single ingress might halt the syncing for other ones.

[nicksardo]

The only consumer of toRuntimeInfo is getting passed a list of one ingress. Could you change the func to take a single ingress instead of a list of ingresses, please.

[MrHohn]

The only consumer of toRuntimeInfo is getting passed a list of one ingress. Could you change the func to take a single ingress instead of a list of ingresses, please.

Oops, I forgot to rebase hence looking at the old codes. Will do!

[MrHohn]

Updated commit, PTAL thanks!

[nicksardo]

/lgtm
Thanks!

[MrHohn]

Tested locally, didn't see error event being emitted. Seems like taskqueue only logs the error:

I0209 18:22:12.350297       1 controller.go:290] Finished syncing default/pre-shared-cert
E0209 18:22:12.350307       1 taskqueue.go:85] Requeuing "default/pre-shared-cert" due to error: cannot get certs for Ingress default/pre-shared-cert: secrets "no-exist" not found (ingresses)

[munnerz]

😬this change has broken both cert-manager and kube-lego with the GCLB: cert-manager/cert-manager#606

We currently rely on the fact that all current ingress controllers will still serve endpoints without TLS (or using a dummy, self signed certtificate) if the provided secretName does not exist.

I am not casting a judgement as to how controllers should behave (this is already a very well discussed topic!), but it's really unfortunate to see another divergence in how the various ingress controllers behave (for some context, this has made developing an effective multi-ingress controller solution for validating HTTP01 acme challenges a nightmare since kube-lego began, and even now with cert-manager).

Is there anything that can be done here? Ideally bringing the behaviour of both nginx and gce together, with minimal disruption to end-users.

/cc @aledbf

[munnerz]

I'd also argue that the change in behaviour (going from a permissive policy of handling the missing secret, to a hard fail) is a change that should be considered 'breaking'.

Would it be possible to alter this to instead only emit an event, and not actually fail/return error on the codepath?

[bowei]

Is it not possible for kube-lego/cert-manager to not configure TLS until after the secret has been provisioned (i.e. challenge succeeded)? That seems to match the intent more closely.

[munnerz]

Neither cert-manager nor kube-lego actually set this field. In order to determine where the secret should be stored for an ingress, we read the secretName field from the Ingress resource and once validation is complete, create a secret which is then picked up by the ingress controller. This means the 'auto tls' (ie annotate your ingress and get a valid cert) isn't possible without the old behaviour. For cert-manager, this is slightly better as a user can opt to manually create a Certificate resource to indicate where the resulting keypair should be signed, but the majority of users prefer this step to be automatic by just annotating their ingress. Whilst I understand the assumption on behaviour here may not be ideal, this is how controllers have operated for a very long time now (since their inception, as evidenced by kube-lego) and imo switching this from a 'soft' warning to a 'hard' fail is a regression/backwards incompatible change. Thanks very much for the quick response 😄

…

On Wed, 6 Jun 2018 at 01:13, Bowei Du ***@***.***> wrote: Is it not possible for kube-lego/cert-manager to not configure TLS until after the secret has been provisioned (i.e. challenge succeeded)? That seems to match the intent more closely. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#112 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAMbPx8FLe9tC1E0UuHHa4aNj3H00DP9ks5t5x6ygaJpZM4Ro4PS> .

[thebigredgeek]

Bump. Any updates here?

[prameshj]

Given that ingress now supports multiple tls secrets/certificates, if we went back to a 'soft' warning, would we need to handle multiple non-existent tls secrets down the line?

[RobinUS2]

We're having the same issue on GKE cluster at 1.10.4-gke.2 the generation of a self signed temporary the certificate above works but is far from ideal.

[munnerz]

I've opened #388 to attempt to address the issues described above.

[bowei]

Would be possible for the cert-manager to detect this and generate a self-signed cert to unblock creation of the Ingress and then do the update to final cert from let's encrypt. I think this is the behavior of ingress-nginx (self-signed cert)

It feels wrong to proceed with the LB configuration in a situation when the configuration is not valid.