Update TLS Interception Helm values by youngnick · Pull Request #37076 · cilium/cilium

youngnick · 2025-01-20T06:28:44Z

This commit updates TLS Interception Helm values to both be more understandable and allow greater
flexibility in rollout.

tls.secretsBackend: k8s|local is DEPRECATED and will be removed in a future version.
tls.readSecretsOnlyFromSecretsNamespace is the replacement for the actual effect of tls.secretsBackend, named to make the tradeoffs clearer. This setting controls whether the agent will read only from the configured Secrets namespace, or from any namespace in the cluster.
tls.secretSync.secretsNamespace is moved up one level, as it now applies in more cases than the secret-sync and SDS one

There are some code changes in the Policy secret handling to ensure that we support three scenarios for where TLS Policy Secrets are located:

Secrets placed in the cilium-secrets namespace, and read from there by the Cilium Agent, then sent to Envoy inline in Network Policy Discovery Service. This case came from user reports about how TLS Interception was being used in the real world - it used to be possible by turning on other config that created the cilium-secrets namespace, but now works by default.
Secrets located anywhere in the cluster, and read directly by the Cilium agent - this requires the tls.readSecretsOnlyFromSecretsNamespace setting to be false, or tls.secretsBackend: k8s to be set.
Secrets located anywhere in the cluster, synchronized by the Cilium Operator into the cilium-secrets namespace, then read from there by the Cilium Agent and sent to Envoy via SDS. This is a new feature added since Cilium 1.16.

It also adds code to handle only enabling secret sync on new clusters. Testing done manually with helm template:

# Check default state for new cluster
❯ helm template cilium /Users/ynick/src/cilium/install/kubernetes/cilium | grep policy-secrets
  enable-policy-secrets-sync: "true"
  policy-secrets-only-from-secrets-namespace: "true"
  policy-secrets-namespace: "cilium-secrets"
# Check default state for upgrade (should be use case 1, policy-secrets-only-from-secrets-namespace true and policy-secrets-namespace set)
❯ helm template cilium /Users/ynick/src/cilium/install/kubernetes/cilium --set upgradeCompatibility=1.16 | grep policy-secrets
  policy-secrets-only-from-secrets-namespace: "true"
  policy-secrets-namespace: "cilium-secrets"
# Check tls.secretsBackend: k8s (from old docs versions)
❯ helm template cilium /Users/ynick/src/cilium/install/kubernetes/cilium --set tls.secretsBackend=k8s | grep policy-secrets

# Check tls.readSecretsOnlyFromSecretsNamespace false (should be the same)
❯ helm template cilium /Users/ynick/src/cilium/install/kubernetes/cilium --set tls.readSecretsOnlyFromSecretsNamespace=false | grep policy-secrets

# Check tls.readSecretsOnlyFromSecretsNamespace true (should be same as default)
❯ helm template cilium /Users/ynick/src/cilium/install/kubernetes/cilium --set tls.readSecretsOnlyFromSecretsNamespace=true | grep policy-secrets
  enable-policy-secrets-sync: "true"
  policy-secrets-only-from-secrets-namespace: "true"
  policy-secrets-namespace: "cilium-secrets"
# Check tls.secretSync.enabled: false (should be the same as upgrade)
❯ helm template cilium /Users/ynick/src/cilium/install/kubernetes/cilium --set tls.secretSync.enabled=false | grep policy-secrets
  policy-secrets-only-from-secrets-namespace: "true"
  policy-secrets-namespace: "cilium-secrets"

The Helm setting tls.secretsBackend is deprecated and should be replaced with the use of the tls.readSecretsOnlyFromSecretsNamespace setting instead. tls.secretsBackend will be removed in a future Cilium version.

youngnick · 2025-01-22T07:40:39Z

/test

youngnick · 2025-01-22T11:10:31Z

Found some issues, looking at them, will have this ready for review tomorrow EU time.

youngnick · 2025-01-22T11:30:09Z

Okay, issue found, and fixed.

This commit updates TLS Interception Helm values to both be more understandable and allow greater flexibility in rollout. * `tls.secretsBackend: k8s|local` is DEPRECATED and will be removed in a future version. * `tls.readSecretsOnlyFromSecretsNamespace` is the replacement for the _actual_ effect of `tls.secretsBackend`, named to make the tradeoffs clearer. * `tls.secretSync.secretsNamespace` is moved up one level, as it now applies in more cases than the secret-sync and SDS one There are some code changes in the Policy secret handling to ensure that we support three scenarios for where TLS Policy Secrets are located: * Secrets placed in the `cilium-secrets` namespace, and read from there by the Cilium Agent, then sent to Envoy inline in Network Policy Discovery Service. This case came from user reports about how TLS Interception was being used in the real world - it used to be possible by turning on other config that created the `cilium-secrets` namespace, but now works by default. * Secrets located anywhere in the cluster, and read directly by the Cilium agent - this requires the `tls.readSecretsOnlyFromSecretsNamespace` setting to be `false`, or `tls.secretsBackend: k8s` to be set. * Secrets located anywhere in the cluster, synchronized by the Cilium Operator into the `cilium-secrets` namespace, then read from there by the Cilium Agent and sent to Envoy via SDS. This is a new feature added since Cilium 1.16. Signed-off-by: Nick Young <nick@isovalent.com>

This commit updates the TLS Interception docs to match the new install process, including more explanation of how the Helm configuration works. Signed-off-by: Nick Young <nick@isovalent.com>

youngnick · 2025-01-22T11:50:14Z

/test

rolinh · 2025-01-23T10:28:12Z

