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 basic support for target port in gateways in Connect #50912

Open
wants to merge 19 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions gen/proto/go/teleport/lib/teleterm/v1/gateway.pb.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 3 additions & 2 deletions gen/proto/ts/teleport/lib/teleterm/v1/gateway_pb.ts

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 3 additions & 2 deletions proto/teleport/lib/teleterm/v1/gateway.proto
Original file line number Diff line number Diff line change
Expand Up @@ -43,12 +43,13 @@ message Gateway {
string local_address = 5;
// local_port is the gateway address on localhost
string local_port = 6;
// protocol is the gateway protocol
// protocol is the protocol used by the gateway. For databases, it matches the type of the
// database that the gateway targets. For apps, it's either "HTTP" or "TCP".
string protocol = 7;
reserved 8;
reserved "cli_command";
// target_subresource_name points at a subresource of the remote resource, for example a
// database name on a database server.
// database name on a database server or a target port of a multi-port TCP app.
string target_subresource_name = 9;
// gateway_cli_client represents a command that the user can execute to connect to the resource
// through the gateway.
Expand Down
12 changes: 12 additions & 0 deletions web/packages/design/src/Menu/Menu.story.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,18 @@ export const MenuItems = () => (
<MenuItem>Amet nisi tempor</MenuItem>
</OpenMenu>
</Flex>
<Flex gap={3} flexDirection="column">
<H3>Label as first child</H3>
<OpenMenu>
<MenuItemSectionLabel>Tempus ut libero</MenuItemSectionLabel>
<MenuItem>Lorem ipsum</MenuItem>
<MenuItem>Dolor sit amet</MenuItem>
<MenuItemSectionSeparator />
<MenuItemSectionLabel>Leo vitae arcu</MenuItemSectionLabel>
<MenuItem>Donec volutpat</MenuItem>
<MenuItem>Mauris sit</MenuItem>
</OpenMenu>
</Flex>
</Flex>
);

Expand Down
30 changes: 16 additions & 14 deletions web/packages/design/src/Menu/MenuItem.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -71,37 +71,39 @@ const MenuItemBase = styled(Flex)`
${fromThemeBase}
`;

export const MenuItemSectionLabel = styled(MenuItemBase).attrs({
px: 2,
export const MenuItemSectionSeparator = styled.hr.attrs({
onClick: event => {
// Make sure that clicks on this element don't trigger onClick set on MenuList.
event.stopPropagation();
},
})`
font-weight: bold;
min-height: 16px;
background: ${props => props.theme.colors.interactive.tonal.neutral[1]};
height: 1px;
border: 0;
font-size: 0;
`;

export const MenuItemSectionSeparator = styled.hr.attrs({
export const MenuItemSectionLabel = styled(MenuItemBase).attrs({
px: 2,
onClick: event => {
// Make sure that clicks on this element don't trigger onClick set on MenuList.
event.stopPropagation();
},
})`
background: ${props => props.theme.colors.interactive.tonal.neutral[1]};
height: 1px;
border: 0;
font-size: 0;
font-weight: bold;
min-height: 16px;

// Add padding to the label for extra visual space, but only when it follows a separator.
// If a separator follows a MenuItem, there's already enough visual space, so no extra space is
// needed. The hover state of MenuItem highlights everything right from the separator start to the
// end of MenuItem.
// Add padding to the label for extra visual space, but only when it follows a separator or is the
// first child.
//
// If a separator follows a MenuItem, there's already enough visual space between MenuItem and
// separator, so no extra space is needed. The hover state of MenuItem highlights everything right
// from the separator start to the end of MenuItem.
//
// Padding is used instead of margin here on purpose, so that there's no empty transparent space
// between Separator and Label – otherwise clicking on that space would count as a click on
// MenuList and not trigger onClick set on Separator or Label.
& + ${MenuItemSectionLabel} {
${MenuItemSectionSeparator} + &, &:first-child {
Copy link
Member Author

Choose a reason for hiding this comment

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

This diff is overall confusing since I had to reorder the definitions and then I moved adding that extra padding-top between components. The previous selector for extra padding was also confusing, because it was defined in MenuItemSectionSeparator, but it looked like this: & + ${MenuItemSectionLabel} {. So even though we were in styles for MenuItemSectionSeparator, we were setting styles for MenuItemSectionLabel when it directly follows a separator. 🙃

It's might be easier to look at the master version and the version from this branch.

padding-top: ${props => props.theme.space[1]}px;
}
`;
Expand Down
55 changes: 33 additions & 22 deletions web/packages/teleterm/src/ui/DocumentGateway/useGateway.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import { retryWithRelogin } from 'teleterm/ui/utils';

export function useGateway(doc: DocumentGateway) {
const ctx = useAppContext();
const { clustersService } = ctx;
const { documentsService } = useWorkspaceContext();
// The port to show as default in the input field in case creating a gateway fails.
// This is typically the case if someone reopens the app and the port of the gateway is already
Expand All @@ -51,7 +52,7 @@ export function useGateway(doc: DocumentGateway) {

try {
gw = await retryWithRelogin(ctx, doc.targetUri, () =>
ctx.clustersService.createGateway({
clustersService.createGateway({
targetUri: doc.targetUri,
localPort: port,
targetUser: doc.targetUser,
Expand Down Expand Up @@ -92,34 +93,44 @@ export function useGateway(doc: DocumentGateway) {
});

const [disconnectAttempt, disconnect] = useAsync(async () => {
await ctx.clustersService.removeGateway(doc.gatewayUri);
await clustersService.removeGateway(doc.gatewayUri);
documentsService.close(doc.uri);
});

const [changeTargetSubresourceNameAttempt, changeTargetSubresourceName] =
useAsync(async (name: string) => {
const updatedGateway =
await ctx.clustersService.setGatewayTargetSubresourceName(
doc.gatewayUri,
name
);

documentsService.update(doc.uri, {
targetSubresourceName: updatedGateway.targetSubresourceName,
});
});
useAsync(
useCallback(
async (name: string) => {
const updatedGateway =
await clustersService.setGatewayTargetSubresourceName(
doc.gatewayUri,
name
);

const [changePortAttempt, changePort] = useAsync(async (port: string) => {
const updatedGateway = await ctx.clustersService.setGatewayLocalPort(
doc.gatewayUri,
port
documentsService.update(doc.uri, {
targetSubresourceName: updatedGateway.targetSubresourceName,
});
},
[clustersService, documentsService, doc.uri, doc.gatewayUri]
)
);

documentsService.update(doc.uri, {
targetSubresourceName: updatedGateway.targetSubresourceName,
port: updatedGateway.localPort,
});
});
const [changePortAttempt, changePort] = useAsync(
useCallback(
async (port: string) => {
const updatedGateway = await clustersService.setGatewayLocalPort(
doc.gatewayUri,
port
);

documentsService.update(doc.uri, {
targetSubresourceName: updatedGateway.targetSubresourceName,
port: updatedGateway.localPort,
});
},
[clustersService, documentsService, doc.uri, doc.gatewayUri]
)
);

useEffect(
function createGatewayOnMount() {
Expand Down
20 changes: 10 additions & 10 deletions web/packages/teleterm/src/ui/DocumentGatewayApp/AppGateway.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -39,21 +39,21 @@ import { PortFieldInput } from '../components/FieldInputs';
export function AppGateway(props: {
gateway: Gateway;
disconnectAttempt: Attempt<void>;
changePort(port: string): void;
changePortAttempt: Attempt<void>;
changeLocalPort(port: string): void;
changeLocalPortAttempt: Attempt<void>;
disconnect(): void;
}) {
const { gateway } = props;
const formRef = useRef<HTMLFormElement>();

const { changePort } = props;
const handleChangePort = useMemo(() => {
const { changeLocalPort } = props;
const handleChangeLocalPort = useMemo(() => {
return debounce((value: string) => {
if (formRef.current.reportValidity()) {
changePort(value);
changeLocalPort(value);
}
}, 1000);
}, [changePort]);
}, [changeLocalPort]);

let address = `${gateway.localAddress}:${gateway.localPort}`;
if (gateway.protocol === 'HTTP') {
Expand All @@ -80,11 +80,11 @@ export function AppGateway(props: {
<PortFieldInput
label="Port"
defaultValue={gateway.localPort}
onChange={e => handleChangePort(e.target.value)}
onChange={e => handleChangeLocalPort(e.target.value)}
mb={2}
/>
</Validation>
{props.changePortAttempt.status === 'processing' && (
{props.changeLocalPortAttempt.status === 'processing' && (
<Indicator
size="large"
pt={3} // aligns the spinner to be at the center of the port input
Expand All @@ -98,8 +98,8 @@ export function AppGateway(props: {
<Text>Access the app at:</Text>
<TextSelectCopy my={1} text={address} bash={false} />

{props.changePortAttempt.status === 'error' && (
<Alert details={props.changePortAttempt.statusText}>
{props.changeLocalPortAttempt.status === 'error' && (
<Alert details={props.changeLocalPortAttempt.statusText}>
Could not change the port number
</Alert>
)}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ import * as types from 'teleterm/ui/services/workspacesService';
type StoryProps = {
appType: 'web' | 'tcp';
online: boolean;
changePort: 'succeed' | 'throw-error';
changeLocalPort: 'succeed' | 'throw-error';
disconnect: 'succeed' | 'throw-error';
};

Expand All @@ -44,7 +44,7 @@ const meta: Meta<StoryProps> = {
control: { type: 'radio' },
options: ['web', 'tcp'],
},
changePort: {
changeLocalPort: {
if: { arg: 'online' },
control: { type: 'radio' },
options: ['succeed', 'throw-error'],
Expand All @@ -58,7 +58,7 @@ const meta: Meta<StoryProps> = {
args: {
appType: 'web',
online: true,
changePort: 'succeed',
changeLocalPort: 'succeed',
disconnect: 'succeed',
},
};
Expand All @@ -80,6 +80,7 @@ export function Story(props: StoryProps) {
targetUser: '',
status: '',
targetName: 'quux',
targetSubresourceName: undefined,
};
if (!props.online) {
documentGateway.gatewayUri = undefined;
Expand All @@ -106,7 +107,7 @@ export function Story(props: StoryProps) {
() =>
new MockedUnaryCall(
{ ...gateway, localPort },
props.changePort === 'throw-error'
props.changeLocalPort === 'throw-error'
? new Error('something went wrong')
: undefined
)
Expand Down
Copy link
Member Author

Choose a reason for hiding this comment

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

It's possible for the OfflineGateway state to "soft lock". If you create a doc for target port 1337, then close the app, then remove that port from TCP ports of the app, then reopen the app, creating the gateway will fail. You won't be able to change the target port until you remove the gateway from the connections list.

This is also something I'm going to address in another PR. But this whole thing definitely requires too much work for how niche it's going to be. 😭

Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,8 @@ export function DocumentGatewayApp(props: {
const { doc } = props;
const {
gateway,
changePort,
changePortAttempt,
changePort: changeLocalPort,
changePortAttempt: changeLocalPortAttempt,
connected,
connectAttempt,
disconnect,
Expand All @@ -53,8 +53,8 @@ export function DocumentGatewayApp(props: {
gateway={gateway}
disconnect={disconnect}
disconnectAttempt={disconnectAttempt}
changePort={changePort}
changePortAttempt={changePortAttempt}
changeLocalPort={changeLocalPort}
changeLocalPortAttempt={changeLocalPortAttempt}
/>
)}
</Document>
Expand Down
2 changes: 2 additions & 0 deletions web/packages/teleterm/src/ui/TabHost/useTabShortcuts.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ function getMockDocuments(): Document[] {
targetUri: '/clusters/bar/dbs/foobar',
targetName: 'foobar',
targetUser: 'foo',
targetSubresourceName: undefined,
origin: 'resource_table',
status: '',
},
Expand All @@ -66,6 +67,7 @@ function getMockDocuments(): Document[] {
targetUri: '/clusters/bar/dbs/foobar',
targetName: 'foobar',
targetUser: 'bar',
targetSubresourceName: undefined,
origin: 'resource_table',
status: '',
},
Expand Down
26 changes: 9 additions & 17 deletions web/packages/teleterm/src/ui/components/FieldInputs.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -16,23 +16,15 @@
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/

import { forwardRef } from 'react';
import styled from 'styled-components';

import FieldInput, { FieldInputProps } from 'shared/components/FieldInput';
import FieldInput from 'shared/components/FieldInput';

export const ConfigFieldInput = forwardRef<HTMLInputElement, FieldInputProps>(
(props, ref) => <FieldInput size="small" ref={ref} {...props} />
);
export const ConfigFieldInput = styled(FieldInput).attrs({ size: 'small' })``;

export const PortFieldInput = forwardRef<HTMLInputElement, FieldInputProps>(
(props, ref) => (
<ConfigFieldInput
type="number"
min={1}
max={65535}
ref={ref}
{...props}
width="110px"
/>
)
);
export const PortFieldInput = styled(ConfigFieldInput).attrs({
type: 'number',
min: 1,
max: 65535,
width: '110px',
})``;
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ describe('document should be added', () => {
targetUri: '/clusters/bar/dbs/quux',
targetName: 'quux',
targetUser: 'foo',
targetSubresourceName: undefined,
origin: 'resource_table',
status: '',
};
Expand Down Expand Up @@ -155,6 +156,7 @@ test('only gateway documents should be returned', () => {
targetUri: '/clusters/bar/dbs/quux',
targetName: 'quux',
targetUser: 'foo',
targetSubresourceName: undefined,
origin: 'resource_table',
status: '',
};
Expand Down
Loading