feat: allow cli to remove cluster by name (#8814)

[danielhelfand]

Closes #8814

This pull request adds the ability to remove cluster credentials by cluster name. Previously, you could only remove clusters using the cluster server.

Checklist:

• Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
• The title of the PR states what changed and the related issues number (used for the release note).
• I've included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
• I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
• Does this PR require documentation updates?
• I've updated documentation as required by this PR.
• Optional. My organization is added to USERS.md.
• I have signed off all my commits as required by DCO
• I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
• My build is green (troubleshooting builds).

[codecov]

Codecov Report

Merging #8823 (7112618) into master (c59b8ea) will increase coverage by 0.02%.
The diff coverage is 20.00%.

@@            Coverage Diff             @@
##           master    #8823      +/-   ##
==========================================
+ Coverage   43.30%   43.32%   +0.02%     
==========================================
  Files         186      186              
  Lines       23359    23371      +12     
==========================================
+ Hits        10116    10126      +10     
+ Misses      11797    11796       -1     
- Partials     1446     1449       +3     

Impacted Files	Coverage Δ
cmd/argocd/commands/cluster.go	5.66% <0.00%> (-0.03%)	⬇️
server/cluster/cluster.go	29.95% <26.66%> (+3.25%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c59b8ea...7112618. Read the comment docs.

[pasha-codefresh]

@danielhelfand i will recheck it during tomorrow, thank you

[alexmt]

Thank you for the improvement @danielhelfand ! Deletion by name is definitely useful.

I think there is an easier way to achieve it without making too many backend changes. We can change existing DeleteCluster API as following:

// Delete deletes a cluster by name
func (s *Server) Delete(ctx context.Context, q *cluster.ClusterQuery) (*cluster.ClusterResponse, error) {
	c, err := s.getClusterWith403IfNotExist(ctx, q)
	if err != nil {
		return nil, err
	}

	if q.Name != "" {
		servers, err := s.db.GetClusterServersByName(ctx, q.Name)
		if err != nil {
			return nil, err
		}
		for _, server := range servers {
			if err := s.enf.EnforceErr(ctx.Value("claims"), rbacpolicy.ResourceClusters, rbacpolicy.ActionDelete, createRBACObject(c.Project, c.Server)); err != nil {
				return nil, err
			}
			err = s.db.DeleteCluster(ctx, server)
			if err != nil {
				return nil, err
			}
		}
	} else {
		if err := s.enf.EnforceErr(ctx.Value("claims"), rbacpolicy.ResourceClusters, rbacpolicy.ActionDelete, createRBACObject(c.Project, c.Server)); err != nil {
			return nil, err
		}
		err = s.db.DeleteCluster(ctx, q.Server)
		if err != nil {
			return nil, err
		}
	}

	return &cluster.ClusterResponse{}, nil
}

WDYT?

[danielhelfand]

I think there is an easier way to achieve it without making too many backend changes. We can change existing DeleteCluster API as following:

@alexmt Thanks for taking a look. I agree that your suggestion of mapping the name to the server is cleaner. Only other change I made in addition is moving the tests to the cluster package. Let me know if there's anything else I can change.

[crenshaw-dev]

@danielhelfand in test/e2e/fixture/cluster/actions.go there's a test called TestClusterListDenied. Would you be willing to mimic that test to verify that delete permissions are being enforced for both cluster names and cluster URLs? I wish there were a place to do it in unit tests instead of e2e, but not seeing anything at a glance.

If you're busy, I wouldn't consider that a requirement for this PR. Just a nice-to-have. :-)

out of date

[crenshaw-dev]

Bleh I thought dismissing reviews would let me request a new review. @alexmt tagging works too I guess :-)

[crenshaw-dev]

Requesting changes to wait for Alex to have another look.

[danielhelfand]

@danielhelfand in test/e2e/fixture/cluster/actions.go there's a test called TestClusterListDenied. Would you be willing to mimic that test to verify that delete permissions are being enforced for both cluster names and cluster URLs? I wish there were a place to do it in unit tests instead of e2e, but not seeing anything at a glance.

If you're busy, I wouldn't consider that a requirement for this PR. Just a nice-to-have. :-)

This pr is by no means urgent, so happy to look into an additional test. Thanks for the suggestion @crenshaw-dev.

[alexmt]

LGTM. Thank you @danielhelfand!

Let me know please if you want to add test, suggested by @crenshaw-dev or PR can be merged now.

[danielhelfand]

@alexmt The test was added. Feel free to merge.

[alexmt]

Could not merge because PR @crenshaw-dev requested changes. @crenshaw-dev could you approve please ?

[pasha-codefresh]

Im sorry for last minute request, could we please move this part to function and reuse it,

if err := s.enf.EnforceErr(ctx.Value("claims"), rbacpolicy.ResourceClusters, 
              rbacpolicy.ActionDelete,createRBACObject(c.Project, c.Server)); err != nil {
     return nil, err
}
err = s.db.DeleteCluster(ctx, server)
if err != nil {
     return nil, err
}

it can be internal function I think, not sure that it will be reused outside

[danielhelfand]

Im sorry for last minute request, could we please move this part to function and reuse it,

@pasha-codefresh Done.

[danielhelfand]

Not sure why, but it looks like there were some generated changes added to the manifests. Not exactly sure why as they do not look relevant to my changes.

[pasha-codefresh]

You have force pushed changes from this commit bce80ac

you need to fix it :(

[danielhelfand]

The manifests on the master branch look like they were not generated so the build seems broken. I think I'll open a separate pr for that and try to rebase around it instead of adding here.

[alexmt]

@danielhelfand this is a broken process, not really related to your PR. Long term fix is here: #8864 .

[pasha-codefresh]

Thank you @danielhelfand, looks good for me

[danielhelfand]

Just rebased around fix on master. Feel free to merge once CI passes. Thanks for all the help everyone!