mutual-auth: Avoid confusion on mTLS wording #25761

sayboras · 2023-05-30T11:41:41Z

Description

Please refer to individual commits for more details.

Tasks

Copied from #24867

rename the field in NetworkPolicy to authentication (from auth)
rename the value in NetworkPolicy to spiffe from mtls-spiffe. Just removing the tls part removes any possibility of confusion here, and we'll address that spiffe uses TLS in other parts of the docs. Something like this is basically a must-do.
rename the existing auth stanza in Helm to authentication instead.
rename the current mTLS stanza in Helm to mutual instead. This means that the complete path will be authentication.mutual. To me, this leaves the authentication level with the possibility to add other types of authentication for Cilium to configure. I don't know if this is viable, or if we should collapse what's currently mTLS into authentication instead.
Change all command-line flags and config file options for cilium to use mutual, mutual-auth, or MutualAuth instead of mtls, mtls-auth, or MTLS respectively.
Refactor code to remove mTLS references (lowest priority).

Testing

Testing was done locally with cilium cli build from cilium/cilium-cli#1673

$ cilium connectivity test --test auth
...
[=] Test [echo-ingress-auth-always-fail]
........
[=] Test [echo-ingress-mutual-auth-spiffe]
........

[=] Skipping Test [pod-to-ingress-service]
...
[=] Skipping Test [to-fqdns]

✅ All 2 tests (16 actions) successful, 53 tests skipped, 0 scenarios skipped.

operator/auth/cell.go

meyskens

One nit but LGTM

mhofstetter

thanks for the renames @sayboras!

only one small concern regarding naming inline.

Documentation/cmdref/cilium-agent.md

Documentation/cmdref/cilium-operator.md

operator/auth/cell.go

youngnick

LGTM with that one change about using mesh-auth-mutual for command line flags instead of mesh-mutual-auth (just so the flags are all grouped together).

sayboras · 2023-05-30T13:29:04Z

/test

sayboras · 2023-06-02T09:45:02Z

/test

pkg/policy/l4.go

pkg/auth/cell.go

pkg/auth/disabled_authhandler.go

pkg/auth/mutual_authhandler.go

mhofstetter

LGTM - thanks for the additional changes 🥇

only one small non-blocking nit regarding the comment of alwaysPassAuthHandler

pkg/auth/always_pass_authhandler.go

bimmlerd

GH is somehow not letting me comment on the file, but tools/maptool/maptool is a binary, I'm guessing it was accidentially commited?

sayboras · 2023-06-02T12:20:00Z

GH is somehow not letting me comment on the file, but tools/maptool/maptool is a binary, I'm guessing it was accidentially commited?

