-
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
✨ Add e2e tests & clusterctl changes for cross-ns CC ref #11395
base: main
Are you sure you want to change the base?
Changes from all commits
ecdfa14
6244a71
f7141a8
f9cd2f5
bcb7482
35eb8e9
0fca69c
0a130d3
d2a4e3b
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))) | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,8 +17,6 @@ limitations under the License. | |
package repository | ||
|
||
import ( | ||
"fmt" | ||
|
||
"github.com/pkg/errors" | ||
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" | ||
"k8s.io/apimachinery/pkg/util/sets" | ||
|
@@ -172,10 +170,6 @@ func MergeTemplates(templates ...Template) (Template, error) { | |
} | ||
} | ||
|
||
if merged.targetNamespace != tmpl.TargetNamespace() { | ||
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. 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."). |
||
return nil, fmt.Errorf("cannot merge templates with different targetNamespaces") | ||
} | ||
|
||
merged.objs = append(merged.objs, tmpl.Objs()...) | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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": | ||
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. comment above
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? |
||
// Namespace name from the cmd flags or from the kubeconfig is used instead of template default. | ||
if val, ok := os.LookupEnv("CLUSTER_CLASS_NAMESPACE"); ok { | ||
variableMap[name] = ptr.To(val) | ||
} else { | ||
variableMap[name] = ptr.To("") | ||
} | ||
} | ||
|
||
if variableMap[name] != nil { | ||
|
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.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.
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
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.
My best guess is that it still works as good as before :)
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.
Okay took another look, seems all good