implement cluster-local-domain-tls in serving #14610

ReToCode · 2023-11-10T10:07:52Z

Context: https://github.com/orgs/knative/projects/63/views/1 and https://github.com/knative/serving/blob/main/docs/encryption/knative-encryption.md

Fixes #14217

Changes

Drops the existing self-built reconcilers for certificates
Drops the creation of a secret for Queue-Proxy
Introduces reconciliation of KnativeCertificates when cluster-local-domain-tls is enabled
Creates a KnativeCertificate for Activator and one in each namespace where Queue-Proxy is
Adds new certificate type labels to KnativeCertificates
Added new tests and e2e tests for the functionality
Extracted some testing stuff to make it work on all existing tests as well. Kourier with enabled cluster-local-domain-tls will also run all existing tests with TLS.
Cherry-pick [Automated] Update net-istio nightly #14706

Depends on these being merged first:

Release Note

Serving now supports the experimental feature `cluster-local-domain-tls` and creates the necessary `KnativeCertificates`

/hold

ReToCode

Added some inline comments to help with reviews.

ReToCode · 2023-11-30T10:22:43Z

pkg/reconciler/route/domains/domains.go

@@ -63,6 +65,28 @@ func GetAllDomainsAndTags(ctx context.Context, r *v1.Route, names []string, visi
 	return domainTagMap, nil
 }

+// GetDomainsForVisibility return all domains for the specified visibility.
+func GetDomainsForVisibility(ctx context.Context, targetName string, r *v1.Route, visibility netv1alpha1.IngressVisibility) (sets.String, error) {


this is moved and renamed from ingress.go

I also added some tests for this function.

ReToCode · 2023-11-30T10:22:47Z

pkg/reconciler/route/resources/certificate.go

+// We use https://golang.org/pkg/hash/adler32/#Checksum to compute the digest which returns a uint32.
+// We represent the digest in unsigned integer format with maximum value of 4,294,967,295 which are 10 digits.
+// The "-[tag digest]" is computed only if there's a tag
+func certNameFromRouteAndTag(route *v1.Route, tag string) string {


extracted from existing function

ReToCode · 2023-11-30T10:23:27Z

pkg/reconciler/route/resources/ingress.go

@@ -205,27 +203,6 @@ func makeIngressSpec(
 	}, nil
 }

-func routeDomain(ctx context.Context, targetName string, r *servingv1.Route, visibility netv1alpha1.IngressVisibility) (sets.String, error) {


moved to domains.go, see above.

ReToCode · 2023-11-30T10:24:27Z

pkg/reconciler/route/route.go

 		}
 	}

 	return unusedCerts, nil
 }

