Skip to content

Commit

Permalink
Add handling for variables with conflicting definitions
Browse files Browse the repository at this point in the history
  • Loading branch information
@killianmuldoon
killianmuldoon committed Feb 27, 2023
1 parent 9e00c95 commit a9c7a35
Show file tree
Hide file tree
Showing 10 changed files with 2,052 additions and 573 deletions.
66 changes: 52 additions & 14 deletions internal/controllers/clusterclass/clusterclass_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ package clusterclass
import (
"context"
"fmt"
"reflect"
"sort"
"strings"

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

func (r *Reconciler) reconcileVariables(ctx context.Context, clusterClass *clusterv1.ClusterClass) error {
errs := []error{}
uniqueVars := map[string]bool{}
allVars := []clusterv1.ClusterClassStatusVariable{}
allVariableDefinitions := map[string]*clusterv1.ClusterClassStatusVariable{}
// Add inline variable definitions to the ClusterClass status.
for _, variable := range clusterClass.Spec.Variables {
uniqueVars[variable.Name] = true
allVars = append(allVars, statusVariableFromClusterClassVariable(variable, clusterv1.VariableDefinitionFromInline))
allVariableDefinitions[variable.Name] = addNewStatusVariable(variable, clusterv1.VariableDefinitionFromInline)
}

// If RuntimeSDK is enabled call the DiscoverVariables hook for all associated Runtime Extensions and add the variables
Expand All @@ -242,14 +242,23 @@ func (r *Reconciler) reconcileVariables(ctx context.Context, clusterClass *clust
continue
}
if resp.Variables != nil {
uniqueNamesForPatch := map[string]bool{}
for _, variable := range resp.Variables {
// TODO: Variables should be validated and deduplicated based on their provided definition.
if _, ok := uniqueVars[variable.Name]; ok {
errs = append(errs, errors.Errorf("duplicate variable name %s discovered in patch: %s", variable.Name, patch.Name))
// Ensure a patch doesn't define multiple variables with the same name.
if uniqueNamesForPatch[variable.Name] {
errs = append(errs, errors.Errorf("variable %q is defined multiple times in variable discovery response from patch %q", variable.Name, patch.Name))
continue
}
uniqueVars[variable.Name] = true
allVars = append(allVars, statusVariableFromClusterClassVariable(variable, patch.Name))
uniqueNamesForPatch[variable.Name] = true

// If a variable of the same name already exists in allVariableDefinitions add the new definition to the existing list.
if _, ok := allVariableDefinitions[variable.Name]; ok {
allVariableDefinitions[variable.Name] = addDefinitionToExistingStatusVariable(variable, patch.Name, allVariableDefinitions[variable.Name])
continue
}

// Add the new variable to the list.
allVariableDefinitions[variable.Name] = addNewStatusVariable(variable, patch.Name)
}
}
}
Expand All @@ -260,10 +269,20 @@ func (r *Reconciler) reconcileVariables(ctx context.Context, clusterClass *clust
"VariableDiscovery failed: %s", kerrors.NewAggregate(errs))
return errors.Wrapf(kerrors.NewAggregate(errs), "failed to discover variables for ClusterClass %s", clusterClass.Name)
}
clusterClass.Status.Variables = allVars

statusVarList := []clusterv1.ClusterClassStatusVariable{}
for _, variable := range allVariableDefinitions {
statusVarList = append(statusVarList, *variable)
}
// Alphabetically sort the variables by name. This ensures no unnecessary updates to the ClusterClass status.
sort.SliceStable(statusVarList, func(i, j int) bool {
return statusVarList[i].Name < statusVarList[j].Name
})
clusterClass.Status.Variables = statusVarList
conditions.MarkTrue(clusterClass, clusterv1.ClusterClassVariablesReconciledCondition)
return nil
}

