Integrate net-certmanager in Serving #15066

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

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

.github/workflows/kind-e2e.yaml

@@ -89,7 +89,7 @@ jobs:

        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

cmd/controller/main.go

@@ -53,5 +71,55 @@ func main() {
 		"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

cmd/controller/main.go

+		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

config/core/configmaps/netcertmanager.yaml

@@ -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

pkg/netcertmanager/certificate.go

@@ -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

test/config/externaldomaintls/certmanager/caissuer/config-certmanager.yaml

@@ -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.

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

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 izabelacg and ReToCode March 29, 2024 11:13

skonto removed request for izabelacg and ReToCode 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 dprotaso and ReToCode 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

skonto added 4 commits April 24, 2024 10:39

add crd check

502c67c

sym link

3de96d8

fix vendor

a279ff7

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
66 of 74 checks passed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate net-certmanager in Serving #15066

Integrate net-certmanager in Serving #15066

skonto commented Mar 29, 2024

knative-prow bot commented Mar 29, 2024

codecov bot commented Mar 29, 2024 •

edited

Loading

skonto commented Mar 29, 2024

skonto Mar 29, 2024

ReToCode Apr 2, 2024

skonto commented Apr 1, 2024

ReToCode Apr 2, 2024

skonto Apr 4, 2024

ReToCode Apr 2, 2024

ReToCode Apr 2, 2024

skonto Apr 4, 2024 •

edited

Loading

ReToCode Apr 4, 2024

dprotaso Apr 4, 2024

skonto Apr 11, 2024

ReToCode Apr 2, 2024

skonto Apr 4, 2024

ReToCode Apr 4, 2024

ReToCode Apr 2, 2024 •

edited

Loading

skonto commented Apr 24, 2024

dprotaso commented Apr 24, 2024

dprotaso commented Apr 24, 2024

ReToCode commented Apr 25, 2024

ReToCode commented Apr 25, 2024

dprotaso commented Apr 25, 2024

knative-prow bot commented Apr 25, 2024

dprotaso commented Apr 25, 2024

dprotaso commented Apr 25, 2024

dprotaso commented Apr 25, 2024

knative-prow bot commented Apr 25, 2024

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

Integrate net-certmanager in Serving #15066

Integrate net-certmanager in Serving #15066

Conversation

skonto commented Mar 29, 2024

Proposed Changes

knative-prow bot commented Mar 29, 2024

codecov bot commented Mar 29, 2024 • edited Loading

Codecov Report

skonto commented Mar 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skonto commented Apr 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skonto Apr 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ReToCode Apr 2, 2024 • edited Loading

Choose a reason for hiding this comment

skonto commented Apr 24, 2024

dprotaso commented Apr 24, 2024

dprotaso commented Apr 24, 2024

ReToCode commented Apr 25, 2024

ReToCode commented Apr 25, 2024

dprotaso commented Apr 25, 2024

knative-prow bot commented Apr 25, 2024

dprotaso commented Apr 25, 2024

dprotaso commented Apr 25, 2024

dprotaso commented Apr 25, 2024

knative-prow bot commented Apr 25, 2024

codecov bot commented Mar 29, 2024 •

edited

Loading

skonto Apr 4, 2024 •

edited

Loading

ReToCode Apr 2, 2024 •

edited

Loading