Skip to content

Commit 463fcca

Browse files
karmada-botkarmada-bot
authored
Merge pull request #6944 from XiShanYongYe-Chang/add-validate-for-binding
add valition for binding resourcebinding.karmada.io/dependencies annotation
2 parents c2609e5 + a1cac59 commit 463fcca

File tree

5 files changed

+93
-20
lines changed

5 files changed

+93
-20
lines changed

‎pkg/dependenciesdistributor/dependencies_distributor.go‎

Lines changed: 3 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -78,13 +78,6 @@ const (
7878
dependedByLabelKeyPrefix = "resourcebinding.karmada.io/depended-by-"
7979
)
8080

81-
// well-know annotations
82-
const (
83-
// dependenciesAnnotationKey is added to the independent binding,
84-
// it describes the names of dependencies (json serialized).
85-
dependenciesAnnotationKey = "resourcebinding.karmada.io/dependencies"
86-
)
87-
8881
// LabelsKey is the object key which is a unique identifier under a cluster, across all resources.
8982
type LabelsKey struct {
9083
keys.ClusterWideKey
@@ -211,7 +204,7 @@ func (d *DependenciesDistributor) reconcileResourceTemplate(key util.QueueKey) e
211204
// matchesWithBindingDependencies tells if the given object(resource template) is matched
212205
// with the dependencies of independent resourceBinding.
213206
func matchesWithBindingDependencies(resourceTemplateKey *LabelsKey, independentBinding *workv1alpha2.ResourceBinding) bool {
214-
dependencies, exist := independentBinding.Annotations[dependenciesAnnotationKey]
207+
dependencies, exist := independentBinding.Annotations[util.DependenciesAnnotationKey]
215208
if !exist {
216209
return false
217210
}
@@ -439,10 +432,10 @@ func (d *DependenciesDistributor) recordDependencies(ctx context.Context, indepe
439432
}
440433

441434
// dependencies are not updated, no need to update annotation.
442-
if oldDependencies, exist := objectAnnotation[dependenciesAnnotationKey]; exist && oldDependencies == dependenciesStr {
435+
if oldDependencies, exist := objectAnnotation[util.DependenciesAnnotationKey]; exist && oldDependencies == dependenciesStr {
443436
return nil
444437
}
445-
objectAnnotation[dependenciesAnnotationKey] = dependenciesStr
438+
objectAnnotation[util.DependenciesAnnotationKey] = dependenciesStr
446439

447440
return retry.RetryOnConflict(retry.DefaultRetry, func() (err error) {
448441
independentBinding.SetAnnotations(objectAnnotation)

‎pkg/dependenciesdistributor/dependencies_distributor_test.go‎

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -329,7 +329,7 @@ func Test_reconcileResourceTemplate(t *testing.T) {
329329
"app": "test",
330330
},
331331
Annotations: map[string]string{
332-
dependenciesAnnotationKey: "[{\"apiVersion\":\"apps/v1\",\"kind\":\"Deployment\",\"namespace\":\"test\",\"name\":\"demo-app\"}]",
332+
util.DependenciesAnnotationKey: "[{\"apiVersion\":\"apps/v1\",\"kind\":\"Deployment\",\"namespace\":\"test\",\"name\":\"demo-app\"}]",
333333
},
334334
},
335335
Spec: workv1alpha2.ResourceBindingSpec{
@@ -394,7 +394,7 @@ func Test_dependentObjectReferenceMatches(t *testing.T) {
394394
},
395395
referenceBinding: &workv1alpha2.ResourceBinding{
396396
ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{
397-
dependenciesAnnotationKey: "[{\"apiVersion\":\"example-stgzr.karmada.io/v1alpha1\",\"kind\":\"Foot5zmh\",\"namespace\":\"karmadatest-vpvll\",\"name\":\"cr-fxzq6\"}]",
397+
util.DependenciesAnnotationKey: "[{\"apiVersion\":\"example-stgzr.karmada.io/v1alpha1\",\"kind\":\"Foot5zmh\",\"namespace\":\"karmadatest-vpvll\",\"name\":\"cr-fxzq6\"}]",
398398
}},
399399
},
400400
},
@@ -415,7 +415,7 @@ func Test_dependentObjectReferenceMatches(t *testing.T) {
415415
},
416416
referenceBinding: &workv1alpha2.ResourceBinding{
417417
ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{
418-
dependenciesAnnotationKey: "[{\"apiVersion\":\"v1\",\"kind\":\"ConfigMap\",\"namespace\":\"karmadatest-h46wh\",\"name\":\"configmap-8w426\"}]",
418+
util.DependenciesAnnotationKey: "[{\"apiVersion\":\"v1\",\"kind\":\"ConfigMap\",\"namespace\":\"karmadatest-h46wh\",\"name\":\"configmap-8w426\"}]",
419419
}},
420420
},
421421
},
@@ -437,7 +437,7 @@ func Test_dependentObjectReferenceMatches(t *testing.T) {
437437
},
438438
referenceBinding: &workv1alpha2.ResourceBinding{
439439
ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{
440-
dependenciesAnnotationKey: "[{\"apiVersion\":\"v1\",\"kind\":\"ConfigMap\",\"namespace\":\"test\",\"labelSelector\":{\"matchExpressions\":[{\"key\":\"app\",\"operator\":\"In\",\"values\":[\"test\"]}]}}]",
440+
util.DependenciesAnnotationKey: "[{\"apiVersion\":\"v1\",\"kind\":\"ConfigMap\",\"namespace\":\"test\",\"labelSelector\":{\"matchExpressions\":[{\"key\":\"app\",\"operator\":\"In\",\"values\":[\"test\"]}]}}]",
441441
}},
442442
},
443443
},
@@ -1324,7 +1324,7 @@ func Test_recordDependencies(t *testing.T) {
13241324
Namespace: "test",
13251325
ResourceVersion: "1001",
13261326
Annotations: map[string]string{
1327-
dependenciesAnnotationKey: "[{\"apiVersion\":\"v1\",\"kind\":\"Pod\",\"namespace\":\"default\",\"name\":\"pod\"}]",
1327+
util.DependenciesAnnotationKey: "[{\"apiVersion\":\"v1\",\"kind\":\"Pod\",\"namespace\":\"default\",\"name\":\"pod\"}]",
13281328
},
13291329
},
13301330
Spec: workv1alpha2.ResourceBindingSpec{
@@ -1352,7 +1352,7 @@ func Test_recordDependencies(t *testing.T) {
13521352
Namespace: "test",
13531353
ResourceVersion: "1000",
13541354
Annotations: map[string]string{
1355-
dependenciesAnnotationKey: "[{\"apiVersion\":\"v1\",\"kind\":\"Pod\",\"namespace\":\"default\",\"name\":\"pod\"}]",
1355+
util.DependenciesAnnotationKey: "[{\"apiVersion\":\"v1\",\"kind\":\"Pod\",\"namespace\":\"default\",\"name\":\"pod\"}]",
13561356
},
13571357
},
13581358
Spec: workv1alpha2.ResourceBindingSpec{
@@ -1375,7 +1375,7 @@ func Test_recordDependencies(t *testing.T) {
13751375
Namespace: "test",
13761376
ResourceVersion: "1000",
13771377
Annotations: map[string]string{
1378-
dependenciesAnnotationKey: "[{\"apiVersion\":\"v1\",\"kind\":\"Pod\",\"namespace\":\"default\",\"name\":\"pod\"}]",
1378+
util.DependenciesAnnotationKey: "[{\"apiVersion\":\"v1\",\"kind\":\"Pod\",\"namespace\":\"default\",\"name\":\"pod\"}]",
13791379
},
13801380
},
13811381
Spec: workv1alpha2.ResourceBindingSpec{
@@ -1403,7 +1403,7 @@ func Test_recordDependencies(t *testing.T) {
14031403
Namespace: "test",
14041404
ResourceVersion: "1000",
14051405
Annotations: map[string]string{
1406-
dependenciesAnnotationKey: "[{\"apiVersion\":\"v1\",\"kind\":\"Pod\",\"namespace\":\"default\",\"name\":\"pod\"}]",
1406+
util.DependenciesAnnotationKey: "[{\"apiVersion\":\"v1\",\"kind\":\"Pod\",\"namespace\":\"default\",\"name\":\"pod\"}]",
14071407
},
14081408
},
14091409
Spec: workv1alpha2.ResourceBindingSpec{
@@ -1431,7 +1431,7 @@ func Test_recordDependencies(t *testing.T) {
14311431
Namespace: "test",
14321432
ResourceVersion: "1000",
14331433
Annotations: map[string]string{
1434-
dependenciesAnnotationKey: "[{\"apiVersion\":\"v1\",\"kind\":\"Pod\",\"namespace\":\"default\",\"name\":\"pod\"}]",
1434+
util.DependenciesAnnotationKey: "[{\"apiVersion\":\"v1\",\"kind\":\"Pod\",\"namespace\":\"default\",\"name\":\"pod\"}]",
14351435
},
14361436
},
14371437
Spec: workv1alpha2.ResourceBindingSpec{
@@ -1479,7 +1479,7 @@ func Test_recordDependencies(t *testing.T) {
14791479
Namespace: "test",
14801480
ResourceVersion: "1001",
14811481
Annotations: map[string]string{
1482-
dependenciesAnnotationKey: "[{\"apiVersion\":\"v1\",\"kind\":\"Pod\",\"namespace\":\"default\",\"name\":\"pod\"}]",
1482+
util.DependenciesAnnotationKey: "[{\"apiVersion\":\"v1\",\"kind\":\"Pod\",\"namespace\":\"default\",\"name\":\"pod\"}]",
14831483
},
14841484
},
14851485
Spec: workv1alpha2.ResourceBindingSpec{

‎pkg/util/constants.go‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -105,6 +105,10 @@ const (
105105

106106
// EndpointSliceProvisionClusterAnnotation is added to work of the dispatch EndpointSlice in consumption clusters' namespace.
107107
EndpointSliceProvisionClusterAnnotation = "endpointslice.karmada.io/provision-cluster"
108+
109+
// DependenciesAnnotationKey is added to the independent binding,
110+
// it describes the names of dependencies (json serialized).
111+
DependenciesAnnotationKey = "resourcebinding.karmada.io/dependencies"
108112
)
109113

110114
// Define finalizers used by karmada system.

‎pkg/webhook/resourcebinding/validating.go‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ package resourcebinding
1818

1919
import (
2020
"context"
21+
"encoding/json"
2122
"errors"
2223
"fmt"
2324
"net/http"
@@ -32,6 +33,7 @@ import (
3233
"sigs.k8s.io/controller-runtime/pkg/client"
3334
"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
3435

36+
configv1alpha1 "github.com/karmada-io/karmada/pkg/apis/config/v1alpha1"
3537
policyv1alpha1 "github.com/karmada-io/karmada/pkg/apis/policy/v1alpha1"
3638
workv1alpha2 "github.com/karmada-io/karmada/pkg/apis/work/v1alpha2"
3739
"github.com/karmada-io/karmada/pkg/features"
@@ -64,6 +66,11 @@ func (v *ValidatingAdmission) Handle(ctx context.Context, req admission.Request)
6466
}
6567
klog.V(2).Infof("Processing ResourceBinding(%s/%s) for request: %s (%s)", rb.Namespace, rb.Name, req.Operation, req.UID)
6668

69+
if err := v.validateBindingAnnotations(rb.Annotations, field.NewPath("metadata").Child("annotations")); err != nil {
70+
klog.Errorf("Admission denied for ResourceBinding %s/%s: %v", rb.Namespace, rb.Name, err)
71+
return admission.Denied(err.Error())
72+
}
73+
6774
if err := v.validateFederatedResourceQuota(ctx, req, rb, oldRB); err != nil {
6875
if apierrors.IsInternalError(err) {
6976
klog.Errorf("Internal error while processing ResourceBinding %s/%s: %v", rb.Namespace, rb.Name, err)
@@ -83,6 +90,22 @@ func (v *ValidatingAdmission) Handle(ctx context.Context, req admission.Request)
8390
return admission.Allowed("")
8491
}
8592

93+
func (v *ValidatingAdmission) validateBindingAnnotations(annotations map[string]string, fldPath *field.Path) error {
94+
var allErrs field.ErrorList
95+
96+
dependencies, exist := annotations[util.DependenciesAnnotationKey]
97+
if !exist {
98+
return nil
99+
}
100+
var dependenciesSlice []configv1alpha1.DependentObjectReference
101+
err := json.Unmarshal([]byte(dependencies), &dependenciesSlice)
102+
if err != nil {
103+
allErrs = append(allErrs, field.Invalid(fldPath.Child(util.DependenciesAnnotationKey), dependencies, "annotation value must be a valid JSON"))
104+
}
105+
106+
return allErrs.ToAggregate()
107+
}
108+
86109
// validateFederatedResourceQuota checks the FederatedResourceQuota for the ResourceBinding.
87110
// It returns a list of errors if validation fails, or nil if it passes.
88111
func (v *ValidatingAdmission) validateFederatedResourceQuota(ctx context.Context, req admission.Request, rb, oldRB *workv1alpha2.ResourceBinding) error {

‎pkg/webhook/resourcebinding/validating_test.go‎

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -118,6 +118,17 @@ func WithResourceRef(ref workv1alpha2.ObjectReference) RBOption {
118118
}
119119
}
120120

121+
func WithAnnotations(annotations map[string]string) RBOption {
122+
return func(rb *workv1alpha2.ResourceBinding) {
123+
if rb.Annotations == nil {
124+
rb.Annotations = make(map[string]string)
125+
}
126+
for k, v := range annotations {
127+
rb.Annotations[k] = v
128+
}
129+
}
130+
}
131+
121132
func makeTestRB(namespace, name string, opts ...RBOption) *workv1alpha2.ResourceBinding {
122133
defaultResourceRef := workv1alpha2.ObjectReference{
123134
APIVersion: "v1", Kind: "Pod", Name: "test-pod-" + name, Namespace: namespace,
@@ -424,6 +435,48 @@ func TestValidatingAdmission_Handle(t *testing.T) {
424435
enableFederatedQuotaEnforcementFeatureGate: true,
425436
wantResponse: admission.Errored(http.StatusBadRequest, errors.New("decode failed")),
426437
},
438+
{
439+
name: "valid dependencies annotation should be allowed",
440+
req: newAdmissionRequestBuilder(t, admissionv1.Create, "default", "rb-valid-deps", "valid-deps").
441+
WithObject(makeTestRB("default", "rb-valid-deps",
442+
WithAnnotations(map[string]string{
443+
"resourcebinding.karmada.io/dependencies": `[{"apiVersion":"v1","kind":"ConfigMap","namespace":"default","name":"my-config"}]`,
444+
}))).
445+
Build(),
446+
decoder: &fakeDecoder{decodeObj: makeTestRB("default", "rb-valid-deps",
447+
WithAnnotations(map[string]string{
448+
"resourcebinding.karmada.io/dependencies": `[{"apiVersion":"v1","kind":"ConfigMap","namespace":"default","name":"my-config"}]`,
449+
}))},
450+
clientObjects: nil,
451+
enableFederatedQuotaEnforcementFeatureGate: false,
452+
wantResponse: admission.Allowed(""),
453+
},
454+
{
455+
name: "invalid dependencies annotation should be denied",
456+
req: newAdmissionRequestBuilder(t, admissionv1.Create, "default", "rb-invalid-deps", "invalid-deps").
457+
WithObject(makeTestRB("default", "rb-invalid-deps",
458+
WithAnnotations(map[string]string{
459+
"resourcebinding.karmada.io/dependencies": `invalid-json-string`,
460+
}))).
461+
Build(),
462+
decoder: &fakeDecoder{decodeObj: makeTestRB("default", "rb-invalid-deps",
463+
WithAnnotations(map[string]string{
464+
"resourcebinding.karmada.io/dependencies": `invalid-json-string`,
465+
}))},
466+
clientObjects: nil,
467+
enableFederatedQuotaEnforcementFeatureGate: false,
468+
wantResponse: admission.Denied("metadata.annotations.resourcebinding.karmada.io/dependencies: Invalid value: \"invalid-json-string\": annotation value must be a valid JSON"),
469+
},
470+
{
471+
name: "no dependencies annotation should be allowed",
472+
req: newAdmissionRequestBuilder(t, admissionv1.Create, "default", "rb-no-deps", "no-deps").
473+
WithObject(makeTestRB("default", "rb-no-deps")).
474+
Build(),
475+
decoder: &fakeDecoder{decodeObj: makeTestRB("default", "rb-no-deps")},
476+
clientObjects: nil,
477+
enableFederatedQuotaEnforcementFeatureGate: false,
478+
wantResponse: admission.Allowed(""),
479+
},
427480
{
428481
name: "create operation not yet scheduled should be allowed",
429482
req: newAdmissionRequestBuilder(t, admissionv1.Create, rbUnscheduledCreate.Namespace, rbUnscheduledCreate.Name, "create-unscheduled").

0 commit comments

Comments
 (0)