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

Regression in kubectl apply for TPR's in v1.4.8 #39906

Closed
huggsboson opened this issue Jan 14, 2017 · 9 comments
Closed

Regression in kubectl apply for TPR's in v1.4.8 #39906

huggsboson opened this issue Jan 14, 2017 · 9 comments
Labels
sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery.

Comments

@huggsboson
Copy link
Contributor

huggsboson commented Jan 14, 2017

BUG REPORT
Kubernetes version (use kubectl version): v1.4.8

Environment:

  • Cloud provider or hardware configuration: Baremetal @ Box
  • OS (e.g. from /etc/os-release): ScientificLinux 6
  • Kernel (e.g. uname -a): Linux compute-master1001.ve.box.net 3.10.0-229.14.1.el7.x86_64 #1 SMP Tue Sep 15 05:09:55 CDT 2015 x86_64 x86_64 x86_64 GNU/Linux
  • Install tools:
  • Others:

What happened:
In kube v1.4.7 multiple invocations of kubectl apply -f instanceOfThirdPartyResource.yaml would succeed in v1.4.8 we get the error:

&{0xc820016d80 0xc82013bea0 user-dev pki-95c36eb44c4a9132082142f7d5e47bf0 /box/deployment-config/release/dsv31/dev/user/app.json &TypeMeta{Kind:,APIVersion:extensions/v1beta1,} &TypeMeta{Kind:,APIVersion:extensions/v1beta1,} 178335915 false}
for: "/box/deployment-config/release/dsv31/dev/user/app.json": error when getting instance of versioned object for box-pki.com/v1, Kind=Pki: "/box/deployment-config/release/dsv31/dev/user/app.json": no kind "Pki" is registered for version "box-pki.com/v1"

What you expected to happen:
Would expect it to succeed. (Though maybe not actually apply changes).

How to reproduce it (as minimally and precisely as possible):
Make a thirdpartyresource make an instance of it. And apply that multiple times on 1.4.8. Sorry I can't share our manifests. I'll try to get another person at Box to produce a test case.

Anything else do we need to know:
We believe the regression was introduced in:
261cdcc

@pwittrock @ymqytw @ahakanbaba

@mengqiy
Copy link
Member

mengqiy commented Jan 14, 2017

That patch is already in 1.4, 1.5 and master branch. We need a fix for all 3 branch.

@GnawNom
Copy link

GnawNom commented Jan 17, 2017

Sample case to reproduce the issue

joewang@mbp-003106:~/git/deployment-config/scripts/ > ./kubectl148 version
Client Version: version.Info{Major:"1", Minor:"4", GitVersion:"v1.4.8", GitCommit:"c5fcb1951afb8150e4574bc234aed957cb586eb0", GitTreeState:"clean", BuildDate:"2017-01-12T02:20:24Z", GoVersion:"go1.7.1", Compiler:"gc", Platform:"darwin/amd64"}
Server Version: version.Info{Major:"1", Minor:"4", GitVersion:"v1.4.8", GitCommit:"c5fcb1951afb8150e4574bc234aed957cb586eb0", GitTreeState:"clean", BuildDate:"2017-01-12T02:14:29Z", GoVersion:"go1.6.3", Compiler:"gc", Platform:"linux/amd64"}

joewang@mbp-003106:~/git/deployment-config/scripts/ > ./kubectl148 apply -f pkidef.json
thirdpartyresource "pki.box-pki.com" configured
joewang@mbp-003106:~/git/deployment-config/scripts/ > ./kubectl148 apply -f pkidef.json
thirdpartyresource "pki.box-pki.com" configured
joewang@mbp-003106:~/git/deployment-config/scripts/ > ./kubectl148 apply -f pkiobj.json
pki "pki-aa627b6931ee3f49b02692a2e2880d9d" created
joewang@mbp-003106:~/git/deployment-config/scripts/ > ./kubectl148 apply -f pkiobj.json
error: error when applying patch:

to:
&{0x42293bd40 0x422aa9030 default pki-aa627b6931ee3f49b02692a2e2880d9d pkiobj.json &TypeMeta{Kind:,APIVersion:extensions/v1beta1,} &TypeMeta{Kind:,APIVersion:extensions/v1beta1,} 370 false}
for: "pkiobj.json": error when getting instance of versioned object for box-pki.com/v1, Kind=Pki: "pkiobj.json": no kind "Pki" is registered for version "box-pki.com/v1"

pkidef.json:

{
   "apiVersion": "extensions/v1beta1",
   "description": "A specification for client certs and trust for a service",
   "kind": "ThirdPartyResource",
   "metadata": {
      "name": "pki.box-pki.com"
   },
   "versions": [
      {
         "name": "v1"
      }
   ]
}

pkiobj.json:

{
   "apiVersion": "box-pki.com/v1",
   "kind": "Pki",
   "metadata": {
      "name": "pki-aa627b6931ee3f49b02692a2e2880d9d",
      "namespace": "default"
   },
   "spec": {
      "arbitraryData": {
         "foo": "bar"
      }
   }
}

@mengqiy
Copy link
Member

mengqiy commented Jan 17, 2017

@GnawNom Thanks for your sample. I'm working on it.

@mengqiy
Copy link
Member

mengqiy commented Jan 18, 2017

@liggitt Created a PR #40096 to fix this issue in 1.4.

