Skip to content

Commit 8f9f9ef

Browse files
karmada-botkarmada-bot
authored
Merge pull request #6611 from liaolecheng/refactor/resourcebinding-validation
Optimize the ResourceBinding validation webhook for better extensibility
2 parents 6bcb477 + e81e311 commit 8f9f9ef

File tree

2 files changed

+86
-98
lines changed

2 files changed

+86
-98
lines changed

‎pkg/webhook/resourcebinding/validating.go‎

Lines changed: 80 additions & 86 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,6 @@ import (
2626
admissionv1 "k8s.io/api/admission/v1"
2727
corev1 "k8s.io/api/core/v1"
2828
apierrors "k8s.io/apimachinery/pkg/api/errors"
29-
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
3029
"k8s.io/client-go/util/retry"
3130
"k8s.io/klog/v2"
3231
"sigs.k8s.io/controller-runtime/pkg/client"
@@ -50,41 +49,70 @@ var _ admission.Handler = &ValidatingAdmission{}
5049
type frqProcessOutcome struct {
5150
validationMessages []string
5251
updatedFRQCount int
53-
noFRQsFound bool // True if this attempt found no FRQs
54-
earlyExitResponse *admission.Response // Response if this attempt decided to exit early (denial, permanent error)
55-
ProcessError error // Error for RetryOnConflict (e.g., conflict error or nil for success/final decision)
52+
noFRQsFound bool // True if this attempt found no FRQs
53+
earlyExitError error // Response if this attempt decided to exit early (denial, permanent error)
54+
ProcessError error // Error for RetryOnConflict (e.g., conflict error or nil for success/final decision)
5655
}
5756

5857
// Handle implements admission.Handler interface.
5958
func (v *ValidatingAdmission) Handle(ctx context.Context, req admission.Request) admission.Response {
59+
rb, oldRB, err := v.decodeRBs(req)
60+
if err != nil {
61+
return admission.Errored(http.StatusBadRequest, err)
62+
}
63+
klog.V(2).Infof("Processing ResourceBinding(%s/%s) for request: %s (%s)", rb.Namespace, rb.Name, req.Operation, req.UID)
64+
65+
if err := v.validateFederatedResourceQuota(ctx, req, rb, oldRB); err != nil {
66+
if apierrors.IsInternalError(err) {
67+
klog.Errorf("Internal error while processing ResourceBinding %s/%s: %v", rb.Namespace, rb.Name, err)
68+
return admission.Errored(http.StatusInternalServerError, err)
69+
}
70+
klog.Errorf("Admission denied for ResourceBinding %s/%s: %v", rb.Namespace, rb.Name, err)
71+
return admission.Denied(err.Error())
72+
}
73+
74+
// further validation can be added here if needed
75+
76+
return admission.Allowed("")
77+
}
78+
79+
// validateFederatedResourceQuota checks the FederatedResourceQuota for the ResourceBinding.
80+
// It returns a list of errors if validation fails, or nil if it passes.
81+
func (v *ValidatingAdmission) validateFederatedResourceQuota(ctx context.Context, req admission.Request, rb, oldRB *workv1alpha2.ResourceBinding) error {
6082
if !features.FeatureGate.Enabled(features.FederatedQuotaEnforcement) {
6183
klog.V(5).Infof("FederatedQuotaEnforcement feature gate is disabled, skipping validation for ResourceBinding %s/%s", req.Namespace, req.Name)
62-
return admission.Allowed("")
84+
return nil
6385
}
6486

65-
isDryRun := req.DryRun != nil && *req.DryRun
87+
if req.Operation == admissionv1.Create && len(rb.Spec.Clusters) == 0 {
88+
klog.V(4).Infof("ResourceBinding %s/%s is being created but not yet scheduled, skipping quota validation.", rb.Namespace, rb.Name)
89+
return nil
90+
}
6691

67-
rb, oldRB, extractResp := v.decodeAndValidateRBs(req)
68-
if extractResp != nil {
69-
return *extractResp
92+
if req.Operation == admissionv1.Update {
93+
if !isQuotaRelevantFieldChanged(oldRB, rb) {
94+
klog.V(4).Infof("ResourceBinding %s/%s updated, but no quota relevant fields changed, skipping quota validation.", rb.Namespace, rb.Name)
95+
return nil
96+
}
7097
}
7198

72-
newRbTotalUsage, oldRbTotalUsage, respCalc := v.calculateRBUsages(rb, oldRB)
73-
if respCalc != nil {
74-
return *respCalc
99+
isDryRun := req.DryRun != nil && *req.DryRun
100+
101+
newRbTotalUsage, oldRbTotalUsage, err := v.calculateRBUsages(rb, oldRB)
102+
if err != nil {
103+
return err
75104
}
76105

77106
totalRbDelta := calculateDelta(newRbTotalUsage, oldRbTotalUsage)
78107
klog.V(4).Infof("Calculated total RB delta for %s/%s: %v", rb.Namespace, rb.Name, totalRbDelta)
79108

80109
if len(totalRbDelta) == 0 {
81110
klog.V(2).Infof("No effective resource quantity delta for ResourceBinding %s/%s. Skipping quota validation.", rb.Namespace, rb.Name)
82-
return admission.Allowed("No effective resource quantity delta for ResourceBinding, skipping quota validation.")
111+
return nil
83112
}
84113

85-
frqOutcome := v.processFRQsWithRetries(ctx, rb, totalRbDelta, isDryRun)
86-
87-
return v.finalizeResponse(rb, frqOutcome)
114+
outcome := v.processFRQsWithRetries(ctx, rb, totalRbDelta, isDryRun)
115+
return v.handleFRQOutcome(rb, outcome)
88116
}
89117

90118
func (v *ValidatingAdmission) processFRQsWithRetries(ctx context.Context, rb *workv1alpha2.ResourceBinding, totalRbDelta corev1.ResourceList, isDryRun bool) frqProcessOutcome {
@@ -108,9 +136,9 @@ func (v *ValidatingAdmission) executeFRQProcessingAttempt(ctx context.Context, r
108136
var currentFrqsToUpdateStatus []*policyv1alpha1.FederatedResourceQuota
109137
var currentValidationMessages []string
110138

111-
frqList, listResp := v.listFRQs(ctx, rb.Namespace)
112-
if listResp != nil {
113-
outcome.earlyExitResponse = listResp
139+
frqList, listError := v.listFRQs(ctx, rb.Namespace)
140+
if listError != nil {
141+
outcome.earlyExitError = listError
114142
return outcome // attemptError is nil, signaling non-retryable handled error
115143
}
116144

@@ -121,9 +149,9 @@ func (v *ValidatingAdmission) executeFRQProcessingAttempt(ctx context.Context, r
121149
}
122150

123151
for _, frqItem := range frqList.Items {
124-
newStatus, msg, denialResp := v.processSingleFRQ(frqItem, rb.Namespace, rb.Name, totalRbDelta)
125-
if denialResp != nil {
126-
outcome.earlyExitResponse = denialResp
152+
newStatus, msg, denialError := v.processSingleFRQ(&frqItem, rb.Namespace, rb.Name, totalRbDelta)
153+
if denialError != nil {
154+
outcome.earlyExitError = denialError
127155
return outcome // attemptError is nil, request denied
128156
}
129157
if newStatus != nil {
@@ -145,8 +173,7 @@ func (v *ValidatingAdmission) executeFRQProcessingAttempt(ctx context.Context, r
145173
}
146174
errMsg := fmt.Sprintf("permanent error updating FRQ statuses for RB %s/%s: %v", rb.Namespace, rb.Name, updateErr)
147175
klog.Error(errMsg)
148-
resp := admission.Errored(http.StatusInternalServerError, errors.New(errMsg))
149-
outcome.earlyExitResponse = &resp
176+
outcome.earlyExitError = apierrors.NewInternalError(errors.New(errMsg))
150177
return outcome // attemptError is nil, non-retryable handled error
151178
}
152179

@@ -155,20 +182,20 @@ func (v *ValidatingAdmission) executeFRQProcessingAttempt(ctx context.Context, r
155182
return outcome // attemptError is nil, successful attempt
156183
}
157184

158-
func (v *ValidatingAdmission) finalizeResponse(rb *workv1alpha2.ResourceBinding, outcome frqProcessOutcome) admission.Response {
185+
func (v *ValidatingAdmission) handleFRQOutcome(rb *workv1alpha2.ResourceBinding, outcome frqProcessOutcome) error {
159186
if outcome.ProcessError != nil {
160187
errMsg := fmt.Sprintf("failed to apply FederatedResourceQuota updates after multiple retries for RB %s/%s: %v", rb.Namespace, rb.Name, outcome.ProcessError)
161188
klog.Error(errMsg)
162-
return admission.Errored(http.StatusInternalServerError, errors.New(errMsg))
189+
return apierrors.NewInternalError(errors.New(errMsg))
163190
}
164191

165-
if outcome.earlyExitResponse != nil {
166-
return *outcome.earlyExitResponse
192+
if outcome.earlyExitError != nil {
193+
return outcome.earlyExitError
167194
}
168195

169196
if outcome.noFRQsFound {
170197
klog.V(2).Infof("No FederatedResourceQuotas found in namespace %s for ResourceBinding %s. Allowing operation.", rb.Namespace, rb.Name)
171-
return admission.Allowed("No FederatedResourceQuotas found in the namespace, skipping quota check.")
198+
return nil
172199
}
173200

174201
finalMessage := fmt.Sprintf("All relevant FederatedResourceQuota checks passed for ResourceBinding %s/%s.", rb.Namespace, rb.Name)
@@ -181,7 +208,7 @@ func (v *ValidatingAdmission) finalizeResponse(rb *workv1alpha2.ResourceBinding,
181208
finalMessage = fmt.Sprintf("%s No FRQs required update.", finalMessage)
182209
}
183210
klog.V(2).Infof("Admission allowed for ResourceBinding %s/%s: %s", rb.Namespace, rb.Name, finalMessage)
184-
return admission.Allowed(finalMessage)
211+
return nil
185212
}
186213

187214
// updateFRQStatusesWithRetrySignal attempts to update FRQ statuses.
@@ -218,53 +245,37 @@ func (v *ValidatingAdmission) updateFRQStatusesWithRetrySignal(ctx context.Conte
218245
return nil
219246
}
220247

221-
// decodeAndValidateRBs decodes current and old (for updates) ResourceBindings,
222-
// performs essential preliminary checks, and checks for relevant changes in update operations.
223-
// Returns the RBs or an admission response for early exit.
224-
func (v *ValidatingAdmission) decodeAndValidateRBs(req admission.Request) (
248+
// decodeRBs decodes current and old (for updates) ResourceBindings.
249+
// Returns the RBs or an admission response for early exit on decoding failure.
250+
func (v *ValidatingAdmission) decodeRBs(req admission.Request) (
225251
currentRB *workv1alpha2.ResourceBinding,
226-
oldRB *workv1alpha2.ResourceBinding, // Will be nil if not an update or if not applicable
227-
earlyExitResp *admission.Response,
252+
oldRB *workv1alpha2.ResourceBinding, // Will be nil if not an update
253+
earlyExitResp error,
228254
) {
229255
decodedRB := &workv1alpha2.ResourceBinding{}
230256
if err := v.Decoder.Decode(req, decodedRB); err != nil {
231257
klog.Errorf("Failed to decode ResourceBinding %s/%s: %v", req.Namespace, req.Name, err)
232-
resp := admission.Errored(http.StatusBadRequest, err)
233-
return nil, nil, &resp
258+
return nil, nil, err
234259
}
235-
klog.V(2).Infof("Processing ResourceBinding(%s/%s) for request: %s (%s)", decodedRB.Namespace, decodedRB.Name, req.Operation, req.UID)
236260

237-
if req.Operation == admissionv1.Create && len(decodedRB.Spec.Clusters) == 0 {
238-
klog.V(4).Infof("ResourceBinding %s/%s is being created but not yet scheduled, skipping quota validation.", decodedRB.Namespace, decodedRB.Name)
239-
resp := admission.Allowed("ResourceBinding not yet scheduled for Create operation.")
240-
return decodedRB, nil, &resp
261+
if req.Operation != admissionv1.Update {
262+
return decodedRB, nil, nil
241263
}
242264

243-
var decodedOldRB *workv1alpha2.ResourceBinding
244-
if req.Operation == admissionv1.Update {
245-
tempOldRB := &workv1alpha2.ResourceBinding{}
246-
if err := v.Decoder.DecodeRaw(req.OldObject, tempOldRB); err != nil {
247-
klog.Errorf("Failed to decode old ResourceBinding %s/%s: %v", req.Namespace, req.Name, err)
248-
resp := admission.Errored(http.StatusBadRequest, err)
249-
return decodedRB, nil, &resp
250-
}
251-
decodedOldRB = tempOldRB
252-
253-
if !isQuotaRelevantFieldChanged(decodedOldRB, decodedRB) {
254-
klog.V(4).Infof("ResourceBinding %s/%s updated, but no quota relevant fields changed, skipping quota validation.", decodedRB.Namespace, decodedRB.Name)
255-
resp := admission.Allowed("No quota relevant fields changed.")
256-
return decodedRB, decodedOldRB, &resp
257-
}
265+
decodedOldRB := &workv1alpha2.ResourceBinding{}
266+
if err := v.Decoder.DecodeRaw(req.OldObject, decodedOldRB); err != nil {
267+
klog.Errorf("Failed to decode old ResourceBinding %s/%s: %v", req.Namespace, req.Name, err)
268+
return nil, nil, err
258269
}
270+
259271
return decodedRB, decodedOldRB, nil
260272
}
261273

262-
func (v *ValidatingAdmission) calculateRBUsages(rb, oldRB *workv1alpha2.ResourceBinding) (corev1.ResourceList, corev1.ResourceList, *admission.Response) {
274+
func (v *ValidatingAdmission) calculateRBUsages(rb, oldRB *workv1alpha2.ResourceBinding) (corev1.ResourceList, corev1.ResourceList, error) {
263275
newRbTotalUsage, err := calculateResourceUsage(rb)
264276
if err != nil {
265277
klog.Errorf("Error calculating resource usage for new ResourceBinding %s/%s: %v", rb.Namespace, rb.Name, err)
266-
resp := admission.Errored(http.StatusInternalServerError, err)
267-
return nil, nil, &resp
278+
return nil, nil, apierrors.NewInternalError(err)
268279
}
269280
klog.V(4).Infof("Calculated total usage for incoming RB %s/%s: %v", rb.Namespace, rb.Name, newRbTotalUsage)
270281

@@ -273,26 +284,24 @@ func (v *ValidatingAdmission) calculateRBUsages(rb, oldRB *workv1alpha2.Resource
273284
oldRbTotalUsage, err = calculateResourceUsage(oldRB)
274285
if err != nil {
275286
klog.Errorf("Error calculating resource usage for old ResourceBinding %s/%s: %v", oldRB.Namespace, oldRB.Name, err)
276-
resp := admission.Errored(http.StatusInternalServerError, err)
277-
return nil, nil, &resp
287+
return nil, nil, apierrors.NewInternalError(err)
278288
}
279289
klog.V(4).Infof("Calculated total usage for old RB %s/%s: %v", oldRB.Namespace, oldRB.Name, oldRbTotalUsage)
280290
}
281291
return newRbTotalUsage, oldRbTotalUsage, nil
282292
}
283293

284-
func (v *ValidatingAdmission) listFRQs(ctx context.Context, namespace string) (*policyv1alpha1.FederatedResourceQuotaList, *admission.Response) {
294+
func (v *ValidatingAdmission) listFRQs(ctx context.Context, namespace string) (*policyv1alpha1.FederatedResourceQuotaList, error) {
285295
frqList := &policyv1alpha1.FederatedResourceQuotaList{}
286296
if err := v.Client.List(ctx, frqList, client.InNamespace(namespace)); err != nil {
287297
klog.Errorf("Failed to list FederatedResourceQuotas in namespace %s: %v", namespace, err)
288-
resp := admission.Errored(http.StatusInternalServerError, fmt.Errorf("failed to list FederatedResourceQuotas: %w", err))
289-
return nil, &resp
298+
return nil, apierrors.NewInternalError(fmt.Errorf("failed to list FederatedResourceQuotas: %w", err))
290299
}
291300
klog.V(2).Infof("Found %d FederatedResourceQuotas in namespace %s to evaluate.", len(frqList.Items), namespace)
292301
return frqList, nil
293302
}
294303

295-
func (v *ValidatingAdmission) processSingleFRQ(frqItem policyv1alpha1.FederatedResourceQuota, rbNamespace, rbName string, totalRbDelta corev1.ResourceList) (newStatusToSet corev1.ResourceList, validationMsg string, denialResp *admission.Response) {
304+
func (v *ValidatingAdmission) processSingleFRQ(frqItem *policyv1alpha1.FederatedResourceQuota, rbNamespace, rbName string, totalRbDelta corev1.ResourceList) (newStatusToSet corev1.ResourceList, validationMsg string, err error) {
296305
klog.V(4).Infof("Evaluating FRQ %s/%s for RB %s/%s", frqItem.Namespace, frqItem.Name, rbNamespace, rbName)
297306
if frqItem.Spec.Overall == nil {
298307
klog.V(4).Infof("FRQ %s/%s has no spec.overall defined, skipping its evaluation.", frqItem.Namespace, frqItem.Name)
@@ -312,31 +321,16 @@ func (v *ValidatingAdmission) processSingleFRQ(frqItem policyv1alpha1.FederatedR
312321

313322
isAllowed, errMsg := isAllowed(potentialNewOverallUsedForThisFRQ, frqItem)
314323
if !isAllowed {
315-
klog.Warningf("Quota exceeded for FederatedResourceQuota %s/%s. ResourceBinding %s/%s will be denied.",
316-
frqItem.Namespace, frqItem.Name, rbNamespace, rbName)
317-
resp := buildDenyResponse(errMsg)
318-
return nil, "", resp
324+
klog.Warningf("Quota exceeded for FederatedResourceQuota %s/%s. ResourceBinding %s/%s will be denied. %s",
325+
frqItem.Namespace, frqItem.Name, rbNamespace, rbName, errMsg)
326+
return nil, "", errors.New(errMsg)
319327
}
320328

321329
msg := fmt.Sprintf("Quota check passed for FRQ %s/%s.", frqItem.Namespace, frqItem.Name)
322330
klog.V(3).Infof("FRQ %s/%s will be updated. New OverallUsed: %v", frqItem.Namespace, frqItem.Name, potentialNewOverallUsedForThisFRQ)
323331
return potentialNewOverallUsedForThisFRQ, msg, nil
324332
}
325333

326-
func buildDenyResponse(errMsg string) *admission.Response {
327-
resp := admission.Response{
328-
AdmissionResponse: admissionv1.AdmissionResponse{
329-
Allowed: false,
330-
Result: &metav1.Status{
331-
Message: errMsg,
332-
Reason: util.QuotaExceededReason,
333-
Code: int32(http.StatusForbidden),
334-
},
335-
},
336-
}
337-
return &resp
338-
}
339-
340334
func calculateResourceUsage(rb *workv1alpha2.ResourceBinding) (corev1.ResourceList, error) {
341335
if rb == nil || rb.Spec.ReplicaRequirements == nil || len(rb.Spec.ReplicaRequirements.ResourceRequest) == 0 || len(rb.Spec.Clusters) == 0 {
342336
return corev1.ResourceList{}, nil
@@ -452,7 +446,7 @@ func addResourceLists(list1, list2 corev1.ResourceList) corev1.ResourceList {
452446
return result
453447
}
454448

455-
func isAllowed(requested corev1.ResourceList, frqItem policyv1alpha1.FederatedResourceQuota) (bool, string) {
449+
func isAllowed(requested corev1.ResourceList, frqItem *policyv1alpha1.FederatedResourceQuota) (bool, string) {
456450
allowedLimits := frqItem.Spec.Overall
457451
if allowedLimits == nil {
458452
return true, ""

‎pkg/webhook/resourcebinding/validating_test.go‎

Lines changed: 6 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -363,7 +363,7 @@ func TestValidatingAdmission_Handle(t *testing.T) {
363363
decoder: &fakeDecoder{decodeObj: rbUnscheduledCreate},
364364
clientObjects: []client.Object{frqForUnscheduledCreate},
365365
featureGateEnabled: true,
366-
wantResponse: admission.Allowed("ResourceBinding not yet scheduled for Create operation."),
366+
wantResponse: admission.Allowed(""),
367367
},
368368
{
369369
name: "update operation with no relevant field change should be allowed",
@@ -374,7 +374,7 @@ func TestValidatingAdmission_Handle(t *testing.T) {
374374
decoder: &fakeDecoder{decodeObj: rbForNoChange, rawDecodedObj: rbForNoChange},
375375
clientObjects: []client.Object{frqForNoChange},
376376
featureGateEnabled: true,
377-
wantResponse: admission.Allowed("No quota relevant fields changed."),
377+
wantResponse: admission.Allowed(""),
378378
},
379379
{
380380
name: "total RB delta is zero should be allowed (update with identical states)",
@@ -385,7 +385,7 @@ func TestValidatingAdmission_Handle(t *testing.T) {
385385
decoder: &fakeDecoder{decodeObj: rbForDeltaZeroNew, rawDecodedObj: rbForDeltaZeroOld},
386386
clientObjects: []client.Object{frqForDeltaZero},
387387
featureGateEnabled: true,
388-
wantResponse: admission.Allowed("No effective resource quantity delta for ResourceBinding, skipping quota validation."),
388+
wantResponse: admission.Allowed(""),
389389
},
390390
{
391391
name: "no FRQs found should be allowed",
@@ -395,7 +395,7 @@ func TestValidatingAdmission_Handle(t *testing.T) {
395395
decoder: &fakeDecoder{decodeObj: rbForNoFRQ},
396396
clientObjects: []client.Object{},
397397
featureGateEnabled: true,
398-
wantResponse: admission.Allowed("No FederatedResourceQuotas found in the namespace, skipping quota check."),
398+
wantResponse: admission.Allowed(""),
399399
},
400400
{
401401
name: "create quota exceeded should deny",
@@ -417,10 +417,7 @@ func TestValidatingAdmission_Handle(t *testing.T) {
417417
decoder: &fakeDecoder{decodeObj: commonRBUpdatePassNew, rawDecodedObj: commonRBUpdatePassOld},
418418
clientObjects: []client.Object{frqForUpdatePassNonDryRun},
419419
featureGateEnabled: true,
420-
wantResponse: admission.Allowed(
421-
fmt.Sprintf("All relevant FederatedResourceQuota checks passed for ResourceBinding %s/%s. Quota check passed for FRQ %s/%s. 1 FRQ(s) updated.",
422-
commonRBUpdatePassNew.Namespace, commonRBUpdatePassNew.Name, frqForUpdatePassNonDryRun.Namespace, frqForUpdatePassNonDryRun.Name),
423-
),
420+
wantResponse: admission.Allowed(""),
424421
},
425422
{
426423
name: "update passes quota (allowed response, dry run)",
@@ -432,10 +429,7 @@ func TestValidatingAdmission_Handle(t *testing.T) {
432429
decoder: &fakeDecoder{decodeObj: commonRBUpdatePassNew, rawDecodedObj: commonRBUpdatePassOld},
433430
clientObjects: []client.Object{frqForUpdatePassDryRun},
434431
featureGateEnabled: true,
435-
wantResponse: admission.Allowed(
436-
fmt.Sprintf("All relevant FederatedResourceQuota checks passed for ResourceBinding %s/%s. Quota check passed for FRQ %s/%s. 1 FRQ(s) updated.",
437-
commonRBUpdatePassNew.Namespace, commonRBUpdatePassNew.Name, frqForUpdatePassDryRun.Namespace, frqForUpdatePassDryRun.Name),
438-
),
432+
wantResponse: admission.Allowed(""),
439433
},
440434
{
441435
name: "update fails quota (denied response)",

0 commit comments

Comments
 (0)