kapp
kapp copied to clipboard
carvel-dev
Strip kustomize-style name hash suffix
What this PR does / why we need it:
This PR tries to demonstrate a way to implement the feature request from #81.
Which issue(s) this PR fixes:
Fixes #81
Does this PR introduce a user-facing change?
Adds an `StripNameHashSuffix` option to config, allowing to strip kustomize-style ConfigMap/Secret hash suffixes.
Additional Notes for your reviewer:
Review Checklist:
- [x] Follows the developer guidelines
- [x] Relevant tests are added or updated
- [x] Relevant docs in this repo added or updated (none found)
- [ ] Relevant carvel.dev docs added or updated in a separate PR and there's a link to that PR (will do that after first feedback here)
- [x] Code is at least as readable and maintainable as it was before this change
Additional documentation e.g., Proposal, usage docs, etc.:
@criztovyl, you must sign our contributor license agreement before your changes are merged. Click here to sign the agreement. If you are a VMware employee, read this for further instruction.
Some thoughts on where to put the activation flag:
- the suffix-strip activation should got to config, like
templateRules - these are passed to
VersionedResource(versioned resource type) fromChangeSetWithVersionedRes(change-set type) - the change-set type needs to be adjusted to pass the activation from config to the versioned resource
- the change-set type does not know about config, it only gets the templateRules directly. thus the places using the change-set will need be adjusted to pass the activation. as the demonstration tests is on change-set level I'll not care about this yet.
While looking how to change the change-set type, it came to my understanding that the versioned resource type seems to be used only in change-set type, for the following two things:
- working with the name / resource itself (
BaseNameAndVersion()etc.) - updating the name on affected resources (
UpdateAffected())
First, I'll need to adjust the places where it's used for 1.
There it seems the change-set type instantiates the versioned-resource type ad-hoc/in-line, with the templateRules niled-out.
It only instantiates with rules when actually calling the update for affected resources. This suggests the templateRules are only required when updating affected resources (i.e. 2.).
On the other hand the suffix-stripping flag is required in all the places the versioned-resource is currently instantiated ad-hoc. This means I need to change all those places. I'll move forward with that for the moment.
I am considering to extract the ad-hoc instantiation into a method in the change-set type.
But generally I think it might be better to separate 1. from the above list into an own type, e.g. VersionedResourceName.
That being a more invasive change, is another reason to, for now, simply changing the ad-hoc instantiations.
I suspect a simple flag will not be enough, might need to be a more complex type, as kustomize allows you to disable the name-suffix-hash, so you might need to exclude certain CMs/Secrets.
It also affects only CMs/Secrets, I think my current implementation will also strip non-suffixes from e.g. deployments like name: my-deployment (i.e. stripping -deployment).
It also affects only CMs/Secrets, I think my current implementation will also strip non-suffixes from e.g. deployments like name: my-deployment (i.e. stripping -deployment).
this is my biggest concern as well. can we have some stricter way of determining what is "suffixed by kustomize" and what is not?
@criztovyl Thank you so much for the PR. I am pretty new to kustomize space, so just trying to understand a bit more about the suffixHash...
Does the suffix get added to only the ConfigMap and Secrets created using the kustomize generators? Is there any way to determine if a resource has a suffix added by kustomize (using some annotation, etc,.) which we can use to remove the suffix. Or is there a way to disable the suffix (I noticed this option provided here, but not sure if this will work for all the resources)
Thanks for the feedback :) I am working on this over the weekends, I'll answer your questions then.
I was one of the first persons to upvote the issue https://github.com/vmware-tanzu/carvel-kapp/issues/81 many months ago and I have been thinking about this issue a lot since then.
I am sorry for being so negative, but I think this issue is close to impossible to solve.
Or is there a way to disable the suffix
Yes. You've already found disableNameSuffixHash: true. This is what we are using right now, but this has to be configured for each and every configMap/secret generator separately and is easily forgotten and also adds a lot of "noise" to the kustomization.yaml files.
Is there any way to determine if a resource has a suffix added by kustomize (using some annotation, etc,.) which we can use to remove the suffix.
No, there are no annotations or anything else to identify names of configMaps or secrets that have been rewritten by kustomize.
This is one of the main reason why I think this issue is next to impossible to solve with perfect accuracy. Kapp would not only have to remove the hash-suffixes from the configMaps and secrets, but also from all resources that are referencing these configMaps and secrets. Again, there a no annotations or anything to identify these references.
To make matters worse and which is probably the final nail in the coffin here: Similar to kapp, kustomize has a set of hardcoded template rules which are called name reference transformers in kustomize. Similar to kapp, users can provide additional transformers via kustomizeconfig.yaml files to enable kustomize to also rewrite configMap/secret references inside Custom Resources (CRs) that kustomize does not support out of the box. For example, we use this kustomizeconfig.yaml to let kustomize transform the name of a Secret reference inside a CR called ServersTransport:
nameReference:
- kind: 'Secret'
fieldSpecs:
- kind: 'ServersTransport'
path: 'spec/rootCAsSecrets'
If we want kapp to strip this hash-suffix from this CR, kapp needs to take this config into account to know about this reference:
apiVersion: kapp.k14s.io/v1alpha1
kind: Config
templateRules:
- resourceMatchers:
- apiVersionKindMatcher: {apiVersion: v1, kind: Secret}
affectedResources:
objectReferences:
- path: [spec, rootCAsSecrets, {allIndexes: true}]
resourceMatchers:
- apiVersionKindMatcher: {apiVersion: traefik.containo.us/v1alpha1, kind: ServersTransport}
I don't know how I feel about that. These templateRules primarily exist so that kapp can add its own "version"-suffix to all resources that reference the given resource (a "Secret" in this example). But now we need these templateRules so that kapp can remove these suffixes that have been added by another tool. This feels... icky.
Oh, true, I did not think about that consequence yet, thank you!
Actually my concern is just cosmetic, I dont necessarily want true versioning. I am happy if the diff shows two hash-suffixed CMs/Secrets as an update, not as delete + add.
For that I can ignore the referneces, they will show as updated anyway. So the stripping of the suffix should not go into final cluster state (as it would do with my current code), it should only be used to identifying potentially related resources.
I'll look into that.
From my understanding the resources are diffed by grouping the old and new one using their name and then diffing these groups-of-two. Thus the challenge should be to "just" get the old and new hash-suffixed CMs/Secrets into the same group (i.e. update) instead of two separate one (delete+add).
The pieces are already there, used for actually versioned resources.
Spoken differently I want to build a VersionedResource that provides a UniqVersionedKey that strips the suffix, but where SetBaseName and UpdateAffected are no-op (return directly without updating resource name nor references).
I think that implementing this into the VersionedResource type is technically possible but will push it to it's edge, I intend to refactor that properly.
Thank you so much for sharing all the insights @ChristianCiach.
Kapp would not only have to remove the hash-suffixes from the configMaps and secrets, but also from all resources that are referencing these configMaps and secrets
This is definitely a valid concern and I can't think of an easy way around it (maybe ytt would be helpful here).
@criztovyl Thank you again for giving this a shot. I will try to take a closer look at all the details again in the morning.
So the stripping of the suffix should not go into final cluster state (as it would do with my current code), it should only be used to identifying potentially related resources.
as an interested observer (of this PR), this comment clears up some of my misunderstanding.
This is what we are using right now, but this has to be configured for each and every configMap/secret generator separately and is easily forgotten and also adds a lot of "noise" to the kustomization.yaml files.
I have been thinking about this and I tried to use ytt overlays to add the versioned annotation and the disableNameSuffixHash option to all the secret generators and configmap generators. But I noticed that kustomize build command only accepts paths to directories and hence it's not possible to pipe the ytt output to kustomize. We would have to update the kustomization.yaml first and then use kustomize, so I guess it's not a viable option either.
So the stripping of the suffix should not go into final cluster state (as it would do with my current code), it should only be used to identifying potentially related resources.
I see. So you want this change to be purely cosmetic, just to show an update operation on deploy instead of a delete and create.
Yes, this sounds a lot less invasive, since there is no need to rename any references in this case.
Just one more thing to keep in mind: It is unlikely, but when stripping (for example) dashboard-config-7bmh4kf599 to dashboard-config, it could conflict with a configMap that is actually named dashboard-config.
I still think this is nice to have, but keeping in mind that this is a purely cosmetic change, I still think it may not be worth it, depending on code complexity and future maintenance overhead.
I still think it may not be worth it, depending on code complexity and future maintenance overhead.
Yes. I am doing this also for the fun of it -- should it get to complex I have no problem to accept the implementation does not fit :)
Hi o/
I have implemented that the diff ignores the nameHashSuffix. The new TestChangeSet_StripKustomizeSuffix unit test should demonstrate what I want(ed):
https://github.com/vmware-tanzu/carvel-kapp/blob/7973475f576cf399e7f7f4b67398360767b130b7/pkg/kapp/diff/change_set_with_versioned_rs_test.go#L187-L196
for me the following points are still open:
- [x] refactor versioned resource type I would like to split it into two types, one for name-matching related stuff and one for name-and-reference-updating stuff. cf https://github.com/vmware-tanzu/carvel-kapp/pull/517#issuecomment-1140425656)
- [x] customizable filter for which resources the suffix should not be trimmed or should be trimmed additonally I would like to implement this using resourceMatchers in (default) config.
- [x] add e2e test for my use-case
Locally all unit tests succeed, e2e also look good.
GitHub tells me the Action workflows need approval from a maintainer, maybe those could also be activated? Then I don't run into funny works-on-my-machine stuff :)
For the VersioneResource refactoring I am considering extracting that into a dedicated PR and (re)basing the diff changes on that later. What you you think?
customizable filter for which resources the suffix should not be trimmed or should be trimmed additonally I would like to implement this using resourceMatchers in (default) config.
Looking forward to seeing this :)
For the VersioneResource refactoring I am considering extracting that into a dedicated PR and (re)basing the diff changes on that later
Sure, we can have a separate PR. (The changes are still reviewable in the same PR so I am actually comfortable with both the options)
@criztovyl, VMware has approved your signed contributor license agreement.
finally took the time to start thinking about e2e test and directly uncovered a bug in the new code.
while the diff is correctly shown, the old CM is not deleted.
christoph@chschulz09:~/Documents/dev/carvel-kapp/test/e2e/res/kustomize/overlays/versioned2
[criztovyl/strip-name-hash-suffix %= 173c57b]$ ../../../../../../kapp deploy -a versioned -f kapp.yml -c
Target cluster 'https://127.0.0.1:37067' (nodes: minikube)
@@ create configmap/config-map-798k5k7g9f (v1) namespace: default @@
...
1, 1 data:
2 - foo: foo
2 + foo: bar
3, 3 kind: ConfigMap
4, 4 metadata:
...
7, 7 kapp.k14s.io/app: "1656313846367027698"
8 - kapp.k14s.io/association: v1.dacacaa0f2d3ae85e5dc5c636cc2c35e
8 + kapp.k14s.io/association: v1.ba177a572163dd4bcbee99aee2c7a5a6
9, 9 managedFields:
10, 10 - apiVersion: v1
Changes
Namespace Name Kind Age Op Op st. Wait to Rs Ri
default config-map-798k5k7g9f ConfigMap - create - reconcile - -
Op: 1 create, 0 delete, 0 update, 0 noop, 0 exists
Wait to: 1 reconcile, 0 delete, 0 noop
Continue? [yN]: y
9:12:19AM: ---- applying 1 changes [0/1 done] ----
9:12:20AM: create configmap/config-map-798k5k7g9f (v1) namespace: default
9:12:20AM: ---- waiting on 1 changes [0/1 done] ----
9:12:20AM: ok: reconcile configmap/config-map-798k5k7g9f (v1) namespace: default
9:12:20AM: ---- applying complete [1/1 done] ----
9:12:20AM: ---- waiting complete [1/1 done] ----
Succeeded
christoph@chschulz09:~/Documents/dev/carvel-kapp/test/e2e/res/kustomize
[criztovyl/strip-name-hash-suffix= c0fd89c]$ kubectl get cm
NAME DATA AGE
config-map-798k5k7g9f 1 7m39s
config-map-7tgk8db28b 1 9m3s
[...]
I'll have a look at that.
In the output above kapp tells 1 create, 0 delete [...] I'll start with checking how kapp marks objects as deleted. :)
while the diff is correctly shown, the old CM is not deleted.
Generally, for versioned resources, kapp keeps upto 5 versions (customisable using kapp.k14s.io/num-versions=int) and noop operation is used for the previous versions. I will have to take a look at the changes again to be sure, but I am guessing that something similar is happening here.
Also, I am wondering why the association label is getting changed.
P.S. There are some tests that are failing on the latest version of k8s (test-gh), so you might wanna rebase with develop once to get the latest changes which have a fix for them.
I am wondering why the association label is getting changed.
I checked this and found that the association label is created on the input name of the CM.
This means if you have an actual versioned resource, the name in input always is the same, the -ver-{n} is added later, with the association label already added.
But with the name-suffix-hash the name in input changes, thus also the association label does.
Generally, for versioned resources, kapp keeps upto 5 versions (customisable using
kapp.k14s.io/num-versions=int) andnoopoperation is used for the previous versions. I will have to take a look at the changes again to be sure, but I am guessing that something similar is happening here.
I have checked this with num-versions=1 and a third change -- then the first CM is deleted.
But I suspect this only works with 1 version to keep; with more the only information kapp has for determining the order of name-suffix-strip CMs is the age.
But with the name-suffix-hash the name in input changes, thus also the association label does.
I see. That makes sense.
But I suspect this only works with 1 version to keep; with more the only information kapp has for determining the order of name-suffix-strip CMs is the age.
That's true. With the current changes I think it would be randomly deleting any of the changes. But do you need to keep more than 1 versions for your use case?
But do you need to keep more than 1 versions for your use case?
Actually, no, I thought about dropping any other version for suffix-stripped resources :)
kapp now behaves like I want it; the diff is shown and the old CM is deleted. \o/
https://gist.github.com/criztovyl/85f594d455a2a413d6e20b0766303033
e2e seem to be running fine locally, there were some failures regarding wait rules, but recreating the kapp-test NS fixed those.
I have not addressed the changing association label yet, but I'll put that at the end of the backlog ^^
Next I'll check how to add some end-to-end tests, or maybe first some more unit tests. If I have that, I'll look into the VersionedResource refactoring.
Finally took the time to write the e2e test, based on them I can now confidently make your suggested changes and/or do the refactorings. :)
I have changed the e2e test as it failed on CI because the diff looks different. I dropped checking the diff now and only check there is one "update" diff and not two "delete and create" diffs.
I also made configurable which resources are covered by the name-hash-suffix-stripping, thus you can now exclude CMs or other resources which do not have a suffix (e.g. because it was disabled in kustomize or does not come from and configMapGenerator).
I plan next to restructure the commits so I have a base with failing tests on top of which I can maybe try different approaches to actually stripping the resources. I'll also look through the review comments, did not do that yet. :)
I plan next to restructure the commits so I have a base with failing tests on top of which I can maybe try different approaches to actually stripping the resources. I'll also look through the review comments, did not do that yet. :)
Awesome, do let me know when you would want me to take another look at the PR :)
I hope the tests are fixed now, I will focus on properly refactoring my changes now.
@praveenrewar Could you review how I have implemented the configuration for my feature?
The config is used in the following place, added with commit https://github.com/vmware-tanzu/carvel-kapp/pull/517/commits/9807418550ed0827f69118fb131da5d7fe8973cb:
https://github.com/vmware-tanzu/carvel-kapp/blob/9807418550ed0827f69118fb131da5d7fe8973cb/pkg/kapp/diff/versioned_resource.go#L27-L29