Skip to content

Commit

Permalink
fix: resource selector uses name or label selector (Azure#688)
Browse files Browse the repository at this point in the history
  • Loading branch information
@Arvindthiru
Arvindthiru authored Feb 20, 2024
1 parent 14712f0 commit 40242db
Show file tree
Hide file tree
Showing 2 changed files with 95 additions and 18 deletions.
29 changes: 12 additions & 17 deletions pkg/controllers/resourcechange/resourcechange_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ import (
"go.goms.io/fleet/pkg/utils/controller"
"go.goms.io/fleet/pkg/utils/informer"
"go.goms.io/fleet/pkg/utils/keys"
"go.goms.io/fleet/pkg/utils/validator"
)

// Reconciler finds the placements that reference to any resource.
Expand Down Expand Up @@ -259,11 +258,6 @@ func collectAllAffectedPlacementsV1Alpha1(res *unstructured.Unstructured, crpLis
match := false
var placement fleetv1alpha1.ClusterResourcePlacement
_ = runtime.DefaultUnstructuredConverter.FromUnstructured(crp.DeepCopyObject().(*unstructured.Unstructured).Object, &placement)
// TODO: remove after we add validation webhooks
if err := validator.ValidateClusterResourcePlacementAlpha(&placement); err != nil {
klog.ErrorS(err, "find one invalid cluster resource placement", "placement", klog.KObj(&placement))
continue
}
// find the placements selected this resource (before this change)
for _, selectedRes := range placement.Status.SelectedResources {
if selectedRes.Group == res.GroupVersionKind().Group && selectedRes.Version == res.GroupVersionKind().Version &&
Expand All @@ -282,11 +276,12 @@ func collectAllAffectedPlacementsV1Alpha1(res *unstructured.Unstructured, crpLis
continue
}
// if there is 1 selector match, it is a placement match, add only once
if selector.Name == res.GetName() {
placements[placement.Name] = true
break
}
if matchSelectorLabelSelectorV1Alpha1(res.GetLabels(), selector) {
if selector.Name != "" {
if selector.Name == res.GetName() {
placements[placement.Name] = true
break
}
} else if matchSelectorLabelSelectorV1Alpha1(res.GetLabels(), selector) {
placements[placement.Name] = true
break
}
Expand All @@ -302,7 +297,6 @@ func collectAllAffectedPlacementsV1Beta1(res *unstructured.Unstructured, crpList
match := false
var placement placementv1beta1.ClusterResourcePlacement
_ = runtime.DefaultUnstructuredConverter.FromUnstructured(crp.DeepCopyObject().(*unstructured.Unstructured).Object, &placement)

// find the placements selected this resource (before this change)
for _, selectedRes := range placement.Status.SelectedResources {
if selectedRes.Group == res.GroupVersionKind().Group && selectedRes.Version == res.GroupVersionKind().Version &&
Expand All @@ -321,11 +315,12 @@ func collectAllAffectedPlacementsV1Beta1(res *unstructured.Unstructured, crpList
continue
}
// if there is 1 selector match, it is a placement match, add only once
if selector.Name == res.GetName() {
placements[placement.Name] = true
break
}
if matchSelectorLabelSelectorV1Beta1(res.GetLabels(), selector) {
if selector.Name != "" {
if selector.Name == res.GetName() {
placements[placement.Name] = true
break
}
} else if matchSelectorLabelSelectorV1Beta1(res.GetLabels(), selector) {
placements[placement.Name] = true
break
}
Expand Down
84 changes: 83 additions & 1 deletion pkg/controllers/resourcechange/resourcechange_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -582,6 +582,47 @@ func TestCollectAllAffectedPlacementsV1Alpha1(t *testing.T) {
},
wantCrp: make(map[string]bool),
},
"don't select placement with name, nil label selector for namespace with different name": {
res: matchRes,
crpList: []*fleetv1alpha1.ClusterResourcePlacement{
{
ObjectMeta: metav1.ObjectMeta{
Name: "resource-selected",
},
Spec: fleetv1alpha1.ClusterResourcePlacementSpec{
ResourceSelectors: []fleetv1alpha1.ClusterResourceSelector{
{
Group: corev1.GroupName,
Version: "v1",
Kind: "Namespace",
Name: "test-namespace-1",
},
},
},
},
},
wantCrp: make(map[string]bool),
},
"select placement with empty name, nil label selector for namespace": {
res: matchRes,
crpList: []*fleetv1alpha1.ClusterResourcePlacement{
{
ObjectMeta: metav1.ObjectMeta{
Name: "resource-selected",
},
Spec: fleetv1alpha1.ClusterResourcePlacementSpec{
ResourceSelectors: []fleetv1alpha1.ClusterResourceSelector{
{
Group: corev1.GroupName,
Version: "v1",
Kind: "Namespace",
},
},
},
},
},
wantCrp: map[string]bool{"resource-selected": true},
},
}
for name, tt := range tests {
t.Run(name, func(t *testing.T) {
Expand Down Expand Up @@ -909,6 +950,47 @@ func TestCollectAllAffectedPlacementsV1Beta1(t *testing.T) {
},
wantCrp: make(map[string]bool),
},
"don't select placement with name, nil label selector for namespace with different name": {
res: matchRes,
crpList: []*placementv1beta1.ClusterResourcePlacement{
{
ObjectMeta: metav1.ObjectMeta{
Name: "resource-selected",
},
Spec: placementv1beta1.ClusterResourcePlacementSpec{
ResourceSelectors: []placementv1beta1.ClusterResourceSelector{
{
Group: corev1.GroupName,
Version: "v1",
Kind: "Namespace",
Name: "test-namespace-1",
},
},
},
},
},
wantCrp: make(map[string]bool),
},
"select placement with empty name, nil label selector for namespace": {
res: matchRes,
crpList: []*placementv1beta1.ClusterResourcePlacement{
{
ObjectMeta: metav1.ObjectMeta{
Name: "resource-selected",
},
Spec: placementv1beta1.ClusterResourcePlacementSpec{
ResourceSelectors: []placementv1beta1.ClusterResourceSelector{
{
Group: corev1.GroupName,
Version: "v1",
Kind: "Namespace",
},
},
},
},
},
wantCrp: map[string]bool{"resource-selected": true},
},
}
for name, tt := range tests {
t.Run(name, func(t *testing.T) {
Expand All @@ -919,7 +1001,7 @@ func TestCollectAllAffectedPlacementsV1Beta1(t *testing.T) {
}
uRes, _ := runtime.DefaultUnstructuredConverter.ToUnstructured(tt.res)
validator.ResourceInformer = validator.MockResourceInformer{}
got := collectAllAffectedPlacementsV1Alpha1(&unstructured.Unstructured{Object: uRes}, crpList)
got := collectAllAffectedPlacementsV1Beta1(&unstructured.Unstructured{Object: uRes}, crpList)
if !reflect.DeepEqual(got, tt.wantCrp) {
t.Errorf("test case `%s` got = %v, wantResult %v", name, got, tt.wantCrp)
}
Expand Down

0 comments on commit 40242db

Please sign in to comment.