Support secret-based and pre-shared certs at the same time #641

michallowicki · 2019-02-14T14:49:01Z

This should allow for a no-downtime migration from secret-based to pre-shared certs, as the secret-based cert will be still used until the secret is removed.

k8s-ci-robot · 2019-02-14T14:49:11Z

Hi @michallowicki. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

michallowicki · 2019-02-14T14:51:03Z

/assign @rramkumar1

pkg/loadbalancers/certificates.go

rramkumar1 · 2019-02-19T03:51:29Z

/ok-to-test

rramkumar1 · 2019-02-20T05:56:01Z

pkg/loadbalancers/loadbalancers_test.go

@@ -966,3 +966,180 @@ func TestList(t *testing.T) {
 		t.Fatalf("pool.List() returned %+v, want %+v", lbNames, expected)
 	}
 }
+
+// TestSecretBasedAndPreSharedCerts creates both pre-shared and
+// secret-based certs that should all be used.


"...secret-based certs and tests that all should be used"

rramkumar1 · 2019-02-20T05:59:08Z

pkg/loadbalancers/loadbalancers_test.go

+	if _, err := pool.Ensure(lbInfo); err != nil {
+		t.Fatalf("pool.Ensure() = err %v", err)
+	}
+	// Pre-shared cert is retained in the fake loadbalancer.


I don't follow this. If you remove the pre-shared cert then why would you expect it after Ensure()?

Removed this part of the test.
Pre-shared cert can be removed from target proxy but retained in the loadbalancer, verifyCertAndProxyLink() verifies that.

rramkumar1 · 2019-02-20T06:01:07Z

pkg/loadbalancers/loadbalancers_test.go

+	delete(expectCerts, certName1)
+	verifyCertAndProxyLink(expectCerts, expectCerts, f, t)
+
+	// Add the secret again.


What are you trying to test by adding the secret again? Seems unnecessary to me but I could be missing something.

Good point, removed this part of the test.
The idea was to also test reverting the update, but it's already tested by TestPreSharedToSecretBasedCertUpdate.

rramkumar1 · 2019-02-20T06:04:57Z

pkg/loadbalancers/certificates.go

@@ -30,23 +30,42 @@ import (
 const SslCertificateMissing = "SslCertificateMissing"

 func (l *L7) checkSSLCert() error {
+	// Use both pre-shared and secret-based certs if available,


So, I just remembered that we have a function in pkg/utils/utils.go to join errors. Can you use that instead?

Also the caller of this function can take care of wrapping the error with "Errors ensuring SSL certs"

prameshj · 2019-02-20T17:45:42Z

pkg/loadbalancers/loadbalancers_test.go

+	preSharedCert2, _ := f.CreateSslCertificate(&compute.SslCertificate{
+		Name:        "test-pre-shared-cert2",
+		Certificate: "xyz",
+		SelfLink:    "existing",


nit: Can you use a different name for SelfLink here?

prameshj · 2019-02-20T18:23:27Z

pkg/loadbalancers/loadbalancers_test.go

+		t.Fatalf("pool.Ensure() = err %v", err)
+	}
+	verifyCertAndProxyLink(expectCerts, expectCerts, f, t)
+}


Can we add a test for a case where 10 ingress secrets/certs are specified and lb is created. Then as an update, 10 pre-shared certs are specified. From the code, looks like we will prefer the pre-shared certs first correct? So all the 10 certs that were initially used will be replaced by the 10 new pre-shared certs that were specified. Correct me if i misunderstood this. I chose 10 because we use that as a fake limit in some other tests.

It would be good to have a test validating this so we can document the behavior.

Good point, added a test for this case:
10 ingress secrets/certs are specified and lb is created.
Then 5 pre-shared are created (reaching FakeCertLimit of 15).
Trying to use all 15 certs fails, and old setup with 10 secret-base certs is not overwritten. This is treated the same as trying to use 15 certs of the same type - if there are more certs than the TargetProxyCertLimit of 10, the update fails.
After removing 5 secret-based certs, the remaining 5 secret-based and 5 pre-shared certs can be used.

Thanks for verifying the behavior and adding the new tests!

Since the update fails due to targetProxy limit exceeding, the certs used by ingress will not be modified, I missed that in my initial comment.

prameshj · 2019-02-21T23:48:14Z

pkg/loadbalancers/loadbalancers_test.go

+	verifyCertAndProxyLink(expectCerts, expectCerts, f, t)
+}
+
+// TestMaxSecretBasedAndPreSharedCerts uploads 10 secret-based certs, which is the limit for the fake loadbalancer.


Can you reword this as "which is the limit for the fake target proxy"?

prameshj · 2019-02-21T23:48:43Z

pkg/loadbalancers/loadbalancers_test.go

+}
+
+// TestMaxSecretBasedAndPreSharedCerts uploads 10 secret-based certs, which is the limit for the fake loadbalancer.
+// Then creates 5 pre-shared certs, reaching the limit of 15 certs for the fake loadbalancer.


