Skip to content
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.

Sign up for GitHub

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

Jump to bottom

SizeLimit violates k8s API convention #50121

Closed
kyessenov opened this issue Aug 4, 2017 · 10 comments · Fixed by #50163
Closed

SizeLimit violates k8s API convention #50121

kyessenov opened this issue Aug 4, 2017 · 10 comments · Fixed by #50163
Assignees
@jingxu97
Labels
kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/bug Categorizes issue or PR as related to a bug. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. sig/storage Categorizes an issue or PR as relevant to SIG Storage.
Milestone
v1.7

Comments

@kyessenov
Copy link

kyessenov commented Aug 4, 2017 •
edited
Loading

Is this a BUG REPORT or FEATURE REQUEST?:

Uncomment only one, leave it on its own line:

/kind bug

/kind feature

What happened:
kubernetes/community#306 added field SizeLimit in PR 85f030c. The field is marked as optional yet has the non-pointer type resource.Quantity. This is against the API convention that requires optional fields to be pointers.

As a consequence, k8s client-go library does not omit this field in serialization, breaking k8s 1.6 compatibility for this API.

What you expected to happen:
client-go 1.7 works with k8s 1.6 in pass-through object processing pipeline.

How to reproduce it (as minimally and precisely as possible):
See kubernetes/client-go#257 (comment)

Anything else we need to know?:

Environment:

  • Kubernetes version (use kubectl version): 1.7
  • Cloud provider or hardware configuration**: GKE
  • OS (e.g. from /etc/os-release):
  • Kernel (e.g. uname -a):
  • Install tools:
  • Others:

cc @caesarxuchao
@k8s-ci-robot k8s-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Aug 4, 2017
@k8s-github-robot
Copy link

@kyessenov
There are no sig labels on this issue. Please add a sig label by:

  1. mentioning a sig: @kubernetes/sig-<group-name>-<group-suffix>
    e.g., @kubernetes/sig-contributor-experience-<group-suffix> to notify the contributor experience sig, OR

  2. specifying the label manually: /sig <label>
    e.g., /sig scalability to apply the sig/scalability label

Note: Method 1 will trigger an email to the group. You can find the group list here and label list here.
The <group-suffix> in the method 1 has to be replaced with one of these: bugs, feature-requests, pr-reviews, test-failures, proposals

@k8s-github-robot k8s-github-robot added the needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. label Aug 4, 2017
@kyessenov
Copy link
Author

/assign @jingxu97

@kyessenov
Copy link
Author

kyessenov commented Aug 4, 2017 •
edited
Loading

cc @andraxylia @ayj

@kyessenov
Copy link
Author

Please cherry-pick this into 1.7 since this might affect everyone who uses client-go 1.7 for this API.

@liggitt liggitt added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API sig/storage Categorizes an issue or PR as relevant to SIG Storage. cherrypick-candidate labels Aug 4, 2017
@liggitt liggitt added this to the v1.7 milestone Aug 4, 2017
@k8s-github-robot k8s-github-robot removed the needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. label Aug 4, 2017
@liggitt liggitt added needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels Aug 4, 2017
@k8s-github-robot k8s-github-robot removed the needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. label Aug 4, 2017
@jingxu97 jingxu97 mentioned this issue Aug 4, 2017
Merged
jingxu97 added a commit to jingxu97/kubernetes that referenced this issue Aug 4, 2017
@jingxu97
Change SizeLimit to a pointer
ca40158 
This PR fixes issue kubernetes#50121
jingxu97 added a commit to jingxu97/kubernetes that referenced this issue Aug 4, 2017
@jingxu97
Change SizeLimit to a pointer
fd8304c 
This PR fixes issue kubernetes#50121
@screeley44
Copy link
Contributor

@kyessenov @liggitt - does this apply to ALL optional api fields or just non-strings? I'm not seeing many types holding this convention? I'm asking in regards to another PR I'm about to do so wanting some clarification. Thanks!

cc @msau42

