Integrate net-certmanager in Serving by skonto · Pull Request #15066 · knative/serving

skonto · 2024-03-29T11:13:40Z

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 · 2024-03-29T11:13:49Z

[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

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [skonto]

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

codecov · 2024-03-29T11:19:00Z

Codecov Report

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

Project coverage is 84.71%. Comparing base (c2d0af1) to head (049ac7b).
Report is 68 commits behind head on main.

❗ Current head 049ac7b differs from pull request most recent head 177f361. Consider uploading reports for the commit 177f361 to get more accurate results

Files	Patch %	Lines
cmd/controller/main.go	0.00%	39 Missing ⚠️
pkg/reconciler/certificate/certificate.go	85.71%	9 Missing and 10 partials ⚠️
.../certificate/resources/cert_manager_certificate.go	84.84%	15 Missing ⚠️
cmd/webhook/main.go	0.00%	12 Missing ⚠️
pkg/netcertmanager/testing/factory.go	80.00%	6 Missing and 2 partials ⚠️
pkg/netcertmanager/testing/listers.go	78.37%	8 Missing ⚠️
pkg/reconciler/certificate/config/cert_manager.go	73.33%	2 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #15066      +/-   ##
==========================================
+ Coverage   84.11%   84.71%   +0.59%     
==========================================
  Files         213      220       +7     
  Lines       16783    13546    -3237     
==========================================
- Hits        14117    11475    -2642     
+ Misses       2315     1699     -616     
- Partials      351      372      +21

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

skonto · 2024-03-29T12:46:52Z

reviewdog fails due to reviewdog/reviewdog#1696

skonto · 2024-03-29T12:59:17Z

        ingress:
        - kourier
-        # - kourier-tls
+        - kourier-tls


Reverted it so I can test. Tests seem to pass here consistently.

#15052 should also help.

skonto · 2024-04-01T07:33:25Z

@dprotaso @ReToCode pls review.

ReToCode · 2024-04-02T07:10:27Z

 		"reconciliation-timeout", reconciler.DefaultTimeout,
 		"The amount of time to give each reconciliation of a resource to complete before its context is canceled.")

-	sharedmain.MainWithContext(signals.NewContext(), "controller", ctors...)


Can we have a second method in sharedmain where we pass a function to do:

if shouldEnableNetCertManagerController(ctx, client) { injection.Default.RegisterInformer(challenge.WithInformer) injection.Default.RegisterInformer(v1certificate.WithInformer) injection.Default.RegisterInformer(certificaterequest.WithInformer) injection.Default.RegisterInformer(clusterissuer.WithInformer) injection.Default.RegisterInformer(issuer.WithInformer) ctors = append(ctors, netcertmanager.NewController) }

to avoid duplicating code here?

Or we just iterate on an array of informers. Moving to sharedmain should be the next iteration imho.

ReToCode · 2024-04-02T07:11:27Z

+		log.Fatalf("Failed to construct network config: %v", err)
+	}
+	return netCfg.ExternalDomainTLS || netCfg.SystemInternalTLSEnabled() || (netCfg.ClusterLocalDomainTLS == netcfg.EncryptionEnabled) ||
+		netCfg.NamespaceWildcardCertSelector != nil


can netCfg.NamespaceWildcardCertSelector != nil be used without netCfg.ExternalDomainTLS == true?

ReToCode · 2024-04-02T07:12:49Z

@@ -0,0 +1,68 @@
+# Copyright 2020 The Knative Authors


can we use a different file name? It is no longer net-xxx. Maybe just certmanager.yaml?

I am open to that but I don't have a good name for it. certmanager.yaml implies that we are configuring certmanager itself but what we do is configuration of our component. So not sure, maybe pick another? 🤔

I see. But the CM is named config-certmanager, so probably still would go with that. In the docs I said "the Knative cert-manager integration".

Let's use certmanager.yaml as the name here - that's the convention we use for other config maps

ReToCode · 2024-04-02T07:13:40Z

