Add additional encryption config flags + labels #891

ReToCode · 2023-11-13T06:59:36Z

Changes

In preparation for [cluster-local-domain-tls] extend our Knative internal CA to sign cluster-local certificates serving#14216
Add CertificateTypeLabelKey label with values CertificateType to indicate different types of a KnativeCertificate
Drop deprecated and now unused ServingInternalCertName (checked in all repos in knative + knative-extensions)
Add helper method GetIngressTLSForVisibility to Ingress to filter cluster-local and external-ip TLS blocks.

/kind enhancement

Example of KnativeIngress with TLS block for cluster-local-domain-tls:

apiVersion: networking.internal.knative.dev/v1alpha1
kind: Ingress
metadata:
  name: helloworld
  namespace: default
spec:
  httpOption: Enabled
  rules:
  - hosts:
    - helloworld.default
    - helloworld.default.svc
    - helloworld.default.svc.cluster.local
    http:
      paths:
      - splits:
        - appendHeaders:
            Knative-Serving-Namespace: default
            Knative-Serving-Revision: helloworld-00001
          percent: 100
          serviceName: helloworld-00001
          serviceNamespace: default
          servicePort: 443
    visibility: ClusterLocal
  - hosts:
    - helloworld.default.10.89.0.200.sslip.io
    http:
      paths:
      - splits:
        - appendHeaders:
            Knative-Serving-Namespace: default
            Knative-Serving-Revision: helloworld-00001
          percent: 100
          serviceName: helloworld-00001
          serviceNamespace: default
          servicePort: 443
  tls:
  - hosts:
    - helloworld.default
    - helloworld.default.svc
    - helloworld.default.svc.cluster.local
    secretName: route-ba07ea4e-2548-4632-9187-e615c876cffc-local
    secretNamespace: default

ReToCode · 2023-11-13T07:00:04Z

PTAL:

/assign @dprotaso
/assign @nak3
/assign @skonto
/assign @KauzClay

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

Codecov Report

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

Comparison is base (25da91b) 82.14% compared to head (db5cba2) 82.18%.
Report is 1 commits behind head on main.

Files	Patch %	Lines
pkg/apis/networking/v1alpha1/ingress_helpers.go	85.71%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #891      +/-   ##
==========================================
+ Coverage   82.14%   82.18%   +0.04%     
==========================================
  Files          45       46       +1     
  Lines        1725     1746      +21     
==========================================
+ Hits         1417     1435      +18     
- Misses        266      268       +2     
- Partials       42       43       +1

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

pkg/apis/networking/v1alpha1/ingress_types.go

pkg/config/config.go

nak3 · 2023-11-14T08:31:28Z

/lgtm
/hold

Thank you!
For knative-extensions/net-istio, I think we need to add IngressTLS.Vibility in the unit test.
/hold for other reviewers. Please feel free to unhold if no one will take a look.

ReToCode · 2023-11-14T09:05:04Z

For knative-extensions/net-istio, I think we need to add IngressTLS.Vibility in the unit test.

👍 I'll do that as a follow-up

dprotaso · 2023-11-14T23:01:38Z

Add visibility to KnativeIngress.TLS (see example below)

Why does the ingress need this?

ReToCode · 2023-11-15T07:25:21Z

Why does the ingress need this?

Hm, thinking about it based on your question, it is probably not totally necessary. But without having it, each net-* solution would need to compare the tls section with the hosts list to find out if the tls config is relevant for a cluster-local domain or an external domain. In net-kourier, we use this flag to distinguish which listener to configure:

nak3 · 2023-11-15T08:41:30Z

Should we restructure the spec.tls and spec.rules at this time? For example,

option a) merge `spec.tls` into `spec.rules` like:

apiVersion: networking.internal.knative.dev/v1alpha1
kind: Ingress
metadata:
  name: helloworld
  namespace: default
spec:
  httpOption: Enabled
  rules:
  - hosts:
    - helloworld.default
    - helloworld.default.svc
    - helloworld.default.svc.cluster.local
    http:
      paths:
      - splits:
        - appendHeaders:
            Knative-Serving-Namespace: default
            Knative-Serving-Revision: helloworld-00001
          percent: 100
          serviceName: helloworld-00001
          serviceNamespace: default
          servicePort: 443
    visibility: ClusterLocal
    secretName: some-local-secret  # optional - omitting means no TLS.
    secretNamespace: default
  - hosts:
    - helloworld.default.10.89.0.200.sslip.io
    http:
      paths:
      - splits:
        - appendHeaders:
            Knative-Serving-Namespace: default
            Knative-Serving-Revision: helloworld-00001
          percent: 100
          serviceName: helloworld-00001
          serviceNamespace: default
          servicePort: 443
    visibility: ExternalIP
    secretName: route-ba07ea4e-2548-4632-9187-e615c876cffc-local # optional - omitting means no TLS.
    secretNamespace: default

option b) drop `spec.tls.hosts[]` and use the `spec.tls.visibility` as a key like:

apiVersion: networking.internal.knative.dev/v1alpha1
kind: Ingress
metadata:
  name: helloworld
  namespace: default
spec:
  httpOption: Enabled
  rules:
  - hosts:
    - helloworld.default
    - helloworld.default.svc
    - helloworld.default.svc.cluster.local
    http:
      paths:
      - splits:
        - appendHeaders:
            Knative-Serving-Namespace: default
            Knative-Serving-Revision: helloworld-00001
          percent: 100
          serviceName: helloworld-00001
          serviceNamespace: default
          servicePort: 443
    visibility: ClusterLocal
  - hosts:
    - helloworld.default.10.89.0.200.sslip.io
    http:
      paths:
      - splits:
        - appendHeaders:
            Knative-Serving-Namespace: default
            Knative-Serving-Revision: helloworld-00001
          percent: 100
          serviceName: helloworld-00001
          serviceNamespace: default
          servicePort: 443
    visibility: ExternalIP
  tls:
  - visibility: ExternalIP
    secretName: secret-for-external
    secretNamespace: default
  - visibility: ClusterLocal
    secretName: route-ba07ea4e-2548-4632-9187-e615c876cffc-local
    secretNamespace: default

Also, httpOption might be better to apply for local and external respectively.

ReToCode · 2023-11-15T10:16:18Z

Hm, not sure if option b) would work. Note we can have multiple entries for both visibilities with different certificates:
Here an example with tags:

apiVersion: networking.internal.knative.dev/v1alpha1
kind: Ingress
metadata:
  labels:
    serving.knative.dev/route: helloworld
    serving.knative.dev/routeNamespace: default
    serving.knative.dev/service: helloworld
  name: helloworld
  namespace: default
spec:
  httpOption: Enabled
  rules:
  - hosts:
    - helloworld.default
    - helloworld.default.svc
    - helloworld.default.svc.cluster.local
    http:
      paths:
      - splits:
        - appendHeaders:
            Knative-Serving-Namespace: default
            Knative-Serving-Revision: helloworld-00001
          percent: 100
          serviceName: helloworld-00001
          serviceNamespace: default
          servicePort: 443
    visibility: ClusterLocal
  - hosts:
    - helloworld.default.192.168.105.100.sslip.io
    http:
      paths:
      - splits:
        - appendHeaders:
            Knative-Serving-Namespace: default
            Knative-Serving-Revision: helloworld-00001
          percent: 100
          serviceName: helloworld-00001
          serviceNamespace: default
          servicePort: 443
    visibility: ExternalIP
  - hosts:
    - latest-helloworld.default
    - latest-helloworld.default.svc
    - latest-helloworld.default.svc.cluster.local
    http:
      paths:
      - splits:
        - appendHeaders:
            Knative-Serving-Namespace: default
            Knative-Serving-Revision: helloworld-00001
          percent: 100
          serviceName: helloworld-00001
          serviceNamespace: default
          servicePort: 443
    visibility: ClusterLocal
  - hosts:
    - latest-helloworld.default.192.168.105.100.sslip.io
    http:
      paths:
      - splits:
        - appendHeaders:
            Knative-Serving-Namespace: default
            Knative-Serving-Revision: helloworld-00001
          percent: 100
          serviceName: helloworld-00001
          serviceNamespace: default
          servicePort: 443
    visibility: ExternalIP
  tls:
  - hosts:
    - helloworld.default
    - helloworld.default.svc
    - helloworld.default.svc.cluster.local
    secretName: route-9f44405d-74a9-496b-a43d-effdf98bd6ca-local
    secretNamespace: default
    visibility: ClusterLocal
  - hosts:
    - latest-helloworld.default
    - latest-helloworld.default.svc
    - latest-helloworld.default.svc.cluster.local
    secretName: route-9f44405d-74a9-496b-a43d-effdf98bd6ca-147587726-local
    secretNamespace: default
    visibility: ClusterLocal

Option a) seems nice, but not sure if we want to change it that much, as this will impact all net-* implementations, regardless of their current state of implementing the new features. Also it might have a reason why it has been done that way in the first place? WDYT @dprotaso ?

nak3 · 2023-11-16T08:35:18Z

I just realized that another concern of ths PR. Current net-* implementaitons does a simple loop against spec.TLS or len(ing.Spec.TLS) > 0. So if we added the cluster local TLS, it will break external TLS without their support of cluster local TLS?
Do you have a plan how to manage it like filter it by spec.TLS.Visiblity or we just document that "Do NOT enable (do not touch) cluster local TLS for net-{istio, contour}"?

ReToCode · 2023-11-16T09:30:08Z

That is a good point @nak3 , but I think not one that we can omit without introducing a totally new field here. The flag is still marked as alpha, so I'd vote to add the hint as you say. As long as this feature is not finished, it should not be enabled in a real environment. Then the cluster-local TLS entries will not occur.

I'll add this filter changes as issues to our tracking umbrella.

dprotaso · 2023-11-16T17:46:35Z

I think I'd like to avoid API changes to our internal Ingress resource - since we'd have to support multiple versions while we perform a migration.

Given that the TLS stanza just maps host names to a secret I think I'd rather just create helper functions in this repo that implementations can use to reduce complexity.

ReToCode · 2023-11-17T10:45:26Z

/hold I'm doing some testing with kourier + serving and add a test for the new function.

skonto · 2023-11-17T10:52:10Z

Should we restructure the spec.tls and spec.rules at this time? For example,

I would vote for that too. It seems there is a lot of repetition of info here.

dprotaso · 2023-11-17T15:33:47Z

I would vote for that too. It seems there is a lot of repetition of info here.

You could argue that you get more repetition if you have a TLS block within the rules block. If you didn't have wildcard certs then you'd have to repeat the entire rule many times (eg. appendHeaders)

skonto · 2023-11-17T15:52:41Z

You could argue that you get more repetition if you have a TLS block within the rules block.

We could have it under rules, my thinking was optimize based on what is common in practice:

spec:
  rules:
  - hosts:
    - helloworld.default
    - helloworld.default.svc
    - helloworld.default.svc.cluster.local
    tls:

Not sure if we are describing the same thing.

ReToCode · 2023-11-22T14:52:20Z

@skonto @dprotaso I don't think we ever have a * in king. It seems to be only in the kcert, see:

But anyway, I added GetIngressTLSForVisibility on Ingress and updated the PR description above. With this there are no changes to the API, we only need to make sure the net-* repos use the new method before one enables cluster-local-domain-tls. Full test run with kourier, net-certmanager is in Serving, see here. Can you take another look?

dprotaso · 2023-11-22T14:58:51Z

Can you update the commit message - it doesn't match the contents of the commit

dprotaso · 2023-11-22T15:04:13Z

pkg/apis/networking/register.go

+
+	// CertificateTypeLabelKey is the label to indicate the type of Knative certificate
+	// used for Knative Serving encryption functionality.
+	CertificateTypeLabelKey = PublicGroupName + "/certificate-type"


What are the corresponding values?

Added a comment to link to config.CertificateType. Is there a better way to do this?

dprotaso · 2023-11-22T15:07:16Z

pkg/apis/networking/v1alpha1/ingress_helpers.go

