Prepend Envoy resources with CEC namespace and name #21500

pippolo84 · 2022-09-29T11:15:24Z

This PR changes the naming of Envoy Listeners and Routes resources.
To avoid possible name clashing between resources in different namespaces, each Envoy Listener and Route name is prepended with its CEC namespace and name.
This is done while translating the CEC specifications to generate the Envoy config, and not during CEC creation. Thanks to this, a user generated CEC does not need to use the format to be similar to an operator-generated CEC.

Relates #19698 #20655

maintainer-s-little-helper · 2022-09-29T15:17:35Z

Commit be4fb78fa82352bc3697521887df29ff45e2af43 does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

maintainer-s-little-helper · 2022-09-29T15:38:47Z

Commit 826d2f53ebdffe009d2ea8e8ccd167ca9cd94fe2 does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

sayboras

one minor comment/suggestion on calling GetName() and GetNamespace() directly, the rest looks good to me.

Also, I just want to highlight one thing. While the diff in this PR looks simple, there are a lot of efforts from Fabio on this work 🥇

pkg/k8s/watchers/cilium_envoy_config.go

sayboras · 2022-10-05T10:43:48Z

One more comment I have is to just take a quick on existing docs, and make any update to shorten the names if there is any place we create CiliumEnvoyConfig manually. This could be done in subsequent PR.

pippolo84 · 2022-10-05T12:18:57Z

one minor comment/suggestion on calling GetName() and GetNamespace() directly, the rest looks good to me.

Also, I just want to highlight one thing. While the diff in this PR looks simple, there are a lot of efforts from Fabio on this work 1st_place_medal

I'm gonna fix the nits. Thank you for the kind words and all the support!

sayboras · 2022-10-08T12:07:24Z

/test

sayboras · 2022-10-10T10:48:41Z

/test-1.25-net-next

julianwiedmann · 2022-10-11T06:32:01Z