@caesarxuchao
Copy link
Member

IMO we don't have to fix every violations unless they break a use case. We need to strictly enforce the convention during our API reviews to prevent more violations.

@liggitt
Copy link
Member

liggitt commented Aug 9, 2017 •
edited
Loading

it's about go's ability to honor the omitempty directive on structs. we need to ensure that an unset (zero-value) optional alpha field does not get included in the serialized API content at all

see https://play.golang.org/p/gEZoDu7LdG for the way go behaves with zero values

@kyessenov
Copy link
Author

Sounds like it could be possible to detect this statically: if the field is marked as optional and omitempty, it should not serialize to a JSON field.

I can't tell if there are more violations like this one, I happened to discover this while using client-go 1.7 on 1.6 cluster.

jingxu97 added a commit to jingxu97/kubernetes that referenced this issue Aug 16, 2017
@jingxu97
Change SizeLimit to a pointer
cf4e434 
This PR fixes issue kubernetes#50121
jingxu97 added a commit to jingxu97/kubernetes that referenced this issue Aug 23, 2017
@jingxu97
Change SizeLimit to a pointer
2e690e6 
This PR fixes issue kubernetes#50121
jingxu97 added a commit to jingxu97/kubernetes that referenced this issue Aug 29, 2017
@jingxu97
Change SizeLimit to a pointer
4ddff32 
This PR fixes issue kubernetes#50121
jingxu97 added a commit to jingxu97/kubernetes that referenced this issue Aug 29, 2017
@jingxu97
Change SizeLimit to a pointer
8f1d648 
This PR fixes issue kubernetes#50121
jingxu97 added a commit to jingxu97/kubernetes that referenced this issue Aug 30, 2017
@jingxu97
Change SizeLimit to a pointer
4d6da1f 
This PR fixes issue kubernetes#50121
yujuhong pushed a commit to yujuhong/kubernetes that referenced this issue Sep 1, 2017
Merge pull request kubernetes#50163 from jingxu97/Aug/sizeLimit
8679a8f 
Automatic merge from submit-queue (batch tested with PRs 51707, 51662, 51723, 50163, 51633)

Change SizeLimit to a pointer

This PR fixes issue kubernetes#50121

```release-note
The `emptyDir.sizeLimit` field is now correctly omitted from API requests and responses when unset.
```
@liggitt liggitt mentioned this issue Sep 1, 2017
Merged
liggitt pushed a commit to liggitt/kubernetes that referenced this issue Sep 6, 2017
@jingxu97 @liggitt
Change SizeLimit to a pointer
c72d31a 
This PR fixes issue kubernetes#50121
k8s-github-robot pushed a commit that referenced this issue Sep 6, 2017
Merge pull request #51763 from liggitt/automated-cherry-pick-of-#5016…
7afd062 
…3-upstream-release-1.7

Automatic merge from submit-queue

Automated cherry pick of #50163

Cherry pick of #50163 on release-1.7.

fixes #50121

#50163: Change SizeLimit to a pointer

```release-note
The alpha `emptyDir.sizeLimit` field is now correctly omitted from API requests and responses when unset.
```
@liggitt
Copy link
Member

liggitt commented Sep 19, 2017

Fixed by #51763

@liggitt liggitt closed this as completed Sep 19, 2017
@mml
Copy link
Contributor

mml commented Sep 28, 2017

FYI #44005 is a bug for a future API linting tool that would have caught this.

@msau42 msau42 mentioned this issue Apr 27, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jingxu97 jingxu97

Labels
kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/bug Categorizes issue or PR as related to a bug. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. sig/storage Categorizes an issue or PR as relevant to SIG Storage.
Projects
None yet
Milestone
v1.7
Development

Successfully merging a pull request may close this issue.

Change SizeLimit to a pointer jingxu97/kubernetes
8 participants
@mml @liggitt @caesarxuchao @screeley44 @kyessenov @k8s-github-robot @jingxu97 @k8s-ci-robot