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

ArgoCD clusters should list to find the cluster instead of get and check for NotFound #266

Closed
w4rgrum opened this issue Apr 26, 2023 · 5 comments · Fixed by #399
Closed
Labels
enhancement New feature or request help wanted Community help wanted! upstream-dependency Issue depends on changes being made to upstream dependencies (e.g. `argoproj/argo-cd`)

Comments

@w4rgrum
Copy link
Contributor

w4rgrum commented Apr 26, 2023

Terraform Version, ArgoCD Provider Version and ArgoCD Version

Terraform version: 1.2.8
ArgoCD provider version: 5.2.0
ArgoCD version: 2.5.6

Affected Resource(s)

  • argocd_cluster

Question

Got this issue when trying to update a cluster with TF that had been previously manually deleted on ArgoCD server side:

Error: could not get cluster information: rpc error: code = PermissionDenied desc = permission denied

After some investigations on this issue I concluded this:

To retrieve a cluster from an ArgoCD server there are basically 2 ways:

  1. directly get the cluster
  2. list the clusters and look for your cluster in the returned list

However, since the implementation of argo-cd PR-7039 the 1) is not reliable as if the cluster is not found it will return a "fake" 403 instead of 404 for security reasons as the feature enabled cluster-level RBAC (see argo-cd discussion 10830)

Currently the provider uses the list api at create time: https://github.com/oboukili/terraform-provider-argocd/blob/master/argocd/resource_argocd_cluster.go#L94-L129

However for all other cases it uses a direct get and checks for NotFound (https://github.com/oboukili/terraform-provider-argocd/blob/master/argocd/resource_argocd_cluster.go#L167-L175) in order to ignore the error in that case, but this will never be returned because of the above-mentioned change.

My question is: shouldn't the provider always use the list api to retrieve a cluster to be able to properly ignore a cluster not found case? (and thus to be more robust to unexpected changes that happened outside of TF)

@w4rgrum w4rgrum added the question Further information is requested label Apr 26, 2023
@onematchfox
Copy link
Collaborator

Hi @w4rgrum,

Nicely done on all the digging. Yes, this is, unfortunately, not an ideal situation. We faced a similar issue recently (#247) when the response on the application get endpoint was changed to PermissionDenied for security reasons. In that case, we could change the application resource to use the List endpoint instead of Get without too much concern since the List endpoint implements filtering.

In the case of clusters, unfortunately, the ArgoCD API does not provide any form of filtering on the List endpoint (see comment in this repo and implementation in argo-cd). This means that there are performance concerns in changing the cluster resource in this provider to use the List endpoint for all reads - particularly since read is guaranteed to be called for every cluster resource under management. So, whilst I am open to the idea, I would suggest first trying to ensure that filtering is implemented on the List endpoint in ArgoCD.

@onematchfox
Copy link
Collaborator

Opened argoproj/argo-cd#13363 to implement filtering on the cluster List endpoint in ArgoCD.

@onematchfox onematchfox added enhancement New feature or request upstream-dependency Issue depends on changes being made to upstream dependencies (e.g. `argoproj/argo-cd`) and removed question Further information is requested labels May 16, 2023
@w4rgrum
Copy link
Contributor Author

w4rgrum commented Sep 28, 2023

@onematchfox I see that the fix was implemented in argo-cd v2.8.0, any news on the TF provider fixes to be able to use the List endpoint?

@onematchfox
Copy link
Collaborator

@w4rgrum yeah, I saw that too. Haven't had time to get around to the provider changes unfortunately. Need to give some thought to how best to implement as we still need to support 2.6 and 2.7 as part of the compatibility promise.

Copy link

github-actions bot commented Oct 4, 2024

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Community help wanted! upstream-dependency Issue depends on changes being made to upstream dependencies (e.g. `argoproj/argo-cd`)
Projects
None yet
3 participants