FYI, resolving the failures in 1.25-net-next requires a rebase. There are some new/updated tests that depend on recent cilium changes (ie. another case of #15474).

pippolo84 · 2022-10-11T08:22:36Z

FYI, resolving the failures in 1.25-net-next requires a rebase. There are some new/updated tests that depend on recent cilium changes (ie. another case of #15474).

Oh, nice, thank you.

Avoid specifying the listener name in the ServiceListener. Leaving it empty, Envoy will automatically pick the first one available. Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>

Simplify the naming of listeners in CEC, removing both CEC namespace and name. This is done to avoid differences between CECs generated by the operator and handcrafted CECs. Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>

pippolo84 · 2022-10-11T10:21:30Z

/test

pippolo84 · 2022-10-11T12:05:40Z

/test-1.16-4.9

pippolo84 · 2022-10-11T12:05:56Z

/test-1.23-5.4

aanm

@sayboras will this change the CECs names? If yes do we need to worry about upgrades?

aanm · 2022-10-11T14:05:13Z

pkg/envoy/ciliumenvoyconfig.go

+// resourceQualifiedName returns the qualified name of an Envoy resource,
+// prepending CEC namespace and CEC name to the resource name and using
+// "-" as a separator.
+// In case of an empty CEC namespace or an empty CEC name, leading separators
+// are stripped away.
+func resourceQualifiedName(namespace, name, resource string) string {
+	var sb strings.Builder
+
+	if namespace != "" {
+		sb.WriteString(namespace)
+		sb.WriteRune('-')
+	}
+	if name != "" {
+		sb.WriteString(name)
+		sb.WriteRune('-')
+	}
+	sb.WriteString(resource)
+
+	return sb.String()
+}


The resourceQualifiedName will not work in the case that one resource has the same namespace name as the other resource:
resource-1 namespace: foo
resource-1 name: bar
resource-1 resource: hello

resource-2 namespace:
resource-2 name: foo-bar
resource-2 resource: hello

Ouch. What about using two different separators, like namespace/name-resource? That should do the trick.

the above example is kind of escaped bug we have before (e.g. in 1.12), I noticed about it a little bit later 1.12 release.

Due to the below comment, changing listener name will cause some packet drop, so we can either:

keep the same name as 1.12 so that upgrade will work, or

change separator convention to fix the potential name overlapping bug

Prepend CEC namespace and name to Envoy Listeners and Routes resources. This will avoid name clashing between resources that belong to different namespaces. Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>

pippolo84 · 2022-10-17T12:20:27Z

/test

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 Sep 29, 2022

pippolo84 changed the title ~~Prepend Envoy resources with CEC namespace and name~~ [WIP] Prepend Envoy resources with CEC namespace and name Sep 29, 2022

maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Sep 29, 2022

pippolo84 force-pushed the pr/pippolo84/envoy-resources-prefix branch from f458fdc to eb2b98d Compare September 29, 2022 15:38

pippolo84 force-pushed the pr/pippolo84/envoy-resources-prefix branch from eb2b98d to 6c475f1 Compare October 3, 2022 14:03

maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Oct 3, 2022

pippolo84 force-pushed the pr/pippolo84/envoy-resources-prefix branch 3 times, most recently from ad74ecb to b24235b Compare October 4, 2022 15:35

pippolo84 added area/proxy Impacts proxy components, including DNS, Kafka, Envoy and/or XDS servers. release-note/minor This PR changes functionality that users may find relevant to operating Cilium. area/servicemesh GH issues or PRs regarding servicemesh labels Oct 4, 2022

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 Oct 4, 2022

pippolo84 marked this pull request as ready for review October 4, 2022 21:11

pippolo84 requested review from a team as code owners October 4, 2022 21:11

pippolo84 requested review from sayboras and nebril October 4, 2022 21:11

pippolo84 changed the title ~~[WIP] Prepend Envoy resources with CEC namespace and name~~ Prepend Envoy resources with CEC namespace and name Oct 4, 2022

sayboras reviewed Oct 5, 2022

View reviewed changes

pkg/k8s/watchers/cilium_envoy_config.go Outdated Show resolved Hide resolved

pkg/k8s/watchers/cilium_envoy_config.go Outdated Show resolved Hide resolved

pkg/k8s/watchers/cilium_envoy_config.go Outdated Show resolved Hide resolved

sayboras approved these changes Oct 5, 2022

View reviewed changes

This was referenced Oct 5, 2022

Service Mesh follow-ups after 1.12 release #19698

Closed

CFP: Add support for k8s Gateway API #20655

Closed

pippolo84 force-pushed the pr/pippolo84/envoy-resources-prefix branch from b24235b to a98bdd5 Compare October 5, 2022 12:32

sayboras added this to the v1.13.0-rc2 milestone Oct 9, 2022

aanm self-requested a review October 10, 2022 13:26

pippolo84 added 2 commits October 11, 2022 10:23

ingress/translation: Do not specify listener name

7ec8d0a

Avoid specifying the listener name in the ServiceListener. Leaving it empty, Envoy will automatically pick the first one available. Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>

pippolo84 force-pushed the pr/pippolo84/envoy-resources-prefix branch from a98bdd5 to d9abdf3 Compare October 11, 2022 08:31

nebril approved these changes Oct 11, 2022

View reviewed changes

aanm requested changes Oct 11, 2022

View reviewed changes

pippolo84 force-pushed the pr/pippolo84/envoy-resources-prefix branch from d9abdf3 to 59988a7 Compare October 11, 2022 14:38

pippolo84 requested a review from aanm October 11, 2022 16:30

envoy: Prepend CEC namespace and name to resources

eaea7f4

Prepend CEC namespace and name to Envoy Listeners and Routes resources. This will avoid name clashing between resources that belong to different namespaces. Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>

pippolo84 force-pushed the pr/pippolo84/envoy-resources-prefix branch from 59988a7 to eaea7f4 Compare October 17, 2022 08:31

aanm approved these changes Oct 17, 2022

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 Oct 17, 2022

qmonnet merged commit ae16e2b into cilium:master Oct 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prepend Envoy resources with CEC namespace and name #21500

Prepend Envoy resources with CEC namespace and name #21500

pippolo84 commented Sep 29, 2022 •

edited

Loading

maintainer-s-little-helper bot commented Sep 29, 2022

maintainer-s-little-helper bot commented Sep 29, 2022

sayboras left a comment

sayboras commented Oct 5, 2022 •

edited

Loading

pippolo84 commented Oct 5, 2022

sayboras commented Oct 8, 2022

sayboras commented Oct 10, 2022

julianwiedmann commented Oct 11, 2022

pippolo84 commented Oct 11, 2022

pippolo84 commented Oct 11, 2022

pippolo84 commented Oct 11, 2022

pippolo84 commented Oct 11, 2022

aanm left a comment

aanm Oct 11, 2022

pippolo84 Oct 11, 2022

sayboras Oct 11, 2022

pippolo84 commented Oct 17, 2022

Prepend Envoy resources with CEC namespace and name #21500

Prepend Envoy resources with CEC namespace and name #21500

Conversation

pippolo84 commented Sep 29, 2022 • edited Loading

maintainer-s-little-helper bot commented Sep 29, 2022

maintainer-s-little-helper bot commented Sep 29, 2022

sayboras left a comment

Choose a reason for hiding this comment

sayboras commented Oct 5, 2022 • edited Loading

pippolo84 commented Oct 5, 2022

sayboras commented Oct 8, 2022

sayboras commented Oct 10, 2022

julianwiedmann commented Oct 11, 2022

pippolo84 commented Oct 11, 2022

pippolo84 commented Oct 11, 2022

pippolo84 commented Oct 11, 2022

pippolo84 commented Oct 11, 2022

aanm left a comment

Choose a reason for hiding this comment

aanm Oct 11, 2022

Choose a reason for hiding this comment

pippolo84 Oct 11, 2022

Choose a reason for hiding this comment

sayboras Oct 11, 2022

Choose a reason for hiding this comment

pippolo84 commented Oct 17, 2022

pippolo84 commented Sep 29, 2022 •

edited

Loading

sayboras commented Oct 5, 2022 •

edited

Loading