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.

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

[WIP] Use GetCertificate for certificate lazy loading #13695

Closed
wants to merge 1 commit into from

Conversation

nak3
Copy link
Contributor

@nak3 nak3 commented Feb 14, 2023

This patch uses GetCertificate to load certificate lazily. Activator loads the server certificates lazily by this patch.

Note, the CA cert, which is used by activator -> QP does not do the lazy loading so activator restart is still necessary.

Part of #13694

This patch uses `GetCertificate` to load certificate lazily.
Activator loads the server certificates lazily by this patch.

Note, the CA cert, which is used by `activator -> QP` does not do the
lazy loading so activator restart is still necessary.
@knative-prow
Copy link

knative-prow bot commented Feb 14, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nak3

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

@knative-prow knative-prow bot added the area/API API objects and controllers label Feb 14, 2023
@knative-prow knative-prow bot requested review from jsanin-vmw and skonto February 14, 2023 04:50
@knative-prow knative-prow bot added area/autoscale approved Indicates a PR has been approved by an approver from all required OWNERS files. area/networking size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 14, 2023
@nak3 nak3 changed the title Use GetCertificate for certificate lazy loading [WIP] Use GetCertificate for certificate lazy loading Feb 14, 2023
@knative-prow knative-prow bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 14, 2023
@codecov
Copy link

codecov bot commented Feb 14, 2023

Codecov Report

Base: 86.23% // Head: 86.20% // Decreases project coverage by -0.03% ⚠️

Coverage data is based on head (8c57c7a) compared to base (e2add5d).
Patch coverage: 0.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #13695      +/-   ##
==========================================
- Coverage   86.23%   86.20%   -0.03%     
==========================================
  Files         197      197              
  Lines       14774    14777       +3     
==========================================
- Hits        12740    12739       -1     
- Misses       1732     1736       +4     
  Partials      302      302              
Impacted Files Coverage Δ
cmd/activator/main.go 0.00% <0.00%> (ø)
pkg/autoscaler/statserver/server.go 77.37% <0.00%> (-0.73%) ⬇️

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 at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@@ -165,6 +165,7 @@ func main() {
// Enable TLS client when queue-proxy-ca is specified.
// At this moment activator with TLS does not disable HTTP.
// See also https://github.com/knative/serving/issues/12808.
// Also, the current activator must be restarted when updating the secret of CA.
Copy link
Contributor

@skonto skonto Feb 14, 2023

Choose a reason for hiding this comment

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

(Thinking out loud) It seems that a cert pool uses a map underneath would it be possible to override the existing rootCA pool when the secret is updated? What are the options here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the comment.
Please let me try to solve only server cert part in this PR. Even only server certs part, it is not easy as some CI is failing and need to figure out it.

@nak3
Copy link
Contributor Author

nak3 commented Mar 1, 2023

I will debug this on my local and hopefully re-open once figured out the probolem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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 do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants