Host cluster-local-domain TLS on local listener with SNI #1156

ReToCode · 2023-11-13T07:12:15Z

Changes

Add cluster-local https listener when cluster-local-domain-tls is enabled and a KSVC exists
Uses SNI to host multiple certificates for all cluster-local domain Knative Services
Rename internal -> local to be consistent with the new encryption flags

/hold requires knative/networking#891 to be merged first.

/kind enhancement

Fixes knative/serving#14218
Partially knative/serving#14624

Release Note

net-kourier now hosts a TLS listener using SNI when `cluster-local-domain-tls` is enabled. Note: this is an experimental alpha-feature.

Docs

Will be done once the features is complete

knative-prow · 2023-11-13T07:12:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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]

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

codecov · 2023-11-13T07:14:44Z

Codecov Report

Attention: 15 lines in your changes are missing coverage. Please review.

Comparison is base (6077768) 80.81% compared to head (9f3405e) 81.12%.
Report is 4 commits behind head on main.

Files	Patch %	Lines
pkg/generator/caches.go	88.15%	6 Missing and 3 partials ⚠️
pkg/generator/ingress_translator.go	87.50%	4 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1156      +/-   ##
==========================================
+ Coverage   80.81%   81.12%   +0.30%     
==========================================
  Files          18       18              
  Lines        1392     1462      +70     
==========================================
+ Hits         1125     1186      +61     
- Misses        213      219       +6     
- Partials       54       57       +3

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

ReToCode · 2023-11-13T08:58:59Z

PTAL, I'll rebase and remove the go mod replace once the networking PR is merged.

/assign @nak3
/assign @skonto

skonto · 2023-11-13T10:26:10Z

pkg/generator/ingress_translator.go

@@ -48,11 +48,13 @@ import (

 type translatedIngress struct {
 	name                    types.NamespacedName
-	sniMatches              []*envoy.SNIMatch
+	localSNIMatches         []*envoy.SNIMatch


Thinking out loud is there a scenario where a ksvc wants to communicate with an egress target via the knative local gateway and the target does not need mutual tls. I was wondering about the use of auto_sni.

Hm sounds to me more like a Service-Mesh case. I don't think we ever want to go through kourier for egress traffic, right?

skonto · 2023-11-13T11:06:16Z

pkg/generator/caches_test.go

@@ -166,14 +166,14 @@ func TestDeleteIngressInfoWhenDoesNotExist(t *testing.T) {
 	assert.DeepEqual(t, listenersBeforeDelete, listenersAfterDelete, protocmp.Transform())
 }

-func TestTLSListenerWithEnvCertsSecret(t *testing.T) {
+func TestExternalTLSListener(t *testing.T) {


We don't want to support

TestExternalTLSListenerWithEnvCertsSecrets

TestExternalTLSListenerWithInternalCertSecret

?
Local is only meant to use the config map config?

I don't think I get your question.

The ExternalTLS Listener is for cluster-external domains and the cluster-local one is for the cluster-local domains. Both can have the SNI option (has already been there for the external one, this PR adds it for cluster-local) when a specific cert is there for a specific domain, and a general certificate for all other traffic (that is already there for both listeners).

Yes but the tests were renamed and the source of the cert can be either env or some cm config. What do we support for internal and external cases?

I felt the naming did not really reflect what they test. Both cases can have a default cert for all domains and specific ones for specified domains, I think we do cover all of them for both cases with:

func TestExternalTLSListener(t *testing.T) { // sets the default cert t.Setenv(envCertsSecretNamespace, "certns") t.Setenv(envCertsSecretName, "secretname") // Test with only default domains t.Run("without SNI matches", func(t *testing.T) {} // Test with one added specific domain t.Run("with a single SNI match", func(t *testing.T) {} // Test with two added specific domains t.Run("with multiple SNI matches", func(t *testing.T) {} }

pkg/generator/ingress_translator.go

pkg/generator/caches.go

skonto · 2023-11-13T11:46:32Z

LGTM in general waiting for @nak3 before stamping.

nak3

Do you have a plan to add conformance or e2e test for this cluster local domain TLS in networking or net-* repos like external domain for test/conformance/ingress/tls.go?

pkg/reconciler/ingress/lister.go

ReToCode · 2023-11-14T06:28:10Z

Do you have a plan to add conformance or e2e test for this cluster local domain TLS in networking or net-* repos like external domain for test/conformance/ingress/tls.go?

Yes this is the plan in knative/serving#13855. I'm not sure yet how they can look like exactly. I think we should finish the implementation end to end for one ingress then add conformance tests. Also Serving will get additional end-2-end tests draft here.

ReToCode · 2023-11-24T08:34:22Z

/unhold

Networking PR is merged: knative/networking#891, conformance tests is tracked in knative/serving#13855. So this is ready to be reviewed again @nak3 @skonto

skonto · 2023-12-04T20:20:34Z

Tested in knative/serving#14703 stamping.

skonto · 2023-12-04T20:20:38Z

/lgtm

knative-prow bot added kind/enhancement 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 Nov 13, 2023

knative-prow bot requested review from krsna-m and skonto November 13, 2023 07:12

knative-prow bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 13, 2023

ReToCode force-pushed the encryption-certmanager-only branch from 8df6096 to bb2aadb Compare November 13, 2023 08:56

knative-prow bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 13, 2023

knative-prow bot assigned nak3 and skonto Nov 13, 2023

ReToCode changed the title ~~[wip] Add TLS for cluster-local-domains~~ Add TLS for cluster-local-domains Nov 13, 2023

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

ReToCode changed the title ~~Add TLS for cluster-local-domains~~ Host cluster-local-domain TLS on local listener with SNI Nov 13, 2023

skonto reviewed Nov 13, 2023

View reviewed changes

pkg/generator/ingress_translator.go Outdated Show resolved Hide resolved

skonto reviewed Nov 13, 2023

View reviewed changes

pkg/generator/caches.go Show resolved Hide resolved

skonto reviewed Nov 13, 2023

View reviewed changes

pkg/generator/caches.go Show resolved Hide resolved

nak3 reviewed Nov 14, 2023

View reviewed changes

pkg/reconciler/ingress/lister.go Show resolved Hide resolved

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

ReToCode force-pushed the encryption-certmanager-only branch from ff34a0d to a216e45 Compare November 22, 2023 12:59

Add TLS for cluster-local-domains

9f3405e

ReToCode force-pushed the encryption-certmanager-only branch from a216e45 to 9f3405e Compare November 24, 2023 08:32

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

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

This was referenced Nov 24, 2023

Make sure all net-* implementations do not break when cluster-local-tls is introduced knative/serving#14624

Closed

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

Merged

skonto mentioned this pull request Dec 4, 2023

[TEST-ONLY] Test cluster local domains knative/serving#14703

Closed

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Dec 4, 2023

knative-prow bot merged commit c3b4a15 into knative-extensions:main Dec 4, 2023
46 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Host cluster-local-domain TLS on local listener with SNI #1156

Host cluster-local-domain TLS on local listener with SNI #1156

ReToCode commented Nov 13, 2023 •

edited

Loading

knative-prow bot commented Nov 13, 2023

codecov bot commented Nov 13, 2023 •

edited

Loading

ReToCode commented Nov 13, 2023

skonto Nov 13, 2023

ReToCode Nov 13, 2023

skonto Nov 13, 2023 •

edited

Loading

ReToCode Nov 13, 2023

skonto Nov 13, 2023

ReToCode Nov 14, 2023

skonto commented Nov 13, 2023

nak3 left a comment

ReToCode commented Nov 14, 2023

ReToCode commented Nov 24, 2023

skonto commented Dec 4, 2023

skonto commented Dec 4, 2023

Host cluster-local-domain TLS on local listener with SNI #1156

Host cluster-local-domain TLS on local listener with SNI #1156

Conversation

ReToCode commented Nov 13, 2023 • edited Loading

Changes

knative-prow bot commented Nov 13, 2023

codecov bot commented Nov 13, 2023 • edited Loading

Codecov Report

ReToCode commented Nov 13, 2023

skonto Nov 13, 2023

Choose a reason for hiding this comment

ReToCode Nov 13, 2023

Choose a reason for hiding this comment

skonto Nov 13, 2023 • edited Loading

Choose a reason for hiding this comment

ReToCode Nov 13, 2023

Choose a reason for hiding this comment

skonto Nov 13, 2023

Choose a reason for hiding this comment

ReToCode Nov 14, 2023

Choose a reason for hiding this comment

skonto commented Nov 13, 2023

nak3 left a comment

Choose a reason for hiding this comment

ReToCode commented Nov 14, 2023

ReToCode commented Nov 24, 2023

skonto commented Dec 4, 2023

skonto commented Dec 4, 2023

ReToCode commented Nov 13, 2023 •

edited

Loading

codecov bot commented Nov 13, 2023 •

edited

Loading

skonto Nov 13, 2023 •

edited

Loading