Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add read-only UI for internet gateways #2488

Merged
merged 80 commits into from
Dec 13, 2024
Merged

Add read-only UI for internet gateways #2488

merged 80 commits into from
Dec 13, 2024

Conversation

charliepark
Copy link
Contributor

@charliepark charliepark commented Oct 7, 2024

This PR creates a new tab on the VPC view page, showing Internet Gateways that are a part of that VPC. It lists out the gateways, along with their attached IP Pools, IP Addresses, and any VPC routes that are targeting that particular internet gateway.
Screenshot 2024-12-10 at 3 21 30 PM

A sidemodal gives slightly more details, including the "Internet Gateway IP Address" and "Internet Gateway IP Pool", which is really more like a join table record between the Internet Gateway and an IP Pool. Current limitations stipulate that an Internet Gateway can have up to one attached IP Pool. The sidemodal also includes a table that shows the routes (and their routers) that include this gateway as a target value.
Screenshot 2024-12-10 at 4 41 58 PM

Finally, this PR also updates the router route form, so that a user can set up an internet gateway as the target for a VPC route.
Screenshot 2024-12-10 at 3 22 29 PM

Closes #2402

Copy link

vercel bot commented Oct 7, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
console ✅ Ready (Inspect) Visit Preview Dec 13, 2024 5:16pm

@benjaminleonard
Copy link
Contributor

In general I think we're better off holding for the next release to get the implementation right for churn reasons.

Let's catch up on this today. I might be a helpful eye for this, since I haven't read up on internet gateways yet, so I can play the idiot seeing this for the first time – a role a know and love.

I notice we're also missing breadcrumbs – is that #1527 ?

@charliepark
Copy link
Contributor Author

Good point on the breadcrumbs. Have added those in, but I think your larger point is a good one — there'll be more than just the breadcrumbs to discuss, and holding back on the feature for a release to get it right could be the better call. Will look forward to talking through it.

@charliepark charliepark changed the title Internet gateways 2 Add read-only UI for internet gateways Oct 7, 2024
@david-crespo
Copy link
Collaborator

Updated the info box copy and added a docs link:

image

className="link-with-underline text-sans-md"
>
{route.name}
</Link>
Copy link
Collaborator

@david-crespo david-crespo Dec 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find clicking this link to the route edit form very jarring. We have some similar links in other places (linked silos and IP pools both link to each other, disks link to the instances they're attached to) but somehow this feels much more confusing. I thought maybe the problem was being dropped in the form (because you're in a modal and can't see much behind it), so maybe landing in the list of routes would be better, but landing on the router route list page instead is similarly disorienting.

This isn't really something wrong with this PR (it's more about the underlying issue of deep nesting in the VPC tree, which is why @benjaminleonard wanted to keep the gateway details in a modal instead of a whole page) so I'm not sure we can fix it here either, unless we were to drop the link altogether. A more elaborate fix would be to open the route details in a side modal on top of the gateways page, but it's never great to have two canonical locations for a thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One option would be to add the name of the router on that table, and to link to the RouterPage, rather than to link to the specific route's edit form. That way, they're taken from a page to a page, rather than into the side modal form. I don't know that it's as useful, though.

We could also include it without a link, but I think the link is useful.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did we figure out a ballpark likelihood of there being multiple routes targeting a gateway? It doesn't seem like that weird a thing to do. In any case we need to figure out how to render this better as a list — there are 3 here and no spaces between them. I think this is why we were considering not putting this in the cell at all.

image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Chatted in Matrix; agreed that a count would be a good way to convey the number of routes, which they can then click to get the more detailed view or router/routes in the sidebar.

@david-crespo
Copy link
Collaborator

Looks pretty good overall. My main open question is how to handle multiple routes in the gateways table cell, possibly by removing that column. Secondary question of whether there's a less jarring thing we can do than linking to the route edit form.

@david-crespo david-crespo enabled auto-merge (squash) December 13, 2024 17:25
@david-crespo david-crespo merged commit ada302c into main Dec 13, 2024
8 checks passed
@david-crespo david-crespo deleted the internet-gateways-2 branch December 13, 2024 17:27
david-crespo added a commit to oxidecomputer/omicron that referenced this pull request Dec 13, 2024
oxidecomputer/console@927c8b6...c1ebd8d

* [c1ebd8d9](oxidecomputer/console@c1ebd8d9) take inventory switches tab back out, DB is empty
* [2a139028](oxidecomputer/console@2a139028) tone down empty switches table message
* [2b0c3f12](oxidecomputer/console@2b0c3f12) oxidecomputer/console#2621
* [c1fbf631](oxidecomputer/console@c1fbf631) oxidecomputer/console#2622
* [1dd25f63](oxidecomputer/console@1dd25f63) oxidecomputer/console#2615
* [11486f8b](oxidecomputer/console@11486f8b) oxidecomputer/console#2620
* [ada302ce](oxidecomputer/console@ada302ce) oxidecomputer/console#2488
* [bc3161ae](oxidecomputer/console@bc3161ae) minor: remove stub e2e test
* [9e1d53c6](oxidecomputer/console@9e1d53c6) sentence case idp form heading
* [aaf1154f](oxidecomputer/console@aaf1154f) add extra assert for instance create with additional disks test flake
* [79d610dc](oxidecomputer/console@79d610dc) oxidecomputer/console#2618
* [bdbc02b7](oxidecomputer/console@bdbc02b7) oxidecomputer/console#2589
* [7a8ee0ab](oxidecomputer/console@7a8ee0ab) oxidecomputer/console#2614
* [0b5220a1](oxidecomputer/console@0b5220a1) npm audit fix
* [0c873cf4](oxidecomputer/console@0c873cf4) oxidecomputer/console#2610
* [d031c8ff](oxidecomputer/console@d031c8ff) bump playwright for navigation hang fix
* [dbd8545e](oxidecomputer/console@dbd8545e) oxidecomputer/console#2609
* [dc5562fe](oxidecomputer/console@dc5562fe) move error log to avoid failing to log certain errors
david-crespo added a commit to oxidecomputer/omicron that referenced this pull request Dec 14, 2024
oxidecomputer/console@927c8b6...c1ebd8d

* [c1ebd8d9](oxidecomputer/console@c1ebd8d9)
take inventory switches tab back out, DB is empty
* [2a139028](oxidecomputer/console@2a139028)
tone down empty switches table message
* [2b0c3f12](oxidecomputer/console@2b0c3f12)
oxidecomputer/console#2621
* [c1fbf631](oxidecomputer/console@c1fbf631)
oxidecomputer/console#2622
* [1dd25f63](oxidecomputer/console@1dd25f63)
oxidecomputer/console#2615
* [11486f8b](oxidecomputer/console@11486f8b)
oxidecomputer/console#2620
* [ada302ce](oxidecomputer/console@ada302ce)
oxidecomputer/console#2488
* [bc3161ae](oxidecomputer/console@bc3161ae)
minor: remove stub e2e test
* [9e1d53c6](oxidecomputer/console@9e1d53c6)
sentence case idp form heading
* [aaf1154f](oxidecomputer/console@aaf1154f)
add extra assert for instance create with additional disks test flake
* [79d610dc](oxidecomputer/console@79d610dc)
oxidecomputer/console#2618
* [bdbc02b7](oxidecomputer/console@bdbc02b7)
oxidecomputer/console#2589
* [7a8ee0ab](oxidecomputer/console@7a8ee0ab)
oxidecomputer/console#2614
* [0b5220a1](oxidecomputer/console@0b5220a1)
npm audit fix
* [0c873cf4](oxidecomputer/console@0c873cf4)
oxidecomputer/console#2610
* [d031c8ff](oxidecomputer/console@d031c8ff)
bump playwright for navigation hang fix
* [dbd8545e](oxidecomputer/console@dbd8545e)
oxidecomputer/console#2609
* [dc5562fe](oxidecomputer/console@dc5562fe)
move error log to avoid failing to log certain errors
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Internet gateways tab on VPC page
3 participants