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.

Sign up for GitHub

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

Jump to bottom

Fix multiple secrets with same certificate #213

Merged
merged 1 commit into from
Apr 13, 2018
Merged

Fix multiple secrets with same certificate #213

merged 1 commit into from
Apr 13, 2018

Conversation

nicksardo
Copy link
Contributor

@nicksardo nicksardo commented Apr 12, 2018 •
edited
Loading

Sometimes we hit timeouts in the e2e test: https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/logs/ci-ingress-gce-e2e/1719 due to a certificate never getting deleted by the controller.

...
I0412 10:38:57.917] Apr 12 10:38:57.917: INFO: No backend services found
I0412 10:38:59.195] Apr 12 10:38:59.194: INFO: Monitoring glbc's cleanup of gce resources:
I0412 10:38:59.195] k8s-ssl-350024a07559f009-83697e1b6f4cfc12--b4d5aa05f1df8c87 (ssl-certificate)
I0412 10:38:59.195] 
I0412 10:39:07.635] Apr 12 10:39:07.634: INFO: No backend services found
I0412 10:39:08.924] Apr 12 10:39:08.924: INFO: Monitoring glbc's cleanup of gce resources:
I0412 10:39:08.925] k8s-ssl-350024a07559f009-83697e1b6f4cfc12--b4d5aa05f1df8c87 (ssl-certificate)
... for a long time

glbc.log

I0412 10:34:39.828468       1 l7.go:611] validating https for e2e-tests-ingress-zqvkk-echomap--b4d5aa05f1df8c87
I0412 10:34:40.122296       1 l7.go:285] Populating ssl cert k8s-ssl-350024a07559f009-83697e1b6f4cfc12--b4d5aa05f1df8c87 for l7 e2e-tests-ingress-zqvkk-echomap--b4d5aa05f1df8c87
I0412 10:34:40.122337       1 l7.go:344] Creating new sslCertificate k8s-ssl-350024a07559f009-62bfca2bfa77e63f--b4d5aa05f1df8c87 for e2e-tests-ingress-zqvkk-echomap--b4d5aa05f1df8c87
I0412 10:34:41.970998       1 l7.go:344] Creating new sslCertificate k8s-ssl-350024a07559f009-62bfca2bfa77e63f--b4d5aa05f1df8c87 for e2e-tests-ingress-zqvkk-echomap--b4d5aa05f1df8c87
E0412 10:34:42.331197       1 l7.go:351] Failed to create new sslCertificate k8s-ssl-350024a07559f009-62bfca2bfa77e63f--b4d5aa05f1df8c87 for e2e-tests-ingress-zqvkk-echomap--b4d5aa05f1df8c87 - googleapi: Error 409: The resource 'projects/k8s-ingress-boskos-17/global/sslCertificates/k8s-ssl-350024a07559f009-62bfca2bfa77e63f--b4d5aa05f1df8c87' already exists, alreadyExists
E0412 10:34:42.331293       1 taskqueue.go:85] Requeuing "e2e-tests-ingress-zqvkk/echomap" due to error: Cert creation failures - k8s-ssl-350024a07559f009-62bfca2bfa77e63f--b4d5aa05f1df8c87 Error:googleapi: Error 409: The resource 'projects/k8s-ingress-boskos-17/global/sslCertificates/k8s-ssl-350024a07559f009-62bfca2bfa77e63f--b4d5aa05f1df8c87' already exists, alreadyExists (ingresses)

At which point, the error causes the controller to forget to cleanup the old certificate, thus an orphan.

I don't yet know why the TLS list has the same certificate multiple times. It may be the test jig causing this; however, the controller should handle this situation.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 12, 2018
@nicksardo nicksardo requested a review from rramkumar1 April 12, 2018 23:01
rramkumar1
rramkumar1 reviewed Apr 12, 2018
View reviewed changes
pkg/loadbalancers/loadbalancers_test.go Outdated
@@ -160,6 +160,31 @@ func TestCertUpdate(t *testing.T) {
verifyCertAndProxyLink(expectCerts, expectCerts, f, t)
}

// Test that multiple secrets
Copy link
Contributor

Choose a reason for hiding this comment

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

Finish the comment

pkg/loadbalancers/l7.go Outdated
gcpCertName := l.namer.SSLCertName(l.Name, tlsCert.CertHash)

if _, exists := newCertMap[gcpCertName]; exists {
glog.V(3).Infof("Secret %q had a certificate already created by another secret", tlsCert.Name)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we put the name of the other secret in the comment if possible?

@rramkumar1
Copy link
Contributor

/lgtm

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

Test fix at kubernetes/kubernetes#62502

@rramkumar1
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 13, 2018
@nicksardo nicksardo merged commit 293f19a into kubernetes:master Apr 13, 2018
@nicksardo nicksardo mentioned this pull request Apr 13, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rramkumar1 rramkumar1 rramkumar1 left review comments

Assignees

@rramkumar1 rramkumar1

Labels
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/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nicksardo @rramkumar1 @k8s-ci-robot