✨ Add e2e tests & clusterctl changes for cross-ns CC ref #11395

Danil-Grigorev · 2024-11-11T10:19:06Z

What this PR does / why we need it:
This PR allows clusterctl to create a template with a reference to CC located in a different namespace.
This change allows to perform e2e tests in a multi-ns environment.

Added e2e tests to verify cluster-class quick-start scenario and runtime extension scenario in a cross-namespaced context.

The PR is built on top of:

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #5673

fabriziopandini · 2024-11-25T14:02:50Z

Renamed wip since it builds on a PR not yet merged

fabriziopandini

Made first pass, I will get back to this when the PR it builds on top are merged

test/e2e/data/infrastructure-docker/main/cluster-template-cross-ns-topology/cluster.yaml

fabriziopandini · 2024-11-25T14:19:28Z

test/e2e/cross-ns-quick-start.go

+// This test is meant to provide a first, fast signal to detect regression; it is recommended to use it as a PR blocker test.
+// NOTE: This test works with Clusters with and without ClusterClass.


Considering we are trying to keep PR-blocking duration as short as possible, and that the code paths touched by this change are limited and very specific to CC management, would it be possible to add this test on a PR only on demand + on periodic (once it is added to a PR, then it became blocking)

Currently using this as a way to quickly test subset of scenarios without having to invoke full-test-suite. I’ll unmark it from pre-submits. Should all added test cases be on demand + perodic?

test/e2e/cross-ns-quick-start.go

fabriziopandini · 2024-11-25T14:45:26Z

cmd/clusterctl/client/clusterclass.go

@@ -44,7 +46,7 @@ func addClusterClassIfMissing(ctx context.Context, template Template, clusterCla
 		return template, nil
 	}

-	clusterClassesTemplate, err := fetchMissingClusterClassTemplates(ctx, clusterClassClient, clusterClient, classes, targetNamespace, listVariablesOnly)
+	clusterClassesTemplate, err := fetchMissingClusterClassTemplates(ctx, clusterClassClient, clusterClient, classes, listVariablesOnly)


Just to check if I'm getting this right.
As of today, when we are deploying objects, clusterctl allows to override the namespace set into templates (use case: the user wants to deploy to namespace xyz instead of namespace original).

With this PR we are changing this behaviour, and always preserving the namespace from the template when we are adding the cluster class to the template being deployed (cc preserve: namespace original).

If this is correct, and if I'm not wrong, this change might break users that are deploying a Cluster with ClassNamespace empty, and are expecting that also the CC gets deployed in the target namespace xyz along side with the cluster.

It shouldn’t break these users, since regular process of overriding the namespace of the templated resources is still performed. This new field was never a part of this logic, and the SoT here is the template initially.

This change avoids problems in discovering the existence of the CC in the specified namespace, allowing to template only a Cluster resource.

There is no support in clusterclt to distinguish that some of the resources should go into A namespace, and some other into B, so this multi-namespace setup is intended to happen in a multiple passes.

ClusterClass and referenced templates

A cluster resource in a different namespace

repeat 2 for any namespace without failures on clusterctl side, or any existing template modifications.

I'm not entirely sure, but I think this shouldn't break anything.

The namespace-thing for cluster-templates is only envsubst, right?

(while for provider installations it's a lot more)

EDIT: I'll take that one back

My best guess is that it still works as good as before :)

Okay took another look, seems all good

fabriziopandini · 2024-11-25T14:48:15Z

cmd/clusterctl/client/clusterclass_test.go

+			clusterInitialized:          false,
+			objs:                        []client.Object{},
+			targetNamespace:             "ns1",
+			clusterClassNamespace:       "ns2",


Can we have a similar test when both namespaces are the same, and one where both are empty?

sbueringer

Initial round of review.

But let's wait a bit with adjustments. I have to talk to @fabriziopandini a bit about the test coverage we want / need

test/e2e/cluster_upgrade_runtimesdk.go

test/e2e/quick_start.go

- Add documentation on securing cross-namespace access for CC - Add ByClusterClassRef index - Support cross-ns CC rebase Signed-off-by: Danil-Grigorev <[email protected]>

Signed-off-by: Danil-Grigorev <[email protected]>

sbueringer · 2025-01-17T12:21:21Z

test/e2e/cluster_upgrade_runtimesdk.go

+			Byf("Deleting namespace used for hosting the %q test spec", specName)
+			framework.DeleteNamespace(ctx, framework.DeleteNamespaceInput{
+				Deleter: input.BootstrapClusterProxy.GetClient(),
+				Name:    clusterClassNamespace.Name,
+			})


Suggested change

Byf("Deleting namespace used for hosting the %q test spec", specName)

