A little bit of coder inflation in this path 😂

TBH there's probably a lot of "pruning" we can do in those fixtures, but I'd like to avoid changing them if possible :)

I feel this seen map, as well as port info, is now a bit misplaced when we're looking at the HostPort and not the container port. This port is under container, but is a host port, so it may appear on multiple containers which would be confusing.

Since we're throwing away the information where this port maps to, it could be container X or Y depending on if it's listening on IPv4 or IPv6, for instance. Thus it would be listed on both container responses but likely only one of those will be reachable.

TL;DR, seen host ports are better placed outside the container structure, on the parent response.

❯ docker run -it --rm --name alpy1 -p 127.0.0.1:8181:8181 alpine
❯ docker run -it --rm --name alpy2 -p :::8181:8182 alpine
❯ docker inspect alpy1 alpy2 | jq '.[] | .Name, .NetworkSettings.Ports'
"/alpy1"
{
  "8181/tcp": [
    {
      "HostIp": "127.0.0.1",
      "HostPort": "8181"
    }
  ]
}
"/alpy2"
{
  "8182/tcp": [
    {
      "HostIp": "::",
      "HostPort": "8181"
    }
  ]
}

Addendum: As a further improvement, I would suggest that this data parsing should also decide (or output if predetermined) which container/port the host port maps to.

Right now having HostIp doesn't seem to be useful to the UI for the purposes of accessing applications listening inside the workspace.

Let's say you have two containers, both listening on the same port but on different interfaces:

docker run --name nginx-a -v /path/to/some/html/a:/usr/share/nginx/html:ro -d -p 127.0.0.1:8000:80 nginx
docker run --name nginx-b -v /path/to/some/html/b:/usr/share/nginx/html:ro -d -p [::1]:8000:80 nginx

When I hit the forwarded port in the Coder UI, it hits the container listening on IPv4.
Similarly, when I curl http://localhost:8000 it also hits the container listening on IPv4. I needed to explicitly specify curl -6 http://localhost:8000 to hit the container listening on the IPv6 interface.

If I stopped either the ipv4 or ipv6 container, it would fall back to whichever one was running.

Whether or not the v4 or v6 address is preferred seems to be down to the underlying OS configuration.

That makes sense, thanks for investigating. So ultimately, this confirms we can't with confidence say which container a HostPort belongs to.

I think the best option for us now is to detect that multiple containers have the same host port mapped, and if that's the case, we flag it via additional parameter. Perhaps (simplified) "conflicts": ["container2:9090/tcp"] or something like that? (Likewise container2 should have "conflicts": ["container1:8080/tcp"].)

This would allow us to surface a ? in the UI, saying "this could go to either container1 port 8080 or container2 port 9090.

Alternative suggestion: a message to this effect can be dropped into warnings as a relatively cheap and easy way to surface this situation. I don't think it's going to be that common.

OK, I've added a test case for this scenario and hopefully improved clarity overall.

Is ids here the sha or a human readable name? The latter may be easier on the eyes but both work as long as we surface the used value in the UI.

I used the ID here for specificity. FriendlyName might be a good call; I'll address that in a follow-up!

I'm finding this quite strange. Why not only return from the API null or undefined? Having the API returning both types look strange to me.

Yeah, this seems like it was meant to be double-bookkeeping, but with how the server types are set up, the null conditionals won't ever trigger. The omitEmpty JSON tag will make sure that if the values of host_ip or host_port are missing, they'll always evaluate to undefined

👍 Will remove the extraneous null checks.

@BrunoQuaresma Have we switched over to Shad for link variants like this?

It depends. I would not expect BE folks migrating the components to be honest, but in this case I think would make more sense to replace them by the new button component like this:

<Button asChild>
  <a href="..." onClick={...} />
</Button>

Oh wait, sorry – got mixed up by the diff. Didn't realize the link was already here

I'd suggest we make this a follow-up?

Yeah, that's totally fine with me

Two questions:

• Is there a reason why this code is using the MUI version of the tooltip, rather than the one we have in our components folder?
• Is the span here doing anything? I could see needing it to change some flexbox behavior, but I don't see anything obvious that would require it

Is there a reason why this code is using the MUI version of the tooltip, rather than the one we have in our components folder?

Copy-pasta, most likely!

Is the span here doing anything? I could see needing it to change some flexbox behavior, but I don't see anything obvious that would require it

The tooltip wouldn't display if the link was disabled; the <span> was a random StackOverflow find.

location.protocol should always be either https or http, right?

Correct.

Why are we making a non-nullable assertion for host_port, when just a few lines above, we're checking whether it's defined? I'd rather do one of two things:

• Update portForwardURL to accept undefined values
• Update the render logic to render something else out if the value is missing

Update the render logic to render something else out if the value is missing

Do you have any suggestions for an alternative "nowhere" URL? If there is no host port, we can't forward in the Coder UI. Maybe about:blank?

Defaulting href to an empty string if the link is disabled.

Is the value of hasHostBind likely to change over time? If I'm reading this right, it feels weird to me that we could be rendering out a link that is always disabled. At that point, why make it an interactive element in the first place?

It could change if the user bound the remote port to a local port and rebuilt the container. My reasoning for including the non-interactive variants was that if they simply didn't show up in the UI, it would be more confusing to users. Having it show up with a tooltip explaining why it was not enabled felt "right" to me.

Yeah, I think that's a solid solution. I guess I'm just usually a little more skittish about disabled UI elements for accessibility reasons. But if/when that becomes a priority, I think it should be easy to update the component

Just making sure: is it possible for there to be variants of the port value that have host_port but not host_ip, or vice-versa?

I don't think it likely in practice.