reword this as "limit of 15 certs which is the fake certs quota"

prameshj · 2019-02-21T23:50:22Z

pkg/loadbalancers/loadbalancers_test.go

+	// Create pre-shared certs up to FakeCertLimit.
+	preSharedCerts := []*compute.SslCertificate{}
+	tlsNames := []string{}
+	for ix := TargetProxyCertLimit; ix < FakeCertLimit; ix++ {


nit: This isn't your change, but if you wouldn't mind, can you rename "FakeCertLimit" to "FakeCertQuota"? We can do it in a follow up change too, if that's easier.

prameshj · 2019-02-21T23:53:41Z

pkg/loadbalancers/loadbalancers_test.go

+		t.Fatalf("pool.Ensure() = err %v", err)
+	}
+	verifyCertAndProxyLink(expectCerts, expectCerts, f, t)
+}


Thanks for verifying the behavior and adding the new tests!

Since the update fails due to targetProxy limit exceeding, the certs used by ingress will not be modified, I missed that in my initial comment.

rramkumar1 · 2019-02-22T16:05:23Z

/lgtm
/approve

k8s-ci-robot · 2019-02-22T16:05:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: michallowicki, rramkumar1

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:

~~OWNERS~~ [rramkumar1]

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 added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 14, 2019

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 14, 2019

k8s-ci-robot requested review from MrHohn and rramkumar1 February 14, 2019 14:49

k8s-ci-robot assigned rramkumar1 Feb 14, 2019

rramkumar1 reviewed Feb 15, 2019

View reviewed changes

pkg/loadbalancers/certificates.go Outdated Show resolved Hide resolved

pkg/loadbalancers/certificates.go Outdated Show resolved Hide resolved

michallowicki force-pushed the mcrt-no-downtime branch from 8de142c to a3c8a07 Compare February 15, 2019 12:05

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 19, 2019

rramkumar1 reviewed Feb 20, 2019

View reviewed changes

michallowicki force-pushed the mcrt-no-downtime branch from a3c8a07 to 25d4165 Compare February 20, 2019 13:20

prameshj reviewed Feb 20, 2019

View reviewed changes

michallowicki force-pushed the mcrt-no-downtime branch from 25d4165 to 7c50e1a Compare February 21, 2019 11:22

prameshj approved these changes Feb 21, 2019

View reviewed changes

Support secret-based and pre-shared certs at the same time

59b9fc1

michallowicki force-pushed the mcrt-no-downtime branch from 7c50e1a to 59b9fc1 Compare February 22, 2019 10:34

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 22, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 22, 2019

k8s-ci-robot merged commit ed01887 into kubernetes:master Feb 22, 2019

michallowicki mentioned this pull request Mar 14, 2019

Fix supporting secret-based and pre-shared certs at the same time #687

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support secret-based and pre-shared certs at the same time #641

Support secret-based and pre-shared certs at the same time #641

michallowicki commented Feb 14, 2019

k8s-ci-robot commented Feb 14, 2019

michallowicki commented Feb 14, 2019

rramkumar1 commented Feb 19, 2019

rramkumar1 Feb 20, 2019

michallowicki Feb 20, 2019

rramkumar1 Feb 20, 2019

michallowicki Feb 20, 2019

rramkumar1 Feb 20, 2019

michallowicki Feb 20, 2019

rramkumar1 Feb 20, 2019

michallowicki Feb 20, 2019

prameshj Feb 20, 2019

michallowicki Feb 21, 2019

prameshj Feb 20, 2019

michallowicki Feb 21, 2019

prameshj Feb 21, 2019

prameshj Feb 21, 2019

michallowicki Feb 22, 2019

prameshj Feb 21, 2019

michallowicki Feb 22, 2019

prameshj Feb 21, 2019

michallowicki Feb 22, 2019

prameshj Feb 21, 2019

rramkumar1 commented Feb 22, 2019

k8s-ci-robot commented Feb 22, 2019

Support secret-based and pre-shared certs at the same time #641

Support secret-based and pre-shared certs at the same time #641

Conversation

michallowicki commented Feb 14, 2019

k8s-ci-robot commented Feb 14, 2019

michallowicki commented Feb 14, 2019

rramkumar1 commented Feb 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rramkumar1 commented Feb 22, 2019

k8s-ci-robot commented Feb 22, 2019