Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Concurrency Issue with Slugify Function #18369

Open
3 tasks done
mhotan opened this issue May 22, 2024 · 2 comments
Open
3 tasks done

Concurrency Issue with Slugify Function #18369

mhotan opened this issue May 22, 2024 · 2 comments
Labels
bug Something isn't working component:application-sets Bulk application management related version:2.11 Latest confirmed affected version is 2.11

Comments

@mhotan
Copy link

mhotan commented May 22, 2024

Checklist:

  • I've searched in the docs and FAQ for my answer: https://bit.ly/argocd-faq.
  • I've included steps to reproduce the bug.
  • I've pasted the output of argocd version.

Describe the bug

The Slugify function is currently not safe when accessed concurrently from multiple goroutines. Uncontrolled access to shared global state can cause data races where Slugify returns inconsistent results with the same input. This is a fairly serious problem for my organization as we utilize ApplicationSets (app of apps pattern) and intentionally don't self-heal. This leads to application sets utilizing slugify to render different application names when there is enough activity where different goroutines utilize SlugifyName. Subsequently, applications are destroyed, recreated, and left in an OutOfSync state.

In our situation, we enabled preserveResourcesOnDeletion for AppSets that use slugify. This prevents deleting underlying K8s resources, but the application and app set recreation are not ideal, as we have an alert condition when they are out of sync.

To Reproduce

It is difficult to reproduce organically. One can create an appset + generator that utilizes Slugify with different max lengths and monitors Kubernetes audit log deletion and recreation.

Running the test in this PR: #18370 without including the fix would demonstrate the problem.

Expected behavior

SlugifyName with the same input parameters should be idempotent regardless of concurrent usage.

Screenshots

Slugify_Concurrency_issue

Version

❯ argocd version
argocd: v2.11.0+d3f33c0
  BuildDate: 2024-05-07T18:31:19Z
  GitCommit: d3f33c00197e7f1d16f2a73ce1aeced464b07175
  GitTreeState: clean
  GoVersion: go1.22.2
  Compiler: gc
  Platform: darwin/arm64
argocd-server: v2.11.0+d3f33c0
  BuildDate: 2024-05-07T16:01:41Z
  GitCommit: d3f33c00197e7f1d16f2a73ce1aeced464b07175
  GitTreeState: clean
  GoVersion: go1.21.9
  Compiler: gc
  Platform: linux/amd64
  Kustomize Version: v5.2.1 2023-10-19T20:13:51Z
  Helm Version: v3.14.3+gf03cc04
  Kubectl Version: v0.26.11
  Jsonnet Version: v0.20.0

Logs

@mhotan mhotan added the bug Something isn't working label May 22, 2024
@mhotan
Copy link
Author

mhotan commented May 22, 2024

There is a four year old open PR: gosimple/slug#51 to make the underlying Go library that would be applicable.

@agaudreault
Copy link
Member

@mhotan Can you add an example of applicationSet using the slugify function that is causing the issue. I think that would greatly help to reproduce.

@andrii-korotkov-verkada andrii-korotkov-verkada added the version:2.11 Latest confirmed affected version is 2.11 label Nov 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working component:application-sets Bulk application management related version:2.11 Latest confirmed affected version is 2.11
Projects
None yet
Development

No branches or pull requests

3 participants