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 field for changing target port if app gateway fails to start #50982

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ravicious
Copy link
Member

This builds on #50912. One of the issues that I haven't taken care of in that PR is that if a target port you created a gateway for is no longer defined in the app spec, then when restoring tabs Connect wouldn't be able to create such gateway, as tshd would return an error about the port not being valid.

This PR adds a Target Port field next to the existing Local Port field. When a gateway fails to start, the user is able to change the target port, just like they're able to change the local port today.

To achieve this, I refactored OfflineGateway. It used to support a prop which dictated whether the Local Port is going to be shown or not, along with another prop to set the default port and another for the label for the input field. Now it expects its callers to pass uncontrolled form fields through renderFormControls and formSchema. When the form is submitted, OfflineGateway runs the form data through the schema and then passes the result to createGateway. This way each caller is able to specify by themselves what fields its going to use and how it's going to validate the form:

  • DocumentGatewayKube doesn't use any fields.
  • DocumentGateway (db gateways) shows just the local port field which is optional.
  • DocumentGatewayApp shows the local port field. If the app is a multi-port TCP app, it also shows the target port field.
    • The local port field is optional, but the target port field is required if it's being shown. An invariant of the gateway doc for multi-port apps is that targetSubresourceName is always present, since that's how we can tell gateway docs for multi-port apps from single-port apps.

zod usage

Uncontrolled forms are naturally not "typeable" since it's just a bunch of strings, so I used zod to transform form data into an object with the expected fields. As long as the form schema is properly defined, the component is going to throw an error if the form is missing one of the fields.

I have to say that I just now realized that using zod with strict null checks off is suboptimal, as in that scenario it's impossible to make zod return a type with non-optional fields. It's not that big of an issue here, but it might be if we use this approach elsewhere. On top of that, zod's errors require some extra work if we want to show them in the UI. For now I just skipped this work since all errors here are the result of a programmer error, not the user submitting invalid data. But in the future, our form components could be adjusted to show zod errors.

Considered alternatives

Another prop for target port field

Just like OfflineGateway had gatewayPort: { isSupported: boolean, defaultPort: string }, it could have an equivalent of this prop for the target subresource name that stores the target port for app gateways. I ruled out this solution as different gateways treat target subresource name in different ways and this design wouldn't be very flexible.

Controlled forms

Instead of using zod and uncontrolled forms, renderFormControls could receive useState return values for each field and then build form controls using that. However, OfflineGateway would still need to receive props with defaults for each field, as it'd be controlling the fields.

Honestly, maybe it wouldn't be that bad. The extra props for default values seem kinda off, but otherwise we wouldn't need to deal with zod. While it'd be impossible to introduce bugs stemming from incorrect input names, this design introduces a potential issue where renderFormControls does not render an expected field and there's no error about it. But in the end it just means that it'd required tests, just like the zod implementation does to ensure that the form schemas are in sync with the forms.

Button for removing the gateway

This is probably the solution I should've went with in the first place. The target port being no longer valid is probably going to be such a niche use case that it feels wrong to dedicate so much time and code to taking care of it. Just remove the gateway and let user create a new one, this time with a correct port.

But well, I've already invested the time and code into this. 🥲 I suppose if we ever find ourselves in need of extending gateway support, e.g. choosing kube namespaces, maybe parts of this implementation will make things easier.

@ravicious ravicious added the no-changelog Indicates that a PR does not require a changelog entry label Jan 13, 2025
@ravicious ravicious force-pushed the r7s/change-target-port branch from 1ea0115 to 8cbbd1b Compare January 13, 2025 13:15
Base automatically changed from r7s/target-port-connect to master January 13, 2025 17:15
This will enable each callsite to establish it's own rules as to which
fields are required. This will be useful once we add required target
subresource name for TCP apps.
@ravicious ravicious force-pushed the r7s/change-target-port branch from 8cbbd1b to f266bf6 Compare January 14, 2025 10:07
@ravicious ravicious marked this pull request as ready for review January 14, 2025 10:45
@ravicious ravicious requested review from avatus and gzdunek January 14, 2025 10:45
@github-actions github-actions bot requested a review from kimlisa January 14, 2025 10:45
@ravicious ravicious removed the request for review from kimlisa January 14, 2025 10:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/branch/v17 no-changelog Indicates that a PR does not require a changelog entry size/md ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant