Strip kustomize-style name hash suffix #517
Conversation
|
@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:
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:
First, I'll need to adjust the places where it's used for 1. On the other hand the suffix-stripping flag is required in all the places the versioned-resource is currently instantiated ad-hoc. I am considering to extract the ad-hoc instantiation into a method in the change-set type. |
|
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 |
|
Will look into some e2e tests now and await initial feedback. :) |
|
Hey! Thanks for the PR <3 |
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. Does the suffix get added to only the ConfigMap and Secrets created using the kustomize generators? |
|
Thanks for the feedback :) |
|
I was one of the first persons to upvote the issue #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.
Yes. You've already found
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 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 |
|
Oh, true, I did not think about that consequence yet, thank you! Actually my concern is just cosmetic, I dont necessarily want true versioning. For that I can ignore the referneces, they will show as updated anyway. 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 I think that implementing this into the |
|
Thank you so much for sharing all the insights @ChristianCiach.
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. |
as an interested observer (of this PR), this comment clears up some of my misunderstanding. |
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 |
I see. So you want this change to be purely cosmetic, just to show an 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) 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. |
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 for me the following points are still open:
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? 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? |
Looking forward to seeing this :)
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. I'll have a look at that. |
Generally, for versioned resources, kapp keeps upto 5 versions (customisable using 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 ( |
I checked this and found that the association label is created on the input name of the CM.
I have checked this with |
to allow implementing alternative versioned resources
dropping previous CMs not yet implemented
|
Ah, forgot to look into the |
criztovyl
requested review from
praveenrewar
and removed request for
cppforlife and
100mik
|
Hey @criztovyl ! Sorry I didn't get a chance to look at the changes properly, will get it to it soon :) |
|
ok, no worries :) |
the operation is "add" because we're only manipulating the "add" diff to show the difference to the previous version
preparation for dropping the enabled flag altogether; I have tried both at once before and the matchers completely broke, so doing the complex part (no matchers matching nothing) first with only *toggling* the default to enabled instead of directly throwing it out.
it seems the field in config does not need to be an object anymore, could as well be a (by default nil) list of matchers, but let's not touch too much at once.
I have now implemented it that way. Completely neglected docs until now, will look into that next. :) |
|
While looking into the docs I realized the name of this feature, which was not very good in the beginning, after the iterations this PR went through, went bad... The central part of this PR now is the alternative implementation of As such, what do you think about renaming everything to that? Maybe you have a better name in mind?
|
Renaming definitely makes sense to me, but I will have to take a closer look at the changes (planning to do that tomorrow) to be able to comment on all the names. |
pkg/kapp/diff/stripnamehashsuffix.go
Outdated
| // N.B. if no config specifies any matchers (the default), then | ||
| // allMatchers will stay uninitialized/nil and the AndMatcher will never | ||
| // match, effectively disabling suffix strip. | ||
| result.ResourceMatcher = ctlres.AndMatcher{Matchers: allMatchers} |
There was a problem hiding this comment.
I am still trying to understand why this is required. Would you be able to share a couple of scenarios that you are trying to cover, maybe we can find an alternative to it.
There was a problem hiding this comment.
I was trying to allow include / exclude semantics.
Rethinking that it probably is better to have explicit fields for include and exclude matchers:
stripNameHashSuffix: # hashSuffixDiffConfig
include:
- apiVersionKindMatcher: {apiVersion: v1, kind: ConfigMap}
- apiVersionKindMatcher: {apiVersion: v1, kind: Secret}
exclude:
- kindNamespaceNameMatcher: {kind: ConfigMap, name: foo},for the sake of discussion (RE) I intentionally chose to not make the config a slice here:
in the end the include/exclude lists will each be merged into a single include/exclude list; there I assume that a list of include+exclude objects does not really add anything besides a layer of merging.
It might make sense in context of consistency, though, as many of the other config fields (template rules, etc) are also lists.
There was a problem hiding this comment.
We already have a notMatcher present. Do you think that will be useful instead of exclude?
So, this how the matchers currently work.
By default, any matcher is used. So you can just have something like this to match ConfigMaps and Secrets.
stripNameHashSuffix:
- resourceMatchers:
- apiVersionKindMatcher: {apiVersion: v1, kind: ConfigMap}
- apiVersionKindMatcher: {apiVersion: v1, kind: Secret}If you want to exclude something, then you can use an andMatcher (with anyMatcher and notMatcher nested inside), something like this. So I think we can rely on the existing capabilities matchers.
There was a problem hiding this comment.
The challenge then is aggregating such nested "and: (any , not)" matchers across configurations.
There was a problem hiding this comment.
across configurations
Ideally we would want to have one Config only (although we could add as many as we want). Do you have any specific scenario where we would need multiple Configs?
There was a problem hiding this comment.
I thought this over, no use-case springs to mind; Ii wanted to include it for completeness and the include/exclude was the latest pragamtic way I found to implement it.
I would then explicitly implement it so that only the suffix-config from last kapp-config is used :)
|
#571 introduced a conflict on To resolve that conflict I think I need to implement the suggestion from #571 (comment), extracting I am thinking about something like below, what do you think? // cmd/app/deploy.go
// func (o *DeployOptions) calculateAndPresentChanges(...)
//...
diffResources := ctldiff.NewDiffResourceFactory(existingResource, newResources,
conf.TemplateRules(), conf.StripNameHashSuffixConfigs()).NewDiffResources()
err := ctldiff.NewRenewableResource(diffResources).Prepare()
// ...
changes, err := ctldiff.NewChangeSetWithVersionedRs(diffResources,
opts ChangeSetOps, changeFactory ChangeFactory).Calculate()
// ...with: package diff
type DiffResources struct { // or any other name, only was the first one that came to my mind ^^
ExisitingResources, NewResources versionedResources
ExistingResourcesGrouped map[string][]VersionedResource
// NB: While RenewableResources wants a map[string]ctlres.Resource
// it should also be able to use [-1]VersionedResource.Res().
}
type DiffResourcesFactory struct { // or VersionedResourcesFactory?
// attributes extracted from ChangeSetWithVersionedRs, only used by the extracted methods (only).
existingRs, newRs []ctlres.Resource
rules []ctlconf.TemplateRules
stripNameHashSuffixConfig stripNameHashSuffixConfig
// extracted attributes end
}
func NewDiffResourcesFactory(exisitingRs, newRs []ctlres.Resource,
rules []ctlconf.TemplateRule,
stripNameHashSuffixConfigs []ctlconf.StripNameHashSuffixConfig) DiffResourcesFactory {
return DiffResourcesFactory{existingRs, newRs,
rules, stripNameHashSuffixConfigFromConf(stripNameHashSuffixConfigs)}
}
func (d DiffResourcesFactory) NewDiffResources() DiffResources {
result := DiffResources{}
result.ExistingResources = d.existingVersionedRs()
result.ExistingResourcesGrouped = d.existingVersionedRsGrouped()
result.NewResources = d.newVersionedRs()
return result
}
// methods extracted from ChangeSetWithVersionedRs
func (d DiffResourcesFactory) existingVersionedRs() versionedResources {
// existingVersionedResources
}
func (d DiffResourcesFactory) existingVersionedRsGrouped() map[string][]VersionedResource {
// groupResources / newGroupedVersionedResources
}
func (d DiffResourcesFactory) newVersionedRs() versionedResources {
// newVersionedResources
} |
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?
Additional Notes for your reviewer:
Review Checklist:
a link to that PR (will do that after first feedback here)
change
Additional documentation e.g., Proposal, usage docs, etc.: