Skip to content

Commit 83e7476

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request #8107 from killianmuldoon/pr-exvars-handle-definition-conflict
 ✨ Handle variable definition conflicts for external variables
2 parents d27476d + 56f9a52 commit 83e7476

File tree

10 files changed

+2252
-592
lines changed

10 files changed

+2252
-592
lines changed

‎internal/controllers/clusterclass/clusterclass_controller.go

Lines changed: 54 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,8 @@ package clusterclass
1919
import (
2020
"context"
2121
"fmt"
22+
"reflect"
23+
"sort"
2224
"strings"
2325

2426
"github.com/pkg/errors"
@@ -213,12 +215,10 @@ func (r *Reconciler) reconcileExternalReferences(ctx context.Context, clusterCla
213215

214216
func (r *Reconciler) reconcileVariables(ctx context.Context, clusterClass *clusterv1.ClusterClass) error {
215217
errs := []error{}
216-
uniqueVars := map[string]bool{}
217-
allVars := []clusterv1.ClusterClassStatusVariable{}
218+
allVariableDefinitions := map[string]*clusterv1.ClusterClassStatusVariable{}
218219
// Add inline variable definitions to the ClusterClass status.
219220
for _, variable := range clusterClass.Spec.Variables {
220-
uniqueVars[variable.Name] = true
221-
allVars = append(allVars, statusVariableFromClusterClassVariable(variable, clusterv1.VariableDefinitionFromInline))
221+
allVariableDefinitions[variable.Name] = addNewStatusVariable(variable, clusterv1.VariableDefinitionFromInline)
222222
}
223223

224224
// If RuntimeSDK is enabled call the DiscoverVariables hook for all associated Runtime Extensions and add the variables
@@ -242,14 +242,23 @@ func (r *Reconciler) reconcileVariables(ctx context.Context, clusterClass *clust
242242
continue
243243
}
244244
if resp.Variables != nil {
245+
uniqueNamesForPatch := sets.Set[string]{}
245246
for _, variable := range resp.Variables {
246-
// TODO: Variables should be validated and deduplicated based on their provided definition.
247-
if _, ok := uniqueVars[variable.Name]; ok {
248-
errs = append(errs, errors.Errorf("duplicate variable name %s discovered in patch: %s", variable.Name, patch.Name))
247+
// Ensure a patch doesn't define multiple variables with the same name.
248+
if uniqueNamesForPatch.Has(variable.Name) {
249+
errs = append(errs, errors.Errorf("variable %q is defined multiple times in variable discovery response from patch %q", variable.Name, patch.Name))
249250
continue
250251
}
251-
uniqueVars[variable.Name] = true
252-
allVars = append(allVars, statusVariableFromClusterClassVariable(variable, patch.Name))
252+
uniqueNamesForPatch.Insert(variable.Name)
253+
254+
// If a variable of the same name already exists in allVariableDefinitions add the new definition to the existing list.
255+
if _, ok := allVariableDefinitions[variable.Name]; ok {
256+
allVariableDefinitions[variable.Name] = addDefinitionToExistingStatusVariable(variable, patch.Name, allVariableDefinitions[variable.Name])
257+
continue
258+
}
259+
260+
// Add the new variable to the list.
261+
allVariableDefinitions[variable.Name] = addNewStatusVariable(variable, patch.Name)
253262
}
254263
}
255264
}
@@ -260,10 +269,22 @@ func (r *Reconciler) reconcileVariables(ctx context.Context, clusterClass *clust
260269
"VariableDiscovery failed: %s", kerrors.NewAggregate(errs))
261270
return errors.Wrapf(kerrors.NewAggregate(errs), "failed to discover variables for ClusterClass %s", clusterClass.Name)
262271
}
263-
clusterClass.Status.Variables = allVars
272+
273+
statusVarList := []clusterv1.ClusterClassStatusVariable{}
274+
for _, variable := range allVariableDefinitions {
275+
statusVarList = append(statusVarList, *variable)
276+
}
277+
// Alphabetically sort the variables by name. This ensures no unnecessary updates to the ClusterClass status.
278+
// Note: Definitions in `statusVarList[i].Definitions` have a stable order as they are added in a deterministic order
279+
// and are always held in an array.
280+
sort.SliceStable(statusVarList, func(i, j int) bool {
281+
return statusVarList[i].Name < statusVarList[j].Name
282+
})
283+
clusterClass.Status.Variables = statusVarList
264284
conditions.MarkTrue(clusterClass, clusterv1.ClusterClassVariablesReconciledCondition)
265285
return nil
266286
}
287+
267288
func reconcileConditions(clusterClass *clusterv1.ClusterClass, outdatedRefs map[*corev1.ObjectReference]*corev1.ObjectReference) {
268289
if len(outdatedRefs) > 0 {
269290
var msg []string
@@ -288,10 +309,9 @@ func reconcileConditions(clusterClass *clusterv1.ClusterClass, outdatedRefs map[
288309
)
289310
}
290311

291-
func statusVariableFromClusterClassVariable(variable clusterv1.ClusterClassVariable, from string) clusterv1.ClusterClassStatusVariable {
292-
return clusterv1.ClusterClassStatusVariable{
293-
Name: variable.Name,
294-
// TODO: In a future iteration this should be true where definitions are equal.
312+
func addNewStatusVariable(variable clusterv1.ClusterClassVariable, from string) *clusterv1.ClusterClassStatusVariable {
313+
return &clusterv1.ClusterClassStatusVariable{
314+
Name: variable.Name,
295315
DefinitionsConflict: false,
296316
Definitions: []clusterv1.ClusterClassStatusVariableDefinition{
297317
{
@@ -302,6 +322,26 @@ func statusVariableFromClusterClassVariable(variable clusterv1.ClusterClassVaria
302322
}}
303323
}
304324

325+
func addDefinitionToExistingStatusVariable(variable clusterv1.ClusterClassVariable, from string, existingVariable *clusterv1.ClusterClassStatusVariable) *clusterv1.ClusterClassStatusVariable {
326+
combinedVariable := existingVariable.DeepCopy()
327+
newVariableDefinition := clusterv1.ClusterClassStatusVariableDefinition{
328+
From: from,
329+
Required: variable.Required,
330+
Schema: variable.Schema,
331+
}
332+
combinedVariable.Definitions = append(existingVariable.Definitions, newVariableDefinition)
333+
334+
// If the new definition is different from any existing definition, set DefinitionsConflict to true.
335+
// If definitions already conflict, no need to check.
336+
if !combinedVariable.DefinitionsConflict {
337+
currentDefinition := combinedVariable.Definitions[0]
338+
if !(currentDefinition.Required == newVariableDefinition.Required && reflect.DeepEqual(currentDefinition.Schema, newVariableDefinition.Schema)) {
339+
combinedVariable.DefinitionsConflict = true
340+
}
341+
}
342+
return combinedVariable
343+
}
344+
305345
func refString(ref *corev1.ObjectReference) string {
306346
return fmt.Sprintf("%s %s/%s", ref.GroupVersionKind().String(), ref.Namespace, ref.Name)
307347
}

‎internal/controllers/clusterclass/clusterclass_controller_test.go

Lines changed: 237 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,14 +23,21 @@ import (
2323
"testing"
2424
"time"
2525

26+
"github.com/google/go-cmp/cmp"
2627
. "github.com/onsi/gomega"
2728
"github.com/pkg/errors"
2829
corev1 "k8s.io/api/core/v1"
2930
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
31+
utilfeature "k8s.io/component-base/featuregate/testing"
32+
"k8s.io/utils/pointer"
3033
"sigs.k8s.io/controller-runtime/pkg/client"
3134

3235
clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
36+
runtimecatalog "sigs.k8s.io/cluster-api/exp/runtime/catalog"
37+
runtimehooksv1 "sigs.k8s.io/cluster-api/exp/runtime/hooks/api/v1alpha1"
38+
"sigs.k8s.io/cluster-api/feature"
3339
tlog "sigs.k8s.io/cluster-api/internal/log"
40+
fakeruntimeclient "sigs.k8s.io/cluster-api/internal/runtime/client/fake"
3441
"sigs.k8s.io/cluster-api/internal/test/builder"
3542
)
3643

@@ -308,3 +315,233 @@ func isOwnerReferenceEqual(a, b metav1.OwnerReference) bool {
308315
}
309316
return true
310317
}
318+
319+
func TestReconciler_reconcileVariables(t *testing.T) {
320+
defer utilfeature.SetFeatureGateDuringTest(t, feature.Gates, feature.RuntimeSDK, true)()
321+
322+
g := NewWithT(t)
323+
catalog := runtimecatalog.New()
324+
_ = runtimehooksv1.AddToCatalog(catalog)
325+
326+
clusterClassWithInlineVariables := builder.ClusterClass(metav1.NamespaceDefault, "class1").
327+
WithVariables(
328+
[]clusterv1.ClusterClassVariable{
329+
{
330+
Name: "cpu",
331+
Schema: clusterv1.VariableSchema{
332+
OpenAPIV3Schema: clusterv1.JSONSchemaProps{
333+
Type: "integer",
334+
},
335+
},
336+
},
337+
{
338+
Name: "memory",
339+
Schema: clusterv1.VariableSchema{
340+
OpenAPIV3Schema: clusterv1.JSONSchemaProps{
341+
Type: "string",
342+
},
343+
},
344+
},
345+
}...,
346+
)
347+
tests := []struct {
348+
name string
349+
clusterClass *clusterv1.ClusterClass
350+
want []clusterv1.ClusterClassStatusVariable
351+
patchResponse *runtimehooksv1.DiscoverVariablesResponse
352+
wantErr bool
353+
}{
354+
{
355+
name: "Reconcile inline variables to ClusterClass status",
356+
clusterClass: clusterClassWithInlineVariables.DeepCopy().Build(),
357+
want: []clusterv1.ClusterClassStatusVariable{
358+
{
359+
Name: "cpu",
360+
Definitions: []clusterv1.ClusterClassStatusVariableDefinition{
361+
{
362+
From: clusterv1.VariableDefinitionFromInline,
363+
Schema: clusterv1.VariableSchema{
364+
OpenAPIV3Schema: clusterv1.JSONSchemaProps{
365+
Type: "integer",
366+
},
367+
},
368+
},
369+
},
370+
},
371+
{
372+
Name: "memory",
373+
Definitions: []clusterv1.ClusterClassStatusVariableDefinition{
374+
{
375+
From: clusterv1.VariableDefinitionFromInline,
376+
Schema: clusterv1.VariableSchema{
377+
OpenAPIV3Schema: clusterv1.JSONSchemaProps{
378+
Type: "string",
379+
},
380+
},
381+
},
382+
},
383+
},
384+
},
385+
},
386+
{
387+
name: "Reconcile variables from inline and external variables to ClusterClass status",
388+
clusterClass: clusterClassWithInlineVariables.DeepCopy().WithPatches(
389+
[]clusterv1.ClusterClassPatch{
390+
{
391+
Name: "patch1",
392+
External: &clusterv1.ExternalPatchDefinition{
393+
DiscoverVariablesExtension: pointer.String("variables-one"),
394+
}}}).
395+
Build(),
396+
patchResponse: &runtimehooksv1.DiscoverVariablesResponse{
397+
CommonResponse: runtimehooksv1.CommonResponse{
398+
Status: runtimehooksv1.ResponseStatusSuccess,
399+
},
400+
Variables: []clusterv1.ClusterClassVariable{
401+
{
402+
Name: "cpu",
403+
Schema: clusterv1.VariableSchema{
404+
OpenAPIV3Schema: clusterv1.JSONSchemaProps{
405+
Type: "string",
406+
},
407+
},
408+
},
409+
{
410+
Name: "memory",
411+
Schema: clusterv1.VariableSchema{
412+
OpenAPIV3Schema: clusterv1.JSONSchemaProps{
413+
Type: "string",
414+
},
415+
},
416+
},
417+
{
418+
Name: "location",
419+
Schema: clusterv1.VariableSchema{
420+
OpenAPIV3Schema: clusterv1.JSONSchemaProps{
421+
Type: "string",
422+
},
423+
},
424+
},
425+
},
426+
},
427+
want: []clusterv1.ClusterClassStatusVariable{
428+
{
429+
Name: "cpu",
430+
DefinitionsConflict: true,
431+
Definitions: []clusterv1.ClusterClassStatusVariableDefinition{
432+
{
433+
From: clusterv1.VariableDefinitionFromInline,
434+
Schema: clusterv1.VariableSchema{
435+
OpenAPIV3Schema: clusterv1.JSONSchemaProps{
436+
Type: "integer",
437+
},
438+
},
439+
},
440+
{
441+
From: "patch1",
442+
Schema: clusterv1.VariableSchema{
443+
OpenAPIV3Schema: clusterv1.JSONSchemaProps{
444+
Type: "string",
445+
},
446+
},
447+
},
448+
},
449+
},
450+
{
451+
Name: "location",
452+
DefinitionsConflict: false,
453+
Definitions: []clusterv1.ClusterClassStatusVariableDefinition{
454+
{
455+
From: "patch1",
456+
Schema: clusterv1.VariableSchema{
457+
OpenAPIV3Schema: clusterv1.JSONSchemaProps{
458+
Type: "string",
459+
},
460+
},
461+
},
462+
},
463+
},
464+
{
465+
Name: "memory",
466+
DefinitionsConflict: false,
467+
Definitions: []clusterv1.ClusterClassStatusVariableDefinition{
468+
{
469+
From: clusterv1.VariableDefinitionFromInline,
470+
Schema: clusterv1.VariableSchema{
471+
OpenAPIV3Schema: clusterv1.JSONSchemaProps{
472+
Type: "string",
473+
},
474+
},
475+
},
476+
{
477+
From: "patch1",
478+
Schema: clusterv1.VariableSchema{
479+
OpenAPIV3Schema: clusterv1.JSONSchemaProps{
480+
Type: "string",
481+
},
482+
},
483+
},
484+
},
485+
},
486+
},
487+
},
488+
{
489+
name: "Error if external patch defines a variable with same name multiple times",
490+
wantErr: true,
491+
clusterClass: clusterClassWithInlineVariables.DeepCopy().WithPatches(
492+
[]clusterv1.ClusterClassPatch{
493+
{
494+
Name: "patch1",
495+
External: &clusterv1.ExternalPatchDefinition{
496+
DiscoverVariablesExtension: pointer.String("variables-one"),
497+
}}}).
498+
Build(),
499+
patchResponse: &runtimehooksv1.DiscoverVariablesResponse{
500+
CommonResponse: runtimehooksv1.CommonResponse{
501+
Status: runtimehooksv1.ResponseStatusSuccess,
502+
},
503+
Variables: []clusterv1.ClusterClassVariable{
504+
{
505+
Name: "cpu",
506+
Schema: clusterv1.VariableSchema{
507+
OpenAPIV3Schema: clusterv1.JSONSchemaProps{
508+
Type: "string",
509+
},
510+
},
511+
},
512+
{
513+
Name: "cpu",
514+
Schema: clusterv1.VariableSchema{
515+
OpenAPIV3Schema: clusterv1.JSONSchemaProps{
516+
Type: "integer",
517+
},
518+
},
519+
},
520+
},
521+
},
522+
},
523+
}
524+
for _, tt := range tests {
525+
t.Run(tt.name, func(t *testing.T) {
526+
fakeRuntimeClient := fakeruntimeclient.NewRuntimeClientBuilder().
527+
WithCallExtensionResponses(
528+
map[string]runtimehooksv1.ResponseObject{
529+
"variables-one": tt.patchResponse,
530+
}).
531+
WithCatalog(catalog).
532+
Build()
533+
534+
r := &Reconciler{
535+
RuntimeClient: fakeRuntimeClient,
536+
}
537+
538+
err := r.reconcileVariables(ctx, tt.clusterClass)
539+
if tt.wantErr {
540+
g.Expect(err).To(HaveOccurred())
541+
return
542+
}
543+
g.Expect(err).NotTo(HaveOccurred())
544+
g.Expect(tt.clusterClass.Status.Variables).To(Equal(tt.want), cmp.Diff(tt.clusterClass.Status.Variables, tt.want))
545+
})
546+
}
547+
}

‎internal/controllers/topology/cluster/cluster_controller_test.go

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ import (
2222
"testing"
2323
"time"
2424

25+
"github.com/google/go-cmp/cmp"
2526
. "github.com/onsi/gomega"
2627
corev1 "k8s.io/api/core/v1"
2728
apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
@@ -1114,7 +1115,7 @@ func TestReconciler_DefaultCluster(t *testing.T) {
11141115
Build(),
11151116
wantCluster: clusterBuilder.DeepCopy().
11161117
WithTopology(topologyBase.DeepCopy().WithVariables(
1117-
clusterv1.ClusterVariable{Name: "location", Value: apiextensionsv1.JSON{Raw: []byte(`"us-east"`)}}).
1118+
clusterv1.ClusterVariable{Name: "location", Value: apiextensionsv1.JSON{Raw: []byte(`"us-east"`)}, DefinitionFrom: ""}).
11181119
Build()).
11191120
Build(),
11201121
},
@@ -1234,7 +1235,7 @@ func TestReconciler_DefaultCluster(t *testing.T) {
12341235
got := &clusterv1.Cluster{}
12351236
g.Expect(fakeClient.Get(ctx, client.ObjectKey{Name: tt.initialCluster.Name, Namespace: tt.initialCluster.Namespace}, got)).To(Succeed())
12361237
// Compare the spec of the two clusters to ensure that variables are defaulted correctly.
1237-
g.Expect(reflect.DeepEqual(got.Spec, tt.wantCluster.Spec)).To(BeTrue())
1238+
g.Expect(reflect.DeepEqual(got.Spec, tt.wantCluster.Spec)).To(BeTrue(), cmp.Diff(got.Spec, tt.wantCluster.Spec))
12381239
})
12391240
}
12401241
}

0 commit comments

Comments
 (0)