Skip to content

Commit

Permalink
fix: validation for property selector name (#808)
Browse files Browse the repository at this point in the history
  • Loading branch information
@Arvindthiru
Arvindthiru authored May 29, 2024
1 parent baeea07 commit 3416e48
Show file tree
Hide file tree
Showing 4 changed files with 186 additions and 22 deletions.
9 changes: 9 additions & 0 deletions pkg/propertyprovider/commons.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,4 +23,13 @@ const (
TotalMemoryCapacityProperty = "resources.kubernetes-fleet.io/total-memory"
AllocatableMemoryCapacityProperty = "resources.kubernetes-fleet.io/allocatable-memory"
AvailableMemoryCapacityProperty = "resources.kubernetes-fleet.io/available-memory"

// ResourcePropertyNamePrefix is the prefix (also known as the subdomain) of the label name
// associated with all resource properties.
ResourcePropertyNamePrefix = "resources.kubernetes-fleet.io/"

// Below are a list of supported capacity types.
TotalCapacityName = "total"
AllocatableCapacityName = "allocatable"
AvailableCapacityName = "available"
)
22 changes: 6 additions & 16 deletions pkg/scheduler/framework/plugins/clusteraffinity/types.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,17 +17,7 @@ import (

clusterv1beta1 "go.goms.io/fleet/apis/cluster/v1beta1"
placementv1beta1 "go.goms.io/fleet/apis/placement/v1beta1"
)

const (
// resourcePropertyNamePrefix is the prefix (also known as the subdomain) of the label name
// associated with all resource properties.
resourcePropertyNamePrefix = "resources.kubernetes-fleet.io/"

// Below are a list of supported capacity types.
totalCapacityName = "total"
allocatableCapacityName = "allocatable"
availableCapacityName = "available"
"go.goms.io/fleet/pkg/propertyprovider"
)

// clusterRequirement is a type alias for ClusterSelectorTerm in the API, which allows
Expand Down Expand Up @@ -55,13 +45,13 @@ func retrieveResourceUsageFrom(cluster *clusterv1beta1.MemberCluster, name strin
var q resource.Quantity
var found bool
switch cn {
case totalCapacityName:
case propertyprovider.TotalCapacityName:
// The property concerns the total capacity of a resource.
q, found = cluster.Status.ResourceUsage.Capacity[corev1.ResourceName(tn)]
case allocatableCapacityName:
case propertyprovider.AllocatableCapacityName:
// The property concerns the allocatable capacity of a resource.
q, found = cluster.Status.ResourceUsage.Allocatable[corev1.ResourceName(tn)]
case availableCapacityName:
case propertyprovider.AvailableCapacityName:
// The property concerns the available capacity of a resource.
q, found = cluster.Status.ResourceUsage.Available[corev1.ResourceName(tn)]
default:
Expand Down Expand Up @@ -89,8 +79,8 @@ func retrievePropertyValueFrom(cluster *clusterv1beta1.MemberCluster, name strin
// Check if the expression concerns a resource property.
var q *resource.Quantity
var err error
if strings.HasPrefix(name, resourcePropertyNamePrefix) {
name, _ := strings.CutPrefix(name, resourcePropertyNamePrefix)
if strings.HasPrefix(name, propertyprovider.ResourcePropertyNamePrefix) {
name, _ := strings.CutPrefix(name, propertyprovider.ResourcePropertyNamePrefix)

// Retrieve the property value from the cluster resource usage data.
q, err = retrieveResourceUsageFrom(cluster, name)
Expand Down
20 changes: 20 additions & 0 deletions pkg/utils/validator/clusterresourceplacement.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ package validator
import (
"errors"
"fmt"
"reflect"
"strings"

corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/meta"
Expand All @@ -22,6 +24,7 @@ import (

placementv1beta1 "go.goms.io/fleet/apis/placement/v1beta1"
fleetv1alpha1 "go.goms.io/fleet/apis/v1alpha1"
"go.goms.io/fleet/pkg/propertyprovider"
"go.goms.io/fleet/pkg/utils/controller"
"go.goms.io/fleet/pkg/utils/informer"
)
Expand All @@ -34,6 +37,10 @@ var (
invalidTolerationKeyErrFmt = "invalid toleration key %+v: %s"
invalidTolerationValueErrFmt = "invalid toleration value %+v: %s"
uniqueTolerationErrFmt = "toleration %+v already exists, tolerations must be unique"

// Below is the map of supported capacity types.
supportedResourceCapacityTypesMap = map[string]bool{propertyprovider.TotalCapacityName: true, propertyprovider.AllocatableCapacityName: true, propertyprovider.AvailableCapacityName: true}
supportedResourceCapacityTypes = reflect.ValueOf(supportedResourceCapacityTypesMap).MapKeys()
)

// ValidateClusterResourcePlacementAlpha validates a ClusterResourcePlacement v1alpha1 object.
Expand Down Expand Up @@ -436,6 +443,19 @@ func validatePropertySorter(propertySorter *placementv1beta1.PropertySorter) err
}

func validateName(name string) error {
// we expect the resource property names to be in this format `[PREFIX]/[CAPACITY_TYPE]-[RESOURCE_NAME]`.
if strings.HasPrefix(name, propertyprovider.ResourcePropertyNamePrefix) {
resourcePropertyName, _ := strings.CutPrefix(name, propertyprovider.ResourcePropertyNamePrefix)
// n=2 since we only care about the first segment to check capacity type.
segments := strings.SplitN(resourcePropertyName, "-", 2)
if len(segments) != 2 {
return fmt.Errorf("invalid resource property name %s, expected format is [PREFIX]/[CAPACITY_TYPE]-[RESOURCE_NAME]", name)
}
if !supportedResourceCapacityTypesMap[segments[0]] {
return fmt.Errorf("invalid capacity type in resource property name %s, supported values are %+v", name, supportedResourceCapacityTypes)
}
}

if err := validation.IsQualifiedName(name); err != nil {
return fmt.Errorf("name is not a valid Kubernetes label name: %v", err)
}
Expand Down
157 changes: 151 additions & 6 deletions pkg/utils/validator/clusterresourceplacement_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -370,7 +370,7 @@ func TestValidateClusterResourcePlacement(t *testing.T) {
t.Errorf("ValidateClusterResourcePlacement() error = %v, wantErr %v", gotErr, testCase.wantErr)
}
if testCase.wantErr && !strings.Contains(gotErr.Error(), testCase.wantErrMsg) {
t.Errorf("ValidateClusterResourcePlacement() got %v, should containt want %s", gotErr, testCase.wantErrMsg)
t.Errorf("ValidateClusterResourcePlacement() got %v, should contain want %s", gotErr, testCase.wantErrMsg)
}
})
}
Expand Down Expand Up @@ -489,7 +489,7 @@ func TestValidateClusterResourcePlacement_RolloutStrategy(t *testing.T) {
t.Errorf("validateRolloutStrategy() error = %v, wantErr %v", gotErr, testCase.wantErr)
}
if testCase.wantErr && !strings.Contains(gotErr.Error(), testCase.wantErrMsg) {
t.Errorf("validateRolloutStrategy() got %v, should containt want %s", gotErr, testCase.wantErrMsg)
t.Errorf("validateRolloutStrategy() got %v, should contain want %s", gotErr, testCase.wantErrMsg)
}
})
}
Expand Down Expand Up @@ -605,7 +605,7 @@ func TestValidateClusterResourcePlacement_PickFixedPlacementPolicy(t *testing.T)
t.Errorf("validatePlacementPolicy() error = %v, wantErr %v", gotErr, testCase.wantErr)
}
if testCase.wantErr && !strings.Contains(gotErr.Error(), testCase.wantErrMsg) {
t.Errorf("validatePlacementPolicy() got %v, should containt want %s", gotErr, testCase.wantErrMsg)
t.Errorf("validatePlacementPolicy() got %v, should contain want %s", gotErr, testCase.wantErrMsg)
}
})
}
Expand Down Expand Up @@ -793,6 +793,92 @@ func TestValidateClusterResourcePlacement_PickAllPlacementPolicy(t *testing.T) {
wantErr: true,
wantErrMsg: "operator Eq requires exactly one value, got 2",
},
"invalid placement policy - PickAll with invalid property selector name, invalid capacity type": {
policy: &placementv1beta1.PlacementPolicy{
PlacementType: placementv1beta1.PickAllPlacementType,
Affinity: &placementv1beta1.Affinity{
ClusterAffinity: &placementv1beta1.ClusterAffinity{
RequiredDuringSchedulingIgnoredDuringExecution: &placementv1beta1.ClusterSelector{
ClusterSelectorTerms: []placementv1beta1.ClusterSelectorTerm{
{
LabelSelector: &metav1.LabelSelector{
MatchLabels: map[string]string{"test-key1": "test-value1"},
},
PropertySelector: &placementv1beta1.PropertySelector{
MatchExpressions: []placementv1beta1.PropertySelectorRequirement{
{
Name: "resources.kubernetes-fleet.io/node-count",
Operator: placementv1beta1.PropertySelectorEqualTo,
Values: []string{"2"},
},
},
},
},
},
},
},
},
},
wantErr: true,
wantErrMsg: "invalid capacity type in resource property name resources.kubernetes-fleet.io/node-count, supported values are [total allocatable available]",
},
"invalid placement policy - PickAll with invalid property selector name, no segments": {
policy: &placementv1beta1.PlacementPolicy{
PlacementType: placementv1beta1.PickAllPlacementType,
Affinity: &placementv1beta1.Affinity{
ClusterAffinity: &placementv1beta1.ClusterAffinity{
RequiredDuringSchedulingIgnoredDuringExecution: &placementv1beta1.ClusterSelector{
ClusterSelectorTerms: []placementv1beta1.ClusterSelectorTerm{
{
LabelSelector: &metav1.LabelSelector{
MatchLabels: map[string]string{"test-key1": "test-value1"},
},
PropertySelector: &placementv1beta1.PropertySelector{
MatchExpressions: []placementv1beta1.PropertySelectorRequirement{
{
Name: "resources.kubernetes-fleet.io/totalcpu",
Operator: placementv1beta1.PropertySelectorEqualTo,
Values: []string{"2"},
},
},
},
},
},
},
},
},
},
wantErr: true,
wantErrMsg: "invalid resource property name resources.kubernetes-fleet.io/totalcpu, expected format is [PREFIX]/[CAPACITY_TYPE]-[RESOURCE_NAME]",
},
"valid placement policy - PickAll with valid property selector name": {
policy: &placementv1beta1.PlacementPolicy{
PlacementType: placementv1beta1.PickAllPlacementType,
Affinity: &placementv1beta1.Affinity{
ClusterAffinity: &placementv1beta1.ClusterAffinity{
RequiredDuringSchedulingIgnoredDuringExecution: &placementv1beta1.ClusterSelector{
ClusterSelectorTerms: []placementv1beta1.ClusterSelectorTerm{
{
LabelSelector: &metav1.LabelSelector{
MatchLabels: map[string]string{"test-key1": "test-value1"},
},
PropertySelector: &placementv1beta1.PropertySelector{
MatchExpressions: []placementv1beta1.PropertySelectorRequirement{
{
Name: "resources.kubernetes-fleet.io/allocatable-memory",
Operator: placementv1beta1.PropertySelectorEqualTo,
Values: []string{"2"},
},
},
},
},
},
},
},
},
},
wantErr: false,
},
}
for testName, testCase := range tests {
t.Run(testName, func(t *testing.T) {
Expand All @@ -801,7 +887,7 @@ func TestValidateClusterResourcePlacement_PickAllPlacementPolicy(t *testing.T) {
t.Errorf("validatePlacementPolicy() error = %v, wantErr %v", gotErr, testCase.wantErr)
}
if testCase.wantErr && !strings.Contains(gotErr.Error(), testCase.wantErrMsg) {
t.Errorf("validatePlacementPolicy() got %v, should containt want %s", gotErr, testCase.wantErrMsg)
t.Errorf("validatePlacementPolicy() got %v, should contain want %s", gotErr, testCase.wantErrMsg)
}
})
}
Expand Down Expand Up @@ -1104,6 +1190,65 @@ func TestValidateClusterResourcePlacement_PickNPlacementPolicy(t *testing.T) {
wantErr: true,
wantErrMsg: "toleration key cannot be empty, when operator is Equal",
},
"invalid placement policy - PickN with invalid property selector name, invalid label name": {
policy: &placementv1beta1.PlacementPolicy{
PlacementType: placementv1beta1.PickNPlacementType,
NumberOfClusters: &positiveNumberOfClusters,
Affinity: &placementv1beta1.Affinity{
ClusterAffinity: &placementv1beta1.ClusterAffinity{
RequiredDuringSchedulingIgnoredDuringExecution: &placementv1beta1.ClusterSelector{
ClusterSelectorTerms: []placementv1beta1.ClusterSelectorTerm{
{
LabelSelector: &metav1.LabelSelector{
MatchLabels: map[string]string{"test-key1": "test-value1"},
},
PropertySelector: &placementv1beta1.PropertySelector{
MatchExpressions: []placementv1beta1.PropertySelectorRequirement{
{
Name: "resources.kubernetes-fleet.io/total-nospecialchars%^=@",
Operator: placementv1beta1.PropertySelectorEqualTo,
Values: []string{"2"},
},
},
},
},
},
},
},
},
},
wantErr: true,
wantErrMsg: "name is not a valid Kubernetes label name",
},
"valid placement policy - PickN with valid property selector name": {
policy: &placementv1beta1.PlacementPolicy{
PlacementType: placementv1beta1.PickNPlacementType,
NumberOfClusters: &positiveNumberOfClusters,
Affinity: &placementv1beta1.Affinity{
ClusterAffinity: &placementv1beta1.ClusterAffinity{
RequiredDuringSchedulingIgnoredDuringExecution: &placementv1beta1.ClusterSelector{
ClusterSelectorTerms: []placementv1beta1.ClusterSelectorTerm{
{
LabelSelector: &metav1.LabelSelector{
MatchLabels: map[string]string{"test-key1": "test-value1"},
},
PropertySelector: &placementv1beta1.PropertySelector{
MatchExpressions: []placementv1beta1.PropertySelectorRequirement{
{
Name: "resources.kubernetes-fleet.io/total-allocatable-cpu",
Operator: placementv1beta1.PropertySelectorEqualTo,
Values: []string{"2"},
},
},
},
},
},
},
},
},
},
wantErr: false,
},
}

