Support secret-based and pre-shared certs at the same time by michallowicki · Pull Request #641 · kubernetes/ingress-gce

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.

Details

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

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

/ok-to-test

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

Details

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

Comment thread pkg/loadbalancers/certificates.go Outdated

Comment thread pkg/loadbalancers/certificates.go Outdated

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

Conversation

michallowicki commented Feb 14, 2019

Uh oh!

k8s-ci-robot commented Feb 14, 2019

Uh oh!

michallowicki commented Feb 14, 2019

Uh oh!

Uh oh!

Uh oh!

rramkumar1 commented Feb 19, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rramkumar1 commented Feb 22, 2019

Uh oh!

k8s-ci-robot commented Feb 22, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants