Change behaviour gateway addressess override in Link command

[mateiidavid]

When linking two clusters, a --gateway-addresses override can be passed to the command. If applied, the override populates the Link resource with a set of addresses received from the flag, instead of reading them from Linkerd's Gateway LoadBalancer service. Currently, the override is used only when the LoadBalancer service does not exist (or does not contain any IPs).

An override should take precedence over default behaviour. As it stands, the user experience is confusing; it can also make manual testing for changes hard (when an override has to be passed) and it can inhibit users from passing in a static IP or DNS addresses to avoid relying on dynamic IPs in the LoadBalancer service.

This change makes the override behave as expected.

Manual QA

Before:

# `linkerd mc link --gateway-addresses="headless-test.default.svc.cluster.loca
l" `
apiVersion: multicluster.linkerd.io/v1alpha1
kind: Link
metadata:
  name: target
  namespace: linkerd-multicluster
spec:
  gatewayAddress: 192.168.224.4

After:

# k get link -n linkerd-multicluster target -o json | jq .spec.gatewayAddress
"headless-test.default.svc.cluster.local"

[alpeb]

How about making this extra-clearer by extracting this logic into a helper function func getGatewayAddresses(gateway *corev1.Service, opts *linkOptions) (string, error) which would immediately return opts.gatewayAddresses if not empty, followed by the other conditions that no longer have to take into account that option?

[mateiidavid]

@alpeb sure! Think there's potential for a clean up here and the suggestion sounds good 👍🏻

[adleong]

an alternative way to structure this for clarity:

if opts.gatewayAddresses != "" {
  gatewayAddresses = opts.gatewayAddresses
} else if len(gwAddresses) > 0 {
  gatewayAddresses = strings.Join(gwAddresses, ",")
} else {
  return fmt.Errorf(...)
}

This condenses the error handling and gateway population into the same if-else block and makes the prescience clearer: opt then LB then error.

[mateiidavid]

Thanks both. Tried a few things, including extracting some of the logic into a separate function. In the end, took Alex's suggestion since it's a relatively simple conditional. Took it a bit further and eliminated gatewayAddresses as a temp variable, we ultimately move that into the link anyway.

# IP of the gateway service
:; k get service -n linkerd-multicluster --context=k3d-target linkerd-gateway -o json | jq .status.loadBalancer.ingress[0].ip
"192.168.224.4"

# Override doesn't work
:; linkerd --context=k3d-target mc link --cluster-name target --api-server-address="https://192.168.224.4:6443" --gateway-addresses="head
less-test.default.svc.cluster.local" | rg 'gatewayAddress'
  gatewayAddress: 192.168.224.4

# Now it does
:; bin/linkerd --context=k3d-target mc link --cluster-name target --api-server-address="https://192.168.224.4:6443" --gateway-addresses="
headless-test.default.svc.cluster.local" | rg 'gatewayAddress'
  gatewayAddress: headless-test.default.svc.cluster.local

# No external IP on gateway service or override => error
:; bin/linkerd --context=k3d-target mc link --cluster-name target --api-server-address="https://192.168.224.4:6443"
Error: Gateway linkerd-gateway.linkerd-multicluster has no ingress addresses