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

Load certificate when they are updated when internal-encryption is enabled #13854

Merged
merged 11 commits into from
Apr 12, 2023
Merged

Load certificate when they are updated when internal-encryption is enabled #13854

merged 11 commits into from
Apr 12, 2023

Conversation

nak3
Copy link
Contributor

@nak3 nak3 commented Apr 6, 2023 •
edited

Currently we need to restart activator when certificates are updated. This patch fixes it by:

  • Using GetCertificate for server certs.
  • Using TLSConf as a pointer for updating custom CA.
  • Caching the cert for peformance.

Fix #13694

Release Note

With enabling internal-encryption, activator pods needed to be restarted when certificates are updated. The restart is not necessary anymore.

@knative-prow knative-prow bot added area/API API objects and controllers area/autoscale area/networking labels Apr 6, 2023
@knative-prow knative-prow bot requested a review from KauzClay April 6, 2023 13:50
@knative-prow knative-prow bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Apr 6, 2023
@knative-prow knative-prow bot requested a review from skonto April 6, 2023 13:50
@knative-prow knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 6, 2023
@codecov
Copy link

codecov bot commented Apr 6, 2023 •
edited

Codecov Report

Patch coverage: 36.50% and project coverage change: -0.05 ⚠️

Comparison is base (8f273e4) 86.27% compared to head (a5795f7) 86.23%.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #13854      +/-   ##
==========================================
- Coverage   86.27%   86.23%   -0.05%     
==========================================
  Files         198      199       +1     
  Lines       14669    14706      +37     
==========================================
+ Hits        12656    12682      +26     
- Misses       1715     1725      +10     
- Partials      298      299       +1
Impacted Files Coverage Δ
cmd/activator/main.go 0.00% <0.00%> (ø)
pkg/activator/certificate/cache.go 41.81% <41.81%> (ø)

... and 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@nak3
Copy link
Contributor Author

nak3 commented Apr 7, 2023

/cc @evankanderson @davidhadas @ReToCode @KauzClay @dprotaso

Could you please take a look?

@nak3 nak3 mentioned this pull request Apr 7, 2023
Closed
davidhadas
davidhadas requested changes Apr 7, 2023
View reviewed changes
Copy link
Contributor

@davidhadas davidhadas left a comment

Choose a reason for hiding this comment

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

On the client side,
We should find a way to avoid using InsecureSkipVerify: false and replace the standard lib verification.

cmd/activator/main.go Outdated Show resolved Hide resolved
cmd/activator/main.go Outdated Show resolved Hide resolved
@nak3
Load certificate when they are updated
7168736 
Currently we need to restart activator when certificates are updated.
This patch fixes it by:
* Using `GetCertificate` for server certs.
* Using `VerifyPeerCertificate` for custom CA verification.
* Caching the cert for peformance.

Fix knative#13694
@nak3
Merge if statement
7be797f
@nak3
Use TLSConfig for cache
32d139c
@nak3
Use LegacyFakeDnsName
04e9970
davidhadas
davidhadas requested changes Apr 10, 2023
View reviewed changes
cmd/activator/main.go Outdated Show resolved Hide resolved
pkg/activator/certificate/cache.go Outdated Show resolved Hide resolved
@davidhadas
Copy link
Contributor

/retest

ReToCode
ReToCode reviewed Apr 11, 2023
View reviewed changes
pkg/activator/certificate/cache.go Show resolved Hide resolved
pkg/activator/certificate/cache.go Outdated Show resolved Hide resolved
ReToCode
ReToCode approved these changes Apr 11, 2023
View reviewed changes
Copy link
Member

@ReToCode ReToCode left a comment

Choose a reason for hiding this comment

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

/lgtm

/hold for other reviewers to chime in. Feel free to unhold if you have all the reviews you'd like.

@knative-prow knative-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 11, 2023
@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Apr 11, 2023
@knative-prow
Copy link

knative-prow bot commented Apr 11, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nak3, ReToCode

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:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@nak3
Copy link
Contributor Author

nak3 commented Apr 12, 2023

Thank you for the reviews! I will unhold if there is no additional reviews in a few days.

@davidhadas
Copy link
Contributor

@nak3
Looks very good, thank you.
/lgtm

@nak3
Copy link
Contributor Author

nak3 commented Apr 12, 2023

/hold cancel

@knative-prow knative-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 12, 2023
@knative-prow knative-prow bot merged commit 9af99e5 into knative:main Apr 12, 2023
54 checks passed
evankanderson
evankanderson reviewed Apr 12, 2023
View reviewed changes
Copy link
Member

@evankanderson evankanderson left a comment

Choose a reason for hiding this comment

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

/approve

pkg/activator/certificate/cache.go
cr.certificate = &cert

pool := x509.NewCertPool()
block, _ := pem.Decode(secret.Data[certificates.CaCertName])
Copy link
Member

Choose a reason for hiding this comment

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

Should we decode all the CA certificates at this key, or only the first one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it is okay to assume for now but we should decode all the CA if we will support the BYO CA certificate in the future. I will create the tracker issue.

@nak3 nak3 mentioned this pull request Apr 13, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidhadas davidhadas davidhadas requested changes

@evankanderson evankanderson evankanderson left review comments

@ReToCode ReToCode ReToCode approved these changes

@KauzClay KauzClay Awaiting requested review from KauzClay

@skonto skonto Awaiting requested review from skonto

@dprotaso dprotaso Awaiting requested review from dprotaso

Assignees

@davidhadas davidhadas

@ReToCode ReToCode

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/API API objects and controllers area/autoscale area/networking lgtm Indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Eliminate the need for activator restart when the certs update of internal encryption
4 participants
@nak3 @davidhadas @evankanderson @ReToCode