Skip to content

Commit

Permalink
review fixes
Browse files Browse the repository at this point in the history
  • Loading branch information
@chrischdi
chrischdi committed Feb 20, 2024
1 parent da4fa22 commit 09f3520
Show file tree
Hide file tree
Showing 4 changed files with 43 additions and 32 deletions.
5 changes: 3 additions & 2 deletions docs/book/src/reference/labels_and_annotations.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@


| Label | Note |
| :---------------------------------------- | :-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
|:------------------------------------------|:----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| cluster.x-k8s.io/cluster-name | It is set on machines linked to a cluster and external objects(bootstrap and infrastructure providers). |
| topology.cluster.x-k8s.io/owned | It is set on all the object which are managed as part of a ClusterTopology. |
| topology.cluster.x-k8s.io/deployment-name | It is set on the generated MachineDeployment objects to track the name of the MachineDeployment topology it represents. |
Expand All @@ -21,11 +21,12 @@
**Supported Annotations:**

| Annotation | Note |
| :--------------------------------------------------------------- | :---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
|:-----------------------------------------------------------------|:------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| clusterctl.cluster.x-k8s.io/skip-crd-name-preflight-check | Can be placed on provider CRDs, so that clusterctl doesn't emit an error if the CRD doesn't comply with Cluster APIs naming scheme. Only CRDs that are referenced by core Cluster API CRDs have to comply with the naming scheme. |
| clusterctl.cluster.x-k8s.io/delete-for-move | DeleteForMoveAnnotation will be set to objects that are going to be deleted from the source cluster after being moved to the target cluster during the clusterctl move operation. It will help any validation webhook to take decision based on it. |
| clusterctl.cluster.x-k8s.io/block-move | BlockMoveAnnotation prevents the cluster move operation from starting if it is defined on at least one of the objects in scope. Provider controllers are expected to set the annotation on resources that cannot be instantaneously paused and remove the annotation when the resource has been actually paused. |
| unsafe.topology.cluster.x-k8s.io/disable-update-class-name-check | It can be used to disable the webhook check on update that disallows a pre-existing Cluster to be populated with Topology information and Class. |
| unsafe.topology.cluster.x-k8s.io/disable-update-version-check | It can be used to disable the webhook checks on update that disallows updating the `.topology.spec.version` on certain conditions. |
| cluster.x-k8s.io/cluster-name | It is set on nodes identifying the name of the cluster the node belongs to. |
| cluster.x-k8s.io/cluster-namespace | It is set on nodes identifying the namespace of the cluster the node belongs to. |
| cluster.x-k8s.io/machine | It is set on nodes identifying the machine the node belongs to. |
Expand Down
48 changes: 20 additions & 28 deletions internal/webhooks/cluster.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import (
"github.com/pkg/errors"
apierrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/runtime"
kerrors "k8s.io/apimachinery/pkg/util/errors"
"k8s.io/apimachinery/pkg/util/validation"
"k8s.io/apimachinery/pkg/util/validation/field"
"k8s.io/apimachinery/pkg/util/wait"
Expand Down Expand Up @@ -363,8 +364,8 @@ func (webhook *Cluster) validateTopology(ctx context.Context, oldCluster, newClu
log.Info(warningMsg)
allWarnings = append(allWarnings, warningMsg)
} else {
if errs := webhook.validateTopologyVersion(ctx, fldPath.Child("version"), newCluster.Spec.Topology.Version, inVersion, oldVersion, oldCluster); len(errs) > 0 {
allErrs = append(allErrs, errs...)
if err := webhook.validateTopologyVersion(ctx, fldPath.Child("version"), newCluster.Spec.Topology.Version, inVersion, oldVersion, oldCluster); err != nil {
allErrs = append(allErrs, err)
}
}

Expand All @@ -390,14 +391,14 @@ func (webhook *Cluster) validateTopology(ctx context.Context, oldCluster, newClu
return allWarnings, allErrs
}

func (webhook *Cluster) validateTopologyVersion(ctx context.Context, fldPath *field.Path, fldValue string, inVersion, oldVersion semver.Version, oldCluster *clusterv1.Cluster) field.ErrorList {
func (webhook *Cluster) validateTopologyVersion(ctx context.Context, fldPath *field.Path, fldValue string, inVersion, oldVersion semver.Version, oldCluster *clusterv1.Cluster) *field.Error {
// Version could only be increased.
if inVersion.NE(semver.Version{}) && oldVersion.NE(semver.Version{}) && version.Compare(inVersion, oldVersion, version.WithBuildTags()) == -1 {
return field.ErrorList{field.Invalid(
return field.Invalid(
fldPath,
fldValue,
fmt.Sprintf("version cannot be decreased from %q to %q", oldVersion, inVersion),
)}
)
}

// A +2 minor version upgrade is not allowed.
Expand All @@ -407,12 +408,11 @@ func (webhook *Cluster) validateTopologyVersion(ctx context.Context, fldPath *fi
Patch: 0,
}
if inVersion.GTE(ceilVersion) {
return field.ErrorList{field.Invalid(
return field.Invalid(
fldPath,
fldValue,
fmt.Sprintf("version cannot be increased from %q to %q", oldVersion, inVersion),
),
}
)
}

// Only check the following cases if the minor version increases by 1 (we already return above for >= 2).
Expand All @@ -427,36 +427,28 @@ func (webhook *Cluster) validateTopologyVersion(ctx context.Context, fldPath *fi
return nil
}

allErrs := field.ErrorList{}
allErrs := []error{}
// minor version cannot be increased if control plane is upgrading or not yet on the current version
if err := validateTopologyControlPlaneVersion(ctx, webhook.Client, oldCluster, oldVersion); err != nil {
allErrs = append(allErrs, field.Invalid(
fldPath,
fldValue,
fmt.Sprintf("blocking version update due to ControlPlane version check: %v", err),
))
allErrs = append(allErrs, fmt.Errorf("blocking version update due to ControlPlane version check: %v", err))
}

// minor version cannot be increased if MachineDeployments are upgrading or not yet on the current version
if err := validateTopologyMachineDeploymentVersions(ctx, webhook.Client, oldCluster, oldVersion); err != nil {
allErrs = append(allErrs, field.Invalid(
fldPath,
fldValue,
fmt.Sprintf("blocking version update due to MachineDeployment version check: %v", err),
))
allErrs = append(allErrs, fmt.Errorf("blocking version update due to MachineDeployment version check: %v", err))
}

// minor version cannot be increased if MachinePools are upgrading or not yet on the current version
if err := validateTopologyMachinePoolVersions(ctx, webhook.Client, webhook.Tracker, oldCluster, oldVersion); err != nil {
allErrs = append(allErrs, field.Invalid(
fldPath,
fldValue,
fmt.Sprintf("blocking version update due to MachinePool version check: %v", err),
))
allErrs = append(allErrs, fmt.Errorf("blocking version update due to MachinePool version check: %v", err))
}

if len(allErrs) > 0 {
return allErrs
return field.Invalid(
fldPath,
fldValue,
fmt.Sprintf("minor version update cannot happen at this time: %v", kerrors.NewAggregate(allErrs)),
)
}

return nil
Expand Down Expand Up @@ -497,7 +489,7 @@ func validateTopologyControlPlaneVersion(ctx context.Context, ctrlClient client.
}

if upgrading {
return errors.New("ControlPlane is currently upgrading")
return errors.New("ControlPlane is still completing a previous upgrade")
}

return nil
Expand Down Expand Up @@ -548,7 +540,7 @@ func validateTopologyMachineDeploymentVersions(ctx context.Context, ctrlClient c
}

if len(mdUpgradingNames) > 0 {
return fmt.Errorf("upgrading MachineDeployments: [%s]", strings.Join(mdUpgradingNames, ", "))
return fmt.Errorf("there are MachineDeployments still completing a previous upgrade: [%s]", strings.Join(mdUpgradingNames, ", "))
}

return nil
Expand Down Expand Up @@ -605,7 +597,7 @@ func validateTopologyMachinePoolVersions(ctx context.Context, ctrlClient client.
}

if len(mpUpgradingNames) > 0 {
return fmt.Errorf("upgrading MachinePools: [%s]", strings.Join(mpUpgradingNames, ", "))
return fmt.Errorf("there are MachinePools still completing a previous upgrade: [%s]", strings.Join(mpUpgradingNames, ", "))
}

return nil
Expand Down
18 changes: 18 additions & 0 deletions internal/webhooks/cluster_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1400,6 +1400,14 @@ func TestClusterTopologyValidation(t *testing.T) {
WithTopology(builder.ClusterTopology().
WithClass("foo").
WithVersion("v1.19.1").
WithMachineDeployment(
builder.MachineDeploymentTopology("workers1").
WithClass("aa").
Build()).
WithMachinePool(
builder.MachinePoolTopology("pool1").
WithClass("aa").
Build()).
Build()).
Build(),
in: builder.Cluster("fooboo", "cluster1").
Expand All @@ -1413,6 +1421,16 @@ func TestClusterTopologyValidation(t *testing.T) {
builder.ControlPlane("fooboo", "cluster1-cp").WithVersion("v1.19.1").
WithStatusFields(map[string]interface{}{"status.version": "v1.19.1"}).
Build(),
builder.MachineDeployment("fooboo", "cluster1-workers1").WithLabels(map[string]string{
clusterv1.ClusterNameLabel: "cluster1",
clusterv1.ClusterTopologyOwnedLabel: "",
clusterv1.ClusterTopologyMachineDeploymentNameLabel: "workers1",
}).WithVersion("v1.19.1").Build(),
builder.MachinePool("fooboo", "cluster1-pool1").WithLabels(map[string]string{
clusterv1.ClusterNameLabel: "cluster1",
clusterv1.ClusterTopologyOwnedLabel: "",
clusterv1.ClusterTopologyMachinePoolNameLabel: "pool1",
}).WithVersion("v1.19.1").Build(),
},
},
{
Expand Down
4 changes: 2 additions & 2 deletions main.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -372,7 +372,7 @@ func setupIndexes(ctx context.Context, mgr ctrl.Manager) {
}
}

func setupReconcilers(ctx context.Context, mgr ctrl.Manager) *remote.ClusterCacheTracker {
func setupReconcilers(ctx context.Context, mgr ctrl.Manager) webhooks.ClusterCacheTrackerReader {
secretCachingClient, err := client.New(mgr.GetConfig(), client.Options{
HTTPClient: mgr.GetHTTPClient(),
Cache: &client.CacheOptions{
Expand Down Expand Up @@ -568,7 +568,7 @@ func setupReconcilers(ctx context.Context, mgr ctrl.Manager) *remote.ClusterCach
return tracker
}

func setupWebhooks(mgr ctrl.Manager, tracker *remote.ClusterCacheTracker) {
func setupWebhooks(mgr ctrl.Manager, tracker webhooks.ClusterCacheTrackerReader) {
// NOTE: ClusterClass and managed topologies are behind ClusterTopology feature gate flag; the webhook
// is going to prevent creating or updating new objects in case the feature flag is disabled.
if err := (&webhooks.ClusterClass{Client: mgr.GetClient()}).SetupWebhookWithManager(mgr); err != nil {
Expand Down

0 comments on commit 09f3520

Please sign in to comment.