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

kubectl-apply silently drops annotations with boolean values #59113

Closed
redbaron opened this issue Jan 31, 2018 · 22 comments · Fixed by #83552
Closed

kubectl-apply silently drops annotations with boolean values #59113

redbaron opened this issue Jan 31, 2018 · 22 comments · Fixed by #83552
Assignees
Labels
kind/bug lifecycle/frozen priority/important-longterm sig/cli
Milestone

Comments

@redbaron
Copy link
Contributor

@redbaron redbaron commented Jan 31, 2018

/kind bug

What happened:
When creating resource with boolean value in annotation, kubectl apply completes sucessfully without any errors, but resource has no annotations.

What you expected to happen:

Error similar to what kubectl create returns

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

$ kubectl apply -f <<EOF
apiVersion: v1
kind: ConfigMap
metadata:
  name: test
  annotations:
    bool: true
data: {}
EOF
configmap "test" created

Creates:

apiVersion: v1
kind: ConfigMap
metadata:
  annotations:
    kubectl.kubernetes.io/last-applied-configuration: |
      {"apiVersion":"v1","data":{},"kind":"ConfigMap","metadata":{"annotations":{},"name":"test","namespace":"kube-system"}}
  creationTimestamp: 2018-01-31T14:23:42Z
  name: test
  namespace: kube-system
  resourceVersion: "9612155"
  selfLink: /api/v1/namespaces/kube-system/configmaps/test
  uid: 5689a651-0692-11e8-a6e3-005056bced2e

Environment:

Client Version: version.Info{Major:"1", Minor:"9", GitVersion:"v1.9.2", GitCommit:"5fa2db2bd46ac79e5e00a4e6ed24191080aa463b", GitTreeState:"clean", BuildDate:"2018-01-18T10:09:24Z", GoVersion:"go1.9.2", Compiler:"gc", Platform:"linux/amd64"}
Server Version: version.Info{Major:"1", Minor:"8", GitVersion:"v1.8.4", GitCommit:"9befc2b8928a9426501d3bf62f72849d5cbcd5a3", GitTreeState:"clean", BuildDate:"2017-11-20T05:17:43Z", GoVersion:"go1.8.3", Compiler:"gc", Platform:"linux/amd64"}
@k8s-ci-robot k8s-ci-robot added needs-sig kind/bug labels Jan 31, 2018
@redbaron
Copy link
Contributor Author

@redbaron redbaron commented Jan 31, 2018

/sig cli

@k8s-ci-robot k8s-ci-robot added sig/cli and removed needs-sig labels Jan 31, 2018
@yue9944882
Copy link
Contributor

@yue9944882 yue9944882 commented Feb 4, 2018

I believe it's by the implementation of json-iterator.

if err := jsoniter.ConfigCompatibleWithStandardLibrary.Unmarshal(data, obj); err != nil {
where jsoniter maps bytes into corresponding resource object without any warning.

@bgrant0607
Copy link
Member

@bgrant0607 bgrant0607 commented Feb 8, 2018

Annotation values must be strings. Something should generate an error, though

@fejta-bot
Copy link

@fejta-bot fejta-bot commented May 9, 2018

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale label May 9, 2018
@redbaron
Copy link
Contributor Author

@redbaron redbaron commented May 9, 2018

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale label May 9, 2018
@fejta-bot
Copy link

@fejta-bot fejta-bot commented Aug 7, 2018

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale label Aug 7, 2018
@redbaron
Copy link
Contributor Author

@redbaron redbaron commented Aug 7, 2018

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale label Aug 7, 2018
@yue9944882
Copy link
Contributor

@yue9944882 yue9944882 commented Aug 7, 2018

sadly, i'm afraid the annotations will keep typed map[string]string for a long time. 😢refactoring this will involve so many changes and we have to provide backward compatibility at the same time. it's almost impossible.

@redbaron
Copy link
Contributor Author

@redbaron redbaron commented Aug 7, 2018

@yue9944882 , thats fine, bug is more about kubectl, rather than annotation type on API side. kubectl create correctly errors when it sees non-conforming annotations, but kubectl apply doesn't

@fejta-bot
Copy link

@fejta-bot fejta-bot commented Nov 5, 2018

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale label Nov 5, 2018
@code-sleuth
Copy link
Member

@code-sleuth code-sleuth commented Nov 6, 2018

/remove-lifecycle stale
/assign code-sleuth

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale label Nov 6, 2018
@hulu1522
Copy link

@hulu1522 hulu1522 commented Nov 12, 2018

👍 +1

There needs to be a way for kubectl apply to return an error and not apply the descriptors without any annotations. I just spent a week diagnosing an issue only to find out it was because my annotations all got removed from a simple int64 error. It could have been prevented if it erred out like normal. :)

