Load certificate when they are updated when internal-encryption is enabled by nak3 · Pull Request #13854 · knative/serving

nak3 · 2023-04-06T13:50:07Z

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.

Release Note

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

codecov · 2023-04-06T13:59:52Z

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 · 2023-04-07T00:59:42Z

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

Could you please take a look?

davidhadas

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

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

davidhadas · 2023-04-10T15:35:14Z

/retest

ReToCode

/lgtm

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

knative-prow · 2023-04-11T11:12:35Z

[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

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [nak3]

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

nak3 · 2023-04-12T02:45:23Z

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

davidhadas · 2023-04-12T07:51:13Z

@nak3
Looks very good, thank you.
/lgtm

nak3 · 2023-04-12T13:57:25Z

/hold cancel

evankanderson

/approve

evankanderson · 2023-04-12T15:49:21Z

+	cr.certificate = &cert
+
+	pool := x509.NewCertPool()
+	block, _ := pem.Decode(secret.Data[certificates.CaCertName])


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

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.

knative-prow Bot added area/API API objects and controllers area/autoscale area/networking labels Apr 6, 2023

knative-prow Bot requested a review from KauzClay April 6, 2023 13:50

knative-prow Bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Apr 6, 2023

knative-prow Bot requested a review from skonto April 6, 2023 13:50

knative-prow Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 6, 2023

knative-prow Bot requested review from ReToCode, davidhadas, dprotaso and evankanderson April 7, 2023 00:59

nak3 mentioned this pull request Apr 7, 2023

Eliminate the need for activator restart when the certs update of internal encryption #13694

Closed

davidhadas suggested changes Apr 7, 2023

View reviewed changes

Comment thread cmd/activator/main.go Outdated

Comment thread cmd/activator/main.go Outdated

knative-prow Bot assigned davidhadas Apr 7, 2023

nak3 added 4 commits April 10, 2023 10:40

Merge if statement

7be797f

Use TLSConfig for cache

32d139c

Use LegacyFakeDnsName

04e9970

nak3 force-pushed the 13694 branch from a6fa270 to 04e9970 Compare April 10, 2023 01:40

davidhadas suggested changes Apr 10, 2023

View reviewed changes

Comment thread cmd/activator/main.go Outdated

Comment thread pkg/activator/certificate/cache.go Outdated

davidhadas reviewed Apr 10, 2023

View reviewed changes

Comment thread pkg/activator/certificate/cache.go Outdated

davidhadas reviewed Apr 10, 2023

View reviewed changes

Comment thread pkg/activator/certificate/cache.go Outdated

davidhadas reviewed Apr 10, 2023

View reviewed changes

Comment thread pkg/activator/certificate/cache.go Outdated

davidhadas reviewed Apr 10, 2023

View reviewed changes

Comment thread pkg/activator/certificate/cache.go Outdated

davidhadas reviewed Apr 10, 2023

View reviewed changes

Comment thread pkg/activator/certificate/cache.go Outdated

davidhadas reviewed Apr 10, 2023

View reviewed changes

Comment thread pkg/activator/certificate/cache.go

nak3 added 2 commits April 10, 2023 20:00

Clean up

14eadfe

Fix CI

21c8033

nak3 force-pushed the 13694 branch from ac0810b to cdab28d Compare April 10, 2023 13:48

Drop delete and lock

cd542d8

nak3 force-pushed the 13694 branch from cdab28d to cd542d8 Compare April 10, 2023 13:53

ReToCode reviewed Apr 11, 2023

View reviewed changes

Comment thread pkg/activator/certificate/cache.go

Comment thread pkg/activator/certificate/cache.go Outdated

nak3 added 2 commits April 11, 2023 14:52

Get secret beforehand

f7a2617

certificates to certificate

75590c8

nak3 force-pushed the 13694 branch from 3f73fef to 75590c8 Compare April 11, 2023 06:04

davidhadas reviewed Apr 11, 2023

View reviewed changes

Comment thread pkg/activator/certificate/cache.go Outdated

nak3 added 2 commits April 11, 2023 19:21

Fill in certificate before reconciler loop

727a752

Rename function

a5795f7

ReToCode approved these changes Apr 11, 2023

View reviewed changes

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 Bot assigned ReToCode Apr 11, 2023

knative-prow Bot added the lgtm Indicates that a PR is ready to be merged. label Apr 11, 2023

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 Bot merged commit 9af99e5 into knative:main Apr 12, 2023

evankanderson reviewed Apr 12, 2023

View reviewed changes

nak3 mentioned this pull request Apr 13, 2023

Decode all CAs in secret for activator #13877

Open

Conversation

nak3 commented Apr 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Apr 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

nak3 commented Apr 7, 2023

Uh oh!

davidhadas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

davidhadas commented Apr 10, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ReToCode left a comment

Choose a reason for hiding this comment

Uh oh!

knative-prow Bot commented Apr 11, 2023

Uh oh!

nak3 commented Apr 12, 2023

Uh oh!

davidhadas commented Apr 12, 2023

Uh oh!

nak3 commented Apr 12, 2023

Uh oh!

evankanderson left a comment

Choose a reason for hiding this comment

Uh oh!

evankanderson Apr 12, 2023

Choose a reason for hiding this comment

Uh oh!

nak3 Apr 13, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nak3 commented Apr 6, 2023 •

edited

Loading

codecov Bot commented Apr 6, 2023 •

edited

Loading