-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WIP] ✨ Add clusterctl changes for cross-ns CC ref #11395
base: main
Are you sure you want to change the base?
Changes from all commits
1dea02a
ccc32d1
65db336
5d9542b
b4b6131
e16aa76
3639364
a7d0322
fa2a857
865ffd1
e155375
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -100,6 +100,7 @@ func TestAddClusterClassIfMissing(t *testing.T) { | |
objs []client.Object | ||
clusterClassTemplateContent []byte | ||
targetNamespace string | ||
clusterClassNamespace string | ||
listVariablesOnly bool | ||
wantClusterClassInTemplate bool | ||
wantError bool | ||
|
@@ -114,6 +115,28 @@ func TestAddClusterClassIfMissing(t *testing.T) { | |
wantClusterClassInTemplate: true, | ||
wantError: false, | ||
}, | ||
{ | ||
name: "should add the cluster class from a different namespace to the template if cluster is not initialized", | ||
clusterInitialized: false, | ||
objs: []client.Object{}, | ||
targetNamespace: "ns1", | ||
clusterClassNamespace: "ns2", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we have a similar test when both namespaces are the same, and one where both are empty? |
||
clusterClassTemplateContent: clusterClassYAML("ns2", "dev"), | ||
listVariablesOnly: false, | ||
wantClusterClassInTemplate: true, | ||
wantError: false, | ||
}, | ||
{ | ||
name: "should add the cluster class form the same explicitly specified namespace to the template if cluster is not initialized", | ||
clusterInitialized: false, | ||
objs: []client.Object{}, | ||
targetNamespace: "ns1", | ||
clusterClassNamespace: "ns1", | ||
clusterClassTemplateContent: clusterClassYAML("ns1", "dev"), | ||
listVariablesOnly: false, | ||
wantClusterClassInTemplate: true, | ||
wantError: false, | ||
}, | ||
{ | ||
name: "should add the cluster class to the template if cluster is initialized and cluster class is not installed", | ||
clusterInitialized: true, | ||
|
@@ -189,17 +212,21 @@ func TestAddClusterClassIfMissing(t *testing.T) { | |
|
||
clusterClassClient := repository1.ClusterClasses("v1.0.0") | ||
|
||
clusterWithTopology := []byte(fmt.Sprintf("apiVersion: %s\n", clusterv1.GroupVersion.String()) + | ||
clusterWithTopology := fmt.Sprintf("apiVersion: %s\n", clusterv1.GroupVersion.String()) + | ||
"kind: Cluster\n" + | ||
"metadata:\n" + | ||
" name: cluster-dev\n" + | ||
fmt.Sprintf(" namespace: %s\n", tt.targetNamespace) + | ||
"spec:\n" + | ||
" topology:\n" + | ||
" class: dev") | ||
" class: dev" | ||
|
||
if tt.clusterClassNamespace != "" { | ||
clusterWithTopology = fmt.Sprintf("%s\n classNamespace: %s", clusterWithTopology, tt.clusterClassNamespace) | ||
} | ||
|
||
baseTemplate, err := repository.NewTemplate(repository.TemplateInput{ | ||
RawArtifact: clusterWithTopology, | ||
RawArtifact: []byte(clusterWithTopology), | ||
ConfigVariablesClient: test.NewFakeVariableClient(), | ||
Processor: yaml.NewSimpleProcessor(), | ||
TargetNamespace: tt.targetNamespace, | ||
|
@@ -210,13 +237,22 @@ func TestAddClusterClassIfMissing(t *testing.T) { | |
} | ||
|
||
g := NewWithT(t) | ||
template, err := addClusterClassIfMissing(ctx, baseTemplate, clusterClassClient, cluster, tt.targetNamespace, tt.listVariablesOnly) | ||
template, err := addClusterClassIfMissing(ctx, baseTemplate, clusterClassClient, cluster, tt.listVariablesOnly) | ||
if tt.wantError { | ||
g.Expect(err).To(HaveOccurred()) | ||
} else { | ||
if tt.wantClusterClassInTemplate { | ||
g.Expect(template.Objs()).To(ContainElement(MatchClusterClass("dev", tt.targetNamespace))) | ||
if tt.clusterClassNamespace == tt.targetNamespace { | ||
g.Expect(template.Objs()).To(ContainElement(MatchClusterClass("dev", tt.targetNamespace))) | ||
} else if tt.clusterClassNamespace != "" { | ||
g.Expect(template.Objs()).To(ContainElement(MatchClusterClass("dev", tt.clusterClassNamespace))) | ||
g.Expect(template.Objs()).ToNot(ContainElement(MatchClusterClass("dev", tt.targetNamespace))) | ||
} else { | ||
g.Expect(template.Objs()).To(ContainElement(MatchClusterClass("dev", tt.targetNamespace))) | ||
g.Expect(template.Objs()).ToNot(ContainElement(MatchClusterClass("dev", tt.clusterClassNamespace))) | ||
} | ||
} else { | ||
g.Expect(template.Objs()).NotTo(ContainElement(MatchClusterClass("dev", tt.clusterClassNamespace))) | ||
g.Expect(template.Objs()).NotTo(ContainElement(MatchClusterClass("dev", tt.targetNamespace))) | ||
} | ||
} | ||
|
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.clusterctl
side, or any existing template modifications.