-
Notifications
You must be signed in to change notification settings - Fork 38.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Dependencies: Update google.golang.org/protobuf v1.30.0 #117350
Dependencies: Update google.golang.org/protobuf v1.30.0 #117350
Conversation
Hi @mohitsharma-in. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/kind cleanup |
/ok-to-test |
/lgtm |
LGTM label has been added. Git tree hash: 369156427cf3ccf9fee1503eb70cc38fafe24349
|
12f1ba4
to
826d551
Compare
/test pull-kubernetes-e2e-gce |
826d551
to
8efeb5a
Compare
/lgtm |
LGTM label has been added. Git tree hash: fd6b20f18de5eb3c4af7f83660fb5933b13afb03
|
@mrunalp Can you Please do |
secs := t.Unix() | ||
if secs < minTimestampSeconds || secs > maxTimestampSeconds { | ||
return d.newError(tok.Pos(), "%v value out of range: %v", genid.Timestamp_message_fullname, tok.RawString()) | ||
} | ||
// Validate subseconds. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jpbetz it looks like cel-go makes use of protojson.Marshal / Unmarshal ... does our use interact with timestamps in a way that would be affected by this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a covers a very specific edge case where the length of the subseconds part of a date data liberal is too long?
We test happy path behavior with https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/validation_test.go#L267 but don't test for this edge case.
At a glance, it appears that this tightening of validation could break CEL expressions, but TBH, I would be shocked if this impacts any actual use of CEL in k8s since data literals of date times are rare.
cc @TristonianJones for broader CEL ecosystem impact.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it looks like this was adjusting to keep identical behavior under go1.20, which included http://go.dev/cl/425037
one question on our use of timestamp marshal/unmarshal, lgtm otherwise |
/lgtm |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Dependencies Update
google.golang.org/protobuf
v1.28.1 to v1.30.0Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: