Skip to content

Commit 5b7d26f

Browse files
Danil-GrigorevDanil-Grigorev
committed
Prevent duplicate CC creation in template processing
Signed-off-by: Danil-Grigorev <[email protected]>
1 parent 272bcf4 commit 5b7d26f

File tree

6 files changed

+43
-24
lines changed

6 files changed

+43
-24
lines changed

‎cmd/clusterctl/client/clusterclass.go

Lines changed: 11 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,8 @@ import (
2323
"github.com/pkg/errors"
2424
apierrors "k8s.io/apimachinery/pkg/api/errors"
2525
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
26+
"k8s.io/apimachinery/pkg/types"
27+
"k8s.io/klog/v2"
2628
"sigs.k8s.io/controller-runtime/pkg/client"
2729

2830
clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
@@ -34,7 +36,7 @@ import (
3436
// addClusterClassIfMissing returns a Template that includes the base template and adds any cluster class definitions that
3537
// are references in the template. If the cluster class referenced already exists in the cluster it is not added to the
3638
// template.
37-
func addClusterClassIfMissing(ctx context.Context, template Template, clusterClassClient repository.ClusterClassClient, clusterClient cluster.Client, targetNamespace string, listVariablesOnly bool) (Template, error) {
39+
func addClusterClassIfMissing(ctx context.Context, template Template, clusterClassClient repository.ClusterClassClient, clusterClient cluster.Client, listVariablesOnly bool) (Template, error) {
3840
classes, err := clusterClassNamesFromTemplate(template)
3941
if err != nil {
4042
return nil, err
@@ -44,7 +46,7 @@ func addClusterClassIfMissing(ctx context.Context, template Template, clusterCla
4446
return template, nil
4547
}
4648

47-
clusterClassesTemplate, err := fetchMissingClusterClassTemplates(ctx, clusterClassClient, clusterClient, classes, targetNamespace, listVariablesOnly)
49+
clusterClassesTemplate, err := fetchMissingClusterClassTemplates(ctx, clusterClassClient, clusterClient, classes, listVariablesOnly)
4850
if err != nil {
4951
return nil, err
5052
}
@@ -62,8 +64,8 @@ func addClusterClassIfMissing(ctx context.Context, template Template, clusterCla
6264
// clusterClassNamesFromTemplate returns the list of ClusterClasses referenced
6365
// by clusters defined in the template. If not clusters are defined in the template
6466
// or if no cluster uses a cluster class it returns an empty list.
65-
func clusterClassNamesFromTemplate(template Template) ([]string, error) {
66-
classes := []string{}
67+
func clusterClassNamesFromTemplate(template Template) ([]types.NamespacedName, error) {
68+
classes := []types.NamespacedName{}
6769

6870
// loop through all the objects and if the object is a cluster
6971
// check and see if cluster.spec.topology.class is defined.
@@ -80,14 +82,14 @@ func clusterClassNamesFromTemplate(template Template) ([]string, error) {
8082
if cluster.Spec.Topology == nil {
8183
continue
8284
}
83-
classes = append(classes, cluster.GetClassKey().Name)
85+
classes = append(classes, cluster.GetClassKey())
8486
}
8587
return classes, nil
8688
}
8789

8890
// fetchMissingClusterClassTemplates returns a list of templates for ClusterClasses that do not yet exist
8991
// in the cluster. If the cluster is not initialized, all the ClusterClasses are added.
90-
func fetchMissingClusterClassTemplates(ctx context.Context, clusterClassClient repository.ClusterClassClient, clusterClient cluster.Client, classes []string, targetNamespace string, listVariablesOnly bool) (Template, error) {
92+
func fetchMissingClusterClassTemplates(ctx context.Context, clusterClassClient repository.ClusterClassClient, clusterClient cluster.Client, classes []types.NamespacedName, listVariablesOnly bool) (Template, error) {
9193
// first check if the cluster is initialized.
9294
// If it is initialized:
9395
// For every ClusterClass check if it already exists in the cluster.
@@ -118,7 +120,7 @@ func fetchMissingClusterClassTemplates(ctx context.Context, clusterClassClient r
118120
templates := []repository.Template{}
119121
for _, class := range classes {
120122
if clusterInitialized {
121-
exists, err := clusterClassExists(ctx, c, class, targetNamespace)
123+
exists, err := clusterClassExists(ctx, c, class.Name, class.Namespace)
122124
if err != nil {
123125
return nil, err
124126
}
@@ -128,7 +130,7 @@ func fetchMissingClusterClassTemplates(ctx context.Context, clusterClassClient r
128130
}
129131
// The cluster is either not initialized or the ClusterClass does not yet exist in the cluster.
130132
// Fetch the cluster class to install.
131-
clusterClassTemplate, err := clusterClassClient.Get(ctx, class, targetNamespace, listVariablesOnly)
133+
clusterClassTemplate, err := clusterClassClient.Get(ctx, class.Name, class.Namespace, listVariablesOnly)
132134
if err != nil {
133135
return nil, errors.Wrapf(err, "failed to get the cluster class template for %q", class)
134136
}
@@ -142,7 +144,7 @@ func fetchMissingClusterClassTemplates(ctx context.Context, clusterClassClient r
142144
if exists, err := objExists(ctx, c, obj); err != nil {
143145
return nil, err
144146
} else if exists {
145-
return nil, fmt.Errorf("%s(%s) already exists in the cluster", obj.GetName(), obj.GetObjectKind().GroupVersionKind())
147+
return nil, fmt.Errorf("%s(%s) already exists in the cluster", klog.KObj(&obj), obj.GetObjectKind().GroupVersionKind())
146148
}
147149
}
148150
}

‎cmd/clusterctl/client/clusterclass_test.go

Lines changed: 28 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,7 @@ func TestAddClusterClassIfMissing(t *testing.T) {
100100
objs []client.Object
101101
clusterClassTemplateContent []byte
102102
targetNamespace string
103+
clusterClassNamespace string
103104
listVariablesOnly bool
104105
wantClusterClassInTemplate bool
105106
wantError bool
@@ -114,6 +115,17 @@ func TestAddClusterClassIfMissing(t *testing.T) {
114115
wantClusterClassInTemplate: true,
115116
wantError: false,
116117
},
118+
{
119+
name: "should add the cluster class from a different namespace to the template if cluster is not initialized",
120+
clusterInitialized: false,
121+
objs: []client.Object{},
122+
targetNamespace: "ns1",
123+
clusterClassNamespace: "ns2",
124+
clusterClassTemplateContent: clusterClassYAML("ns2", "dev"),
125+
listVariablesOnly: false,
126+
wantClusterClassInTemplate: true,
127+
wantError: false,
128+
},
117129
{
118130
name: "should add the cluster class to the template if cluster is initialized and cluster class is not installed",
119131
clusterInitialized: true,
@@ -189,17 +201,21 @@ func TestAddClusterClassIfMissing(t *testing.T) {
189201

190202
clusterClassClient := repository1.ClusterClasses("v1.0.0")
191203

192-
clusterWithTopology := []byte(fmt.Sprintf("apiVersion: %s\n", clusterv1.GroupVersion.String()) +
204+
clusterWithTopology := fmt.Sprintf("apiVersion: %s\n", clusterv1.GroupVersion.String()) +
193205
"kind: Cluster\n" +
194206
"metadata:\n" +
195207
" name: cluster-dev\n" +
196208
fmt.Sprintf(" namespace: %s\n", tt.targetNamespace) +
197209
"spec:\n" +
198210
" topology:\n" +
199-
" class: dev")
211+
" class: dev"
212+
213+
if tt.clusterClassNamespace != "" {
214+
clusterWithTopology = fmt.Sprintf("%s\n classNamespace: %s", clusterWithTopology, tt.clusterClassNamespace)
215+
}
200216

201217
baseTemplate, err := repository.NewTemplate(repository.TemplateInput{
202-
RawArtifact: clusterWithTopology,
218+
RawArtifact: []byte(clusterWithTopology),
203219
ConfigVariablesClient: test.NewFakeVariableClient(),
204220
Processor: yaml.NewSimpleProcessor(),
205221
TargetNamespace: tt.targetNamespace,
@@ -210,13 +226,20 @@ func TestAddClusterClassIfMissing(t *testing.T) {
210226
}
211227

212228
g := NewWithT(t)
213-
template, err := addClusterClassIfMissing(ctx, baseTemplate, clusterClassClient, cluster, tt.targetNamespace, tt.listVariablesOnly)
229+
template, err := addClusterClassIfMissing(ctx, baseTemplate, clusterClassClient, cluster, tt.listVariablesOnly)
214230
if tt.wantError {
215231
g.Expect(err).To(HaveOccurred())
216232
} else {
217233
if tt.wantClusterClassInTemplate {
218-
g.Expect(template.Objs()).To(ContainElement(MatchClusterClass("dev", tt.targetNamespace)))
234+
if tt.clusterClassNamespace != "" {
235+
g.Expect(template.Objs()).To(ContainElement(MatchClusterClass("dev", tt.clusterClassNamespace)))
236+
g.Expect(template.Objs()).ToNot(ContainElement(MatchClusterClass("dev", tt.targetNamespace)))
237+
} else {
238+
g.Expect(template.Objs()).To(ContainElement(MatchClusterClass("dev", tt.targetNamespace)))
239+
g.Expect(template.Objs()).ToNot(ContainElement(MatchClusterClass("dev", tt.clusterClassNamespace)))
240+
}
219241
} else {
242+
g.Expect(template.Objs()).NotTo(ContainElement(MatchClusterClass("dev", tt.clusterClassNamespace)))
220243
g.Expect(template.Objs()).NotTo(ContainElement(MatchClusterClass("dev", tt.targetNamespace)))
221244
}
222245
}

‎cmd/clusterctl/client/config.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -344,7 +344,7 @@ func (c *clusterctlClient) getTemplateFromRepository(ctx context.Context, cluste
344344

345345
clusterClassClient := repo.ClusterClasses(version)
346346

347-
template, err = addClusterClassIfMissing(ctx, template, clusterClassClient, cluster, targetNamespace, listVariablesOnly)
347+
template, err = addClusterClassIfMissing(ctx, template, clusterClassClient, cluster, listVariablesOnly)
348348
if err != nil {
349349
return nil, err
350350
}

‎cmd/clusterctl/client/repository/template.go

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -17,8 +17,6 @@ limitations under the License.
1717
package repository
1818

1919
import (
20-
"fmt"
21-
2220
"github.com/pkg/errors"
2321
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
2422
"k8s.io/apimachinery/pkg/util/sets"
@@ -172,10 +170,6 @@ func MergeTemplates(templates ...Template) (Template, error) {
172170
}
173171
}
174172

175-
if merged.targetNamespace != tmpl.TargetNamespace() {
176-
return nil, fmt.Errorf("cannot merge templates with different targetNamespaces")
177-
}
178-
179173
merged.objs = append(merged.objs, tmpl.Objs()...)
180174
}
181175

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -37,8 +37,8 @@ type CrossNsSpecInput struct {
3737
QuickStartSpecInput
3838
}
3939

40-
// QuickStartSpec implements a spec that mimics the operation described in the Cluster API quick start, that is
41-
// creating a workload cluster.
40+
// CrossNsSpecQuickstart implements a spec that mimics the operation described in the Cluster API quick start, that is
41+
// creating a workload cluster, but uses a cross-namespaced cluster class reference.
4242
// This test is meant to provide a first, fast signal to detect regression; it is recommended to use it as a PR blocker test.
4343
// NOTE: This test works with Clusters with and without ClusterClass.
4444
func CrossNsSpecQuickstart(ctx context.Context, inputGetter func() CrossNsSpecInput) {

‎test/framework/clusterctl/clusterctl_helpers.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -433,7 +433,7 @@ func ApplyCustomClusterTemplateAndWait(ctx context.Context, input ApplyCustomClu
433433
if result.Cluster.Spec.Topology != nil {
434434
result.ClusterClass = framework.GetClusterClassByName(ctx, framework.GetClusterClassByNameInput{
435435
Getter: input.ClusterProxy.GetClient(),
436-
Namespace: input.Namespace,
436+
Namespace: result.Cluster.GetInfrastructureNamespace(),
437437
Name: result.Cluster.Spec.Topology.Class,
438438
})
439439
}

0 commit comments

Comments
 (0)