Missing annotation objects are not treated deterministically

[braunsonm]

What steps did you take:

1. Deploy a resource without an annotations block like so:

---
apiVersion: admissionregistration.k8s.io/v1beta1
kind: ValidatingWebhookConfiguration
metadata:
  name: validation.webhook.kpack.io
webhooks:
- admissionReviewVersions:
  - v1beta1
  clientConfig:
    service:
      name: kpack-webhook
      namespace: kpack
  failurePolicy: Fail
  sideEffects: None
  name: validation.webhook.kpack.io

2. Now deploy it again with a diff

          +   ...
          +   2,  2   metadata:
          +   3     -   annotations: {}
          +   4,  3     creationTimestamp: "2020-11-16T15:31:49Z"
          +   5,  4     generation: 388

What happened: A diff is always created for this resource even though it has not changed.

What did you expect: The kapp deployment should be idempotent. It seems like kapp removes the kapp specific annotations but leaves behind a empty annotations object which causes a deployment to always trigger again.

Additional Information: Automation in our deployments and reproducable deploys are important to us as we deploy with kapp often. This bug creates a lot of noise in some of our deployments and we do not want to have to change our manifest to fit a format that kapp likes.

[pivotaljohn]

One observed case is when the only annotations on the resource in the cluster are those applied by kapp itself.

When considering a resource without its history, those annotations are removed leaving the annotations: key with an empty map value.

https://github.com/vmware-tanzu/carvel-kapp/blob/cb948046934a0ed1fae3bb3865a63ac863b6f7b3/pkg/kapp/diff/resource_with_history.go#L187-L192

In this specific situation, should the annotations: entry be removed entirely?

[joaopapereira]

Hey @braunsonm I was not able to reproduce this particular problem, these were the steps I did.

1. create the file test.yml with the following content:

   ---
   apiVersion: admissionregistration.k8s.io/v1beta1
   kind: ValidatingWebhookConfiguration
   metadata:
     name: validation.webhook.kpack.io
   webhooks:
   - admissionReviewVersions:
     - v1beta1
     clientConfig:
       service:
         name: kpack-webhook
         namespace: kpack
     failurePolicy: Fail
     sideEffects: None
     name: validation.webhook.kpack.io
   

2. Executed

   ❯ kapp deploy -a missing-obj -f test.yml --diff-changes
   Target cluster 'https://127.0.0.1:51295' (nodes: kind-control-plane)
   
   @@ create validatingwebhookconfiguration/validation.webhook.kpack.io (admissionregistration.k8s.io/v1beta1) cluster @@
         0 + apiVersion: admissionregistration.k8s.io/v1beta1
         1 + kind: ValidatingWebhookConfiguration
         2 + metadata:
         3 +   labels:
         4 +     kapp.k14s.io/app: "1612903099649952000"
         5 +     kapp.k14s.io/association: v1.3c4cb9c91d8d8f6e41b6cf3b105f3d9f
         6 +   name: validation.webhook.kpack.io
         7 + webhooks:
         8 + - admissionReviewVersions:
         9 +   - v1beta1
        10 +   clientConfig:
        11 +     service:
        12 +       name: kpack-webhook
        13 +       namespace: kpack
        14 +   failurePolicy: Fail
        15 +   name: validation.webhook.kpack.io
        16 +   sideEffects: None
        17 +
   
   Changes
   
   Namespace  Name                         Kind                            Conds.  Age  Op      Op st.  Wait to    Rs  Ri
   (cluster)  validation.webhook.kpack.io  ValidatingWebhookConfiguration  -       -    create  -       reconcile  -   -
   
   Op:      1 create, 0 delete, 0 update, 0 noop
   Wait to: 1 reconcile, 0 delete, 0 noop
   
   Continue? [yN]: y
   
   3:38:26PM: ---- applying 1 changes [0/1 done] ----
   3:38:26PM: create validatingwebhookconfiguration/validation.webhook.kpack.io (admissionregistration.k8s.io/v1beta1) cluster
   3:38:26PM: ---- waiting on 1 changes [0/1 done] ----
   3:38:26PM: ok: reconcile validatingwebhookconfiguration/validation.webhook.kpack.io (admissionregistration.k8s.io/v1beta1) cluster
   3:38:26PM: ---- applying complete [1/1 done] ----
   3:38:26PM: ---- waiting complete [1/1 done] ----
   
   Succeeded
   

