Skip to content

Track default SSL cert if TLS.SecretName is empty #611

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

Closed
wants to merge 1 commit into from

Conversation

jcmoraisjr
Copy link
Contributor

This PR fixes secret not found warning and correctly track default SSL cert if Ingress' TLS.SecretName is empty.

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

This change is Reviewable

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.005%) to 46.206% when pulling 1d9047e on jcmoraisjr:jm-default-cert into 0f9f082 on kubernetes:master.

@aledbf
Copy link
Member

aledbf commented Apr 16, 2017

@jcmoraisjr why? By doing this you are always adding a secret to the tls section which is not correct.

@jcmoraisjr
Copy link
Contributor Author

This code add the secret to the secret tracker. It is iterating the spec.tls slice with host(s) and an optional secretName field. If the field is empty, the correct behaviour is to use the default secret.

@aledbf
Copy link
Member

aledbf commented Apr 19, 2017

@jcmoraisjr secretTracker contains information about valid secrets that exist in the cluster. If some secret is missing we cannot replace it with the default or self generated by the controller.

@aledbf aledbf closed this Apr 19, 2017
@jcmoraisjr
Copy link
Contributor Author

An empty tls[].secretName is a feature that means that the default or self generated cert should be used. A provided default cert has a secret whose name is stored in ic.cfg.DefaultSSLCertificate. This PR is just checking and using the secret of the default cert instead complain every 10s that a secret, built using an empty secretName, wasn't found.

I just updated this branch checking if the secret of the default cert was provided. If so and tls[].secretName is empty, the secret name of the default cert is used. If both are empty the tracking is ignored.

@jcmoraisjr
Copy link
Contributor Author

@aledbf I still need to know if this is understood as a misbehaviour. Perhaps you prefer implement your own approach instead of mine, I don't mind, I just need to pick a new and healthy commit from your repo without the need to fork it again.

@aledbf
Copy link
Member

aledbf commented Apr 21, 2017

I still need to know if this is understood as a misbehaviour.

secretTracker must contain only valid secrets, not the default and never overwrite secret information.
Besides that if the user did not specify a secretName or does not exist the controller should use the default server and ssl port.

@jcmoraisjr
Copy link
Contributor Author

Let me know what I didn't understand and sorry if I missed something.

What I want to see working is leave secretName optional and this will assign the default TLS cert for me. This is already working: if TLS[].secretName is empty, the default cert is being used on TLS connections.

What I want to fix is a misbehaviour when I try to use this approach. Note: everything is working but ingress is complaining every 10s that default/ is an invalid secret name - because of a concatenation with an empty string.

How I want to fix: if secretName is empty and ingress is started with a secret name that points to a valid TLS cert, this secret name is tracked instead - so I can hot-swap the default TLS cert. But if for some reason (if so please let me know why and I'll learn a bit more) a secret name used in --default-ssl-certificate command line argument cannot be used, I'll be happy to update my approach in order to see this fix being merged.

@aledbf
Copy link
Member

aledbf commented Apr 21, 2017

@jcmoraisjr right, now I understand what you are trying to do. Why not just check for empty string and continue? Like:

if tls.SecretName == "" {
        continue
}

@jcmoraisjr
Copy link
Contributor Author

Because if I track the secret of the default TLS cert I can update it without restarting the controller.

@jcmoraisjr
Copy link
Contributor Author

@aledbf Any news on this PR?

@aledbf
Copy link
Member

aledbf commented May 5, 2017

@jcmoraisjr here is my wip to remove the sync loop #690

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