proposal: Change application identifier to allow app names longer than 63 chars #6425
Conversation
Signed-off-by: jannfis <jann@mistrust.net>
jannfis
requested review from
alexmt,
jessesuen,
jgwest,
kshamajain99,
mayzhang2000 and
sbose78
jannfis
changed the title
Codecov Report
@@ Coverage Diff @@
## master #6425 +/- ##
==========================================
+ Coverage 41.01% 41.03% +0.02%
==========================================
Files 152 158 +6
Lines 20088 21299 +1211
==========================================
+ Hits 8239 8740 +501
- Misses 10716 11306 +590
- Partials 1133 1253 +120
Continue to review full report at Codecov.
|
|
|
||
| On a rollback to a previous Argo CD version, this change would be reverted | ||
| and the resource would look like the first shown example above. | ||
|
|
There was a problem hiding this comment.
How would Argo CD know whether a resource's app.kubernetes.io/instance was applied using the old system or the new system (the one described in this proposal)?
For example:
- A cluster is running with the old version of Argo CD
- All the application resources are labelled using the old system
- Argo CD is upgraded
- The new Argo CD looks at all the existing
app.kubernetes.io/instancefields of the existing resources, discovers that they don't match any expected hash (and thus don't belong to any Application), and ignores them (recreating new Application resources).
I think we need the ability to look at the app.kubernetes.io/instance field of a resource and know which system it was created with.
One option we could use is look at the app.kubernetes.io/instance value and determine if it is a 32 character hexadecimal value, but this is not foolproof (users may be using Applications with names that are 32-byte hexadecimal values).
There was a problem hiding this comment.
That's a good point you raise there.
I haven't yet verified, but I think it would be similar to the behavior if you rename an application, e.g. by deleting an Application and re-creating it with a different name, but same parameters (source + destination).
Argo CD would just go and overwrite what's in app.kubernetes.io/instance - at least as long as another application with the previous name does not exist, in which case Argo CD would issue a Shared Resource Warning and prevent the overwrite.
There was a problem hiding this comment.
I think if an Argo CD instance encounters a resource with a app.kubernetes.io/instance value that it doesn't recognize (eg it does not correspond to a hash of a known application name+install id value), then Argo CD must assume that that resource is owned by another Argo CD instance (the goal of 'allow multiple Argo CD instances to manage resources on the same cluster').
So, presuming that Argo CD encounters a resource it thinks is owned by someone else, and it has the same name/namespace as a resouce managed by an Application managed by the current Argo CD instance, do we:
- Take ownership of it with the current Argo CD instance
- Leave it alone
Probably 2), and report an error ('error: Argo CD Application would overwrite a resource that may be owned by another Argo CD instance').
It might be worthwhile to document the algorithm that Argo CD will use to move existing resources from the old style to the new style, in the upgrade scenario.
I'm guessing it would be:
- If we encounter a resource that contains a
app.kubernetes.io/instancethat is not a 32/64 character hexadecimal value, AND it matches the name field of an existing Argo CD Application, then convert it to the new system (ensure theapp.kubernetes.io/instancefield is updated)- As you said, this might already be handled by the rename-style behaviour.
- Another option would be to delete the resource, and hope that Argo CD recreates it.
- If we encounter a resource that contains a
app.kubernetes.io/instancethat is a 32/64 character hexadecimal value, leave it alone. (Assume it's a valid instance id from another Argo CD instance).- Unless it has the same Kubernetes resource name as a resource that this Argo CD instance is trying to create, in which case report the above error ('error: would overwrite a resource that may be owned...')
- Else, either delete or leave it alone (orphan it).
(in very rare cases this might orphan or delete old resources, but it's probably fine.)
| hash algorithms on Stack Exchange that suggests that `FNV-1a` might be more | ||
| than sufficient for our use-cases: | ||
| https://softwareengineering.stackexchange.com/a/145633 | ||
|
|
There was a problem hiding this comment.
Re: hash choice -
If we're going to go with SHA-based algorithm, I recommend SHA-256, as SHA-1 is considered broken. It doesn't matter for this use case (we're not using the cryptographic properties of a hash, as you note later), but we will constantly get people telling us it's broken and asking us when we will move to another algorithm 😄 .
There was a problem hiding this comment.
Also, correct me if I'm wrong, but I don't think hash performance is an issue: for each Application, we only need to calculate this value once, and then store it in memory (eg memoize the result of the hash function for each application).
There was a problem hiding this comment.
You have a good argument there. I favored the SHA-1 variant for producing the shorter hash values, but I think you are right in that we may want to consider usage of SHA-256 instead. Although we do not need to be secure in cryptographic terms here, probably SHA-1 will become utterly deprecated soon enough (maybe up to the point of being removed from any standard libraries?), and even Git will move away from it. So yeah, it leaves a "bad aftertaste" kind of thing :)
Thanks for bringing it up, I will change it in the proposal accordingly.
There was a problem hiding this comment.
Looks like we've got two similar discussion threads in this PR. Copying comments from another thread here.
As I understand there are trying to solve three issues:
- allow app with long names
- support two argocd managing apps in the same cluster
- avoid argocd confusion that happened when something else adds
app.kubernetes.io/instancelabel
Also, I think additional requirements are: try to keep it human-friendly and try avoid breaking changes.
To solve I want to propose:
- use annotations instead of labels for tracking to solve long names issue
- use three annotations (e.g.
argo-cd.argoproj.io/application,argo-cd.argoproj.io/resourceKey, ``argo-cd.argoproj.io/installation` ) that includes app name, resource key and argocd installation id - allow the user to switch to annotation-based tracking when necessary using new
app.spec.trackingfield. (this comment tries to justify why new way of tracking should be optional)
So main changes in my proposal:
- use annotations + human-readable strings instead of hash + labels.
- new way of tracking should be explicitly enabled per app
There was a problem hiding this comment.
I think we should not provide two different methods for tracking applications, as it will complicate things greatly and probably will cause some confusion among users.
What speaks against using the label for tracking and the annotations for human-readable identifiers together? So the annotations will only serve informational purposes, and the label is used to select and identify resources.
To solve
avoid argocd confusion that happened when something else adds app.kubernetes.io/instance label
I would rather propose to change the default app.kubernetes.io/instance label to something more sensible, Argo CD specific. E.g. argocd.argoproj.io/instance - I think the Argo CD Helm chart already sets this by default.
IMHO, I think we shouldn't sacrifice the label in order to enable longer-than-63-chars application names.
There was a problem hiding this comment.
I'm trying to avoid painful migration for end-users: if Argo CD changes the tracking method then all managed resources suddenly became out-of-sync.
However, I agree that two ways if tracking is not great. In order to make it easier to migrate, we should support using "legacy" way of tracking for 1~2 releases.
Signed-off-by: jannfis <jann@mistrust.net>
| of the Argo CD installation into the final hash value, e.g. | ||
|
|
||
| ``` | ||
| value = hashFunc(installationIdentifier + "." + applicationName) |
There was a problem hiding this comment.
My suggestion would be that hashValue can be applicationName+"."+resourceKey. So that we know what the name of original resourceName in case when some kubernetes operator clone the resource.
By that we can also aim to solve another usecase of ignoreExtraneous resources outside target namespace
There was a problem hiding this comment.
I really like the idea to bring in resourceKey somewhere. But I think if we put it into the application instance label, we will end up with a different value on each resource. This makes it utterly hard to use the label as a selector, e.g. to select all resources belonging to a given application by running kubectl get somekind -l app.kubernetes.io/instance=<somevalue>
Maybe it would make more sense to add it as an additional annotation on the resource? E.g. argo-cd.argoproj.io/resourceKey instead of trying to condense it.
But at the end, this will be a design decision: Do we want to support above use-case, e.g. using the label as a selector for resources managed by a given application, or not. I clearly see the benefits of using the label as a selector, but I don't have any strong opinions on it. We should just be aware of the consequences.
There was a problem hiding this comment.
I agree that it is very convenient to be compatible with kubectl get -l app.kubernetes.io/instance=<somevalue> . This works most of the time except few edge cases:
- app name is too long
- something else add
app.kubernetes.io/instancelabel and confuses argo cd. This is the most annoying issue and we don't have a very good resolution.
So I think it would be great if keep using app.kubernetes.io/instance label by default and allow to switch to new way when necessary.
@jessesuen had a good point about trying to keep app mark human readable. How about we add new app-level field resourceTracking that might be label or annotation. If user select annotation that Argo CD should apply app.kubernetes.io/instance annotation where annotation value is <appName>-<resouceName> ?
This would allow us to upgrade smoothly and takes care about both edge cases. WDYT?
There was a problem hiding this comment.
The argo-cd.argoproj.io/resourceKey annotation instead of embedding resource key into app.kubernetes.io/instance works too
Signed-off-by: Alexander Matyushentsev <amatyushentsev@gmail.com>
|
We brainstormed about this change with @jannfis and @jessesuen offline. Created the PR to @jannfis 's fork to capture the proposed changes: jannfis#17 |
docs: update proposal to use annotations with human friendly values
…n 63 chars (argoproj#6425) proposal: Change application identifier to allow app names longer than 63 chars (argoproj#6425) Signed-off-by: jannfis <jann@mistrust.net> Signed-off-by: viktorplakida <plakyda1@gmail.com>
This is a proposal to change the way application resources are identified within Argo CD to allow application names longer than 63 characters.
Signed-off-by: jannfis jann@mistrust.net
Note on DCO:
If the DCO action in the integration test fails, one or more of your commits are not signed off. Please click on the Details link next to the DCO action for instructions on how to resolve this.
Checklist: