Skip to content

Use ca.NewCA() for generating certs and keys for the proxy injector#2163

Merged
alpeb merged 10 commits intomasterfrom
alpeb/webhook-reuse-ca
Jan 30, 2019
Merged

Use ca.NewCA() for generating certs and keys for the proxy injector#2163
alpeb merged 10 commits intomasterfrom
alpeb/webhook-reuse-ca

Conversation

@alpeb
Copy link
Member

@alpeb alpeb commented Jan 28, 2019 •
edited
Loading

Use ca.NewCA() for generating certs and keys for the proxy injector instead of grabbing the trust anchor from the linkerd-ca-bundle ConfigMap, as well as avoiding having to watch linkerd-proxy-injector-service-tls-linkerd-io secrets for the cert and key used for the injector's TLS server.

This is just a POC. If this approach is accepted, the next step is to remove all the code that was dealing with those secrets.

Fixes #2095

@alpeb alpeb force-pushed the alpeb/webhook-reuse-ca branch from 6ae8d62 to 0ed4083 Compare January 28, 2019 14:49
@alpeb alpeb mentioned this pull request Jan 28, 2019
Closed
@alpeb alpeb self-assigned this Jan 28, 2019
@olix0r
Copy link
Member

olix0r commented Jan 28, 2019 •
edited
Loading

Should the ca logic be put somewhere in pkg/ to indicate it's a library? Is the controller/ca package generic library code or code related to our current CA? I'd opt to depend on generic code, rather than code related to our CA, which will have to change substantially in an upcoming change.

This isn't a blocker, just an organizational note.

@alpeb
Copy link
Member Author

alpeb commented Jan 28, 2019 •
edited
Loading

@ver yea, ca.go is pretty generic so we can move it into /pkg. The more specific stuff dealing with the resources watching and secrets updating lives in /controller/ca/controller.go.

klingerf
klingerf reviewed Jan 28, 2019
View reviewed changes
Copy link
Contributor

@klingerf klingerf left a comment

Choose a reason for hiding this comment

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

This looks like the right approach to me. I'd love to see some refactoring here to allow the proxy-injector to use as much of the generic CA code as possible, and I called that out in my comments below.

@klingerf klingerf mentioned this pull request Jan 28, 2019
Closed
@alpeb
Copy link
Member Author

alpeb commented Jan 29, 2019

My last push is broken into 4 commits like so:

  • Moved ca.go into /pkg/tls
  • Merged the smaller tls.go with ca.go and addressed @klingerf's comments
  • Cleaned up and de-coupled the CA controller and the proxy-injector
  • Removed the need to use --tls during install to have the proxy injector work
  • Updated tests

Let me know what you think 🙏

klingerf
klingerf reviewed Jan 30, 2019
View reviewed changes
Copy link
Contributor

@klingerf klingerf left a comment

Choose a reason for hiding this comment

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

Awesome! I am really excited for this change. Just had a few more cleanup suggestions, and then it should be good to go.

image

😻

adleong
adleong reviewed Jan 30, 2019
View reviewed changes
klingerf
klingerf approved these changes Jan 30, 2019
View reviewed changes
Copy link
Contributor

@klingerf klingerf left a comment

Choose a reason for hiding this comment

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

⭐️ Looks great, thanks for updating!

alpeb added 10 commits January 30, 2019 15:13
@alpeb
Use ca.NewCA() for generating certs and keys for the proxy injector
ec8c8f5 
Fixes #2095

Signed-off-by: Alejandro Pedraza <[email protected]>
@alpeb
Move ca.go into /pkg/tls
3ab47ee 
Signed-off-by: Alejandro Pedraza <[email protected]>
@alpeb
Merge tls.go into ca.go and address reviews
da2593d 
Signed-off-by: Alejandro Pedraza <[email protected]>
@alpeb
Cleanup CA controller and proxy-injector
60cefe2 
- Remove from CA controller everything that dealt with the
webhook/proxy-injector
- Remove no longer needed proxy-injector volumes for 'trust-anchors' and
'webhook-secrets'
- Remove from the proxy-injector the retrieval of the trust anchor and
secrets

Signed-off-by: Alejandro Pedraza <[email protected]>
@alpeb
tls flag during install is no longer needed for auto-inject to work
5cb4e08 
Signed-off-by: Alejandro Pedraza <[email protected]>
@alpeb
Update tests
c723d36 
Signed-off-by: Alejandro Pedraza <[email protected]>
@alpeb
Remove no longer needed RBAC rule, simplify cert encoding functions
44fe2b6 
Signed-off-by: Alejandro Pedraza <[email protected]>
@alpeb
I had forgotten to update the test
a8529a5 
Signed-off-by: Alejandro Pedraza <[email protected]>
@alpeb
Remove unused consts and fix test failure reporting
13006bf 
Signed-off-by: Alejandro Pedraza <[email protected]>
@alpeb
Update install_no_init_container_auto_inject.golden after rebase
118764b 
Signed-off-by: Alejandro Pedraza <[email protected]>
@alpeb alpeb force-pushed the alpeb/webhook-reuse-ca branch from 16f6399 to 118764b Compare January 30, 2019 20:41
@alpeb alpeb merged commit fe234ca into master Jan 30, 2019
@alpeb alpeb deleted the alpeb/webhook-reuse-ca branch January 30, 2019 21:04
@klingerf klingerf mentioned this pull request Jan 30, 2019
Closed
@kleimkuhler kleimkuhler mentioned this pull request Feb 4, 2019
Merged
@klingerf klingerf mentioned this pull request Feb 11, 2019
Merged
This was referenced Mar 1, 2019
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@adleong adleong adleong left review comments

+1 more reviewer

@klingerf klingerf klingerf approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@alpeb alpeb

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@alpeb @olix0r @klingerf @adleong