fix(sdk): validate labels in SetLabel to prevent retry loops#4386
fix(sdk): validate labels in SetLabel to prevent retry loops#4386markmandel merged 3 commits intoagones-dev:mainfrom
Conversation
|
/gcbrun |
lacroixthomas
left a comment
There was a problem hiding this comment.
Hi @rajanarahul93,
Thanks for the contribution !
I've put a minor change, but will need confirmation first
| if kv == nil { | ||
| return nil, status.Error(codes.InvalidArgument, "label key/value cannot be nil") | ||
| } | ||
|
|
||
| if errs := validation.IsQualifiedName(kv.Key); len(errs) > 0 { | ||
| return nil, status.Errorf( | ||
| codes.InvalidArgument, | ||
| "invalid label key %q: %s", | ||
| kv.Key, | ||
| strings.Join(errs, ", "), | ||
| ) | ||
| } | ||
|
|
||
| if errs := validation.IsValidLabelValue(kv.Value); len(errs) > 0 { | ||
| return nil, status.Errorf( | ||
| codes.InvalidArgument, | ||
| "invalid label value %q for key %q: %s", | ||
| kv.Value, | ||
| kv.Key, | ||
| strings.Join(errs, ", "), | ||
| ) | ||
| } |
There was a problem hiding this comment.
Even though that seems totally valid, I wonder if we should not use errors.Errorf instead, to be consistent with the rest of the code ? 🤔 @markmandel
There was a problem hiding this comment.
You know - I'm not against it. But it does mean this method could return a 4xx rather than a 5xx like in most (all?) other cases.
I'm almost wondering if we should create a ticket to go back and make our grpc errors proper gRPC errors. The only thing I can think it might break is manual REST implementations that expect a 500 response in some way. WDYT?
|
Build Succeeded 🥳 Build Id: ef21dadb-64dc-41b3-b0df-79a0ff209b94 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version: |
|
Good question, thanks for raising it! I used Using Happy to adjust if there’s a preferred pattern for returning gRPC status errors elsewhere in the SDK. |
cf25183 to
d8ede0b
Compare
|
Hey ! @rajanarahul93 I don't know what you think about it @markmandel but I'm fine with it, LGTM, but if you're also good with it, I'll create some issues to ensure the other functions from the sdkServers also returns gRPC status code to be consistent (and also give more context / proper status code for the user) /gcbrun |
That's a fair point. I should go look, but I'm willing to bet that most SDK methods don't even return errors ever? Or if they do, it's very rare. Pretty much everything goes straight into a worker queue. So this shouldn't be a big breaking change really. (Totally on mobile and haven't looked at the code at all 🙂) |
|
From what I've seen, they only returns go errors (even for invalidArguments - which is why I think we can create issues to improve the other ones as well), but the SDKs either returns it directly or wrap the errors (depending of the language - most of them don't return any errors indeed), I can only see benefit from it, and in term of breaking changes, I only see improvements of the error handling, for the SetLabels from this PR, it was previously returning no errors at all (No pressure at all, was only going through some PR reviews 😄) |
|
Build Succeeded 🥳 Build Id: 47e2143e-7dd3-4282-9bca-3b8da6456159 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version: |
|
Thanks a lot for the review and the discussion! Glad to hear this approach makes sense and improves the user-facing error handling. Let me know if you’d like me to do anything else here. |
markmandel
left a comment
There was a problem hiding this comment.
whoops, never submitted this pending review.
| if kv == nil { | ||
| return nil, status.Error(codes.InvalidArgument, "label key/value cannot be nil") | ||
| } | ||
|
|
||
| if errs := validation.IsQualifiedName(kv.Key); len(errs) > 0 { | ||
| return nil, status.Errorf( | ||
| codes.InvalidArgument, | ||
| "invalid label key %q: %s", | ||
| kv.Key, | ||
| strings.Join(errs, ", "), | ||
| ) | ||
| } | ||
|
|
||
| if errs := validation.IsValidLabelValue(kv.Value); len(errs) > 0 { | ||
| return nil, status.Errorf( | ||
| codes.InvalidArgument, | ||
| "invalid label value %q for key %q: %s", | ||
| kv.Value, | ||
| kv.Key, | ||
| strings.Join(errs, ", "), | ||
| ) | ||
| } |
There was a problem hiding this comment.
You know - I'm not against it. But it does mean this method could return a 4xx rather than a 5xx like in most (all?) other cases.
I'm almost wondering if we should create a ticket to go back and make our grpc errors proper gRPC errors. The only thing I can think it might break is manual REST implementations that expect a 500 response in some way. WDYT?
|
Let's get this merged! Shall we also do SetAnnotation() at some point? If someone could make an issue for it, that would be ace. /gcbrun |
|
Build Failed 😭 Build Id: 0fdbb16c-b47c-4589-8437-791600a3155d Status: FAILURE To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
|
/gcbrun |
|
Build Succeeded 🥳 Build Id: 067e4ff2-7025-491b-95bd-e749e7077f1f The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version: |
…dev#4386) Co-authored-by: Mark Mandel <mark@compoundtheory.com>
/kind bug
What this PR does / Why we need it:
This PR adds early validation to the SDK sidecar
SetLabelRPC to prevent invalid Kubernetes labels from being enqueued and retried indefinitely.Previously,
SetLabelaccepted any key/value pair and queued it for synchronization. When the label was invalid (for example, containing spaces), the Kubernetes API server rejected the update. Because the failure was deterministic, the SDK worker queue retried forever, resulting in a retry loop and blocking subsequent label updates.This change validates label keys and values up front using Kubernetes validation rules and returns a clear
InvalidArgumentgRPC error to the client, avoiding unnecessary retries and improving debuggability.Which issue(s) this PR fixes:
Closes #4385
Special notes for your reviewer:
This PR intentionally limits its scope to
SetLabel, as described in the linked issue.A similar validation pattern may also apply to
SetAnnotation, but that is left out here to keep the fix minimal and focused.