for testName, testCase := range tests {
Expand All @@ -1113,7 +1258,7 @@ func TestValidateClusterResourcePlacement_PickNPlacementPolicy(t *testing.T) {
t.Errorf("validatePlacementPolicy() error = %v, wantErr %v", gotErr, testCase.wantErr)
}
if testCase.wantErr && !strings.Contains(gotErr.Error(), testCase.wantErrMsg) {
t.Errorf("validatePlacementPolicy() got %v, should containt want %s", gotErr, testCase.wantErrMsg)
t.Errorf("validatePlacementPolicy() got %v, should contain want %s", gotErr, testCase.wantErrMsg)
}
})
}
Expand Down Expand Up @@ -1414,7 +1559,7 @@ func TestValidateTolerations(t *testing.T) {
t.Errorf("validateTolerations() error = %v, wantErr %v", gotErr, testCase.wantErr)
}
if testCase.wantErr && !strings.Contains(gotErr.Error(), testCase.wantErrMsg) {
t.Errorf("validateTolerations() got %v, should containt want %s", gotErr, testCase.wantErrMsg)
t.Errorf("validateTolerations() got %v, should contain want %s", gotErr, testCase.wantErrMsg)
}
})
}
Expand Down

0 comments on commit 3416e48

Please sign in to comment.