kubectl-apply silently drops annotations with boolean values

[redbaron]

/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"}

[redbaron]

/sig cli

[yue9944882]

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

kubernetes/staging/src/k8s.io/apimachinery/pkg/runtime/serializer/json/json.go

Line 189 in 1e6ebd4

if err := jsoniter.ConfigCompatibleWithStandardLibrary.Unmarshal(data, obj); err != nil {

where jsoniter maps bytes into corresponding resource object without any warning.

[bgrant0607]

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

[fejta-bot]

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

[redbaron]

/remove-lifecycle stale

[fejta-bot]

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

[redbaron]

/remove-lifecycle stale

[yue9944882]

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]

@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]

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

[code-sleuth]

/remove-lifecycle stale
/assign code-sleuth

[hulu1522]

👍 +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]

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

[code-sleuth]

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]

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

[code-sleuth]

/remove-lifecycle stale

[nbrasz]

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]

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]

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

[d-baranowski]

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

[griffin]

👋 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]

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.