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

Preserve target apiVersion when decoding into unstructured lists #88995

Merged
merged 1 commit into from Mar 10, 2020

Conversation

@liggitt
Copy link
Member

liggitt commented Mar 10, 2020

What type of PR is this?
/kind bug
/area custom-resources
/sig api-machinery
/priority critical-urgent

What this PR does / why we need it:
Fixes in-memory list decoding of the custom resource handler. Etcd items were getting decoded into an &unstructured.Unstructured object created using reflect.New(), which meant individual list items did not inform the decoder what version they wanted to be. This means whatever version was in storage was what was populated into the list in memory.

The results were used by two callers:

  1. Handlers sending list output in response to API requests; conversion of individual items to the requested version was done when writing the response, so the end result was correct.
  2. The lister used to fill the watch cache store; conversion was not done here, so a watch cache could end up with in-memory versions that did not match the request version.

To trigger this:

  1. Create a CRD with two versions (e.g. storage version v1, non-storage version v2), and spec/status
  2. Create an CR instance in the non-storage version (v2). Note the generation is set to 1
  3. Edit something inconsequential in the crd spec (like a category or shortname). This triggers destruction/recreation of the handlers, which will create new watch caches, which will fill via a list of existing CRs. This means the non-storage handler (v2) has a watch cache populated with v1 objects.
  4. Patch the CR instance via the non-storage version with a no-op patch. Note the generation increments to 2. This is because the check for any changes outside metadata sees a diff in the apiVersion between the old in-memory object (v1) and the new in-memory object (v2). The old one should have been converted to v2.

Which issue(s) this PR fixes:
Fixes #88473

Special notes for your reviewer:
This is a minimal fix for backporting. A more complete fix would plumb the NewFunc() already held by the store through more layers to be used for list decoding.

Does this PR introduce a user-facing change?:

Fixes conversion error in multi-version custom resources that could cause metadata.generation to increment on no-op patches or updates of a custom resource.

/cc @sttts @mattmoor

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Mar 10, 2020

@liggitt: GitHub didn't allow me to request PR reviews from the following users: mattmoor.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

What type of PR is this?
/kind bug
/area custom-resources
/sig api-machinery
/priority critical-urgent

What this PR does / why we need it:
Fixes in-memory list decoding of the custom resource handler. Etcd items were getting decoded into an &unstructured.Unstructured object created using reflect.New(), which meant individual list items did not inform the decoder what version they wanted to be. This means whatever version was in storage was what was populated into the list in memory.

The results were used by two callers:

  1. Handlers sending list output in response to API requests; conversion of individual items to the requested version was done when writing the response
  2. The lister used to fill the watch cache store; conversion was not done here, so a watch cache could end up with in-memory versions that did not match the request version.

To trigger this:

  1. Create a CRD with two versions (e.g. storage version v1, non-storage version v2), and spec/status
  2. Create an CR instance in the non-storage version (v2). Note the generation is set to 1
  3. Edit something inconsequential in the crd spec (like a category or shortname). This triggers destruction/recreation of the handlers, which will create new watch caches, which will fill via a list of existing CRs. This means the non-storage handler (v2) has a watch cache populated with v1 objects.
  4. Patch the CR instance via the non-storage version with a no-op patch. Note the generation increments to 2. This is because the check for any changes outside metadata sees a diff in the apiVersion between the old in-memory object (v1) and the new in-memory object (v2). The old one should have been converted to v2.

Which issue(s) this PR fixes:
Fixes #88473

Special notes for your reviewer:
This is a minimal fix for backporting. A more complete fix would plumb the NewFunc() already held by the store through more layers to be used for list decoding.

  • add a test demonstrating the bug and the fix.

Does this PR introduce a user-facing change?:

Fixes conversion error in multi-version custom resources that could cause metadata.generation to increment on no-op patches or updates of a custom resource.

/cc @sttts @mattmoor

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.

@liggitt liggitt changed the title Preserve target apiVersion when decoding into unstructured lists WIP - Preserve target apiVersion when decoding into unstructured lists Mar 10, 2020
@liggitt

This comment has been minimized.

Copy link
Member Author

liggitt commented Mar 10, 2020

