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.

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

Protobuf serialization does not distinguish between [] and null #44593

Closed
siggy opened this issue Apr 18, 2017 · 5 comments
Closed

Protobuf serialization does not distinguish between [] and null #44593

siggy opened this issue Apr 18, 2017 · 5 comments
Assignees
Labels
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/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery.
Milestone

Comments

@siggy
Copy link
Contributor

siggy commented Apr 18, 2017

Is this a request for help? (If yes, you should use our troubleshooting guide and community support channels, see http://kubernetes.io/docs/troubleshooting/.): Not a request for help. Observed undocumented change in 1.6.1 api.

What keywords did you search in Kubernetes issues before filing this one? (If you have found any duplicates, you should instead reply there.): endpoints subsets api


Is this a BUG REPORT or FEATURE REQUEST? (choose one): BUG REPORT

Kubernetes version (use kubectl version):

Client Version: version.Info{Major:"1", Minor:"6", GitVersion:"v1.6.0", GitCommit:"fff5156092b56e6bd60fff75aad4dc9de6b6ef37", GitTreeState:"clean", BuildDate:"2017-03-28T16:36:33Z", GoVersion:"go1.7.5", Compiler:"gc", Platform:"darwin/amd64"}
Server Version: version.Info{Major:"1", Minor:"6", GitVersion:"v1.6.1", GitCommit:"b0b7a323cc5a4a2019b2e9520c21c7830b7f708e", GitTreeState:"clean", BuildDate:"2017-04-03T20:33:27Z", GoVersion:"go1.7.5", Compiler:"gc", Platform:"linux/amd64"}

Environment:

  • Cloud provider or hardware configuration: GKE
  • OS (e.g. from /etc/os-release):
BUILD_ID=9000.104.0
NAME="Container-Optimized OS"
GOOGLE_CRASH_ID=Lakitu
VERSION_ID=56
BUG_REPORT_URL=https://crbug.com/new
PRETTY_NAME="Container-Optimized OS from Google"
VERSION=56
GOOGLE_METRICS_PRODUCT_ID=26
HOME_URL="https://cloud.google.com/compute/docs/containers/vm-image/"
ID=cos
  • Kernel (e.g. uname -a):
Linux gke-alpha-default-pool-3f2b4945-188b 4.4.21+ #1 SMP Wed Apr 5 14:40:46 PDT 2017 x86_64 Intel(R) Xeon(R) CPU @ 2.30GHz GenuineIntel GNU/Linux
  • Install tools:
  • Others:
    1.6.1 alpha cluster set up with:
gcloud alpha container clusters create alpha --cluster-version 1.6.1 --zone us-central1-b --enable-kubernetes-alpha

What happened:

Observed API change in 1.6.1. api/v1/namespaces/{namespace}/endpoints returns null for .items[].subsets. 1.6.0 returns []

What you expected to happen:

Expect 1.6.1 to match 1.6.0, and return []

How to reproduce it (as minimally and precisely as possible):

1.6.0

$ kubectl -n=kube-system get ep kube-controller-manager -o jsonpath='{.subsets}'
[]

$ kubectl -n=kube-system get ep -o jsonpath='{.items[?(@.metadata.name=="kube-controller-manager")].subsets}'
[]

1.6.1

$ kubectl -n=kube-system get ep kube-controller-manager -o jsonpath='{.subsets}'
[]

$ kubectl -n=kube-system get ep -o jsonpath='{.items[?(@.metadata.name=="kube-controller-manager")].subsets}'
<nil> # <---- BOOM

Anything else we need to know:

Original bug reported as an exception in linkerd:
linkerd/linkerd#1219

@liggitt
Copy link
Member

liggitt commented Apr 18, 2017

nothing related to this changed between 1.6.0 and 1.6.1:

v1.6.0...v1.6.1

@liggitt
Copy link
Member

liggitt commented Apr 18, 2017

the issue with null slices was supposed to be fixed by #43422, but I'm seeing different behavior between a list and an individual item.

this issue exists in v1.6.0 as well. it only shows up when your etcd storage media type is protobuf

@liggitt liggitt self-assigned this Apr 18, 2017
@liggitt liggitt added kind/bug Categorizes issue or PR as related to a bug. kind/support Categorizes issue or PR as a support question. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. labels Apr 18, 2017
@liggitt liggitt added this to the v1.6 milestone Apr 18, 2017
@liggitt liggitt added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Apr 24, 2017
@liggitt
Copy link
Member

liggitt commented Jun 27, 2017

xref #46733

@liggitt liggitt changed the title Endpoints API returning null in subsets field API objects do not persist [] for empty fields Jun 27, 2017
@liggitt
Copy link
Member

liggitt commented Jun 27, 2017

copied from #45294 (comment):

The more I work on this, the more concerned I get with trying to fix up data our storage layer is dropping.

Compressing null -> [] is just as much of an API breakage as [] -> null, it just happens to cause fewer problems in clients that don't do null checks.

1.6 was released with this behavior half-exposed already (listing objects doesn't apply the null->[] hack), and the world didn't explode. I'm inclined to document a breaking change in 1.7 that null and [] are considered equivalent, unless otherwise noted on a particular field, and remove that hack for 1.8, instead of this monster PR.

Thoughts?

@liggitt liggitt changed the title API objects do not persist [] for empty fields Protobuf serialization does not distinguish between [] and null Jun 29, 2017
@castrojo
Copy link
Member

/remove-kind support

@k8s-ci-robot k8s-ci-robot removed the kind/support Categorizes issue or PR as a support question. label Jun 30, 2017
@liggitt liggitt modified the milestones: v1.8, v1.6 Jul 15, 2017
k8s-github-robot pushed a commit that referenced this issue Aug 26, 2017
Automatic merge from submit-queue

Remove null -> [] slice hack

Closes #44593

When 1.6 added protobuf storage, the storage layer lost the ability to persist slice fields with empty but non-null values.

As a workaround, we tried to convert empty slice fields to `[]`, rather than `null`. Compressing `null` -> `[]` was just as much of an API breakage as `[]` -> `null`, but was hoped to cause fewer problems in clients that don't do null checks.

Because of conversion optimizations around converting lists of objects, the `null` -> `[]` hack was discovered to only apply to individual get requests, not to a list of objects. 1.6 and 1.7 was released with this behavior, and the world didn't explode. 1.7 documented the breaking API change that `null` and `[]` should be considered equivalent, unless otherwise noted on a particular field.

This PR:

* Reverts the earlier attempt (#43422) at ensuring non-null json slice output in conversion
* Makes results of `get` consistent with the results of `list` (which helps naive clients that do deepequal comparisons of objects obtained via list/watch and get), and allows empty slice fields to be returned as `null`

```release-note
Protobuf serialization does not distinguish between `[]` and `null`.
API fields previously capable of storing and returning either `[]` and `null` via JSON API requests (for example, the Endpoints `subsets` field) can now store only `null` when created using the protobuf content-type or stored in etcd using protobuf serialization (the default in 1.6+). JSON API clients should tolerate `null` values for such fields, and treat `null` and `[]` as equivalent in meaning unless specifically documented otherwise for a particular field.
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery.
Projects
None yet
Development

No branches or pull requests

4 participants