It looks like the Hubble team is called out for review because of changes on pkg/crypto/certificatemanager/certificate_manager.go. While the Hubble team originally created the pkg/crypto package, this was because of the addition of the pkg/crypto/certloader package. I think the CODEOWNERS file needs an update, the Hubble team never contributed to pkg/crypto/certificatemanager and is thus not really the ideal team to review changes in this package.

ghost

Docs good.

The Helm changes should be documented in the upgrade notes, but main has 1.18 upgrade notes already, so this would fall in the backport.

rolinh

As the Hubble team doesn't need to review changes in this PR (see #37204) and the Envoy team is already a reviewer, I'm blindly approving the PR to unblock it (I can't remove the review request for sig-hubble until the aforementioned PR is merged and this one rebased).

sayboras

Detailed context and explanation 👍

Since #37076 was merged we have a new mode of synchronizing TLS secrets to Envoy. Previously we used NPDS, while new way uses SDS. However, by setting secretsBackend=k8s we were forcing old behaviour. Let's start testing SDS as it's a new default. Signed-off-by: Marcel Zieba <marcel.zieba@isovalent.com>

Since cilium#37076 was merged we have a new mode of synchronizing TLS secrets to Envoy. Previously we used NPDS, while new way uses SDS. However, by setting secretsBackend=k8s we were forcing old behaviour. Let's start testing SDS as it's a new default. Signed-off-by: Marcel Zieba <marcel.zieba@isovalent.com>

maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jan 20, 2025

youngnick force-pushed the update-tls-interception-helm branch 2 times, most recently from 6d88a2d to 0e66f25 Compare January 22, 2025 07:35

youngnick added the release-note/major This PR introduces major new functionality to Cilium. label Jan 22, 2025

maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jan 22, 2025

youngnick marked this pull request as ready for review January 22, 2025 07:36

youngnick requested review from a team as code owners January 22, 2025 07:36

youngnick requested review from a user, rolinh, sayboras and squeed January 22, 2025 07:36

youngnick force-pushed the update-tls-interception-helm branch from 0e66f25 to ed99b19 Compare January 22, 2025 07:42

youngnick marked this pull request as draft January 22, 2025 09:55

youngnick force-pushed the update-tls-interception-helm branch from ed99b19 to 96c9f3f Compare January 22, 2025 10:01

youngnick marked this pull request as ready for review January 22, 2025 10:02

youngnick force-pushed the update-tls-interception-helm branch 2 times, most recently from b3da573 to a150dda Compare January 22, 2025 10:24

youngnick marked this pull request as draft January 22, 2025 11:10

youngnick force-pushed the update-tls-interception-helm branch from a150dda to f351ca0 Compare January 22, 2025 11:11

youngnick force-pushed the update-tls-interception-helm branch from f351ca0 to e0bcaa6 Compare January 22, 2025 11:22

youngnick marked this pull request as ready for review January 22, 2025 11:30

youngnick force-pushed the update-tls-interception-helm branch from e0bcaa6 to 1a0b150 Compare January 22, 2025 11:37

youngnick added 2 commits January 22, 2025 22:43

Update TLS Interception docs

66f1f0f

This commit updates the TLS Interception docs to match the new install process, including more explanation of how the Helm configuration works. Signed-off-by: Nick Young <nick@isovalent.com>

youngnick force-pushed the update-tls-interception-helm branch from 1a0b150 to 66f1f0f Compare January 22, 2025 11:43

rolinh mentioned this pull request Jan 23, 2025

CODEOWNERS: adjust ownership of the pkg/crypto subpackages #37204

Merged

squeed approved these changes Jan 23, 2025

View reviewed changes

ghost approved these changes Jan 23, 2025

View reviewed changes

rolinh approved these changes Jan 23, 2025

View reviewed changes

sayboras approved these changes Jan 23, 2025

View reviewed changes

maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jan 23, 2025

youngnick added this pull request to the merge queue Jan 23, 2025

Merged via the queue into cilium:main with commit 2845fab Jan 23, 2025

youngnick deleted the update-tls-interception-helm branch January 23, 2025 22:55

youngnick mentioned this pull request Jan 23, 2025

[Backport] Update TLS Interception Helm values #37232

Merged

youngnick removed the needs-backport/1.17 This PR / issue needs backporting to the v1.17 branch label Jan 24, 2025

github-actions bot added the backport-done/1.17 The backport for Cilium 1.17.x for this PR is done. label Jan 24, 2025

cilium-release-bot bot mentioned this pull request Jan 24, 2025

Prepare for release v1.17.0-rc.2 #37256

Merged

joestringer mentioned this pull request Feb 10, 2025

Missing preflight documentation #37465

Closed

3 tasks

marseel mentioned this pull request Mar 4, 2025

ci: enable SDS in cloud provider tests #37987

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update TLS Interception Helm values#37076

Update TLS Interception Helm values#37076
youngnick merged 2 commits intocilium:mainfrom
youngnick:update-tls-interception-helm

youngnick commented Jan 20, 2025 •

edited

Loading

Uh oh!

youngnick commented Jan 22, 2025

Uh oh!

youngnick commented Jan 22, 2025

Uh oh!

youngnick commented Jan 22, 2025

Uh oh!

youngnick commented Jan 22, 2025

Uh oh!

rolinh commented Jan 23, 2025

Uh oh!

ghost left a comment

Uh oh!

rolinh left a comment

Uh oh!

sayboras left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

youngnick commented Jan 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

youngnick commented Jan 22, 2025

Uh oh!

youngnick commented Jan 22, 2025

Uh oh!

youngnick commented Jan 22, 2025

Uh oh!

youngnick commented Jan 22, 2025

Uh oh!

rolinh commented Jan 23, 2025

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

rolinh left a comment

Choose a reason for hiding this comment

Uh oh!

sayboras left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

youngnick commented Jan 20, 2025 •

edited

Loading