func reconcileConditions(clusterClass *clusterv1.ClusterClass, outdatedRefs map[*corev1.ObjectReference]*corev1.ObjectReference) {
if len(outdatedRefs) > 0 {
var msg []string
Expand All @@ -288,10 +307,9 @@ func reconcileConditions(clusterClass *clusterv1.ClusterClass, outdatedRefs map[
)
}

func statusVariableFromClusterClassVariable(variable clusterv1.ClusterClassVariable, from string) clusterv1.ClusterClassStatusVariable {
return clusterv1.ClusterClassStatusVariable{
Name: variable.Name,
// TODO: In a future iteration this should be true where definitions are equal.
func addNewStatusVariable(variable clusterv1.ClusterClassVariable, from string) *clusterv1.ClusterClassStatusVariable {
return &clusterv1.ClusterClassStatusVariable{
Name: variable.Name,
DefinitionsConflict: false,
Definitions: []clusterv1.ClusterClassStatusVariableDefinition{
{
Expand All @@ -302,6 +320,26 @@ func statusVariableFromClusterClassVariable(variable clusterv1.ClusterClassVaria
}}
}

func addDefinitionToExistingStatusVariable(variable clusterv1.ClusterClassVariable, from string, existingVariable *clusterv1.ClusterClassStatusVariable) *clusterv1.ClusterClassStatusVariable {
combinedVariable := existingVariable.DeepCopy()
newVariableDefinition := clusterv1.ClusterClassStatusVariableDefinition{
From: from,
Required: variable.Required,
Schema: variable.Schema,
}
combinedVariable.Definitions = append(existingVariable.Definitions, newVariableDefinition)

// If the new definition is different from any existing definition, set DefinitionsConflict to true.
// If definitions already conflict, no need to check.
if !combinedVariable.DefinitionsConflict {
currentDefinition := combinedVariable.Definitions[0]
if !(currentDefinition.Required == newVariableDefinition.Required && reflect.DeepEqual(currentDefinition.Schema, newVariableDefinition.Schema)) {
combinedVariable.DefinitionsConflict = true
}
}
return combinedVariable
}

func refString(ref *corev1.ObjectReference) string {
return fmt.Sprintf("%s %s/%s", ref.GroupVersionKind().String(), ref.Namespace, ref.Name)
}
Expand Down
237 changes: 237 additions & 0 deletions internal/controllers/clusterclass/clusterclass_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,14 +23,21 @@ import (
"testing"
"time"

"github.com/google/go-cmp/cmp"
. "github.com/onsi/gomega"
"github.com/pkg/errors"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
utilfeature "k8s.io/component-base/featuregate/testing"
"k8s.io/utils/pointer"
"sigs.k8s.io/controller-runtime/pkg/client"

clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
runtimecatalog "sigs.k8s.io/cluster-api/exp/runtime/catalog"
runtimehooksv1 "sigs.k8s.io/cluster-api/exp/runtime/hooks/api/v1alpha1"
"sigs.k8s.io/cluster-api/feature"
tlog "sigs.k8s.io/cluster-api/internal/log"
fakeruntimeclient "sigs.k8s.io/cluster-api/internal/runtime/client/fake"
"sigs.k8s.io/cluster-api/internal/test/builder"
)

Expand Down Expand Up @@ -308,3 +315,233 @@ func isOwnerReferenceEqual(a, b metav1.OwnerReference) bool {
}
return true
}

func TestReconciler_reconcileVariables(t *testing.T) {
defer utilfeature.SetFeatureGateDuringTest(t, feature.Gates, feature.RuntimeSDK, true)()

g := NewWithT(t)
catalog := runtimecatalog.New()
_ = runtimehooksv1.AddToCatalog(catalog)

clusterClassWithInlineVariables := builder.ClusterClass(metav1.NamespaceDefault, "class1").
WithVariables(
[]clusterv1.ClusterClassVariable{
{
Name: "cpu",
Schema: clusterv1.VariableSchema{
OpenAPIV3Schema: clusterv1.JSONSchemaProps{
Type: "integer",
},
},
},
{
Name: "memory",
Schema: clusterv1.VariableSchema{
OpenAPIV3Schema: clusterv1.JSONSchemaProps{
Type: "string",
},
},
},
}...,
)
tests := []struct {
name string
clusterClass *clusterv1.ClusterClass
want []clusterv1.ClusterClassStatusVariable
patchResponse *runtimehooksv1.DiscoverVariablesResponse
wantErr bool
}{
{
name: "Reconcile inline variables to ClusterClass status",
clusterClass: clusterClassWithInlineVariables.DeepCopy().Build(),
want: []clusterv1.ClusterClassStatusVariable{
{
Name: "cpu",
Definitions: []clusterv1.ClusterClassStatusVariableDefinition{
{
From: clusterv1.VariableDefinitionFromInline,
Schema: clusterv1.VariableSchema{
OpenAPIV3Schema: clusterv1.JSONSchemaProps{
Type: "integer",
},
},
},
},
},
{
Name: "memory",
Definitions: []clusterv1.ClusterClassStatusVariableDefinition{
{
From: clusterv1.VariableDefinitionFromInline,
Schema: clusterv1.VariableSchema{
OpenAPIV3Schema: clusterv1.JSONSchemaProps{
Type: "string",
},
},
},
},
},
},
},
{
name: "Reconcile variables from inline and external variables to ClusterClass status",
clusterClass: clusterClassWithInlineVariables.DeepCopy().WithPatches(
[]clusterv1.ClusterClassPatch{
{
Name: "patch1",
External: &clusterv1.ExternalPatchDefinition{
DiscoverVariablesExtension: pointer.String("variables-one"),
}}}).
Build(),
patchResponse: &runtimehooksv1.DiscoverVariablesResponse{
CommonResponse: runtimehooksv1.CommonResponse{
Status: runtimehooksv1.ResponseStatusSuccess,
},
Variables: []clusterv1.ClusterClassVariable{
{
Name: "cpu",
Schema: clusterv1.VariableSchema{
OpenAPIV3Schema: clusterv1.JSONSchemaProps{
Type: "string",
},
},
},
{
Name: "memory",
Schema: clusterv1.VariableSchema{
OpenAPIV3Schema: clusterv1.JSONSchemaProps{
Type: "string",
},
},
},
{
Name: "location",
Schema: clusterv1.VariableSchema{
OpenAPIV3Schema: clusterv1.JSONSchemaProps{
Type: "string",
},
},
},
},
},
want: []clusterv1.ClusterClassStatusVariable{
{
Name: "cpu",
DefinitionsConflict: true,
Definitions: []clusterv1.ClusterClassStatusVariableDefinition{
{
From: clusterv1.VariableDefinitionFromInline,
Schema: clusterv1.VariableSchema{
OpenAPIV3Schema: clusterv1.JSONSchemaProps{
Type: "integer",
},
},
},
{
From: "patch1",
Schema: clusterv1.VariableSchema{
OpenAPIV3Schema: clusterv1.JSONSchemaProps{
Type: "string",
},
},
},
},
},
{
Name: "location",
DefinitionsConflict: false,
Definitions: []clusterv1.ClusterClassStatusVariableDefinition{
{
From: "patch1",
Schema: clusterv1.VariableSchema{
OpenAPIV3Schema: clusterv1.JSONSchemaProps{
Type: "string",
},
},
},
},
},
{
Name: "memory",
DefinitionsConflict: false,
Definitions: []clusterv1.ClusterClassStatusVariableDefinition{
{
From: clusterv1.VariableDefinitionFromInline,
Schema: clusterv1.VariableSchema{
OpenAPIV3Schema: clusterv1.JSONSchemaProps{
Type: "string",
},
},
},
{
From: "patch1",
Schema: clusterv1.VariableSchema{
OpenAPIV3Schema: clusterv1.JSONSchemaProps{
Type: "string",
},
},
},
},
},
},
},
{
name: "Error if external patch defines a variable with same name multiple times",
wantErr: true,
clusterClass: clusterClassWithInlineVariables.DeepCopy().WithPatches(
[]clusterv1.ClusterClassPatch{
{
Name: "patch1",
External: &clusterv1.ExternalPatchDefinition{
DiscoverVariablesExtension: pointer.String("variables-one"),
}}}).
Build(),
patchResponse: &runtimehooksv1.DiscoverVariablesResponse{
CommonResponse: runtimehooksv1.CommonResponse{
Status: runtimehooksv1.ResponseStatusSuccess,
},
Variables: []clusterv1.ClusterClassVariable{
{
Name: "cpu",
Schema: clusterv1.VariableSchema{
OpenAPIV3Schema: clusterv1.JSONSchemaProps{
Type: "string",
},
},
},
{
Name: "cpu",
Schema: clusterv1.VariableSchema{
OpenAPIV3Schema: clusterv1.JSONSchemaProps{
Type: "integer",
},
},
},
},
},
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
fakeRuntimeClient := fakeruntimeclient.NewRuntimeClientBuilder().
WithCallExtensionResponses(
map[string]runtimehooksv1.ResponseObject{
"variables-one": tt.patchResponse,
}).
WithCatalog(catalog).
Build()

r := &Reconciler{
RuntimeClient: fakeRuntimeClient,
}

err := r.reconcileVariables(ctx, tt.clusterClass)
if tt.wantErr {
g.Expect(err).To(HaveOccurred())
return
}
g.Expect(err).NotTo(HaveOccurred())
g.Expect(tt.clusterClass.Status.Variables).To(Equal(tt.want), cmp.Diff(tt.clusterClass.Status.Variables, tt.want))
})
}
}
5 changes: 3 additions & 2 deletions internal/controllers/topology/cluster/cluster_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"testing"
"time"

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

0 comments on commit a9c7a35

Please sign in to comment.