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

[wip] Integrate net-certmanager in Serving #14955

Closed
wants to merge 24 commits into from
Closed

[wip] Integrate net-certmanager in Serving #14955

wants to merge 24 commits into from

Conversation

skonto
Copy link
Contributor

@skonto skonto commented Feb 27, 2024 •
edited
Loading

Fixes #14740

Proposed Changes

  • Moves net-certmanager into Serving under pkg/net-certmanager. This brings certmanager deps for creating certificates.
  • Migration path for users should be straightforward just remove the net-certmanager deployment before doing an upgrade (should not create downtime).
  • Enables the netcertmanager controller and the related informers only when it is required.

Release Note

The net-certmanager controller is now part of the Serving core and specifically of the Serving controller.
To upgrade from an existing deployment you need to delete the net-certmanager deployment first.

@knative-prow knative-prow bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Feb 27, 2024
@knative-prow Knative Prow
Copy link

knative-prow bot commented Feb 27, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: skonto

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 approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. area/API API objects and controllers area/networking area/test-and-release It flags unit/e2e/conformance/perf test issues for product features labels Feb 27, 2024
skonto
skonto commented Feb 27, 2024
View reviewed changes
vendor/knative.dev/pkg/injection/injection.go Outdated
@@ -59,10 +59,36 @@ func EnableInjectionOrDie(ctx context.Context, cfg *rest.Config) (context.Contex

ctx, informers := Default.SetupInformers(ctx, cfg)

var finalInformers []controller.Informer

for _, inf := range informers {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is minimal change I can think of to skip informers.

@skonto skonto changed the title [wip] [TEST- ONLY] Test integrate nc [wip] [TEST- ONLY] Test integrating netcertmanager with optional controller start Feb 27, 2024
@codecov Codecov
Copy link

codecov bot commented Feb 28, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 77.87934% with 121 lines in your changes are missing coverage. Please review.

Project coverage is 83.95%. Comparing base (74622fb) to head (df32c33).

Files Patch % Lines
cmd/controller/main.go 0.00% 43 Missing ⚠️
...-certmanager/reconciler/certificate/certificate.go 85.54% 14 Missing and 10 partials ⚠️
.../certificate/resources/cert_manager_certificate.go 84.42% 19 Missing ⚠️
cmd/webhook/main.go 0.00% 12 Missing ⚠️
pkg/net-certmanager/reconciler/testing/factory.go 83.92% 7 Missing and 2 partials ⚠️
pkg/net-certmanager/reconciler/testing/listers.go 81.39% 8 Missing ⚠️
...ager/reconciler/certificate/config/cert_manager.go 71.42% 4 Missing and 2 partials ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #14955      +/-   ##
==========================================
- Coverage   84.03%   83.95%   -0.08%     
==========================================
  Files         213      220       +7     
  Lines       16783    17318     +535     
==========================================
+ Hits        14103    14539     +436     
- Misses       2324     2413      +89     
- Partials      356      366      +10

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@skonto skonto mentioned this pull request Feb 28, 2024
Closed
@knative-prow-robot knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 28, 2024
@knative-prow-robot knative-prow-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Mar 7, 2024
@knative-prow-robot knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 14, 2024
@skonto skonto mentioned this pull request Mar 14, 2024
Merged
@knative-prow-robot knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 15, 2024
@knative-prow-robot knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 21, 2024
@skonto
Copy link
Contributor Author

skonto commented Mar 21, 2024 

 Returned an error err=secret serving-tests/serving-certs is not ready yet: secret could not be found

@skonto
Copy link
Contributor Author

skonto commented Mar 21, 2024

/test istio-latest-mesh-tls

@skonto skonto changed the title [wip] Integrate net-certmanager in Serving with optionally starting the controller [wip] Integrate net-certmanager in Serving Mar 27, 2024
@skonto
Copy link
Contributor Author

skonto commented Mar 28, 2024

/test https

@knative-prow Knative Prow
Copy link

knative-prow bot commented Mar 29, 2024

@skonto: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
istio-latest-no-mesh_serving_main df32c33 link true /test istio-latest-no-mesh
https_serving_main df32c33 link false /test https

Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@skonto
Copy link
Contributor Author

skonto commented Mar 29, 2024

closing in favor of #15066

@skonto skonto closed this Mar 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dprotaso dprotaso Awaiting requested review from dprotaso

@ReToCode ReToCode Awaiting requested review from ReToCode

Assignees

@dprotaso dprotaso

@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/networking area/test-and-release It flags unit/e2e/conformance/perf test issues for product features do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Discussion: do we want to keep net-certmanager or can we integrate in Serving
4 participants
@skonto @dprotaso @ReToCode @knative-prow-robot