Add conformance test flag to set implementation labels on mesh namespaces #1957

frankbu · 2023-04-21T18:43:08Z

What type of PR is this?

/test
/area conformance

What this PR does / why we need it:

Some mesh implementations require namespaces that are part of the mesh to include implementation-specific labels. For example, Istio requires an istio-injection=enabled label on mesh namespaces to turn on sidecar injection.

This PR adds a new flag that can be used to pass in labels that should be added to namespaces where mesh-enabled services are deployed (e.g., --namespace-labels "istio-injection=enabled" can be set to run on Istio).

This new flag is implemented using the existing NamespaceLabels field in suite.Options. This PR is only making it settable via the command line.

Note that this PR also includes a small fix in pod.go to check that the expected backends are being called. Without this fix, the split-mesh test isn't actually confirming that the correct backends are called, just that some backend has been called.

Which issue(s) this PR fixes:

Fixes #

Does this PR introduce a user-facing change?:

NONE

k8s-ci-robot · 2023-04-21T18:43:16Z

Hi @frankbu. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot · 2023-04-21T18:43:22Z

@frankbu: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

What type of PR is this?

/test
/area conformance

What this PR does / why we need it:

Some mesh implementations require namespaces that are part of the mesh to include implementation-specific labels. For example, Istio requires an istio-injection=enabled label on mesh namespaces to turn on sidecar injection.

This PR adds a new flag that can be used to pass in labels that should be added to namespaces where mesh-enabled services are deployed (e.g., --mesh-namespace-labels "istio-injection=enabled" can be set to run on Istio).

This PR is currently adding the extra label(s) to namespaces that have a gateway-conformance: mesh label, which may not be ideal, depending on exactly what gateway-conformance=mesh is supposed to represent. Thoughts?

Which issue(s) this PR fixes:

Fixes #

Does this PR introduce a user-facing change?:
NONE

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

howardjohn · 2023-04-24T13:14:46Z

conformance/utils/suite/suite.go

+
+	// MeshNamespaceLabels are implentation-specific labels that should be added
+	// to namespaces where mesh-enabled services will be deployed.
+	MeshNamespaceLabels map[string]string


Why do we need this when we already have NamespaceLabels? It only applies to "mesh workload" namespaces not everywhere?

It only applies to "mesh workload" namespaces not everywhere?

Yes, that is the difference.

It wasn't clear to me what the intended use of NamespaceLabels is, so I added another field. What kind of labels would we want to add to ALL namespaces, mesh or not?

If I'm understanding correctly we expect that there will be tests where some but not all namespaces will be included in the mesh?

@shaneutt, yes, that is my assumption. Mesh egress tests, for example.

Actually the more general problem is that if one wants to run both the Gateway and Mesh tests, there is no way to label only the mesh namespace(s), but not the ones hosting plain k8s services that the Gateway/Route tests use. Maybe that's what we want? When running Mesh and Gateway tests together, the Gateway tests switch to mesh-ingress (instead of k8s-ingress) tests?

@frankbu This seems like something that's maybe a bit too implementation-specific to include in our conformance test helpers unless I'm missing something. Would it be possible to just have mesh tests runs separately from ingress tests?

@robscott, yes, absolutely, I think that would be fine. The only potential issue is if we have mesh tests that want to call services outside of the mesh (egress testing). In Istio, we use a "non-mesh" namespace to run services that simulate external services for this kind of testing.

@robscott @shaneutt I removed MeshNamespaceLabels and just use the existing NamespaceLabels for now. If in the future we need to differentiate mesh/not-mesh namespaces we can figure out a way to do it then.

So now this PR is only adding a flag to set NamespaceLabels and fixing up pod.go to properly check that the version routing test is actually calling the expected backends.

robscott · 2023-05-05T16:58:33Z

@frankbu can you update the PR description to match what we've settled on? It looks like one of your latest commits caused CLA to fail, so that will also need to be fixed before this can be merged.

/ok-to-test

frankbu · 2023-05-05T17:28:21Z

/easycla

frankbu · 2023-05-08T17:29:43Z

/easycla

frankbu · 2023-05-12T18:23:06Z

@robscott I added the unit tests you requested. Are there any other issues holding it up?

robscott · 2023-05-12T18:26:32Z

Hey @frankbu we're effectively frozen right now until v0.7.0 releases, but we're expecting that to happen on Monday.

robscott