@@ -0,0 +1,272 @@
+/*


I assume all the files from net-certmanager were just copied over, right?

Yes should I update the date in the heard or something else?

No, just wanted to make sure, so people know not to really in-depth review them.

ReToCode · 2024-04-02T07:14:43Z

@@ -21,6 +21,8 @@ metadata:
    app.kubernetes.io/name: knative-serving


we now maybe can symlink the config file to the one in config - if our default values match.

skonto · 2024-04-24T09:59:23Z

Reviewdog PR was merged I will update our action stuff.

dprotaso · 2024-04-24T12:54:45Z

sometimes re-running actions doesn't pull latest action changes (eg. when you use matrix values) let me try reopening the PR

dprotaso · 2024-04-24T13:00:26Z

nope :/

ReToCode · 2024-04-25T09:24:48Z

I tried rerunning a single action, did not help either. @skonto mind pushing an empty commit?

ReToCode · 2024-04-25T13:55:39Z

@knative/productivity-writers any idea what is going wrong here (see Code Style errors)?

dprotaso · 2024-04-25T14:58:00Z

reviewdog is having issues grabbing the PR diff because it's so big. GitHub broke something :/

You can follow
reviewdog/reviewdog#1696

knative-prow · 2024-04-25T15:37:52Z

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

Test name	Commit	Details	Required	Rerun command
https_serving_main	`177f361`	link	false	`/test https`

Your PR dashboard.

Details

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.

dprotaso · 2024-04-25T18:26:19Z

/lgtm

/hold in case you want to wait for the reviewdog fix

dprotaso · 2024-04-25T19:32:41Z

Talked to @skonto we'll merge this now and fix issues in a follow up.

/hold cancel

dprotaso · 2024-04-25T19:33:45Z

/override "style / Golang / Lint"
/override "style / Golang / Do Not Submit"
/override "style / Golang / Boilerplate Check (go)"
/override "style / Golang / Boilerplate Check (sh)"
/override "style / suggester / shell"
/override "style / suggester / yaml"
/override "style / suggester / github_actions"

knative-prow · 2024-04-25T19:33:50Z

@dprotaso: Overrode contexts on behalf of dprotaso: style / Golang / Boilerplate Check (go), style / Golang / Boilerplate Check (sh), style / Golang / Do Not Submit, style / Golang / Lint, style / suggester / github_actions, style / suggester / shell, style / suggester / yaml

Details

In response to this:

/override "style / Golang / Lint"
/override "style / Golang / Do Not Submit"
/override "style / Golang / Boilerplate Check (go)"
/override "style / Golang / Boilerplate Check (sh)"
/override "style / suggester / shell"
/override "style / suggester / yaml"
/override "style / suggester / github_actions"

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.

* integrate net-certmanager in Serving * Revert "disable kourier-tls (knative#15053)" This reverts commit 8bda840. * fix imports * add netcert conformance tests * fix vendor * add vendor networking test files * some fixes + rebase * fixes * add crd check * sym link * fix vendor * move reconciler * fix style * empty * move to pkg/client (cherry picked from commit 6ccb82f)

* Integrate net-certmanager in Serving (knative#15066) * integrate net-certmanager in Serving * Revert "disable kourier-tls (knative#15053)" This reverts commit 8bda840. * fix imports * add netcert conformance tests * fix vendor * add vendor networking test files * some fixes + rebase * fixes * add crd check * sym link * fix vendor * move reconciler * fix style * empty * move to pkg/client (cherry picked from commit 6ccb82f) * Run `openshift/release/generate-release.sh release-v1.14` --------- Co-authored-by: Stavros Kontopoulos <st.kontopoulos@gmail.com>

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 Mar 29, 2024

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. labels Mar 29, 2024

knative-prow Bot requested review from ReToCode and izabelacg March 29, 2024 11:13

skonto removed request for ReToCode and izabelacg March 29, 2024 11:15

skonto force-pushed the integrate_netcertmanager branch from 4225455 to d922b5b Compare March 29, 2024 11:46

skonto commented Mar 29, 2024

View reviewed changes

knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 29, 2024

skonto mentioned this pull request Mar 29, 2024

[wip] Integrate net-certmanager in Serving #14955

Closed

skonto force-pushed the integrate_netcertmanager branch from 5490ae1 to b688951 Compare March 29, 2024 18:15

knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 29, 2024

skonto requested review from ReToCode and dprotaso March 29, 2024 18:57

skonto assigned ReToCode and dprotaso Mar 29, 2024

skonto changed the title ~~[wip] Integrate net-certmanager in Serving - cleaned up~~ Integrate net-certmanager in Serving - cleaned up Mar 29, 2024

knative-prow Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 29, 2024

skonto changed the title ~~Integrate net-certmanager in Serving - cleaned up~~ Integrate net-certmanager in Serving Mar 29, 2024

knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 1, 2024

ReToCode reviewed Apr 2, 2024

View reviewed changes

move reconciler

740f03a

skonto force-pushed the integrate_netcertmanager branch from 06cbb7a to 740f03a Compare April 24, 2024 07:40

knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 24, 2024

fix style

04805a8

dprotaso closed this Apr 24, 2024

dprotaso reopened this Apr 24, 2024

empty

049ac7b

move to pkg/client

177f361

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

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 25, 2024

knative-prow Bot merged commit 6ccb82f into knative:main Apr 25, 2024

This was referenced Apr 29, 2024

[RELEASE-1.14] Backport: Integrate net-certmanager in Serving openshift-knative/serving#695

Merged

Cleanup net-certmanager integration #15168

Merged

tiangolo mentioned this pull request Oct 29, 2024

Kourier with cert-manager and HTTP01 challenges knative-extensions/net-kourier#1304

Closed

KauzClay mentioned this pull request Feb 3, 2025

add back selfsigned issuer #15748

Closed

This was referenced Feb 27, 2026

🌱 CNCF mission generation 2026-02-27 kubestellar/console-kb#6

Closed

🌱 CNCF mission generation 2026-02-27 kubestellar/console-kb#11

Merged

		@@ -21,6 +21,8 @@ metadata:
		app.kubernetes.io/name: knative-serving

Conversation

skonto commented Mar 29, 2024

Proposed Changes

Uh oh!

knative-prow Bot commented Mar 29, 2024

Uh oh!

codecov Bot commented Mar 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

skonto commented Mar 29, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skonto commented Apr 1, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skonto Apr 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ReToCode Apr 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skonto commented Apr 24, 2024

Uh oh!

dprotaso commented Apr 24, 2024

Uh oh!

dprotaso commented Apr 24, 2024

Uh oh!

ReToCode commented Apr 25, 2024

Uh oh!

ReToCode commented Apr 25, 2024

Uh oh!

dprotaso commented Apr 25, 2024

Uh oh!

knative-prow Bot commented Apr 25, 2024

Uh oh!

dprotaso commented Apr 25, 2024

Uh oh!

dprotaso commented Apr 25, 2024

Uh oh!

dprotaso commented Apr 25, 2024

Uh oh!

knative-prow Bot commented Apr 25, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov Bot commented Mar 29, 2024 •

edited

Loading

skonto Apr 4, 2024 •

edited

Loading

ReToCode Apr 2, 2024 •

edited

Loading