Remove the Application spec.destination.name field

[crenshaw-dev]

Summary

We should remove the name field from the Application CRD's spec.destination field.

- apiVersion: argoproj.io/v1alpha1
+ apiVersion: argoproj.io/v1 
  kind: Application
  spec:
    destination:
      server: https://kubernetes.default.svc
-     name: in-cluster

Motivation

Having both a name and a server field has these downsides:

• Confusing: looking at the spec, the user doesn't know that they're only allowed to specify one of the two fields; users are also confused why they can't use server names in RBAC
• Complex: handling both possibilities on the backend is complex causing bugs (see, all the issues with destination inference) and CVEs

Proposal

Remove the field and replace it with nice CLI and UI features to make it very easy for users to find the server URL for a given cluster name.

[sratslla]

I'd like to work on this. The CR changes seem straightforward. However, I’d love to get more details on the kind of UI enhancements you're looking for, as you mentioned in your message. Do you mean something like showing it with -o wide

[crenshaw-dev]

@sratslla CRD changes wouldn't be bad, but versioning the CRD (setting up conversion webhooks, writing docs, deprecation notices, etc.) would all be a lot of work. I'd like to start planning for all that though.

After talking with my team, I think there's a decent case to be made for deleting server instead of name. But I wanna write up the different options so the community and maintainers can evaluate them.

[sratslla]

Makes sense. I’d be interested in seeing the write-up once it’s done. In the meantime, I’ll do some research and put together some ideas as well. Then we can coordinate.

[rouke-broersma]

We heavily use templating based on a standardized name field with app-of-apps where the remote cluster credentials are provided by a third party in a predictable manner (crossplane, cluster-api etc). We manage close to a hundred clusters in this way. The server URI only exists in the cluster secret at this time.

[crenshaw-dev]

@rouke-broersma that's fair... we could go the other direction and remove server in favor of name. But then we drift away from the RBAC standard, which is the URL. Maybe that's fine?

[rouke-broersma]

I prefer 'symbolic' names over 'technical' references. A consumer shouldn't necessarily need to know where the remote cluster lives, only how they can reference the correct credential to get there.

Tbh if we're going to introduce a breaking change I would prefer to break away from string-typed matching with a secret that may or may not have been picked up and move to secretRef/parameterRef like a lot of the kubernetes ecosystem seems to be doing, but I can't judge how feasible that is for the argocd ecosystem.

[crenshaw-dev]

Yeah, I think a secretRef would be the way to go. It would be an even bigger breaking change, but I agree that it would really help bring us into line with the rest of the k8s ecosystem.

[blakepettersson]

I'd also argue the opposite - remove server and only use name. At Akuity we have as a general recommendation for our customers to use name where possible.

[rumstead]

I also selfishly would prefer name since it's human-readable.

[gdsoumya]

+1 to keeping name, if we have to remove something I think it's the server that adds less value to the end user. It's harder to recognise the target cluster with the server url if they had a bunch of them with similar url pattern, where as the name can be easily customised according to user needs.

[christianh814]

I am in favor of removing server and keeping name

If we were to remove one, it should definitely be server

[crenshaw-dev]

Sounds like a consensus!

What are everyone's feelings about having name (or some other field name) refer to the cluster Secret name instead of the Secret's data.name field?

Advantages:

1. Clear reference to a single resource, in line with k8s ecosystem
2. Opens door to potential Cluster CRD in the future

Disadvantages:

1. Bigger change
2. Name must abide by RFC 1123

[rumstead]

Sounds like a consensus!

What are everyone's feelings about having name (or some other field name) refer to the cluster Secret name instead of the Secret's data.name field?

Advantages:

1. Clear reference to a single resource, in line with k8s ecosystem
2. Opens door to potential Cluster CRD in the future

Disadvantages:

1. Bigger change
2. ?...

I wouldn't be opposed if the argo cd cli would create the secret with same name as the kube config / a cli argument. So a secret named cluster-k3d-dev-serverlb-422902893 could be dev (or a more descriptive name when not running on my local mac).

k get secret cluster-k3d-dev-serverlb-422902893 -o jsonpath='{.data.name}' | base64 -D
dev

EDIT: that would mean that cluster names have to abide by RFC 1123.

[blakepettersson]

The one disadvantage I can think of is that when a secret is created imperatively the cluster secret has an auto-generated name like repo-1234566, that would not be a great experience to have to insert in the app spec. I guess if the name is enforced to be unique and that the name has to be declared in the UI/CLI then 👍

[gdsoumya]

If the name of the secret and the cluster name are the same then it works fine but if they are different then it kind of introduces again 2 different identifiers that the users have to juggle, either we remove name as a separate id in the definition and just use the name of the secret as the cluster name or we don't care about the secret at all and just use the cluster name as we are doing rn.

[rouke-broersma]

Sounds like a consensus!

What are everyone's feelings about having name (or some other field name) refer to the cluster Secret name instead of the Secret's data.name field?

If the secret name is used as a reference directly I would preferably make it a more abstract reference so that multiple reference implementations may be added in the future:

destination:
  namespace: destinationTargetNamespace
  clusterRef:
    apiVersion: v1 # can be optional, default value, since it is most likely
    kind: Secret # can be optional, default value, since it is most likely
    name: secretName

destination:
  namespace: destinationTargetNamespace
  clusterRef:
    apiVersion: v1  # can be optional, default value, since it is most likely
    kind: ConfigMap
    name: cmName # for example if using federated login secret is not needed

destination:
  namespace: destinationTargetNamespace
  clusterRef:
    apiVersion: v1  # can be optional, default value, since it is most likely
    kind: Secret
    type: cluster.x-k8s.io/secret # Different providers may have different secret format, by supporting secret type specific implementations for specific providers could be added if needed. Contents should of course be normalized to argocd cluster secret by the specific cluster secret provider implementation so argocd core does not itself need knowledge of different formats. 
    name: secretName
    namespace: secretNamespace

destination:
  namespace: destinationTargetNamespace
  clusterRef:
    apiVersion: cluster.x-k8s.io/v1beta1
    kind: Cluster
    name: clusterResourceName
    namespace: clusterResourceNamespace

If the name of the secret and the cluster name are the same then it works fine but if they are different then it kind of introduces again 2 different identifiers that the users have to juggle, either we remove name as a separate id in the definition and just use the name of the secret as the cluster name or we don't care about the secret at all and just use the cluster name as we are doing rn.

I think you may still want a human readable cluster name field, simply because people may refer to a cluster with a certain name, and the secret can for technical reasons need to have a different name. However only one of the two should be accepted as a reference, the other should be for display purposes only. I also think the Argocd UI should show both the secret name and the cluster name in the clusters UI, and make it clear that one of the two is display only.

[crenshaw-dev]

Yep, I think I'd want to settle on the resource name as the single canonical name for programmatic reference. Then we could use a displayName field for UI purposes.

[crenshaw-dev]

@rouke-broersma rather than embedding arbitrary resource ref fields in the Application spec, I think I'd suggest building an ClusterSet controller to translate those arbitrary resources into Argo CD's cluster representation (whether a Secret or a CRD). It would better separate the platform concern (managing cluster resources) from the user concern (referencing some cluster destination).

[rouke-broersma]

That also seems fine to me, main point is that if some component can do the translation to something argocd understands it would be more extensible to different cluster management tooling.