search

carvel-dev / kapp

kapp is a simple deployment tool focused on the concept of "Kubernetes application" — a set of resources with the same label
https://carvel.dev/kapp
Apache License 2.0
922 stars 110 forks source link

Missing annotation objects are not treated deterministically #190

Open braunsonm opened 3 years ago

braunsonm commented 3 years ago

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
  1. 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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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