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

Wizard: Firewall ports input (HMS-5332) #2750

Merged
merged 1 commit into from
Jan 16, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
import React from 'react';

import { FormGroup } from '@patternfly/react-core';

import { useAppSelector } from '../../../../../store/hooks';
import {
addPort,
removePort,
selectPorts,
} from '../../../../../store/wizardSlice';
import ChippingInput from '../../../ChippingInput';
import { isPortValid } from '../../../validators';

const PortsInput = () => {
const ports = useAppSelector(selectPorts);

return (
<FormGroup label="Ports">
<ChippingInput
ariaLabel="Add ports"
placeholder="Add ports"
validator={isPortValid}
list={ports}
item="Port"
addAction={addPort}
removeAction={removePort}
/>
</FormGroup>
);
};

export default PortsInput;
3 changes: 3 additions & 0 deletions src/Components/CreateImageWizard/steps/Firewall/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,16 @@ import React from 'react';

import { Text, Form, Title } from '@patternfly/react-core';

import PortsInput from './components/PortsInput';

const FirewallStep = () => {
return (
<Form>
<Title headingLevel="h1" size="xl">
Firewall
</Title>
<Text>Customize firewall settings for your image.</Text>
<PortsInput />
</Form>
);
};
Expand Down
18 changes: 17 additions & 1 deletion src/Components/CreateImageWizard/utilities/requestMapper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ import {
selectHostname,
selectUsers,
selectMetadata,
selectPorts,
} from '../../../store/wizardSlice';
import { FileSystemConfigurationType } from '../steps/FileSystem';
import {
Expand Down Expand Up @@ -323,6 +324,9 @@ function commonRequestToState(
ntpservers: request.customizations.timezone?.ntpservers || [],
},
hostname: request.customizations.hostname || '',
firewall: {
ports: request.customizations.firewall?.ports || [],
},
};
}

Expand Down Expand Up @@ -523,7 +527,7 @@ const getCustomizations = (state: RootState, orgID: string): Customizations => {
groups: undefined,
timezone: getTimezone(state),
locale: getLocale(state),
firewall: undefined,
firewall: getFirewall(state),
installation_device: undefined,
fdo: undefined,
ignition: undefined,
Expand Down Expand Up @@ -689,6 +693,18 @@ const getLocale = (state: RootState) => {
}
};

const getFirewall = (state: RootState) => {
const ports = selectPorts(state);

if (ports.length === 0) {
return undefined;
} else {
return {
ports: ports,
};
}
};

const getCustomRepositories = (state: RootState) => {
const customRepositories = selectCustomRepositories(state);
const recommendedRepositories = selectRecommendedRepositories(state);
Expand Down
4 changes: 4 additions & 0 deletions src/Components/CreateImageWizard/validators.ts
Original file line number Diff line number Diff line change
Expand Up @@ -103,3 +103,7 @@ export const isHostnameValid = (hostname: string) => {
)
);
};

export const isPortValid = (port: string) => {
return /^(\d{1,5}|[a-z]{1,6})(-\d{1,5})?:[a-z]{1,6}$/.test(port);
};
23 changes: 23 additions & 0 deletions src/store/wizardSlice.ts
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,9 @@ export type wizardState = {
};
timezone: Timezone;
hostname: string;
firewall: {
ports: string[];
};
metadata?: {
parent_id: string | null;
exported_at: string;
Expand Down Expand Up @@ -216,6 +219,9 @@ export const initialState: wizardState = {
ntpservers: [],
},
hostname: '',
firewall: {
ports: [],
},
firstBoot: { script: '' },
users: [],
};
Expand Down Expand Up @@ -418,6 +424,10 @@ export const selectHostname = (state: RootState) => {
return state.wizard.hostname;
};

export const selectPorts = (state: RootState) => {
return state.wizard.firewall.ports;
};