ah thanks for pointing it out, it the left over file in old releases :( (probably due to my backport duty)

This commit is to make sure that the name and language for authentication are explicit. Relates: cilium#24867 Signed-off-by: Tam Mach <tam.mach@cilium.io>

This commit is to rename `auth` attribute in both Ingress and Egress rules to `authentication` for more clarity. Also, `type` attribute is renamed to `mode` with applicable values as disabled, required and test-always-fail. Relates: cilium#24867 Signed-off-by: Tam Mach <tam.mach@cilium.io>

This commit is to change all command-line flags and config file options for cilium to use mutual, mutual-auth, or MutualAuth instead of mtls, mtls-auth, or MTLS respectively. Signed-off-by: Tam Mach <tam.mach@cilium.io>

Signed-off-by: Tam Mach <tam.mach@cilium.io>

sayboras · 2023-06-02T12:23:15Z

/test

bimmlerd

contributing review requirement is gone now, so I can :disappear:

qmonnet

Looks all good from my side, thanks!

Documentation/operations/upgrade.rst

aanm · 2023-06-05T09:46:06Z

The majority of the codeowneres reviewed the PR. Merging since it's blocking a lot of other PRs and this PR is simply a name refactoring.

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

sayboras added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label May 30, 2023

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

sayboras added the area/servicemesh GH issues or PRs regarding servicemesh label May 30, 2023

sayboras force-pushed the tam/auth-terminology branch 4 times, most recently from f3a8b36 to 7c6a486 Compare May 30, 2023 12:35

sayboras marked this pull request as ready for review May 30, 2023 12:35

sayboras requested review from a team as code owners May 30, 2023 12:35

sayboras requested review from nathanjsweet, meyskens, joamaki, kaworu and zacharysarah May 30, 2023 12:35

meyskens reviewed May 30, 2023

View reviewed changes

operator/auth/cell.go Outdated Show resolved Hide resolved

meyskens approved these changes May 30, 2023

View reviewed changes

mhofstetter reviewed May 30, 2023

View reviewed changes

Documentation/cmdref/cilium-agent.md Outdated Show resolved Hide resolved

Documentation/cmdref/cilium-operator.md Outdated Show resolved Hide resolved

youngnick reviewed May 30, 2023

View reviewed changes

operator/auth/cell.go Outdated Show resolved Hide resolved

youngnick approved these changes May 30, 2023

View reviewed changes

sayboras force-pushed the tam/auth-terminology branch from 7c6a486 to 474bd06 Compare May 30, 2023 13:28

sayboras force-pushed the tam/auth-terminology branch from 0fe3851 to f572c62 Compare June 2, 2023 09:13

sayboras marked this pull request as ready for review June 2, 2023 10:07

sayboras requested review from meyskens, mhofstetter and youngnick June 2, 2023 10:07

mhofstetter requested changes Jun 2, 2023

View reviewed changes

joamaki approved these changes Jun 2, 2023

View reviewed changes

sayboras force-pushed the tam/auth-terminology branch from f572c62 to de7e5b2 Compare June 2, 2023 11:04

sayboras requested a review from a team as a code owner June 2, 2023 11:04

sayboras requested review from bimmlerd and mhofstetter June 2, 2023 11:04

sayboras force-pushed the tam/auth-terminology branch from de7e5b2 to 3e23a32 Compare June 2, 2023 11:07

mhofstetter approved these changes Jun 2, 2023

View reviewed changes

pkg/auth/always_pass_authhandler.go Outdated Show resolved Hide resolved

bimmlerd requested changes Jun 2, 2023

View reviewed changes

sayboras added 4 commits June 2, 2023 22:21

helm: Update the terminology for authentication

97e8c16

This commit is to make sure that the name and language for authentication are explicit. Relates: cilium#24867 Signed-off-by: Tam Mach <tam.mach@cilium.io>

operator/agent: Rename mtls command line flags options

dabb34e

This commit is to change all command-line flags and config file options for cilium to use mutual, mutual-auth, or MutualAuth instead of mtls, mtls-auth, or MTLS respectively. Signed-off-by: Tam Mach <tam.mach@cilium.io>

operator/agent: Refactor to remove mTLS references

acdfea3

Signed-off-by: Tam Mach <tam.mach@cilium.io>

sayboras force-pushed the tam/auth-terminology branch from 3e23a32 to acdfea3 Compare June 2, 2023 12:22

sayboras requested a review from bimmlerd June 2, 2023 12:23

bimmlerd approved these changes Jun 2, 2023

View reviewed changes

qmonnet approved these changes Jun 5, 2023

View reviewed changes

Documentation/operations/upgrade.rst Show resolved Hide resolved

aanm approved these changes Jun 5, 2023

View reviewed changes

aanm merged commit afb6b2b into cilium:main Jun 5, 2023
61 checks passed

sayboras deleted the tam/auth-terminology branch June 5, 2023 09:46

sayboras mentioned this pull request Jun 5, 2023

Confusing name for CNP authentication type #24867

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mutual-auth: Avoid confusion on mTLS wording #25761

mutual-auth: Avoid confusion on mTLS wording #25761

sayboras commented May 30, 2023 •

edited

meyskens left a comment

mhofstetter left a comment

youngnick left a comment

sayboras commented May 30, 2023

sayboras commented Jun 2, 2023

mhofstetter left a comment

bimmlerd left a comment

sayboras commented Jun 2, 2023

sayboras commented Jun 2, 2023

bimmlerd left a comment

qmonnet left a comment

aanm commented Jun 5, 2023

mutual-auth: Avoid confusion on mTLS wording #25761

mutual-auth: Avoid confusion on mTLS wording #25761

Conversation

sayboras commented May 30, 2023 • edited

Description

Tasks

Testing

meyskens left a comment

Choose a reason for hiding this comment

mhofstetter left a comment

Choose a reason for hiding this comment

youngnick left a comment

Choose a reason for hiding this comment

sayboras commented May 30, 2023

sayboras commented Jun 2, 2023

mhofstetter left a comment

Choose a reason for hiding this comment

bimmlerd left a comment

Choose a reason for hiding this comment

sayboras commented Jun 2, 2023

sayboras commented Jun 2, 2023

bimmlerd left a comment

Choose a reason for hiding this comment

qmonnet left a comment

Choose a reason for hiding this comment

aanm commented Jun 5, 2023

sayboras commented May 30, 2023 •

edited