service-mirror: support gateway resolving to multiple addresses

[MrFreezeex]

If the gateway address was a name resolving to multiple addresses, previously service-mirror took only one of those address. This behavior could led to downtime in case the gateway behind the IP picked by service-mirror is down. This commit fix this behavior by considering all the IPs resolved.

[mateiidavid]

Thanks for the PR @MrFreezeex, at a glance this looks good. An issue would have been nice for us to have a couple of examples and to understand the problem better. I suppose the issue is that an endpoint might go down and you get downtime while the service mirror is resolving the address again?

As far as the code is concerned, it looks good (modulo the comment I left on the sort).

[MrFreezeex]

Thanks for the PR @MrFreezeex, at a glance this looks good. An issue would have been nice for us to have a couple of examples and to understand the problem better. I suppose the issue is that an endpoint might go down and you get downtime while the service mirror is resolving the address again?

Thanks for the review!

Sorry for not creating the issue prior to this, so we did have a small downtime probably because of that but I am not 100% sure of it. The gist of it is that if you give an address to the gateway that resolve to multiple IPs without this code change it mirrored only one IP. In our case we have an AWS load balancer that resolves to 3 IPs for instance and dumping the mirrored kubernetes endpoints only show one of those IP.

[mateiidavid]

@MrFreezeex the change looks great! I tested it out manually by creating a headless service with a manual EndpointSlice to create DNS records (much easier than hacking CoreDNS).

:; k exec dnsutil-pod -- nslookup headless-test.default.svc.cluster.local
Server:         10.43.0.10
Address:        10.43.0.10#53

Name:   headless-test.default.svc.cluster.local
Address: 192.168.224.4
Name:   headless-test.default.svc.cluster.local
Address: 192.168.224.5

If we pass --gateway-addresses=headless-test.default.svc.cluster.local to the Link command we can see the IPs being resolved in the probe service's endpoints

[
  {
    "addresses": [
      "192.168.224.4"
    ],
    "conditions": {
      "ready": true
    }
  },
  {
    "addresses": [
      "192.168.224.5"
    ],
    "conditions": {
      "ready": true
    }
  }
]

I considered asking for an integration test. It'd be nice to have but also painful to wire up without over abstracting this particular piece of code to take in a DNS resolver. All in all, I'm confident with the manual test. If you feel like contributing and know how to solve this easily in an integration test we'd appreciate a follow-up! :)

Thanks again 🚢

[MrFreezeex]

Thanks @mateiidavid 🙏. I was wondering do you think this patch could be considered for backport to the 2.14 branch? Happy to submit a PR with the cherry-pick if yes! :D

[mateiidavid]

@MrFreezeex we generally consider only bug fixes for backports. New features are expected to release in new stable versions. We could probably argue that this is a bug fix though :).

With that being said, we should let the change simmer for a bit in an edge release (or two). We almost exclusively release all changes to an edge version first to ensure they don't break anyone's production environment. If an edge version considered as a release candidate doesn't raise any major flags, we'll cherrypick fixes into point releases.

I know you're probably eager to see this shipped. When it makes it into an edge (should happen this week) you could only upgrade the multicluster extension and leave the rest of the control plane on stable. We'll consider backporting this into a stable point release; if you can only stay on stable then it might take a bit more time until you can rollout the new change. The sooner we get feedback around changes from our user base the quicker fixes can be released usually.

I hope this all makes sense, and thank you for your patience and for your contribution! I really appreciate all of the effort you've put in. Please feel free to reply here, or in a discussion, or anywhere else if you want to continue the conversation.

[MrFreezeex]

@MrFreezeex we generally consider only bug fixes for backports. New features are expected to release in new stable versions. We could probably argue that this is a bug fix though :).

With that being said, we should let the change simmer for a bit in an edge release (or two). We almost exclusively release all changes to an edge version first to ensure they don't break anyone's production environment. If an edge version considered as a release candidate doesn't raise any major flags, we'll cherrypick fixes into point releases.

I hope this all makes sense, and thank you for your patience and for your contribution! I really appreciate all of the effort you've put in. Please feel free to reply here, or in a discussion, or anywhere else if you want to continue the conversation.

No worries and thanks for the kind words. Please keep it on your radar for 2.14.4 if there was enough edge releases in the meantime 🙏.

I know you're probably eager to see this shipped. When it makes it into an edge (should happen this week) you could only upgrade the multicluster extension and leave the rest of the control plane on stable. We'll consider backporting this into a stable point release; if you can only stay on stable then it might take a bit more time until you can rollout the new change. The sooner we get feedback around changes from our user base the quicker fixes can be released usually.

Oh indeed thanks that's a great idea, I will do that!

[MrFreezeex]

Hi @mateiidavid, I saw that there was two new release of linkerd since then and it's been a couple of weeks that the edge release is out. Would it be possible to include this patch in the next one 🙏 ?

[mateiidavid]

Hi @mateiidavid, I saw that there was two new release of linkerd since then and it's been a couple of weeks that the edge release is out. Would it be possible to include this patch in the next one 🙏 ?

Hey @MrFreezeex! Wanted to let you know that I did see your message even though I didn't reply immediately. I raised it with the team :) I think including it makes sense.