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

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

Closed
nak3 opened this issue Feb 14, 2023 · 8 comments · Fixed by #13854
Closed

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

nak3 opened this issue Feb 14, 2023 · 8 comments · Fixed by #13854
Assignees
@nak3
Labels
kind/feature Well-understood/specified features, ready for coding. triage/accepted Issues which should be fixed (post-triage)
Milestone
Icebox

Comments

@nak3
Copy link
Contributor

nak3 commented Feb 14, 2023

Describe the feature

Currently activator must be restarted when certs are updated. The requirement should be eliminated.
@nak3 nak3 added the kind/feature Well-understood/specified features, ready for coding. label Feb 14, 2023
@nak3 nak3 mentioned this issue Feb 14, 2023
Closed
@ReToCode ReToCode mentioned this issue Feb 14, 2023
Open
6 tasks
@ReToCode ReToCode added triage/accepted Issues which should be fixed (post-triage) and removed triage/accepted Issues which should be fixed (post-triage) labels Feb 15, 2023
@ReToCode ReToCode mentioned this issue Feb 28, 2023
Open
@davidhadas
Copy link
Contributor

Fixes: 13754

@dprotaso
Copy link
Member

dprotaso commented Mar 1, 2023

/triage accepted
/milestone Icebox

#13754 highlights that if you don't restart the activator it will start serving 503s

@knative-prow knative-prow bot added the triage/accepted Issues which should be fixed (post-triage) label Mar 1, 2023
@dprotaso dprotaso added this to the Icebox milestone Mar 1, 2023
@nak3
Copy link
Contributor Author

nak3 commented Mar 2, 2023

Just a quick note for this:

  • ingress -> activator

It would be possible to support by using GetCertificate() like #13695.
However, it causes a performance degradation especially TestActivatorOverload test always fail.

  • activator -> QP

Golang does not have the feature to reload root certificate and some requests are alrady opened like golang/go#41888 golang/go#35887

@ReToCode
Copy link
Member

ReToCode commented Mar 2, 2023

However, it causes a performance degradation especially TestActivatorOverload test always fail.

I'm wondering if we could do something like this: https://github.com/ReToCode/serving/blob/803b9f9d232a8e202df6edabe276f512279697b3/pkg/activator/certificate/resolver.go to improve the performance issue?

@nak3
Copy link
Contributor Author

nak3 commented Mar 2, 2023

Great! Yes, CertResolver caches the certificates so it should improve the performance. I would like to see if it will pass the CI result.

@nak3 nak3 self-assigned this Mar 29, 2023
nak3 added a commit to nak3/serving that referenced this issue Apr 6, 2023
@nak3
Load certificate when they are updated
11b26fb 
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 nak3 mentioned this issue Apr 6, 2023
Merged
@davidhadas
Copy link
Contributor

davidhadas commented Apr 7, 2023 •
edited
Loading

Certificates may also expire and be replaced.
There needs to be a mechanism to update certificates.

@nak3
Copy link
Contributor Author

nak3 commented Apr 7, 2023 •
edited
Loading

Certificates may also expire and be replaced.
There needs to be a mechanism to update certificates.

That's already done by contro-protocol - https://github.com/knative-sandbox/control-protocol/blob/main/pkg/certificates/certs.go#L194-L200
So, the certificate in the secret can be updated automatically then #13854 will load the new cert.

@nak3
Copy link
Contributor Author

nak3 commented Apr 7, 2023

With said, I think there is a bug in the rotation - knative-extensions/control-protocol#272 😞

nak3 added a commit to nak3/serving that referenced this issue Apr 10, 2023
@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
knative-prow bot pushed a commit that referenced this issue Apr 12, 2023
@nak3
Load certificate when they are updated when internal-encryption is en…
9af99e5 
…abled (#13854)

* Load certificate when they are updated

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 #13694

* Merge if statement

* Use TLSConfig for cache

* Use LegacyFakeDnsName

* Clean up

* Fix CI

* Drop delete and lock

* Get secret beforehand

* certificates to certificate

* Fill in certificate before reconciler loop

* Rename function
@ReToCode ReToCode mentioned this issue Jul 24, 2023
Closed
@KauzClay KauzClay mentioned this issue Aug 14, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@nak3 nak3

Labels
kind/feature Well-understood/specified features, ready for coding. triage/accepted Issues which should be fixed (post-triage)
Projects
Serving Encryption
Status: Done
Milestone
Icebox
Development

Successfully merging a pull request may close this issue.

Load certificate when they are updated when internal-encryption is enabled nak3/serving
4 participants
@dprotaso @nak3 @davidhadas @ReToCode