+		if r.Visibility == visibility {
+			for _, t := range i.Spec.TLS {
+				// Check if hosts slices are equal ignoring the order
+				if cmp.Diff(r.Hosts, t.Hosts, cmpopts.SortSlices(func(a, b string) bool { return a < b })) == "" {


This shouldn't be an equal comparison but tls.Hosts.Contains(host)

In theory KIngress can express different TLS certs for different hosts that are in the same rules block

I see, switched it to check if every host in Rules.Hosts is contained in TLS.Hosts. Also added a test case for this.

dprotaso · 2023-11-23T18:47:45Z

/lgtm
/approve

knative-prow · 2023-11-23T18:47:51Z

[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]

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 kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Nov 13, 2023

knative-prow bot requested review from kauana and KauzClay November 13, 2023 06:59

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

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

ReToCode changed the title ~~Add encryption config~~ Add additional encryption config flags + labels Nov 13, 2023

This was referenced Nov 13, 2023

Host cluster-local-domain TLS on local listener with SNI knative-extensions/net-kourier#1156

Merged

Add a new issuer configuration to net-certmanager for system-internal-tls certificates knative-extensions/net-certmanager#627

Merged

nak3 reviewed Nov 14, 2023

View reviewed changes

pkg/apis/networking/v1alpha1/ingress_types.go Outdated Show resolved Hide resolved

pkg/config/config.go Outdated Show resolved Hide resolved

ReToCode force-pushed the add-encryption-config branch from 67c22d0 to c09afcb Compare November 14, 2023 08:05

knative-prow bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Nov 14, 2023

nak3 removed the lgtm Indicates that a PR is ready to be merged. label Nov 16, 2023

ReToCode mentioned this pull request Nov 16, 2023

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

Closed

4 tasks

ReToCode force-pushed the add-encryption-config branch from 6f8a375 to 9ec4fd5 Compare November 17, 2023 10:44

ReToCode force-pushed the add-encryption-config branch from 9ec4fd5 to da1b703 Compare November 22, 2023 13:57

knative-prow bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 22, 2023

ReToCode added kind/enhancement and removed kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API labels Nov 22, 2023

ReToCode removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 22, 2023

dprotaso reviewed Nov 22, 2023

View reviewed changes

Add new CertificateType config and IngressTLS helper

44caafa

ReToCode force-pushed the add-encryption-config branch 2 times, most recently from 839bec7 to 467173c Compare November 23, 2023 08:31

Switch IngressTLS helper from equality to contains

db5cba2

ReToCode force-pushed the add-encryption-config branch from 467173c to db5cba2 Compare November 23, 2023 08:33

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

knative-prow bot merged commit bb18aab into knative:main Nov 23, 2023
28 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add additional encryption config flags + labels #891

Add additional encryption config flags + labels #891

ReToCode commented Nov 13, 2023 •

edited

Loading

ReToCode commented Nov 13, 2023

codecov bot commented Nov 13, 2023 •

edited

Loading

nak3 commented Nov 14, 2023

ReToCode commented Nov 14, 2023

dprotaso commented Nov 14, 2023

ReToCode commented Nov 15, 2023

nak3 commented Nov 15, 2023 •

edited

Loading

ReToCode commented Nov 15, 2023 •

edited

Loading

nak3 commented Nov 16, 2023 •

edited

Loading

ReToCode commented Nov 16, 2023

dprotaso commented Nov 16, 2023

ReToCode commented Nov 17, 2023

skonto commented Nov 17, 2023

dprotaso commented Nov 17, 2023

skonto commented Nov 17, 2023 •

edited

Loading

ReToCode commented Nov 22, 2023 •

edited

Loading

dprotaso commented Nov 22, 2023

dprotaso Nov 22, 2023

ReToCode Nov 23, 2023

dprotaso Nov 22, 2023

ReToCode Nov 23, 2023

dprotaso commented Nov 23, 2023

knative-prow bot commented Nov 23, 2023

Add additional encryption config flags + labels #891

Add additional encryption config flags + labels #891

Conversation

ReToCode commented Nov 13, 2023 • edited Loading

Changes

ReToCode commented Nov 13, 2023

codecov bot commented Nov 13, 2023 • edited Loading

Codecov Report

nak3 commented Nov 14, 2023

ReToCode commented Nov 14, 2023

dprotaso commented Nov 14, 2023

ReToCode commented Nov 15, 2023

nak3 commented Nov 15, 2023 • edited Loading

ReToCode commented Nov 15, 2023 • edited Loading

nak3 commented Nov 16, 2023 • edited Loading

ReToCode commented Nov 16, 2023

dprotaso commented Nov 16, 2023

ReToCode commented Nov 17, 2023

skonto commented Nov 17, 2023

dprotaso commented Nov 17, 2023

skonto commented Nov 17, 2023 • edited Loading

ReToCode commented Nov 22, 2023 • edited Loading

dprotaso commented Nov 22, 2023

dprotaso Nov 22, 2023

Choose a reason for hiding this comment

ReToCode Nov 23, 2023

Choose a reason for hiding this comment

dprotaso Nov 22, 2023

Choose a reason for hiding this comment

ReToCode Nov 23, 2023

Choose a reason for hiding this comment

dprotaso commented Nov 23, 2023

knative-prow bot commented Nov 23, 2023

ReToCode commented Nov 13, 2023 •

edited

Loading

codecov bot commented Nov 13, 2023 •

edited

Loading

nak3 commented Nov 15, 2023 •

edited

Loading

ReToCode commented Nov 15, 2023 •

edited

Loading

nak3 commented Nov 16, 2023 •

edited

Loading

skonto commented Nov 17, 2023 •

edited

Loading

ReToCode commented Nov 22, 2023 •

edited

Loading