framework.DeleteNamespace(ctx, framework.DeleteNamespaceInput{

Deleter: input.BootstrapClusterProxy.GetClient(),

Name: clusterClassNamespace.Name,

})

Duplicate with below

sbueringer · 2025-01-17T12:21:47Z

test/e2e/cluster_upgrade_runtimesdk.go

+			})
+
+			if input.DeployClusterClassInSeparateNamespace {
+				Byf("Deleting namespace used for optionally hosting the %q infrastructure spec", specName)


Suggested change

Byf("Deleting namespace used for optionally hosting the %q infrastructure spec", specName)

Byf("Deleting namespace used for hosting the %q test spec ClusterClass", specName)

maybe

sbueringer · 2025-01-17T12:55:44Z

cmd/clusterctl/client/repository/template.go

@@ -172,10 +170,6 @@ func MergeTemplates(templates ...Template) (Template, error) {
 			}
 		}

-		if merged.targetNamespace != tmpl.TargetNamespace() {


I get why we need this change and I also saw that it doesn't lead to problems.

The way we are using this func now leads to an invalid merged.targetNamespace. Before this PR targetNamespace was correct because all templates are in the same namespace. Now merged.targetNamespace is not the namespace of all objects in the template anymore. This doesn't lead to problems because after we call MergeTemplates we only call YAML() and VariableMap() on the returned template, but this seems a bit brittle

Not sure what we should do, maybe rather not set the targetNamespace field if the templates are not all in the same namespace? Let's wait what @fabriziopandini thinks.

In any case, if we keep dropping this check here, please update the godoc of this func ("The merge operation returns an error if all the templates do not have the same TargetNamespace.").

sbueringer · 2025-01-17T13:07:35Z

cmd/clusterctl/cmd/util.go

@@ -111,6 +111,13 @@ func printVariablesOutput(template client.Template, options client.GetClusterTem
 			} else if val, ok := os.LookupEnv("KUBERNETES_VERSION"); ok {
 				variableMap[name] = ptr.To(val)
 			}
+		case "CLUSTER_CLASS_NAMESPACE":


comment above

// Fix up default for well-know variables that have a special logic implemented in clusterctl. // NOTE: this logic mimics the defaulting rules implemented in client.GetClusterTemplate;

Sounds like if we do this here we should also add the same in client.GetClusterTemplate? (which is actually in client/config.go templateOptionsToVariables)

@fabriziopandini WDYT?

sbueringer · 2025-01-17T15:08:29Z

test/e2e/cluster_upgrade_runtimesdk.go

@@ -95,6 +95,9 @@ type ClusterUpgradeWithRuntimeSDKSpecInput struct {
 	ExtensionServiceNamespace string
 	// ExtensionServiceName is the name of the service to configure in the test-namespace scoped ExtensionConfig.
 	ExtensionServiceName string
+
+	// DeployClusterClassInSeparateNamespace is set if a second namespace is required for the ClusterClass deployment


Suggested change

// DeployClusterClassInSeparateNamespace is set if a second namespace is required for the ClusterClass deployment

// DeployClusterClassInSeparateNamespace defines if the ClusterClass should be deployed in a separate namespace.

sbueringer · 2025-01-17T15:26:42Z

test/e2e/quick_start.go

+			variables["CLUSTER_CLASS_NAMESPACE"] = clusterClassNamespace.Name
+			By("Creating a cluster referencing a clusterClass from another namespace")
+		} else {
+			By("Creating a cluster referencing a clusterClass")


Suggested change

variables["CLUSTER_CLASS_NAMESPACE"] = clusterClassNamespace.Name

By("Creating a cluster referencing a clusterClass from another namespace")

} else {

By("Creating a cluster referencing a clusterClass")

variables["CLUSTER_CLASS_NAMESPACE"] = clusterClassNamespace.Name

By("Creating a cluster referencing a ClusterClass from another namespace")

} else {

By("Creating a cluster referencing a ClusterClass")

nit

Hm quickstart also works with clusters without ClusterClass. Do we need these logs? :)

sbueringer · 2025-01-17T15:27:56Z

test/e2e/quick_start.go

+		framework.DeleteNamespace(ctx, framework.DeleteNamespaceInput{
+			Deleter: input.BootstrapClusterProxy.GetClient(),
+			Name:    clusterClassNamespace.Name,
+		})


Suggested change

framework.DeleteNamespace(ctx, framework.DeleteNamespaceInput{

Deleter: input.BootstrapClusterProxy.GetClient(),

Name: clusterClassNamespace.Name,

})

if input.DeployClusterClassInSeparateNamespace {

framework.DeleteNamespace(ctx, framework.DeleteNamespaceInput{

Deleter: input.BootstrapClusterProxy.GetClient(),

Name: clusterClassNamespace.Name,

})

}

sbueringer · 2025-01-17T15:31:48Z

test/e2e/quick_start_test.go

@@ -120,6 +120,7 @@ var _ = Describe("When following the Cluster API quick-start with ClusterClass [
 				By("Checking that resourceVersions are stable")
 				framework.ValidateResourceVersionStable(ctx, proxy, namespace, clusterctlcluster.FilterClusterObjectsWithNameFilter(clusterName))
 			},
+			DeployClusterClassInSeparateNamespace: true,


Hm. I would like to preserve the existing test coverage here for C + CC in the same namespace. Which is the mainstream use case at the moment.

I'm fine with implementing the flag for the quickstart coverage, but I wonder if we have to run the test with the flag in core CAPI. The quickstart part of the test is a subset of what we test in the Runtime SDK test

test/e2e/cross-ns-quick-start.go

sbueringer · 2025-01-17T15:34:55Z

cmd/clusterctl/client/clusterclass.go

@@ -44,7 +46,7 @@ func addClusterClassIfMissing(ctx context.Context, template Template, clusterCla
 		return template, nil
 	}

-	clusterClassesTemplate, err := fetchMissingClusterClassTemplates(ctx, clusterClassClient, clusterClient, classes, targetNamespace, listVariablesOnly)
+	clusterClassesTemplate, err := fetchMissingClusterClassTemplates(ctx, clusterClassClient, clusterClient, classes, listVariablesOnly)


Okay took another look, seems all good

Signed-off-by: Danil-Grigorev <[email protected]>

k8s-ci-robot · 2025-01-17T18:59:31Z

@Danil-Grigorev: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-cluster-api-e2e-blocking-main	`d2a4e3b`	link	true	`/test pull-cluster-api-e2e-blocking-main`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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-sigs/prow repository. I understand the commands that are listed here.

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-area PR is missing an area label labels Nov 11, 2024

k8s-ci-robot requested a review from elmiko November 11, 2024 10:19

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Nov 11, 2024

k8s-ci-robot requested a review from jackfrancis November 11, 2024 10:19

Danil-Grigorev force-pushed the standardize-references-full branch 5 times, most recently from d92a5c2 to 7560e66 Compare November 12, 2024 11:33

Danil-Grigorev mentioned this pull request Nov 12, 2024

🐛 Use namespace of the reference on external.Get #11361

Merged

Danil-Grigorev changed the title ~~[WIP] ✨ E2E test for the classNamespace usage~~ [WIP] ✨ Add clusterctl changes for cross-ns CC ref Nov 12, 2024

Danil-Grigorev changed the title ~~[WIP] ✨ Add clusterctl changes for cross-ns CC ref~~ ✨ Add clusterctl changes for cross-ns CC ref Nov 12, 2024

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Nov 12, 2024

Danil-Grigorev force-pushed the standardize-references-full branch from 7560e66 to 09ce978 Compare November 18, 2024 19:46

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 18, 2024

fabriziopandini mentioned this pull request Nov 21, 2024

Enabling option to ignore namespaces #11193

Open

fabriziopandini changed the title ~~✨ Add clusterctl changes for cross-ns CC ref~~ [WIP] ✨ Add clusterctl changes for cross-ns CC ref Nov 25, 2024

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 25, 2024

fabriziopandini reviewed Nov 25, 2024

View reviewed changes

Danil-Grigorev force-pushed the standardize-references-full branch from 09ce978 to 2d4a64e Compare December 4, 2024 14:34

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 4, 2024

Danil-Grigorev force-pushed the standardize-references-full branch from 2d4a64e to c181e3e Compare December 4, 2024 15:22

k8s-ci-robot removed the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Dec 4, 2024

Danil-Grigorev force-pushed the standardize-references-full branch from 9b2cce5 to b2cbee3 Compare January 14, 2025 17:37

sbueringer reviewed Jan 14, 2025

View reviewed changes

test/e2e/cluster_upgrade_runtimesdk.go Outdated Show resolved Hide resolved

test/e2e/cluster_upgrade_runtimesdk.go Outdated Show resolved Hide resolved

test/e2e/quick_start.go Outdated Show resolved Hide resolved

Danil-Grigorev force-pushed the standardize-references-full branch 7 times, most recently from bb6604e to 9c1fec9 Compare January 16, 2025 12:55

Danil-Grigorev added 7 commits January 17, 2025 13:47

Add classNamespace to topology

ecdfa14

- Add documentation on securing cross-namespace access for CC - Add ByClusterClassRef index - Support cross-ns CC rebase Signed-off-by: Danil-Grigorev <[email protected]>

Add e2e test to cover crossNamespace provisionging

6244a71

Signed-off-by: Danil-Grigorev <[email protected]>

Prevent duplicate CC creation in template processing

f7141a8

Signed-off-by: Danil-Grigorev <[email protected]>

Test runtimextension integration

f9cd2f5

Signed-off-by: Danil-Grigorev <[email protected]>

Refactor to use empty CC namespace, add tests

bcb7482

Signed-off-by: Danil-Grigorev <[email protected]>

Add changes to CC rebase e2e test

35eb8e9

Signed-off-by: Danil-Grigorev <[email protected]>

Unify CLUSTER_CLASS_NAMESPACE to be set based on bool flag

0fca69c

Signed-off-by: Danil-Grigorev <[email protected]>

Danil-Grigorev force-pushed the standardize-references-full branch from 9c1fec9 to ce94b11 Compare January 17, 2025 13:17

Apply all changes on rebased clusterClass

0a130d3

Signed-off-by: Danil-Grigorev <[email protected]>

Danil-Grigorev force-pushed the standardize-references-full branch from ce94b11 to 0a130d3 Compare January 17, 2025 13:26

Danil-Grigorev changed the title ~~[WIP] ✨ Add clusterctl changes for cross-ns CC ref~~ ✨ Add clusterctl changes for cross-ns CC ref Jan 17, 2025

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 17, 2025

sbueringer changed the title ~~✨ Add clusterctl changes for cross-ns CC ref~~ ✨ Add e2e tests & clusterctl changes for cross-ns CC ref Jan 17, 2025

sbueringer added the area/clusterclass Issues or PRs related to clusterclass label Jan 17, 2025

k8s-ci-robot removed the do-not-merge/needs-area PR is missing an area label label Jan 17, 2025

sbueringer reviewed Jan 17, 2025

View reviewed changes

Danil-Grigorev force-pushed the standardize-references-full branch 2 times, most recently from dff6c42 to bf08d8d Compare January 17, 2025 18:25

e2e fixes

d2a4e3b

Signed-off-by: Danil-Grigorev <[email protected]>

Danil-Grigorev force-pushed the standardize-references-full branch from bf08d8d to d2a4e3b Compare January 17, 2025 18:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Add e2e tests & clusterctl changes for cross-ns CC ref #11395

✨ Add e2e tests & clusterctl changes for cross-ns CC ref #11395

Danil-Grigorev commented Nov 11, 2024 •

edited

Loading

fabriziopandini commented Nov 25, 2024

fabriziopandini left a comment

fabriziopandini Nov 25, 2024

Danil-Grigorev Jan 15, 2025

fabriziopandini Nov 25, 2024

Danil-Grigorev Dec 4, 2024

sbueringer Jan 14, 2025 •

edited

Loading

sbueringer Jan 14, 2025 •

edited

Loading

sbueringer Jan 17, 2025

fabriziopandini Nov 25, 2024

sbueringer left a comment

sbueringer Jan 17, 2025

sbueringer Jan 17, 2025

sbueringer Jan 17, 2025

sbueringer Jan 17, 2025

sbueringer Jan 17, 2025

sbueringer Jan 17, 2025

sbueringer Jan 17, 2025

sbueringer Jan 17, 2025

sbueringer Jan 17, 2025

sbueringer Jan 17, 2025

k8s-ci-robot commented Jan 17, 2025

		// This test is meant to provide a first, fast signal to detect regression; it is recommended to use it as a PR blocker test.
		// NOTE: This test works with Clusters with and without ClusterClass.

	Byf("Deleting namespace used for optionally hosting the %q infrastructure spec", specName)
	Byf("Deleting namespace used for hosting the %q test spec ClusterClass", specName)

	// DeployClusterClassInSeparateNamespace is set if a second namespace is required for the ClusterClass deployment
	// DeployClusterClassInSeparateNamespace defines if the ClusterClass should be deployed in a separate namespace.

✨ Add e2e tests & clusterctl changes for cross-ns CC ref #11395

Are you sure you want to change the base?

✨ Add e2e tests & clusterctl changes for cross-ns CC ref #11395

Conversation

Danil-Grigorev commented Nov 11, 2024 • edited Loading

fabriziopandini commented Nov 25, 2024

fabriziopandini left a 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

sbueringer Jan 14, 2025 • edited Loading

Choose a reason for hiding this comment

sbueringer Jan 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbueringer left a 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

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

k8s-ci-robot commented Jan 17, 2025

Danil-Grigorev commented Nov 11, 2024 •

edited

Loading

sbueringer Jan 14, 2025 •

edited

Loading

sbueringer Jan 14, 2025 •

edited

Loading