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 network forwards #572

Merged
merged 1 commit into from
Jan 9, 2024
Merged

Add network forwards #572

merged 1 commit into from
Jan 9, 2024

Conversation

edlerd
Copy link
Collaborator

@edlerd edlerd commented Dec 7, 2023

Done

  • add forwards tab under network detail page
  • added create network forward in the new tab
  • added edit network forward in the new tab
  • added Network name and subnet details to the create/edit forward screens
  • removed html5 tooltips from required fields/edit forward screens
  • changed help text for ports
  • added description for forward

Fixes WD-7876

QA

  1. Run the LXD-UI:
    • On the demo server via the link posted by @webteam-app below. This is only available for PRs created by collaborators of the repo. Ask @lorumic or @edlerd for access.
    • With a local copy of this branch, run as described here.
  2. Perform the following QA steps:
    • create a network forward
    • edit a network forward with ports

@webteam-app
Copy link

Demo starting at https://lxd-ui-572.demos.haus

@piperdeck
Copy link

1.

My first thought is that on that page for the new network forward, there's nothing on this screen reminding you what of the subnet for the network that you're making this forward for. So if you didn't think to copy the network's IP address, you'll need to close this page and go back, then start over.

Here are my networks, with their subnet addresses showing.
image

Here's the details page for the network I want to forward to. It's IP address is still showing.
image

Now I go to the forwards tab....
image

And create a new forward, but it's not giving me any help by reminding me of the subnet address, which is most likely what I'll want here.
image

2.

Is this tooltip necessary? It appears as soon as I start editing the network forward
image

3.

I completely thought that this placeholder text was specifying the range of legal ports, and that we just have a weird restriction on which ports you can use. I would use support text like "eg. '80,90-100'" below the field rather than placeholder text here.

Incidentally, I noticed that the field won't validate that I've entered an illegal port number here. I suspect it won't break anything, but it's probably at least worth a yellow warning validation informing me that this won't work because ports only go up to 65353
image

4.

Is there a reason we don't have names or descriptions for forwards and ports? Both are supported on the back end

@edlerd
Copy link
Collaborator Author

edlerd commented Dec 11, 2023

Thanks for the review @piperdeck

I addressed all mentioned points, please have a look at the updated result.

One point is still unresolved: The description for ports, I can't see how to fit them into the design. Maybe you have an idea, or maybe we don't need them in the UI?

@mas-who
Copy link
Contributor

mas-who commented Dec 12, 2023

Questions

1. Network forward with listen address that overlaps with another network's subnet?

I created a network forward, setting the listen address to an IP that coincides with the subnet range of another network. I was expecting an error from the backend but was able to create the network forward successfully. According to the docs this is not allowed for a bridge network. Maybe my understanding is off?

Screenshot from 2023-12-12 14-23-40
Screenshot from 2023-12-12 14-36-53

2. When creating ports for network forwards, duplicated listen ports are not allowed?

I'm not a networking expert, thought maybe you could listen on one ip:port then forward to many internal ip:port addresses. But that's not true, I assume that's the correct behaviour?

3. Invalid port numbers.

This is related to the query from @piperdeck. When entering invalid port numbers we get a massive error output from the server. Maybe it would be a better user experience if we have some client side validation? Also noted that you can forward from different external ports to the same internal address ports.

@edlerd
Copy link
Collaborator Author

edlerd commented Dec 12, 2023

Thanks for the comments @mas-who

Network forward with listen address that overlaps with another network's subnet?
I created a network forward, setting the listen address to an IP that coincides with the subnet range of another network. I was expecting an error from the backend but was able to create the network forward successfully. According to the docs this is not allowed for a bridge network. Maybe my understanding is off?

That is okay, the forwards are firewall rules for the host, so any incoming traffic to a "listen address" is taken over by this rule, independent to which subnet or interface it belongs.

When creating ports for network forwards, duplicated listen ports are not allowed?
I'm not a networking expert, thought maybe you could listen on one ip:port then forward to many internal ip:port addresses. But that's not true, I assume that's the correct behaviour?

Yes, you can only have one target for a given listen ip-port combination. What is pretty neat is, that for the same ip you can have different targets based on the port. So port 80 and 443 might go to one instance, while port 21 goes to another instance.

Invalid port numbers.
This is related to the query from @piperdeck. When entering invalid port numbers we get a massive error output from the server. Maybe it would be a better user experience if we have some client side validation? Also noted that you can forward from different external ports to the same internal address ports.

True, when entering something like 500000 as a port, LXD responds with the raw error from the iptables config it tries to create. Though I would avoid validating high ports, as that becomes rather tricky with the supported formats for port ranges like 80-90 and lists like 80,443.

@piperdeck
Copy link

1

Can we please replace these with actual arrow symbols? Would look a little tidier

E.g. :8000 ➝ 10.40.196.50:3000 (tcp)

image

2

It seems like this error notification has messed up the div sizes, leading to some weird scrolling behaviour. Also, the error is pretty unreadable. If we can't validate the input before the user presses the submit button, would it be possible to replace the error text with something more informative, along with highlighting the problematic field?

Screencast.from.2024-01-05.12-52-47.webm

@edlerd
Copy link
Collaborator Author

edlerd commented Jan 5, 2024

It seems like this error notification has messed up the div sizes, leading to some weird scrolling behaviour. Also, the error is pretty unreadable. If we can't validate the input before the user presses the submit button, would it be possible to replace the error text with something more informative, along with highlighting the problematic field?

We can reposition the error notification, so it is rendered in the correct container and doesn't mess up the page layout.

We can also validate the port numbers, so they are positive integers below 65535. It is a bit tricky to still allow ranges, and comma separated values, and a combination of the two. But that should be possible with regexp.

@mas-who
Copy link
Contributor

mas-who commented Jan 9, 2024

QA looks good! Just a few minor comments with the code review.

@edlerd edlerd force-pushed the network-forwards branch 4 times, most recently from 9a2580f to 942d31d Compare January 9, 2024 12:58
@mas-who
Copy link
Contributor

mas-who commented Jan 9, 2024

LGTM. Thanks for making the changes

Signed-off-by: David Edler <david.edler@canonical.com>
@piperdeck
Copy link

Awesome, looks good.

@edlerd edlerd merged commit 903b497 into canonical:main Jan 9, 2024
6 checks passed
@edlerd edlerd deleted the network-forwards branch January 9, 2024 22:37
github-actions bot pushed a commit that referenced this pull request Jan 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants