Host cluster-local-domain-tls on cluster-local gateway with SNI #1228

ReToCode · 2023-12-19T13:10:13Z

Changes

Add cluster-local https servers when cluster-local-domain-tls is enabled and a KSVC exists
Uses SNI to host multiple certificates for all cluster-local domain Knative Services

/kind enhancement

Release Note

net-istio now hosts an additional gateway with TLS for each Knative Service 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-12-19T13:10:21Z

[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-12-19T13:12:41Z

Codecov Report

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

Comparison is base (fadb996) 81.84% compared to head (4cbe505) 81.12%.
Report is 18 commits behind head on main.

Files	Patch %	Lines
pkg/reconciler/ingress/ingress.go	60.86%	10 Missing and 8 partials ⚠️
pkg/reconciler/ingress/resources/gateway.go	84.61%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1228      +/-   ##
==========================================
- Coverage   81.84%   81.12%   -0.72%     
==========================================
  Files          19       19              
  Lines        1680     1706      +26     
==========================================
+ Hits         1375     1384       +9     
- Misses        218      230      +12     
- Partials       87       92       +5

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

ReToCode · 2023-12-21T09:23:52Z

Did some testing with Serving @ knative/serving#14610, looks good. Test setup here.

@skonto PTAL. Note: I'm fine with waiting until Serving is merged, then we can add this here and also do the full test-set with with net-istio in Serving.

ReToCode · 2024-01-26T07:01:32Z

/unhold

Serving PR has been merged, @skonto PTAL.

dprotaso · 2024-01-31T22:42:43Z

config/203-local-gateway.yaml

@@ -55,3 +55,6 @@ spec:
    - name: http2
      port: 80
      targetPort: 8081
+    - name: https
+      port: 443
+      targetPort: 8443


To confirm my understanding - the two istio gateways we have actually use the same envoy instance but we use different ports for tenancy

For external ingress we open

port 80

port 443 dynamically when we have TLS certs

For internal traffic we have

port 8081 (this service exposes it on port 80)

port 8443 (this service exposes it on port 443)

I see 443 server being added here for external traffic:

net-istio/pkg/reconciler/ingress/resources/gateway.go

Lines 321 to 323 in 6dd62fe

Name: fmt.Sprintf(portNamePrefix(ing.GetNamespace(), ing.GetName())+":%d", i),

Number: 443,

Protocol: "HTTPS",

But I don't see us adding TLS servers on port 8443 for cluster local traffic

Unfortunately istio is quite confusing in that config:

Istio has an already open and exposed port 8443 for https (https://github.com/knative-extensions/net-istio/blob/main/third_party/istio-latest/istio-ci-no-mesh/istio.yaml#L9955)

traffic from external (LB) will end up on 8443 (https://github.com/knative-extensions/net-istio/blob/main/third_party/istio-latest/istio-ci-no-mesh/istio.yaml#L10240)

The change here will just forward traffic on the local gateway service to the same port

Istio will use SNI to determine which route to take, if we look at the envoy config for two Knative Services we get

istioctl proxy-config listeners -n istio-system deploy/istio-ingressgateway ADDRESSES PORT MATCH DESTINATION 0.0.0.0 8080 ALL Route: http.8080 0.0.0.0 8081 ALL Route: http.8081 0.0.0.0 8443 SNI: helloworld.second,helloworld.second.svc,helloworld.second.svc.cluster.local Route: https.443.second/helloworld:0.helloworld-975966116.second 0.0.0.0 8443 SNI: helloworld.second.172.17.0.100.sslip.io Route: https.443.second/helloworld:0.helloworld-3797421420.second 0.0.0.0 8443 SNI: helloworld.first,helloworld.first.svc,helloworld.first.svc.cluster.local Route: https.443.first/helloworld:0.helloworld-975966116.first 0.0.0.0 8443 SNI: helloworld.first.172.17.0.100.sslip.io Route: https.443.first/helloworld:0.helloworld-3797421420.first 0.0.0.0 15021 ALL Inline Route: /healthz/ready* 0.0.0.0 15090 ALL Inline Route: /stats/prometheus*

So basically, everything https will be on 8443 and routed to 443 to the Knative Service.

I'm not even sure if we would need to specify the port or not, but that 443 just maps to the containers 8443 port.

I assume we could use a completely different port for the internal TLS traffic, but not sure what we would gain. As you said, it's a shared component without tenancy anyway.

It's using tenancy via port isolation so external traffic cannot hit the 'local' gateway ports.

Essentially these ports are public - so we should use a different port for internal tls - otherwise we risk exposing private Knative Services to the internet.

net-istio/third_party/istio-latest/istio-ci-no-mesh/istio.yaml

Lines 10232 to 10247 in 54d8f41

- name: status-port

port: 15021

protocol: TCP

targetPort: 15021

- name: http2

port: 80

protocol: TCP

targetPort: 8080

- name: https

port: 443

protocol: TCP

targetPort: 8443

selector:

app: istio-ingressgateway

istio: ingressgateway

type: LoadBalancer

Maybe this can be configurable after this lands - #1249

Essentially these ports are public - so we should use a different port for internal tls - otherwise we risk exposing private Knative Services to the internet.

That's a valid point. I'll take a look tomorrow.

I updated the PR to use a dedicated port for cluster-local-tls (8444). With that I get the following envoy config:

istioctl proxy-config listeners -n istio-system deploy/istio-ingressgateway ADDRESSES PORT MATCH DESTINATION 0.0.0.0 8080 ALL Route: http.8080 0.0.0.0 8081 ALL Route: http.8081 0.0.0.0 8443 SNI: helloworld.default.172.17.0.100.sslip.io Route: https.443.default/helloworld:0.helloworld-3797421420.default 0.0.0.0 8444 SNI: helloworld.default,helloworld.default.svc,helloworld.default.svc.cluster.local Route: https.8444.default/helloworld:0.helloworld-975966116.default 0.0.0.0 15021 ALL Inline Route: /healthz/ready* 0.0.0.0 15090 ALL Inline Route: /stats/prometheus*

As 8444 is not exposed, it is now only available cluster locally.

skonto · 2024-02-02T11:14:37Z

As discussed with Reto the landing page of this repo is not ideal. There is no doc explaining what is supported and how. For example here we are moving to a design choice of having one gateway per svc for cluster local stuff but it is not captured somewhere etc. I think we need to spend some time adding content here but also to other net-* repos.
It is kind of hard for newcomers to understand what is happening.

skonto · 2024-02-02T13:10:15Z

pkg/reconciler/ingress/ingress.go

@@ -57,10 +57,9 @@ import (
 )

 const (
-	virtualServiceConditionReconciled = "Reconciled"


+1 this is a relic of some functionality never fixed at the istio side.

skonto · 2024-02-02T13:14:22Z

pkg/reconciler/ingress/ingress.go

 }

-func (r *Reconciler) reconcileDeletion(ctx context.Context, ing *v1alpha1.Ingress) error {
-	if !shouldReconcileTLS(ing) {
+func (r *Reconciler) cleanupCertificateSecrets(ctx context.Context, ing *v1alpha1.Ingress) error {


Maybe deleteAllDomainCertificateSecrets.

skonto · 2024-02-02T14:35:09Z

pkg/reconciler/ingress/ingress.go

-	ingressGateways := []*v1beta1.Gateway{}
-	if shouldReconcileTLS(ing) {
-		originSecrets, err := resources.GetSecrets(ing, r.secretLister)
+	externalIngressGateways := []*v1beta1.Gateway{}


var externalIngressGateways []*v1beta1.Gateway or we want an empty slice instead?

skonto · 2024-02-02T14:46:26Z

minor comments, LGTM. We can stamp if @dprotaso has no other comments.

dprotaso · 2024-02-06T18:30:57Z

/lgtm

As discussed with Reto the landing page of this repo is not ideal. There is no doc explaining what is supported and how. For example here we are moving to a design choice of having one gateway per svc for cluster local stuff but it is not captured somewhere etc. I think we need to spend some time adding content here but also to other net-* repos.
It is kind of hard for newcomers to understand what is happening.

Yeah that's a good point - want to create an issue to capture this work?

…ive-extensions#1228) * Host cluster-local-domain TLS on local listener with SNI * Use port 8444 for cluster-local TLS traffic (cherry picked from commit 83db165)

* Host cluster-local-domain-tls on cluster-local gateway with SNI (knative-extensions#1228) * Host cluster-local-domain TLS on local listener with SNI * Use port 8444 for cluster-local TLS traffic (cherry picked from commit 83db165) * Update patch to reflect upstream changes

knative-prow bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. kind/enhancement do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Dec 19, 2023

knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 19, 2023

knative-prow bot requested review from KauzClay and nak3 December 19, 2023 13:10

knative-prow bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 19, 2023

ReToCode changed the title ~~[wip] Host cluster-local-domain TLS on local listener with SNI~~ [wip] Host cluster-local-domain-tls on cluster-local gateway with SNI Dec 19, 2023

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

48aafe4

ReToCode force-pushed the cluster-local-tls branch from a9c85e4 to 48aafe4 Compare December 20, 2023 08:37

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 Dec 20, 2023

ReToCode changed the title ~~[wip] Host cluster-local-domain-tls on cluster-local gateway with SNI~~ Host cluster-local-domain-tls on cluster-local gateway with SNI Dec 21, 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 Dec 21, 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 Jan 26, 2024

ReToCode assigned skonto Jan 26, 2024

skonto requested review from skonto and removed request for nak3 January 31, 2024 11:56

dprotaso reviewed Jan 31, 2024

View reviewed changes

Use port 8444 for cluster-local TLS traffic

4cbe505

skonto reviewed Feb 2, 2024

View reviewed changes

knative-prow bot assigned dprotaso Feb 6, 2024

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

knative-prow bot merged commit 83db165 into knative-extensions:main Feb 6, 2024
48 checks passed

ReToCode mentioned this pull request Feb 7, 2024

[RELEASE-1.13] Back-port https://github.com/knative-extensions/net-istio/pull/1228 openshift-knative/net-istio#79

Merged

ReToCode added this to the v1.14.0 milestone Apr 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Host cluster-local-domain-tls on cluster-local gateway with SNI #1228

Host cluster-local-domain-tls on cluster-local gateway with SNI #1228

ReToCode commented Dec 19, 2023 •

edited

Loading

knative-prow bot commented Dec 19, 2023

codecov bot commented Dec 19, 2023 •

edited

Loading

ReToCode commented Dec 21, 2023

ReToCode commented Jan 26, 2024

dprotaso Jan 31, 2024

dprotaso Jan 31, 2024

dprotaso Jan 31, 2024

ReToCode Feb 1, 2024

ReToCode Feb 1, 2024 •

edited

Loading

dprotaso Feb 1, 2024

dprotaso Feb 1, 2024

ReToCode Feb 1, 2024

ReToCode Feb 2, 2024

skonto commented Feb 2, 2024 •

edited

Loading

skonto Feb 2, 2024

skonto Feb 2, 2024

skonto Feb 2, 2024

skonto commented Feb 2, 2024

dprotaso commented Feb 6, 2024

	Name: fmt.Sprintf(portNamePrefix(ing.GetNamespace(), ing.GetName())+":%d", i),
	Number: 443,
	Protocol: "HTTPS",

	- name: status-port
	port: 15021
	protocol: TCP
	targetPort: 15021
	- name: http2
	port: 80
	protocol: TCP
	targetPort: 8080
	- name: https
	port: 443
	protocol: TCP
	targetPort: 8443
	selector:
	app: istio-ingressgateway
	istio: ingressgateway
	type: LoadBalancer

Host cluster-local-domain-tls on cluster-local gateway with SNI #1228

Host cluster-local-domain-tls on cluster-local gateway with SNI #1228

Conversation

ReToCode commented Dec 19, 2023 • edited Loading

Changes

knative-prow bot commented Dec 19, 2023

codecov bot commented Dec 19, 2023 • edited Loading

Codecov Report

ReToCode commented Dec 21, 2023

ReToCode commented Jan 26, 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

ReToCode Feb 1, 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

skonto commented Feb 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skonto commented Feb 2, 2024

dprotaso commented Feb 6, 2024

ReToCode commented Dec 19, 2023 •

edited

Loading

codecov bot commented Dec 19, 2023 •

edited

Loading

ReToCode Feb 1, 2024 •

edited

Loading

skonto commented Feb 2, 2024 •

edited

Loading