Thanks @frankbu! This LGTM other than one tiny nit + needing a rebase.

/hold cancel

robscott · 2023-05-16T22:59:21Z

conformance/utils/suite/suite_test.go

@@ -0,0 +1,69 @@
+/*
+Copyright 2022 The Kubernetes Authors.


Tiny nit:

Suggested change

Copyright 2022 The Kubernetes Authors.

Copyright 2023 The Kubernetes Authors.

frankbu · 2023-05-17T13:45:54Z

@robscott should be good to go now, thanks.

robscott · 2023-05-17T16:54:29Z

Thanks @frankbu!

/lgtm
/approve

k8s-ci-robot · 2023-05-17T16:54:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: frankbu, robscott

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:

~~conformance/OWNERS~~ [robscott]
~~site-src/OWNERS~~ [robscott]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Add labels to mesh namespaces

762f129

k8s-ci-robot added area/conformance cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 21, 2023

k8s-ci-robot requested review from howardjohn and mikemorris April 21, 2023 18:43

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 21, 2023

howardjohn reviewed Apr 24, 2023

View reviewed changes

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. and removed cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 24, 2023

shaneutt added this to the v0.7.1 milestone Apr 24, 2023

fix merge conflict

3350608

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Apr 24, 2023

add doc

88f695c

shaneutt requested review from shaneutt and howardjohn April 24, 2023 23:06

frankbu added 2 commits April 26, 2023 14:37

remove MeshNamespaceLabels

aed0bea

one more

48c00c5

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. and removed cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 27, 2023

frankbu mentioned this pull request May 5, 2023

mesh: mark GEPs as Implementable #1994

Merged

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 5, 2023

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels May 5, 2023

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label May 8, 2023

add unit tests

5444084

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 16, 2023

robscott reviewed May 16, 2023

View reviewed changes

merge conflict

4f346aa

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 17, 2023

robscott added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label May 17, 2023

k8s-ci-robot assigned robscott May 17, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 17, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 17, 2023

k8s-ci-robot merged commit 123fe3c into kubernetes-sigs:main May 17, 2023

shaneutt mentioned this pull request Jul 24, 2023

shaneutt/1822 shaneutt/gateway-api#3

Closed

shaneutt mentioned this pull request Aug 7, 2023

update owner aliases shaneutt/gateway-api#4

Closed

frankbu mentioned this pull request Sep 15, 2023

REQUEST: New membership for @frankbu kubernetes/org#4466

Closed

9 tasks

shaneutt mentioned this pull request Sep 18, 2023

shaneutt/conformance tests for gateway addresses shaneutt/gateway-api#5

Closed

frankbu deleted the mesh-labels branch September 25, 2023 14:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add conformance test flag to set implementation labels on mesh namespaces #1957

Add conformance test flag to set implementation labels on mesh namespaces #1957

frankbu commented Apr 21, 2023 •

edited

Loading

k8s-ci-robot commented Apr 21, 2023

k8s-ci-robot commented Apr 21, 2023

howardjohn Apr 24, 2023

frankbu Apr 24, 2023

shaneutt Apr 25, 2023

frankbu Apr 25, 2023

frankbu Apr 25, 2023

robscott Apr 25, 2023

frankbu Apr 26, 2023

frankbu Apr 26, 2023

robscott commented May 5, 2023

frankbu commented May 5, 2023

frankbu commented May 8, 2023

frankbu commented May 12, 2023

robscott commented May 12, 2023

robscott left a comment

robscott May 16, 2023

frankbu commented May 17, 2023

robscott commented May 17, 2023

k8s-ci-robot commented May 17, 2023

	Copyright 2022 The Kubernetes Authors.
	Copyright 2023 The Kubernetes Authors.

Add conformance test flag to set implementation labels on mesh namespaces #1957

Add conformance test flag to set implementation labels on mesh namespaces #1957

Conversation

frankbu commented Apr 21, 2023 • edited Loading

k8s-ci-robot commented Apr 21, 2023

k8s-ci-robot commented Apr 21, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robscott commented May 5, 2023

frankbu commented May 5, 2023

frankbu commented May 8, 2023

frankbu commented May 12, 2023

robscott commented May 12, 2023

robscott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frankbu commented May 17, 2023

robscott commented May 17, 2023

k8s-ci-robot commented May 17, 2023

frankbu commented Apr 21, 2023 •

edited

Loading