But in branch 1.5 and master, apply breaks without reaching the code introduced in #38982. So #40096 is not enough to fix it.
The error message in 1.5 and master is error: unable to decode "../pkiobj.json": no kind "Pki" is registered for version "box-pki.com/v1".
kubectl apply fails to Decode() when calling InfoForData(). kubectl edit also calls this function, and it doesn't work for TPR neither.

@pires
Copy link
Member

pires commented Jan 18, 2017

/cc @aaronlevy because of kubectl edit TPR failures we've seen before.

@calebamiles
Copy link
Member

calebamiles commented Jan 20, 2017

@pires, aaronlevy is on vacation until the end of January

@pires
Copy link
Member

pires commented Jan 21, 2017

@luxas I can fix this but your guidance is much appreciated.

@liggitt
Copy link
Member

liggitt commented Jan 21, 2017

@luxas I can fix this but your guidance is much appreciated.

the correct fix is... invasive.

kubectl 1.4.7 was not applying patches to third party resources correctly at all. if you look at the wire transfer (--v=8) of applying the above example with kubectl 1.4.7, you can see that it is actually sending a patch of a ThirdPartyResourceData object, not the Pki object:

Request Body: "{\"data\":\"ewogICAiYXBpVmVyc2lvbiI6ICJib3gtcGtpLmNvbS92MSIsCiAgI
CJraW5kIjogIlBraSIsCiAgICJtZXRhZGF0YSI6IHsKICAgICAgIm5hbWUiOiAicGtpLWFhNjI3YjY5M
zFlZTNmNDliMDI2OTJhMmUyODgwZDlkIiwKICAgICAgIm5hbWVzcGFjZSI6ICJkZWZhdWx0IgogICB9L
AogICAic3BlYyI6IHsKICAgICAgImFyYml0cmFyeURhdGEiOiB7CiAgICAgICAgICJmb28iOiAiYmFyI
gogICAgICB9CiAgIH0KfQ==\",\"metadata\":{\"creationTimestamp\":null}}"

that's concerning for multiple reasons:

  1. it means no actual patching is happening, it's a complete replace every time (since the full object lives in the single data field of a ThirdPartyResourceData object)
  2. it means the server-side patch application was also being done on the internal JSON serialization of a ThirdPartyResourceData object

in kubectl 1.4.8, a fix was made to the way patches were computed to do so using the versioned struct. that exposed the fact that third party resource data patching was being done incorrectly.

the correct fix would be:

  1. make kubectl deal with unknown types using runtime.Unstructured, which simply preserves the original JSON content, instead of decoding to ThirdPartyResourceData. (see a WIP version of this in make kubectl generic commands work with unstructured objects #40260)
  2. add support for computing merge patches between two JSON blobs without a go struct (see a WIP version of this in support generic 3-way json merge patch for patch obj w/o a schema #40096)

both of those changes are large, complex, and not something I'd recommend picking into 1.4 (or probably 1.5)

given that TPRs were not patching correctly even in 1.4.7, my recommendation would be to use update instead for TPRs until patching support is added for them.

k8s-github-robot pushed a commit that referenced this issue Jan 27, 2017
Automatic merge from submit-queue (batch tested with PRs 39223, 40260, 40082, 40389)

make kubectl generic commands work with unstructured objects

part of making apply, edit, label, annotate, and patch work with third party resources

fixes #35149
fixes #34413

prereq of:
#35496
#40096

related to:
#39906
#40119

kubectl is currently decoding any resource it doesn't have compiled-in to a ThirdPartyResourceData struct, which means it computes patches using that struct, and would try to send a ThirdPartyResourceData object to the API server when running `apply`

This PR removes the behavior that decodes unknown objects into ThirdPartyResourceData structs internally, and fixes up the following generic commands to work with unstructured objects

- [x] apply
  - [x] decode into runtime.Unstructured objects
  - [x] successfully use `--record` with unregistered objects 
- [x] patch
  - [x] decode into runtime.Unstructured objects
  - [x] successfully use `--record` with unregistered objects 
- [x] describe
  - [x] decode into runtime.Unstructured objects
  - [x] implement generic describer
- [x] fix other generic kubectl commands to work with unstructured objects
  - [x] label
  - [x] annotate

follow-ups for pre-existing issues:
- [ ] `explain` doesn't work with unregistered resources
- [ ] remove special casing of federation group in clientset lookups, etc
- [ ] `patch`
  - [ ] doesn't honor output formats when persisting to server (`kubectl patch -f svc.json --type merge -p '{}' -o json` doesn't output json)
  - [ ] --local throws exception (`kubectl patch -f svc.json --type merge -p '{}' --local`)
- [ ] `apply`
  - [ ] fall back to generic JSON patch computation if no go struct is registered for the target GVK (e.g. #40096)
  - [ ] ensure subkey deletion works in CreateThreeWayJSONMergePatch
  - [ ] ensure type stomping works in CreateThreeWayJSONMergePatch
  - [ ] lots of tests for generic json patch computation
  - [ ] prevent generic apply patch computation among different versions
  - [ ] reconcile treatment of nulls with #35496
- [ ] `edit`
  - [ ] decode into runtime.Unstructured objects
  - [ ] fall back to generic JSON patch computation if no go struct is registered for the target GVK
@bgrant0607 bgrant0607 added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Feb 7, 2017
@liggitt
Copy link
Member

liggitt commented Feb 8, 2017

apply has been updated to work with third party resources in 1.6 in #40666

@liggitt liggitt closed this as completed Feb 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery.
Projects
None yet
Development

No branches or pull requests

7 participants