Skip to content

Commit

Permalink
Merge pull request #5847 from killianmuldoon/lint/add-predeclared
Browse files Browse the repository at this point in the history 
🌱 enable predeclared linter and fix problems
  • Loading branch information
@k8s-ci-robot
k8s-ci-robot authored Dec 10, 2021
2 parents 801e6a7 + 8f4de90 commit c3e5f09
Show file tree
Hide file tree
Showing 3 changed files with 51 additions and 50 deletions.
1 change: 1 addition & 0 deletions .golangci.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ linters:
- nilerr
- nolintlint
- prealloc
- predeclared
- revive
- rowserrcheck
- staticcheck
Expand Down
58 changes: 29 additions & 29 deletions internal/webhooks/cluster.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -120,39 +120,39 @@ func (webhook *Cluster) ValidateDelete(_ context.Context, obj runtime.Object) er
return nil
}

func (webhook *Cluster) validate(ctx context.Context, old, new *clusterv1.Cluster) error {
func (webhook *Cluster) validate(ctx context.Context, oldCluster, newCluster *clusterv1.Cluster) error {
var allErrs field.ErrorList
if new.Spec.InfrastructureRef != nil && new.Spec.InfrastructureRef.Namespace != new.Namespace {
if newCluster.Spec.InfrastructureRef != nil && newCluster.Spec.InfrastructureRef.Namespace != newCluster.Namespace {
allErrs = append(
allErrs,
field.Invalid(
field.NewPath("spec", "infrastructureRef", "namespace"),
new.Spec.InfrastructureRef.Namespace,
newCluster.Spec.InfrastructureRef.Namespace,
"must match metadata.namespace",
),
)
}

if new.Spec.ControlPlaneRef != nil && new.Spec.ControlPlaneRef.Namespace != new.Namespace {
if newCluster.Spec.ControlPlaneRef != nil && newCluster.Spec.ControlPlaneRef.Namespace != newCluster.Namespace {
allErrs = append(
allErrs,
field.Invalid(
field.NewPath("spec", "controlPlaneRef", "namespace"),
new.Spec.ControlPlaneRef.Namespace,
newCluster.Spec.ControlPlaneRef.Namespace,
"must match metadata.namespace",
),
)
}

// Validate the managed topology, if defined.
if new.Spec.Topology != nil {
allErrs = append(allErrs, webhook.validateTopology(ctx, old, new)...)
if newCluster.Spec.Topology != nil {
allErrs = append(allErrs, webhook.validateTopology(ctx, oldCluster, newCluster)...)
}

// On update.
if old != nil {
if oldCluster != nil {
// Error if the update moves the cluster from Managed to Unmanaged i.e. the managed topology is removed on update.
if old.Spec.Topology != nil && new.Spec.Topology == nil {
if oldCluster.Spec.Topology != nil && newCluster.Spec.Topology == nil {
allErrs = append(allErrs, field.Forbidden(
field.NewPath("spec", "topology"),
"cannot be removed from an existing Cluster",
Expand All @@ -161,12 +161,12 @@ func (webhook *Cluster) validate(ctx context.Context, old, new *clusterv1.Cluste
}

if len(allErrs) > 0 {
return apierrors.NewInvalid(clusterv1.GroupVersion.WithKind("Cluster").GroupKind(), new.Name, allErrs)
return apierrors.NewInvalid(clusterv1.GroupVersion.WithKind("Cluster").GroupKind(), newCluster.Name, allErrs)
}
return nil
}

func (webhook *Cluster) validateTopology(ctx context.Context, old, new *clusterv1.Cluster) field.ErrorList {
func (webhook *Cluster) validateTopology(ctx context.Context, oldCluster, newCluster *clusterv1.Cluster) field.ErrorList {
// NOTE: ClusterClass and managed topologies are behind ClusterTopology feature gate flag; the web hook
// must prevent the usage of Cluster.Topology in case the feature flag is disabled.
if !feature.Gates.Enabled(feature.ClusterTopology) {
Expand All @@ -181,7 +181,7 @@ func (webhook *Cluster) validateTopology(ctx context.Context, old, new *clusterv
var allErrs field.ErrorList

// class should be defined.
if new.Spec.Topology.Class == "" {
if newCluster.Spec.Topology.Class == "" {
allErrs = append(
allErrs,
field.Required(
Expand All @@ -192,12 +192,12 @@ func (webhook *Cluster) validateTopology(ctx context.Context, old, new *clusterv
}

// version should be valid.
if !version.KubeSemver.MatchString(new.Spec.Topology.Version) {
if !version.KubeSemver.MatchString(newCluster.Spec.Topology.Version) {
allErrs = append(
allErrs,
field.Invalid(
field.NewPath("spec", "topology", "version"),
new.Spec.Topology.Version,
newCluster.Spec.Topology.Version,
"version must be a valid semantic version",
),
)
Expand All @@ -206,23 +206,23 @@ func (webhook *Cluster) validateTopology(ctx context.Context, old, new *clusterv
// clusterClass must exist.
clusterClass := &clusterv1.ClusterClass{}
// Check to see if the ClusterClass referenced in the Cluster currently exists.
if err := webhook.Client.Get(ctx, client.ObjectKey{Namespace: new.Namespace, Name: new.Spec.Topology.Class}, clusterClass); err != nil {
if err := webhook.Client.Get(ctx, client.ObjectKey{Namespace: newCluster.Namespace, Name: newCluster.Spec.Topology.Class}, clusterClass); err != nil {
allErrs = append(
allErrs, field.Invalid(
field.NewPath("spec", "topology", "class"),
new.Name,
fmt.Sprintf("ClusterClass with name %q could not be found", new.Spec.Topology.Class)))
newCluster.Name,
fmt.Sprintf("ClusterClass with name %q could not be found", newCluster.Spec.Topology.Class)))
return allErrs
}

allErrs = append(allErrs, check.MachineDeploymentTopologiesAreUniqueAndDefinedInClusterClass(new, clusterClass)...)
allErrs = append(allErrs, check.MachineDeploymentTopologiesAreUniqueAndDefinedInClusterClass(newCluster, clusterClass)...)

// Check if the variables defined in the clusterClass are valid.
allErrs = append(allErrs, variables.ValidateClusterVariables(new.Spec.Topology.Variables, clusterClass.Spec.Variables, field.NewPath("spec", "topology", "variables"))...)
allErrs = append(allErrs, variables.ValidateClusterVariables(newCluster.Spec.Topology.Variables, clusterClass.Spec.Variables, field.NewPath("spec", "topology", "variables"))...)

if old != nil { // On update
if oldCluster != nil { // On update
// Topology or Class can not be added on update.
if old.Spec.Topology == nil || old.Spec.Topology.Class == "" {
if oldCluster.Spec.Topology == nil || oldCluster.Spec.Topology.Class == "" {
allErrs = append(
allErrs,
field.Forbidden(
Expand All @@ -235,25 +235,25 @@ func (webhook *Cluster) validateTopology(ctx context.Context, old, new *clusterv
}

// Version could only be increased.
inVersion, err := semver.ParseTolerant(new.Spec.Topology.Version)
inVersion, err := semver.ParseTolerant(newCluster.Spec.Topology.Version)
if err != nil {
allErrs = append(
allErrs,
field.Invalid(
field.NewPath("spec", "topology", "version"),
new.Spec.Topology.Version,
newCluster.Spec.Topology.Version,
"version must be a valid semantic version",
),
)
}
oldVersion, err := semver.ParseTolerant(old.Spec.Topology.Version)
oldVersion, err := semver.ParseTolerant(oldCluster.Spec.Topology.Version)
if err != nil {
// NOTE: this should never happen. Nevertheless, handling this for extra caution.
allErrs = append(
allErrs,
field.Invalid(
field.NewPath("spec", "topology", "version"),
old.Spec.Topology.Version,
oldCluster.Spec.Topology.Version,
fmt.Sprintf("old version %q cannot be compared with %q", oldVersion, inVersion),
),
)
Expand All @@ -263,22 +263,22 @@ func (webhook *Cluster) validateTopology(ctx context.Context, old, new *clusterv
allErrs,
field.Invalid(
field.NewPath("spec", "topology", "version"),
new.Spec.Topology.Version,
newCluster.Spec.Topology.Version,
fmt.Sprintf("version cannot be decreased from %q to %q", oldVersion, inVersion),
),
)
}

// If the ClusterClass referenced in the Topology has changed compatibility checks are needed.
if old.Spec.Topology.Class != new.Spec.Topology.Class {
if oldCluster.Spec.Topology.Class != newCluster.Spec.Topology.Class {
// Check to see if the ClusterClass referenced in the old version of the Cluster exists.
oldClusterClass, err := webhook.getClusterClassForCluster(ctx, old)
oldClusterClass, err := webhook.getClusterClassForCluster(ctx, oldCluster)
if err != nil {
allErrs = append(
allErrs, field.Forbidden(
field.NewPath("spec", "topology", "class"),
fmt.Sprintf("ClusterClass with name %q could not be found, change from class %[1]q to class %q cannot be validated",
old.Spec.Topology.Class, new.Spec.Topology.Class)))
oldCluster.Spec.Topology.Class, newCluster.Spec.Topology.Class)))

// Return early with errors if the ClusterClass can't be retrieved.
return allErrs
Expand Down
42 changes: 21 additions & 21 deletions internal/webhooks/clusterclass.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ func (webhook *ClusterClass) ValidateDelete(ctx context.Context, obj runtime.Obj
return nil
}

func (webhook *ClusterClass) validate(ctx context.Context, old, new *clusterv1.ClusterClass) error {
func (webhook *ClusterClass) validate(ctx context.Context, oldClusterClass, newClusterClass *clusterv1.ClusterClass) error {
// NOTE: ClusterClass and managed topologies are behind ClusterTopology feature gate flag; the web hook
// must prevent creating new objects new case the feature flag is disabled.
if !feature.Gates.Enabled(feature.ClusterTopology) {
Expand All @@ -137,43 +137,43 @@ func (webhook *ClusterClass) validate(ctx context.Context, old, new *clusterv1.C
var allErrs field.ErrorList

// Ensure all references are valid.
allErrs = append(allErrs, check.ClusterClassReferencesAreValid(new)...)
allErrs = append(allErrs, check.ClusterClassReferencesAreValid(newClusterClass)...)

// Ensure all MachineDeployment classes are unique.
allErrs = append(allErrs, check.MachineDeploymentClassesAreUnique(new)...)
allErrs = append(allErrs, check.MachineDeploymentClassesAreUnique(newClusterClass)...)

// Validate variables.
allErrs = append(allErrs,
variables.ValidateClusterClassVariables(new.Spec.Variables, field.NewPath("spec", "variables"))...,
variables.ValidateClusterClassVariables(newClusterClass.Spec.Variables, field.NewPath("spec", "variables"))...,
)

// Validate patches.
allErrs = append(allErrs, validatePatches(new)...)
allErrs = append(allErrs, validatePatches(newClusterClass)...)

// If this is an update run additional validation.
if old != nil {
if oldClusterClass != nil {
// Ensure spec changes are compatible.
allErrs = append(allErrs, check.ClusterClassesAreCompatible(old, new)...)
allErrs = append(allErrs, check.ClusterClassesAreCompatible(oldClusterClass, newClusterClass)...)

// Retrieve all clusters using the ClusterClass.
clusters, err := webhook.getClustersUsingClusterClass(ctx, old)
clusters, err := webhook.getClustersUsingClusterClass(ctx, oldClusterClass)
if err != nil {
allErrs = append(allErrs, field.InternalError(field.NewPath(""),
errors.Wrapf(err, "Clusters using ClusterClass %v can not be retrieved", old.Name)))
return apierrors.NewInvalid(clusterv1.GroupVersion.WithKind("ClusterClass").GroupKind(), new.Name, allErrs)
errors.Wrapf(err, "Clusters using ClusterClass %v can not be retrieved", oldClusterClass.Name)))
return apierrors.NewInvalid(clusterv1.GroupVersion.WithKind("ClusterClass").GroupKind(), newClusterClass.Name, allErrs)
}

// Ensure no MachineDeploymentClass currently in use has been removed from the ClusterClass.
allErrs = append(allErrs,
webhook.validateRemovedMachineDeploymentClassesAreNotUsed(clusters, old, new)...)
webhook.validateRemovedMachineDeploymentClassesAreNotUsed(clusters, oldClusterClass, newClusterClass)...)

// Ensure no Variable would be invalidated by the update in spec
allErrs = append(allErrs,
validateVariableUpdates(clusters, old, new, field.NewPath("spec", "variables"))...)
validateVariableUpdates(clusters, oldClusterClass, newClusterClass, field.NewPath("spec", "variables"))...)
}

if len(allErrs) > 0 {
return apierrors.NewInvalid(clusterv1.GroupVersion.WithKind("ClusterClass").GroupKind(), new.Name, allErrs)
return apierrors.NewInvalid(clusterv1.GroupVersion.WithKind("ClusterClass").GroupKind(), newClusterClass.Name, allErrs)
}
return nil
}
Expand All @@ -184,15 +184,15 @@ func (webhook *ClusterClass) validate(ctx context.Context, old, new *clusterv1.C
// 2) Removed ClusterClassVariables are not in use on any Cluster using the ClusterClass.
// 3) Added ClusterClassVariables defined on any exiting Cluster are still valid with the updated Schema.
// 4) Required ClusterClassVariables are defined on each Cluster using the ClusterClass.
func validateVariableUpdates(clusters []clusterv1.Cluster, old, new *clusterv1.ClusterClass, path *field.Path) field.ErrorList {
func validateVariableUpdates(clusters []clusterv1.Cluster, oldClusterClass, newClusterClass *clusterv1.ClusterClass, path *field.Path) field.ErrorList {
tracker := map[string][]string{}

// Get the old ClusterClassVariables as a map
oldVars, _ := getClusterClassVariablesMapWithReverseIndex(old.Spec.Variables)
oldVars, _ := getClusterClassVariablesMapWithReverseIndex(oldClusterClass.Spec.Variables)

// Get the new ClusterClassVariables as a map with an index linking them to their place in the ClusterClass Variable array.
// Note: The index is used to improve the error recording below.
newVars, clusterClassVariablesIndex := getClusterClassVariablesMapWithReverseIndex(new.Spec.Variables)
newVars, clusterClassVariablesIndex := getClusterClassVariablesMapWithReverseIndex(newClusterClass.Spec.Variables)

// Compute the diff between old and new ClusterClassVariables.
varsDiff := getClusterClassVariablesForValidation(oldVars, newVars)
Expand Down Expand Up @@ -350,10 +350,10 @@ func getClusterClassVariablesForValidation(oldVars, newVars map[string]*clusterv
return out
}

func (webhook *ClusterClass) validateRemovedMachineDeploymentClassesAreNotUsed(clusters []clusterv1.Cluster, old, new *clusterv1.ClusterClass) field.ErrorList {
func (webhook *ClusterClass) validateRemovedMachineDeploymentClassesAreNotUsed(clusters []clusterv1.Cluster, oldClusterClass, newClusterClass *clusterv1.ClusterClass) field.ErrorList {
var allErrs field.ErrorList

removedClasses := webhook.removedMachineClasses(old, new)
removedClasses := webhook.removedMachineClasses(oldClusterClass, newClusterClass)
// If no classes have been removed return early as no further checks are needed.
if len(removedClasses) == 0 {
return nil
Expand All @@ -374,11 +374,11 @@ func (webhook *ClusterClass) validateRemovedMachineDeploymentClassesAreNotUsed(c
return allErrs
}

func (webhook *ClusterClass) removedMachineClasses(old, new *clusterv1.ClusterClass) sets.String {
func (webhook *ClusterClass) removedMachineClasses(oldClusterClass, newClusterClass *clusterv1.ClusterClass) sets.String {
removedClasses := sets.NewString()

classes := webhook.classNamesFromWorkerClass(new.Spec.Workers)
for _, oldClass := range old.Spec.Workers.MachineDeployments {
classes := webhook.classNamesFromWorkerClass(newClusterClass.Spec.Workers)
for _, oldClass := range oldClusterClass.Spec.Workers.MachineDeployments {
if !classes.Has(oldClass.Class) {
removedClasses.Insert(oldClass.Class)
}
Expand Down

0 comments on commit c3e5f09

Please sign in to comment.