feat: Loosening webhook to allow for kubernetes-fleet.io/ labels to be modified (#210)

Author: audrastump

apis/placement/v1beta1/binding_types.go

@@ -28,7 +28,7 @@ const (
 	// SchedulerBindingCleanupFinalizer is a finalizer added to bindings to ensure we can look up the
 	// corresponding CRP name for deleting bindings to trigger a new scheduling cycle.
 	// TODO: migrate the finalizer to the new name "scheduler-binding-cleanup" in the future.
-	SchedulerBindingCleanupFinalizer = fleetPrefix + "scheduler-crb-cleanup"
+	SchedulerBindingCleanupFinalizer = FleetPrefix + "scheduler-crb-cleanup"
 )
 
 // make sure the BindingObj and BindingObjList interfaces are implemented by the

apis/placement/v1beta1/clusterresourceplacement_types.go

@@ -29,11 +29,11 @@ import (
 const (
 	// PlacementCleanupFinalizer is a finalizer added by the placement controller to all placement objects, to make sure
 	// that the placement controller can react to placement object deletions if necessary.
-	PlacementCleanupFinalizer = fleetPrefix + "crp-cleanup"
+	PlacementCleanupFinalizer = FleetPrefix + "crp-cleanup"
 
 	// SchedulerCleanupFinalizer is a finalizer added by the scheduler to placement objects, to make sure
 	// that all bindings derived from a placement object can be cleaned up after the placement object is deleted.
-	SchedulerCleanupFinalizer = fleetPrefix + "scheduler-cleanup"
+	SchedulerCleanupFinalizer = FleetPrefix + "scheduler-cleanup"
 )
 
 // make sure the PlacementObj and PlacementObjList interfaces are implemented by the
@@ -1521,7 +1521,7 @@ func (m *ClusterResourcePlacement) SetPlacementStatus(status PlacementStatus) {
 const (
 	// ResourcePlacementCleanupFinalizer is a finalizer added by the RP controller to all RPs, to make sure
 	// that the RP controller can react to RP deletions if necessary.
-	ResourcePlacementCleanupFinalizer = fleetPrefix + "rp-cleanup"
+	ResourcePlacementCleanupFinalizer = FleetPrefix + "rp-cleanup"
 )
 
 // +genclient

apis/placement/v1beta1/commons.go

@@ -58,32 +58,32 @@ const (
 )
 
 const (
-	// fleetPrefix is the prefix used for official fleet labels/annotations.
+	// FleetPrefix is the prefix used for official fleet labels/annotations.
 	// Unprefixed labels/annotations are reserved for end-users
 	// See https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#label-selector-and-annotation-conventions
-	fleetPrefix = "kubernetes-fleet.io/"
+	FleetPrefix = "kubernetes-fleet.io/"
 
 	// MemberClusterFinalizer is used to make sure that we handle gc of all the member cluster resources on the hub cluster.
-	MemberClusterFinalizer = fleetPrefix + "membercluster-finalizer"
+	MemberClusterFinalizer = FleetPrefix + "membercluster-finalizer"
 
 	// WorkFinalizer is used by the work generator to make sure that the binding is not deleted until the work objects
 	// it generates are all deleted, or used by the work controller to make sure the work has been deleted in the member
 	// cluster.
-	WorkFinalizer = fleetPrefix + "work-cleanup"
+	WorkFinalizer = FleetPrefix + "work-cleanup"
 
 	// ClusterResourcePlacementStatusCleanupFinalizer is a finalizer added by the controller to all ClusterResourcePlacementStatus objects, to make sure
 	// that the controller can react to ClusterResourcePlacementStatus deletions if necessary.
-	ClusterResourcePlacementStatusCleanupFinalizer = fleetPrefix + "cluster-resource-placement-status-cleanup"
+	ClusterResourcePlacementStatusCleanupFinalizer = FleetPrefix + "cluster-resource-placement-status-cleanup"
 
 	// PlacementTrackingLabel points to the placement that creates this resource binding.
 	// TODO: migrate the label content to "parent-placement" to work with both the PR and CRP
-	PlacementTrackingLabel = fleetPrefix + "parent-CRP"
+	PlacementTrackingLabel = FleetPrefix + "parent-CRP"
 
 	// IsLatestSnapshotLabel indicates if the snapshot is the latest one.
-	IsLatestSnapshotLabel = fleetPrefix + "is-latest-snapshot"
+	IsLatestSnapshotLabel = FleetPrefix + "is-latest-snapshot"
 
 	// FleetResourceLabelKey indicates that the resource is a fleet resource.
-	FleetResourceLabelKey = fleetPrefix + "is-fleet-resource"
+	FleetResourceLabelKey = FleetPrefix + "is-fleet-resource"
 
 	// FirstWorkNameFmt is the format of the name of the work generated with the first resource snapshot.
 	// The name of the first work is {crpName}-work.
@@ -105,59 +105,59 @@ const (
 	WorkNameWithEnvelopeCRFmt = "%s-envelope-%s"
 
 	// ParentClusterResourceOverrideSnapshotHashAnnotation is the annotation to work that contains the hash of the parent cluster resource override snapshot list.
-	ParentClusterResourceOverrideSnapshotHashAnnotation = fleetPrefix + "parent-cluster-resource-override-snapshot-hash"
+	ParentClusterResourceOverrideSnapshotHashAnnotation = FleetPrefix + "parent-cluster-resource-override-snapshot-hash"
 
 	// ParentResourceOverrideSnapshotHashAnnotation is the annotation to work that contains the hash of the parent resource override snapshot list.
-	ParentResourceOverrideSnapshotHashAnnotation = fleetPrefix + "parent-resource-override-snapshot-hash"
+	ParentResourceOverrideSnapshotHashAnnotation = FleetPrefix + "parent-resource-override-snapshot-hash"
 
 	// ParentResourceSnapshotNameAnnotation is the annotation applied to work that contains the name of the master resource snapshot that generates the work.
-	ParentResourceSnapshotNameAnnotation = fleetPrefix + "parent-resource-snapshot-name"
+	ParentResourceSnapshotNameAnnotation = FleetPrefix + "parent-resource-snapshot-name"
 
 	// ParentResourceSnapshotIndexLabel is the label applied to work that contains the index of the resource snapshot that generates the work.
-	ParentResourceSnapshotIndexLabel = fleetPrefix + "parent-resource-snapshot-index"
+	ParentResourceSnapshotIndexLabel = FleetPrefix + "parent-resource-snapshot-index"
 
 	// ParentBindingLabel is the label applied to work that contains the name of the binding that generates the work.
-	ParentBindingLabel = fleetPrefix + "parent-resource-binding"
+	ParentBindingLabel = FleetPrefix + "parent-resource-binding"
 
 	// ParentNamespaceLabel is the label applied to work that contains the namespace of the binding that generates the work.
-	ParentNamespaceLabel = fleetPrefix + "parent-placement-namespace"
+	ParentNamespaceLabel = FleetPrefix + "parent-placement-namespace"
 
 	// CRPGenerationAnnotation indicates the generation of the placement from which an object is derived or last updated.
 	// TODO: rename this variable
-	CRPGenerationAnnotation = fleetPrefix + "CRP-generation"
+	CRPGenerationAnnotation = FleetPrefix + "CRP-generation"
 
 	// EnvelopeConfigMapAnnotation indicates the configmap is an envelope configmap containing resources we need to apply to the member cluster instead of the configMap itself.
-	EnvelopeConfigMapAnnotation = fleetPrefix + "envelope-configmap"
+	EnvelopeConfigMapAnnotation = FleetPrefix + "envelope-configmap"
 
 	// EnvelopeTypeLabel marks the work object as generated from an envelope object.
 	// The value of the annotation is the type of the envelope object.
-	EnvelopeTypeLabel = fleetPrefix + "envelope-work"
+	EnvelopeTypeLabel = FleetPrefix + "envelope-work"
 
 	// EnvelopeNamespaceLabel contains the namespace of the envelope object that the work is generated from.
-	EnvelopeNamespaceLabel = fleetPrefix + "envelope-namespace"
+	EnvelopeNamespaceLabel = FleetPrefix + "envelope-namespace"
 
 	// EnvelopeNameLabel contains the name of the envelope object that the work is generated from.
-	EnvelopeNameLabel = fleetPrefix + "envelope-name"
+	EnvelopeNameLabel = FleetPrefix + "envelope-name"
 
 	// PreviousBindingStateAnnotation records the previous state of a binding.
 	// This is used to remember if an "unscheduled" binding was moved from a "bound" state or a "scheduled" state.
-	PreviousBindingStateAnnotation = fleetPrefix + "previous-binding-state"
+	PreviousBindingStateAnnotation = FleetPrefix + "previous-binding-state"
 
 	// ClusterStagedUpdateRunFinalizer is used by the ClusterStagedUpdateRun controller to make sure that the ClusterStagedUpdateRun
 	// object is not deleted until all its dependent resources are deleted.
-	ClusterStagedUpdateRunFinalizer = fleetPrefix + "stagedupdaterun-finalizer"
+	ClusterStagedUpdateRunFinalizer = FleetPrefix + "stagedupdaterun-finalizer"
 
 	// TargetUpdateRunLabel indicates the target update run on a staged run related object.
-	TargetUpdateRunLabel = fleetPrefix + "targetupdaterun"
+	TargetUpdateRunLabel = FleetPrefix + "targetupdaterun"
 
 	// UpdateRunDeleteStageName is the name of delete stage in the staged update run.
-	UpdateRunDeleteStageName = fleetPrefix + "deleteStage"
+	UpdateRunDeleteStageName = FleetPrefix + "deleteStage"
 
 	// IsLatestUpdateRunApprovalLabel indicates if the approval is the latest approval on a staged run.
-	IsLatestUpdateRunApprovalLabel = fleetPrefix + "isLatestUpdateRunApproval"
+	IsLatestUpdateRunApprovalLabel = FleetPrefix + "isLatestUpdateRunApproval"
 
 	// TargetUpdatingStageNameLabel indicates the updating stage name on a staged run related object.
-	TargetUpdatingStageNameLabel = fleetPrefix + "targetUpdatingStage"
+	TargetUpdatingStageNameLabel = FleetPrefix + "targetUpdatingStage"
 
 	// ApprovalTaskNameFmt is the format of the approval task name.
 	ApprovalTaskNameFmt = "%s-%s"

apis/placement/v1beta1/overridesnapshot_types.go

@@ -21,17 +21,17 @@ import metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 const (
 
 	// OverrideIndexLabel is the label that indicate the policy snapshot index of a cluster policy.
-	OverrideIndexLabel = fleetPrefix + "override-index"
+	OverrideIndexLabel = FleetPrefix + "override-index"
 
 	// OverrideSnapshotNameFmt is clusterResourceOverrideSnapshot name format: {CROName}-{OverrideSnapshotIndex}.
 	OverrideSnapshotNameFmt = "%s-%d"
 
 	// OverrideTrackingLabel is the label that points to the cluster resource override that creates a resource snapshot.
-	OverrideTrackingLabel = fleetPrefix + "parent-resource-override"
+	OverrideTrackingLabel = FleetPrefix + "parent-resource-override"
 
 	// OverrideFinalizer is a finalizer added by the override controllers to all override, to make sure
 	// that the override controller can react to override deletions if necessary.
-	OverrideFinalizer = fleetPrefix + "override-cleanup"
+	OverrideFinalizer = FleetPrefix + "override-cleanup"
 )
 
 // +genclient

apis/placement/v1beta1/policysnapshot_types.go

@@ -26,13 +26,13 @@ import (
 
 const (
 	// PolicyIndexLabel is the label that indicate the policy snapshot index of a cluster policy.
-	PolicyIndexLabel = fleetPrefix + "policy-index"
+	PolicyIndexLabel = FleetPrefix + "policy-index"
 
 	// PolicySnapshotNameFmt is clusterPolicySnapshot name format: {CRPName}-{PolicySnapshotIndex}.
 	PolicySnapshotNameFmt = "%s-%d"
 
 	// NumberOfClustersAnnotation is the annotation that indicates how many clusters should be selected for selectN placement type.
-	NumberOfClustersAnnotation = fleetPrefix + "number-of-clusters"
+	NumberOfClustersAnnotation = FleetPrefix + "number-of-clusters"
 )
 
 // make sure the PolicySnapshotObj and PolicySnapshotList interfaces are implemented by the

apis/placement/v1beta1/resourcesnapshot_types.go

@@ -27,23 +27,23 @@ import (
 
 const (
 	// ResourceIndexLabel is the label that indicate the resource snapshot index of a cluster resource snapshot.
-	ResourceIndexLabel = fleetPrefix + "resource-index"
+	ResourceIndexLabel = FleetPrefix + "resource-index"
 
 	// ResourceGroupHashAnnotation is the annotation that contains the value of the sha-256 hash
 	// value of all the snapshots belong to the same snapshot index.
-	ResourceGroupHashAnnotation = fleetPrefix + "resource-hash"
+	ResourceGroupHashAnnotation = FleetPrefix + "resource-hash"
 
 	// NumberOfEnvelopedObjectsAnnotation is the annotation that contains the number of the enveloped objects in the resource snapshot group.
-	NumberOfEnvelopedObjectsAnnotation = fleetPrefix + "number-of-enveloped-object"
+	NumberOfEnvelopedObjectsAnnotation = FleetPrefix + "number-of-enveloped-object"
 
 	// NumberOfResourceSnapshotsAnnotation is the annotation that contains the total number of resource snapshots.
-	NumberOfResourceSnapshotsAnnotation = fleetPrefix + "number-of-resource-snapshots"
+	NumberOfResourceSnapshotsAnnotation = FleetPrefix + "number-of-resource-snapshots"
 
 	// SubindexOfResourceSnapshotAnnotation is the annotation to store the subindex of resource snapshot in the group.
-	SubindexOfResourceSnapshotAnnotation = fleetPrefix + "subindex-of-resource-snapshot"
+	SubindexOfResourceSnapshotAnnotation = FleetPrefix + "subindex-of-resource-snapshot"
 
 	// NextResourceSnapshotCandidateDetectionTimeAnnotation is the annotation to store the time of next resourceSnapshot candidate detected by the controller.
-	NextResourceSnapshotCandidateDetectionTimeAnnotation = fleetPrefix + "next-resource-snapshot-candidate-detection-time"
+	NextResourceSnapshotCandidateDetectionTimeAnnotation = FleetPrefix + "next-resource-snapshot-candidate-detection-time"
 
 	// ResourceSnapshotNameFmt is resourcePolicySnapshot name format: {CRPName}-{resourceIndex}-snapshot.
 	ResourceSnapshotNameFmt = "%s-%d-snapshot"

apis/placement/v1beta1/work_types.go

@@ -40,10 +40,10 @@ import (
 // The following definitions are originally declared in the controllers/workv1alpha1/manager.go file.
 const (
 	// ManifestHashAnnotation is the annotation that indicates whether the spec of the object has been changed or not.
-	ManifestHashAnnotation = fleetPrefix + "spec-hash"
+	ManifestHashAnnotation = FleetPrefix + "spec-hash"
 
 	// LastAppliedConfigAnnotation is to record the last applied configuration on the object.
-	LastAppliedConfigAnnotation = fleetPrefix + "last-applied-configuration"
+	LastAppliedConfigAnnotation = FleetPrefix + "last-applied-configuration"
 
 	// WorkConditionTypeApplied represents workload in Work is applied successfully on the spoke cluster.
 	WorkConditionTypeApplied = "Applied"

pkg/webhook/validation/uservalidation.go

@@ -18,6 +18,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
 
 	clusterv1beta1 "github.com/kubefleet-dev/kubefleet/apis/cluster/v1beta1"
+	placementv1beta1 "github.com/kubefleet-dev/kubefleet/apis/placement/v1beta1"
 	fleetv1alpha1 "github.com/kubefleet-dev/kubefleet/apis/v1alpha1"
 	"github.com/kubefleet-dev/kubefleet/pkg/utils"
 )
@@ -109,14 +110,11 @@ func ValidateFleetMemberClusterUpdate(currentMC, oldMC clusterv1beta1.MemberClus
 		return admission.Denied(err.Error())
 	}
 
-	// Users are no longer allowed to modify labels of fleet member cluster through webhook.
-	// This will be disabled until member labels are accessible through CLI
-	if denyModifyMemberClusterLabels {
-		isLabelUpdated := isMapFieldUpdated(currentMC.GetLabels(), oldMC.GetLabels())
-		if isLabelUpdated && !isUserInGroup(userInfo, mastersGroup) {
-			klog.V(2).InfoS(DeniedModifyMemberClusterLabels, "user", userInfo.Username, "groups", userInfo.Groups, "operation", req.Operation, "GVK", req.RequestKind, "subResource", req.SubResource, "namespacedName", namespacedName)
-			return admission.Denied(DeniedModifyMemberClusterLabels)
-		}
+	isLabelUpdated := isMapFieldUpdated(currentMC.GetLabels(), oldMC.GetLabels())
+	if isLabelUpdated && !isUserInGroup(userInfo, mastersGroup) && shouldDenyLabelModification(currentMC.GetLabels(), oldMC.GetLabels(), denyModifyMemberClusterLabels) {
+		// allow any user to modify kubernetes-fleet.io/* labels, but restricts other label modifications given denyModifyMemberClusterLabels is true.
+		klog.V(2).InfoS(DeniedModifyMemberClusterLabels, "user", userInfo.Username, "groups", userInfo.Groups, "operation", req.Operation, "GVK", req.RequestKind, "subResource", req.SubResource, "namespacedName", namespacedName)
+		return admission.Denied(DeniedModifyMemberClusterLabels)
 	}
 
 	isAnnotationUpdated := isFleetAnnotationUpdated(currentMC.Annotations, oldMC.Annotations)
@@ -179,6 +177,29 @@ func isUserInGroup(userInfo authenticationv1.UserInfo, groupName string) bool {
 	return slices.Contains(userInfo.Groups, groupName)
 }
 
+// shouldDenyLabelModification returns true if any labels (besides kubernetes-fleet.io/* labels) are being modified and denyModifyMemberClusterLabels is true.
+func shouldDenyLabelModification(currentLabels, oldLabels map[string]string, denyModifyMemberClusterLabels bool) bool {
+	if !denyModifyMemberClusterLabels {
+		return false
+	}
+	for k, v := range currentLabels {
+		oldV, exists := oldLabels[k]
+		if !exists || oldV != v {
+			if !strings.HasPrefix(k, placementv1beta1.FleetPrefix) {
+				return true
+			}
+		}
+	}
+	for k := range oldLabels {
+		if _, exists := currentLabels[k]; !exists {
+			if !strings.HasPrefix(k, placementv1beta1.FleetPrefix) {
+				return true
+			}
+		}
+	}
+	return false
+}
+
 // isMemberClusterMapFieldUpdated return true if member cluster label is updated.
 func isMapFieldUpdated(currentMap, oldMap map[string]string) bool {
 	return !reflect.DeepEqual(currentMap, oldMap)