@vithati
Copy link
Contributor

@vithati vithati commented Nov 21, 2018

@code-sleuth, are you working on this issue? if not, can i look at this issue?

@code-sleuth
Copy link
Member

@code-sleuth code-sleuth commented Nov 21, 2018

Hey @vithati , i'm working on it. I have some changes that i'm still discussing with @juanvallejo before i raise my PR.

@fejta-bot
Copy link

@fejta-bot fejta-bot commented Feb 19, 2019

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale label Feb 19, 2019
@code-sleuth
Copy link
Member

@code-sleuth code-sleuth commented Feb 20, 2019

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale label Feb 20, 2019
@nbrasz
Copy link

@nbrasz nbrasz commented Apr 2, 2019

I understand boolean values are not allowed, however I am seeing that if i attempt to kubectl apply a yaml with a new boolean annotation, not only will it not add the new annotation, but it will also delete all existing annotations silently.

this seems like more than an error messaging bug

@jbeda
Copy link
Contributor

@jbeda jbeda commented Apr 6, 2019

FWIW -- this hit me and it was very confusing. What it looks like happens now (at least with kubectl v1.14.0 against v1.12.6-gke.10 is that the entire annotation map is not updated. No error is returned from kubectl.

@alexmt
Copy link

@alexmt alexmt commented May 6, 2019

This hit me too. Very confusing behavior. It cost 2 man-hours to understand the root cause.

@d-baranowski
Copy link

@d-baranowski d-baranowski commented May 13, 2019

I'm currently unable to deploy via kubectl apply -f due to this issue. Is there a way around it?

Error from server (Timeout): error when applying patch:
{"metadata":{"annotations":{}}}
to:
&{0xc4218ee600 0xc420366690 s6 db-proxy STDIN 0xc42026e478 13632802 false}
for: "STDIN": Timeout: request did not complete within requested timeout 30s

@liggitt liggitt added lifecycle/frozen priority/important-longterm labels Jun 12, 2019
@griffin
Copy link

@griffin griffin commented Jul 19, 2019

👋 Hello!

I have tracked down the root cause of this problem being the omission of error propagation in this Object interface. The error is dropped in the implementation of GetAnnotations, but is emitted by NestedStringMap.

I’m going to present all of the solutions that I can think of:

  1. Add a GetAnnotationsSafely() (map[string]string, error) prototype to the Object interface interface. This will allow the error to be propagated up naturally and would be the most elegant solution. I don’t fully understand any other consequences of this change other than reimplementing this function for other types that implement this interface.
  2. Add a warning using utilruntime.HandleError as done in other functions in this package like SetOwnerReferences. I think this should be done no matter what to catch other users of this prototype from experiencing bugs like this.
  3. Add a type check earlier in the call stack to check if the type of Object is Unstructured and call NestedStringSlice so that we can capture the error and default on using the interface function. An example location would be in MetadataAccessor’s Annotations function here. I don’t think that this is a good idea because is doesn’t solve the root cause of this error which could pop up in other areas.
  4. Cast other types to strings. Would break compatibility and would cause confusion to users when it would work only with specific versions of kubectl

Any other suggestions are welcome, and I’d like to hear from the community to find the most elegant solution to this critical problem.

There may be other functions in this interface where we should pass an error as well.

I’ve submitted a PR to implement what was described in bullet 1 and 2.

Thank you!

Griffin Dunn

@redbaron
Copy link
Contributor Author

@redbaron redbaron commented Jul 20, 2019

Hm, Set* methods also return no errors, therefore you can SetAnnotation and receive different result back on subsequent GetAnotations breaking symmetry.

I doubt API change is acceptable on that level, so there should be either some higher level type check of arts before descending into Unstructured codepath , for instance instantiate desired object as a typed object to see if passes all the checks.

@liggitt liggitt added this to the v1.17 milestone Oct 6, 2019
takotab added a commit to takotab/Kubernetes-Starter-Kit-Developers that referenced this issue Dec 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug lifecycle/frozen priority/important-longterm sig/cli
Projects
None yet