3. Without changing anything I did

   ❯ kapp deploy -a missing-obj -f test.yml --diff-changes
   Target cluster 'https://127.0.0.1:51295' (nodes: kind-control-plane)
   
   Changes
   
   Namespace  Name  Kind  Conds.  Age  Op  Op st.  Wait to  Rs  Ri
   
   Op:      0 create, 0 delete, 0 update, 0 noop
   Wait to: 0 reconcile, 0 delete, 0 noop
   
   Succeeded
   

In my case it doesn't show any diff. What do I need to do to replicate this problem? I was using kapp version 0.35.0 on a MacOS. What version of kapp are you using and what OS?

[braunsonm]

Ok @joaopapereira I figured out why you weren't able to replicate this issue. In this case the resource is being edited on the cluster (not related to the annotations block) and I have rebaseRules to ignore it. You can reproduce this by the following:

1. Deploy the resource I have above
2. kubectl edit ValidatingWebhookConfiguration validation.webhook.kpack.io to simulate the cluster making a change. You can set failurePolicy: Ignore as an example.
3. Try to deploy again. In addition to the changes made to the spec (which is expected as Kubernetes changed those and I did not send you my copy of the rebase rules) you will always see annotations object attempting to be deleted. Even though you made no changes to it and there is no changes to the annotations object on the cluster.

[joaopapereira]

As you suggested updating failurePolicy to Ignore before step 3 I get:

$ kapp deploy -a app-1 -f test.yml --diff-changes
Target cluster 'https://127.0.0.1:51295' (nodes: kind-control-plane)

  2,  2   metadata:
  3     -   annotations: {}
  4,  3     creationTimestamp: "2021-02-11T17:00:51Z"
  5,  4     generation: 2
  ...
 63, 62         namespace: kpack
 64     -       port: 443
 65     -   failurePolicy: Ignore
 66     -   matchPolicy: Exact
     63 +   failurePolicy: Fail
 67, 64     name: validation.webhook.kpack.io
 68     -   namespaceSelector: {}
 69     -   objectSelector: {}
 70, 65     sideEffects: None
 71     -   timeoutSeconds: 30
 72, 66

Changes

Namespace  Name                         Kind                            Conds.  Age  Op      Op st.  Wait to    Rs  Ri
(cluster)  validation.webhook.kpack.io  ValidatingWebhookConfiguration  -       2h   update  -       reconcile  ok  -

Op:      0 create, 0 delete, 1 update, 0 noop
Wait to: 1 reconcile, 0 delete, 0 noop

[joaopapereira]

Going to update the label and move it to our unprioritized list of issues. Let me know if this is something that is currently blocking you and needs a resolution ASAP so that we can prioritize it ASAP as well.

If you are interested in creating a PR for this go for it and if you need some help talk with any maintainer in Slack we are more than happy you help out.

[braunsonm]

Hey @joaopapereira glad you were able to reproduce it.

This is currently blocking fixing this issue on the cf-for-k8s project: https://github.com/cloudfoundry/cf-for-k8s/issues/110 It would be nice to see a fix for it as this issue spans back awhile now.

[renuy]

Took another look at this issue along with cloudfoundry/cf-for-k8s#110 There are couple of things going on here. Diff shown in the service account added by token controller: this is a valid diff. There is no way of handling it as of now. However there is a PR underway to use rebase rules accept the new token. Diff with regards to empty annotations : this one is of no consequence and can be ignored. There is diff in FailurePolicy and nameSpace selector : these can be handled by adding rebase rules

[braunsonm]

The diff regarding empty annotations can be ignored but should not be printed in the diff or allow a rebase rule to toggle that. It creates noise which is what this issue is trying to resolve

[renuy]

Agreed, its noise to see empty annotation in diff like this, But it comes only when there is a valid diff elsewhere.