/milestone v1.18

We'll want to pick a fix for this issue back to 1.15/1.16/1.17

@dims

This comment has been minimized.

Copy link
Member

dims commented Mar 10, 2020

This is pure sorcery! 🧙

@k8s-ci-robot k8s-ci-robot added size/L and removed size/M labels Mar 10, 2020
@liggitt liggitt changed the title WIP - Preserve target apiVersion when decoding into unstructured lists Preserve target apiVersion when decoding into unstructured lists Mar 10, 2020
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Mar 10, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@liggitt liggitt force-pushed the liggitt:crd-list-conversion branch from 8d2b1a2 to 0aa5691 Mar 10, 2020
@liggitt liggitt force-pushed the liggitt:crd-list-conversion branch from 0aa5691 to fa12441 Mar 10, 2020
@smarterclayton

This comment has been minimized.

Copy link
Contributor

smarterclayton commented Mar 10, 2020

Looked fine at a high level

@enj
enj approved these changes Mar 10, 2020
Copy link
Member

enj left a comment

/lgtm

Some comments / nits.

if err := crdInformer.Informer().GetStore().Add(crd); err != nil {
t.Fatal(err)
}
Comment on lines +463 to +465

This comment has been minimized.

Copy link
@enj

enj Mar 10, 2020

Member

I think you need this is because you never start the informers? Probably worth a comment.

@enj

This comment has been minimized.

Copy link
Member

enj commented Mar 10, 2020

Ran the test against master to confirm it catches the bug:

$ go test k8s.io/apiextensions-apiserver/pkg/apiserver

--- FAIL: TestHandlerConversionWithWatchCache (5.70s)
    customresource_handler_test.go:579: expected list item to be schema.GroupVersionKind{Group:"stable.example.com", Version:"v1alpha1", Kind:"MultiVersion"}, got schema.GroupVersionKind{Group:"stable.example.com", Version:"v1beta1", Kind:"MultiVersion"}
    customresource_handler_test.go:579: expected list item to be schema.GroupVersionKind{Group:"stable.example.com", Version:"v1alpha1", Kind:"MultiVersion"}, got schema.GroupVersionKind{Group:"stable.example.com", Version:"v1beta1", Kind:"MultiVersion"}

--- FAIL: TestHandlerConversionWithoutWatchCache (2.32s)
    customresource_handler_test.go:579: expected list item to be schema.GroupVersionKind{Group:"stable.example.com", Version:"v1alpha1", Kind:"MultiVersion"}, got schema.GroupVersionKind{Group:"stable.example.com", Version:"v1beta1", Kind:"MultiVersion"}
    customresource_handler_test.go:579: expected list item to be schema.GroupVersionKind{Group:"stable.example.com", Version:"v1alpha1", Kind:"MultiVersion"}, got schema.GroupVersionKind{Group:"stable.example.com", Version:"v1beta1", Kind:"MultiVersion"}

FAIL	k8s.io/apiextensions-apiserver/pkg/apiserver	8.388s
@enj

This comment has been minimized.

Copy link
Member

enj commented Mar 10, 2020

/retest

@k8s-ci-robot k8s-ci-robot merged commit 307bafb into kubernetes:master Mar 10, 2020
17 checks passed
17 checks passed
cla/linuxfoundation liggitt authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-kind-ga-only-parallel Job succeeded.
Details
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-kind Job succeeded.
Details
pull-kubernetes-e2e-kind-ipv6 Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
tide In merge pool.
Details
@liggitt liggitt deleted the liggitt:crd-list-conversion branch Mar 10, 2020
k8s-ci-robot added a commit that referenced this pull request Mar 17, 2020
…5-upstream-release-1.15

Automated cherry pick of #88995: Preserve target apiVersion when decoding into unstructured
k8s-ci-robot added a commit that referenced this pull request Mar 17, 2020
…5-upstream-release-1.16

Automated cherry pick of #88995: Preserve target apiVersion when decoding into unstructured
k8s-ci-robot added a commit that referenced this pull request Mar 17, 2020
…5-upstream-release-1.17

Automated cherry pick of #88995: Preserve target apiVersion when decoding into unstructured
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants
You can’t perform that action at this time.