export const wizardSlice = createSlice({
name: 'wizard',
initialState,
Expand Down Expand Up @@ -838,6 +848,17 @@ export const wizardSlice = createSlice({
setUserSshKeyByIndex: (state, action: PayloadAction<UserSshKeyPayload>) => {
state.users[action.payload.index].ssh_key = action.payload.sshKey;
},
addPort: (state, action: PayloadAction<string>) => {
if (!state.firewall.ports.some((port) => port === action.payload)) {
state.firewall.ports.push(action.payload);
}
},
removePort: (state, action: PayloadAction<string>) => {
state.firewall.ports.splice(
state.firewall.ports.findIndex((port) => port === action.payload),
1
);
},
},
});

Expand Down Expand Up @@ -906,6 +927,8 @@ export const {
addNtpServer,
removeNtpServer,
changeHostname,
addPort,
removePort,
addUser,
setUserNameByIndex,
setUserPasswordByIndex,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,14 @@ import type { Router as RemixRouter } from '@remix-run/router';
import { screen, waitFor } from '@testing-library/react';
import { userEvent } from '@testing-library/user-event';

import { CREATE_BLUEPRINT } from '../../../../../constants';
import {
blueprintRequest,
clickBack,
clickNext,
enterBlueprintName,
interceptBlueprintRequest,
openAndDismissSaveAndBuildModal,
verifyCancelButton,
} from '../../wizardTestUtils';
import { clickRegisterLater, renderCreateMode } from '../../wizardTestUtils';
Expand Down Expand Up @@ -32,6 +37,19 @@ const goToFirewallStep = async () => {
await clickNext(); // Firewall
};

const goToReviewStep = async () => {
await clickNext(); // First boot script
await clickNext(); // Details
await enterBlueprintName();
await clickNext(); // Review
};

const addPort = async (port: string) => {
const user = userEvent.setup();
const portsInput = await screen.findByPlaceholderText(/add port/i);
await waitFor(() => user.type(portsInput, port.concat(',')));
};

describe('Step Firewall', () => {
beforeEach(() => {
vi.clearAllMocks();
Expand Down Expand Up @@ -59,6 +77,50 @@ describe('Step Firewall', () => {
await goToFirewallStep();
await verifyCancelButton(router);
});

test('duplicate ports cannnot be added', async () => {
await renderCreateMode();
await goToFirewallStep();
expect(screen.queryByText('Port already exists.')).not.toBeInTheDocument();
await addPort('22:tcp');
await addPort('22:tcp');
await screen.findByText('Port already exists.');
});

test('port in an invalid format cannot be added', async () => {
await renderCreateMode();
await goToFirewallStep();
expect(screen.queryByText('Invalid format.')).not.toBeInTheDocument();
await addPort('00:wrongFormat');
await screen.findByText('Invalid format.');
});
});

describe('Firewall request generated correctly', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it make sense to also put the test for edit mode in this PR? Import mode, too?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wrote edit mode tests when all parts of the step functionality were implemented. That way I wouldn't forget to update them later. Similar with import mode tests, those need a PR to setup on-prem import first.

Would prefer to continue the same workflow here to keep the PRs compact, but I'd say both approaches make sense. So whatever feels better I guess 🤷 😅

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, that's fine then! Just want to make sure we don't forget. 😄

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Got it. I'm following the same checklist we applied to steps that are already in stage-stable 😎

test('with ports added', async () => {
await renderCreateMode();
await goToFirewallStep();
await addPort('22:tcp');
await addPort('imap:tcp');
await goToReviewStep();
// informational modal pops up in the first test only as it's tied
// to a 'imageBuilder.saveAndBuildModalSeen' variable in localStorage
await openAndDismissSaveAndBuildModal();
const receivedRequest = await interceptBlueprintRequest(CREATE_BLUEPRINT);

const expectedRequest = {
...blueprintRequest,
customizations: {
firewall: {
ports: ['22:tcp', 'imap:tcp'],
},
},
};

await waitFor(() => {
expect(receivedRequest).toEqual(expectedRequest);
});
});
});

// TO DO Step Firewall -> revisit step button on Review works
Expand Down
Loading