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
921 stars 110 forks source link

Kapp shows "update" operation for ConfigMap that has no changes #603

Open ChristianCiach opened 2 years ago

ChristianCiach commented 2 years ago

What steps did you take:

I have a simple, static ConfigMap where kapp insists that deploying the ConfigMap will result in an update operation, although the ConfigMap is exactly the same as the deployed one. If I deploy the file multiple times in a row, kapp still announces the update operation every time.

What did you expect:

When the ConfigMap has not changed, kapp should not show an update operation.

Anything else you would like to add:

Using kapp deploy -c shows the removal of an empty metadata.annotations element:

kapp deploy -a clustering  -f configmap.yaml -c
Target cluster 'https://127.0.0.1:6443' (nodes: mngr1.censored 8+)

@@ update configmap/clustering-cacerts-m7b68mk2mm (v1) namespace: default @@
  ...
4776,4776   metadata:
4777     -   annotations: {}
4778,4777     creationTimestamp: "2022-09-12T14:26:59Z"
4779,4778     labels:

Changes

Namespace  Name                           Kind       Age  Op      Op st.  Wait to    Rs  Ri  
default    clustering-cacerts-m7b68mk2mm  ConfigMap  16m  update  -       reconcile  ok  -  

I don't know where this annotations field comes from. The metadata of our ConfigMap does not include an annotations field:

kind: ConfigMap
metadata:
  labels:
    app.kubernetes.io/part-of: clustering
  name: clustering-cacerts-m7b68mk2mm
  namespace: default

Kapp seems to think that the absence of an annotations field is different from an empty one.

Environment:


Vote on this request

This is an invitation to the community to vote on issues, to help us prioritize our backlog. Use the "smiley face" up to the right of this comment to vote.

👍 "I would like to see this addressed as soon as possible" 👎 "There are other more important things to focus on right now"

We are also happy to receive and review Pull Requests if you want to help working on this issue.

ChristianCiach commented 2 years ago

The actual deployment of the not-really-changed ConfigMap is very fast, less than a second:

3:00:12PM: ---- applying 1 changes [0/1 done] ----
3:00:12PM: update configmap/clustering-cacerts-m7b68mk2mm (v1) namespace: default
3:00:12PM: ---- waiting on 1 changes [0/1 done] ----
3:00:12PM: ok: reconcile configmap/clustering-cacerts-m7b68mk2mm (v1) namespace: default
3:00:12PM: ---- applying complete [1/1 done] ----
3:00:12PM: ---- waiting complete [1/1 done] ----
ChristianCiach commented 2 years ago

Kapp version 0.51.0 behaves the same, so this is not a very new bug.

praveenrewar commented 2 years ago

Hi @ChristianCiach! I am not able to reproduce the issue with kapp version 0.52.0 and kubernetes version 1.24. Do you have any kapp config (like rebase rules) along with the manifest or are there any external factors that could effect this resource?

I don't know where this annotations field comes from. The metadata of our ConfigMap does not include an annotations field

kapp adds annotations to store the original resource (kapp.k14s.io/original) and the diff md5 (kapp.k14s.io/original-diff-md5).

ChristianCiach commented 2 years ago

Thank you for your fast response!

Well, we do have a custom kapp config, but I can reproduce this issue even without those. This is very confusing. I will try to investigate this further tomorrow and I will report back then.

ChristianCiach commented 2 years ago

There is something inside the ConfigMap contents that triggers this issue. It is a root-certiticate bundle and after stripping more and more certificates from the ConfigMap, the issue eventually disappears. I don't think the file is confidential, so I will upload it after a quick verification.

ChristianCiach commented 2 years ago

Here you go: https://gist.github.com/ChristianCiach/3ac9b7261ab9b14d7cdefe7b171b542f

Can you reproduce the issue when deploying this file using kapp deploy -a app-name -f configmap-with-bug.yaml?

By the way, the file has been automatically created from a cacert.pem file using a Kustomize configMapGenerator.

praveenrewar commented 2 years ago

Yeah, I see what's happening. We add the kapp.k14s.io/original annotation for every resource, but if the kapp.k14s.io/disable-original annotation is present or if the resource size is large then this annotation is not added (as it would exceed the maximum size allowed for an annotation). Right now it seems to be the latter case and I think this is a bug because of which this diff is being produced every time. It doesn't happen when I manually add the kapp.k14s.io/disable-original annotation (you could maybe use this as a workaround until this is resolved).

ChristianCiach commented 2 years ago

According to https://github.com/vmware-tanzu/carvel-kapp/issues/410#issuecomment-1015259613 , adding kapp.k14s.io/disable-original may also trigger the issue I am currently seeing:

Redeploying with the unchanged configuration file will show operation as an update due to dumb diffing.

This issue seems to be purely cosmetic, so I am okay with not doing anything on my side for the time being,.

praveenrewar commented 2 years ago

It's true that adding the kapp.k14s.io/disable-original could result in unwanted diffs because the kapp.k14s.io/original annotation is used to calculate diff in a much smarter way. Although, right now, the reason is a bit different. kapp adds the kapp.k14s.io/original, kapp.k14s.io/original-diff-md5 and the kapp.k14s.io/identity annotation for all the resources. The identity annotation is removed as part of diffing and since the other 2 annotations are already absent, it causes the removal of the empty annotations: {}, so even if we add any random annotation and not just the disable-original annotation, I think the diff would be gone as there will be some annotation present in the manifest. We can also use a rebase rule to copy the existing annotations.

ChristianCiach commented 2 years ago

Thanks so far! I can confirm that adding the annotation removes the bogus update operation in this case. Of course it would be nice to see this fixed properly some day, but this works fine for us for the time being. Thanks so much for your quick help!