+func (c *Reconciler) deleteOrphanedCerts(ctx context.Context, orphanCerts []*netv1alpha1.Certificate) {


extracted to be reused from both callees.

ReToCode · 2023-11-30T10:24:49Z

pkg/testing/v1/route.go

-// WithRouteConditionsTLSNotEnabledForClusterLocalMessage calls
-// MarkTLSNotEnabled with TLSNotEnabledForClusterLocalMessage after initialized
-// the Service's conditions.
-func WithRouteConditionsTLSNotEnabledForClusterLocalMessage(rt *v1.Route) {


we don't longer need this, as we support it now.

ReToCode · 2023-11-30T10:27:17Z

test/e2e/service_to_service_test.go

 	"knative.dev/serving/pkg/apis/autoscaling"
 	"knative.dev/serving/pkg/apis/serving"
 	rtesting "knative.dev/serving/pkg/testing/v1"
 	"knative.dev/serving/test"
 	v1test "knative.dev/serving/test/v1"
 )

-const (


moved to separate file, see above.

ReToCode · 2023-11-30T10:28:12Z

test/e2e/service_to_service_test.go

@@ -229,6 +101,13 @@ func TestServiceToServiceCall(t *testing.T) {
 	}
 	t.Logf("helloworld internal domain is %s.", resources.Route.Status.URL.Host)

+	// if cluster-local-domain-tls is enabled, this will return the CA used to sign the certificates.
+	// TestProxyToHelloworld will use this CA to verify the https connection
+	secret, err := GetCASecret(clients)


All our tests that call cluster-local domains need to trust if we enable TLS, this helper gets the CA secret and the following TestProxyToHelloworld will add this CA to it's trust pool and thus validate the TLS connection.

ReToCode · 2023-11-30T10:29:05Z

test/test_images/httpproxy/httpproxy.go

@@ -103,27 +103,20 @@ func newTLSEnabledTransport() http.RoundTripper {
 		}
 		transport.TLSClientConfig = &tls.Config{
 			RootCAs: rootCAs,
-			// If SERVER_NAME is not set the empty value will make the


httpproxy no longer needs to trust a defined SERVER_NAME. The certificates will be valid for the actual service. Also we get the CA directly in the env variable.

third_party/cert-manager-latest/net-certmanager.yaml

third_party/kourier-latest/kourier.yaml

dprotaso · 2024-01-16T14:46:39Z

I disabled istio-tls testing - unsure if you want to re-enable it again as part of this PR

see github action here

https://github.com/knative/serving/pull/14706/files#diff-8d196c4ce9b53dec57fab83c31e0f2a460d671683b19c3ef3d662003e1305d1cR122

and test here

https://github.com/knative/serving/pull/14706/files#diff-1fd4217168a94dda8aedaef1eda6a4ea09553ed4fec17afcf9a67523cce45571R47

dprotaso · 2024-01-16T14:57:06Z

pkg/reconciler/revision/controller.go

@@ -131,6 +136,7 @@ func newControllerWithOptions(
 	}
 	deploymentInformer.Informer().AddEventHandler(handleMatchingControllers)
 	paInformer.Informer().AddEventHandler(handleMatchingControllers)
+	certificateInformer.Informer().AddEventHandler(handleMatchingControllers)


Since the namespace is the owner of the certificate - this won't work.

You could re-sync all the revisions when the cert changes by listing and enqueing (then filtering) - or you can use a tracker https://pkg.go.dev/knative.dev/pkg/tracker

Ah I see, makes sense. Updated it to using a tracker, I added it after the initial creation of a Certificate was successful. If that would fail (e.g. on the first Revision), we'd have an error on the Revision reconciliation anyway.

ReToCode · 2024-01-18T08:11:51Z

I disabled istio-tls testing - unsure if you want to re-enable it again as part of this PR

Let's keep that as a follow-up thing, as we also have flakiness with ambient that we'd need to look into.

ReToCode · 2024-01-18T09:35:51Z

/test unit-tests

pkg/reconciler/revision/controller.go

pkg/reconciler/revision/reconcile_resources.go

pkg/reconciler/revision/controller.go

dprotaso · 2024-01-18T15:17:11Z

pkg/reconciler/revision/reconcile_resources.go

+	// Tell our trackers to reconcile when the KnativeCertificate changes
+	gvk := rev.GetGroupVersionKind()
+	apiVersion, kind := gvk.ToAPIVersionAndKind()
+	if err := c.tracker.TrackReference(tracker.Reference{


this is backwards

you want to track the cert and reconcile the revision so the invocation is

TrackReference(certificate, rev)

Uh that API is quite confusing, but it makes sense that it has to be this way. Thanks for explaining and bearing with me on this.

ReToCode · 2024-01-24T06:25:26Z

/hold

We'll bring this in after the release cut

ReToCode · 2024-01-24T11:49:50Z

/unhold

dprotaso · 2024-01-25T20:28:39Z

/lgtm
/approve

knative-prow · 2024-01-25T20:28:46Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso, ReToCode

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~~ [ReToCode,dprotaso]
~~pkg/apis/OWNERS~~ [dprotaso]

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

knative-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 10, 2023

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

knative-prow bot added area/API API objects and controllers size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. area/autoscale area/networking area/test-and-release It flags unit/e2e/conformance/perf test issues for product features labels Nov 10, 2023

knative-prow bot requested review from evankanderson, jsanin-vmw and kauana November 10, 2023 10:08

ReToCode force-pushed the encryption-certmanager-only branch from c96a9cb to 68c4e4c Compare November 10, 2023 10:11

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 Nov 10, 2023

ReToCode force-pushed the encryption-certmanager-only branch from d6939f2 to b297bec Compare November 14, 2023 09:14

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 Nov 14, 2023

ReToCode force-pushed the encryption-certmanager-only branch 3 times, most recently from 0eeae3d to 6198bf3 Compare November 22, 2023 13:43

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

ReToCode force-pushed the encryption-certmanager-only branch from 6198bf3 to a8d4cde Compare November 22, 2023 14:04

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

ReToCode force-pushed the encryption-certmanager-only branch from a8d4cde to 33bc391 Compare November 30, 2023 10:17

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

ReToCode commented Nov 30, 2023

View reviewed changes

ReToCode changed the title ~~[TEST-ONLY] testing cluster-local-domain-tls in Serving~~ [WIP] implement cluster-local-domain-tls in serving Nov 30, 2023

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 Nov 30, 2023

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

ReToCode force-pushed the encryption-certmanager-only branch from 4be9d8f to 8ad22bc Compare November 30, 2023 12:59

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

ReToCode added 2 commits January 16, 2024 08:00

Review improvements

a6c363f

Update net-istio

67cf527

ReToCode force-pushed the encryption-certmanager-only branch from bf85971 to 67cf527 Compare January 16, 2024 07:01

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

dprotaso reviewed Jan 16, 2024

View reviewed changes

dprotaso removed request for evankanderson, kauana and jsanin-vmw January 16, 2024 14:58

This was referenced Jan 18, 2024

Github actions e2e tests with istio-ambient are very flaky #14637

Open

[cluster-local-domain-tls] revisit reconciliation of Knative Certificates per namespace #14807

Open

ReToCode force-pushed the encryption-certmanager-only branch from ae3acea to 535f6fa Compare January 18, 2024 09:11

dprotaso reviewed Jan 18, 2024

View reviewed changes

pkg/reconciler/revision/controller.go Outdated Show resolved Hide resolved

pkg/reconciler/revision/reconcile_resources.go Outdated Show resolved Hide resolved

ReToCode force-pushed the encryption-certmanager-only branch from 535f6fa to 8a08a96 Compare January 18, 2024 15:08

dprotaso reviewed Jan 18, 2024

View reviewed changes

ReToCode force-pushed the encryption-certmanager-only branch from 8a08a96 to fb1065f Compare January 19, 2024 06:27

Use a revision tracker to reconcile on KCerts

e04287b

ReToCode force-pushed the encryption-certmanager-only branch from fb1065f to e04287b Compare January 19, 2024 06:29

knative-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 24, 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 Jan 24, 2024

knative-prow bot assigned dprotaso Jan 25, 2024

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

knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 25, 2024

knative-prow bot merged commit 0bae8a2 into knative:main Jan 25, 2024
49 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement cluster-local-domain-tls in serving #14610

implement cluster-local-domain-tls in serving #14610

ReToCode commented Nov 10, 2023 •

edited

ReToCode left a comment

ReToCode Nov 30, 2023

ReToCode Nov 30, 2023

ReToCode Nov 30, 2023

ReToCode Nov 30, 2023

ReToCode Nov 30, 2023

ReToCode Nov 30, 2023

ReToCode Nov 30, 2023

ReToCode Nov 30, 2023

dprotaso commented Jan 16, 2024

dprotaso Jan 16, 2024

ReToCode Jan 18, 2024

ReToCode commented Jan 18, 2024

ReToCode commented Jan 18, 2024

dprotaso Jan 18, 2024

ReToCode Jan 19, 2024

ReToCode commented Jan 24, 2024

ReToCode commented Jan 24, 2024

dprotaso commented Jan 25, 2024

knative-prow bot commented Jan 25, 2024

implement cluster-local-domain-tls in serving #14610

implement cluster-local-domain-tls in serving #14610

Conversation

ReToCode commented Nov 10, 2023 • edited

Changes

ReToCode left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dprotaso commented Jan 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ReToCode commented Jan 18, 2024

ReToCode commented Jan 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ReToCode commented Jan 24, 2024

ReToCode commented Jan 24, 2024

dprotaso commented Jan 25, 2024

knative-prow bot commented Jan 25, 2024

ReToCode commented Nov 10, 2023 •

edited