Skip to content

Install MWC and VWC During Installation#2806

Merged
ihcsim merged 11 commits intomasterfrom
isim/install-webhooks-tls
May 20, 2019
Merged

Install MWC and VWC During Installation#2806
ihcsim merged 11 commits intomasterfrom
isim/install-webhooks-tls

Conversation

@ihcsim
Copy link
Contributor

@ihcsim ihcsim commented May 9, 2019

This PR supersedes #2796. Per #2796 (comment), this PR changes the way how the MWC and VWC are managed.

When installing the control plane,

  1. A self-signed cert and private key pair are created for each webhook
  2. These assets are stored in Secret resources, which are shared by the webhook replicas via read-only volume.
  3. The MWC and VWC resources spec are created as part of the installation process.
    1. In a multi-stage set-up, they are created during the config stage
  4. The webhooks no longer recreate the MWC and VWC when their pods are restarted, because the config resources are now upgraded via the linkerd upgrade command.

Fixes #2176

@ihcsim ihcsim mentioned this pull request May 9, 2019
Closed
@ihcsim ihcsim force-pushed the isim/install-webhooks-tls branch from f7711eb to 8d97f52 Compare May 9, 2019 23:20
@l5d-bot
Copy link
Collaborator

l5d-bot commented May 10, 2019

Integration test results for 8d97f52: success 🎉
Log output: https://gist.github.com/923fbcf611c091b0c2996b4a1dcee0aa

@ihcsim ihcsim requested review from alpeb and siggy May 10, 2019 04:27
@ihcsim ihcsim self-assigned this May 10, 2019
siggy
siggy approved these changes May 10, 2019
View reviewed changes
Copy link
Member

@siggy siggy left a comment

Choose a reason for hiding this comment

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

this change looks great, much simpler than #2796. left a couple tioli comments, but overall looks good (and works!) 🚢 👍

cli/cmd/install.go
}

profileValidatorValues struct {
*tlsValues
Copy link
Member

@siggy siggy May 10, 2019

Choose a reason for hiding this comment

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

curious: why the extra level of indirection? ProxyInjector and ProfileValidator are already distinguished as separate members of installValues.

Copy link
Contributor Author

@ihcsim ihcsim May 20, 2019

Choose a reason for hiding this comment

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

I decided to declare the tlsValues struct so that it can be used as the return type of the generateWebhookTLS() function, instead of returning two strings. Yeah, I thought of omitting the nested type here, but then decided w/e it's easy enough to change, if it doesn't work out.

alpeb
alpeb approved these changes May 10, 2019
View reviewed changes
Copy link
Member

@alpeb alpeb left a comment

Choose a reason for hiding this comment

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

This looked great to me!
Upgrade and HA tested fine 👍
Only thing I think is missing is the removal of the old template files for for the mutating and validating webhooks.

Ivan Sim added 9 commits May 20, 2019 09:11
Update helm charts to include webhooks config and TLS secret
fe2c432 
Signed-off-by: Ivan Sim <[email protected]>
Update the webhooks to read the secret cert and key
3e8b1e2 
Signed-off-by: Ivan Sim <[email protected]>
Update webhooks to not recreate config on restart
9fabdd5 
Signed-off-by: Ivan Sim <[email protected]>
Fix install unit tests
b546805 
Signed-off-by: Ivan Sim <[email protected]>
Ensure upgrade preserve existing secrets
9cbce43 
Signed-off-by: Ivan Sim <[email protected]>
Revert the change to rename the webhook configs
3c69372 
The renaming change breaks upgrade, where the new webhook configs conflict with
the existing ones. The older resources  aren't deleted during upgrade because
they are dynamically created.

Signed-off-by: Ivan Sim <[email protected]>
Make the secret volume read-only
a0796c0 
Signed-off-by: Ivan Sim <[email protected]>
Remove unnecessary exported getter functions
84fc9dd 
Signed-off-by: Ivan Sim <[email protected]>
Rebase with master
49da3c8 
Signed-off-by: Ivan Sim <[email protected]>
@ihcsim ihcsim force-pushed the isim/install-webhooks-tls branch from 8d97f52 to 49da3c8 Compare May 20, 2019 16:19
@l5d-bot
Copy link
Collaborator

l5d-bot commented May 20, 2019

Integration test results for 49da3c8: fail 😕
Log output: https://gist.github.com/f2e99ed28cda8ccd91f9064aa8ddfaf2

Siggy's feedback
4f963de 
Signed-off-by: Ivan Sim <[email protected]>
@l5d-bot
Copy link
Collaborator

l5d-bot commented May 20, 2019

Integration test results for 4f963de: fail 😕
Log output: https://gist.github.com/b0ac4d6f0c498af61a06fa599d146d81

@ihcsim
Copy link
Contributor Author

ihcsim commented May 20, 2019

The integration test passed on my machine:tm: twice, so I'm gonna go ahead and merge this PR :crossed_fingers:.

@ihcsim ihcsim merged commit 5a5f8bb into master May 20, 2019
@ihcsim ihcsim deleted the isim/install-webhooks-tls branch May 20, 2019 19:43
@annismckenzie annismckenzie mentioned this pull request May 28, 2019
Closed
This was referenced May 31, 2019
Closed
Closed
@alpeb alpeb mentioned this pull request Jun 4, 2019
Closed
@adleong adleong mentioned this pull request Jul 19, 2019
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@siggy siggy siggy approved these changes

@alpeb alpeb alpeb approved these changes

Assignees

@ihcsim ihcsim

Labels

area/controller

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add MutatingWebhookConfiguration objects to install YAML

4 participants

@ihcsim @l5d-bot @siggy @alpeb