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.

Sign up for GitHub

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

Support arbitrary role resource version in the editor #51160

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Support arbitrary role resource version in the editor #51160

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 2 additions & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@
"d3-time-format": "^4.1.0",
"date-fns": "^2.28.0",
"history": "^4.9.0",
"immer": "^10.1.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

In the PR description, can you also explain a bit more on why we chose "immer" over other alternatives please?

That information will be valuable for anyone looking for origin of immer as dependency.

"prop-types": "^15.8.1",
"react": "^18.3.1",
"react-day-picker": "^8.10.1",
Expand All @@ -95,6 +96,7 @@
"react-transition-group": "^4.4.5",
"styled-components": "^6.1.13",
"tslib": "^2.8.1",
"use-immer": "^0.11.0",
"whatwg-fetch": "^3.6.20"
},
"msw": {
Expand Down
29 changes: 22 additions & 7 deletions pnpm-lock.yaml
View file
Open in desktop

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

31 changes: 22 additions & 9 deletions web/packages/teleport/src/Roles/RoleEditor/StandardEditor/MetadataSection.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,16 @@
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/

import Box from 'design/Box';
import Text from 'design/Text';
import FieldInput from 'shared/components/FieldInput';
import { FieldSelect } from 'shared/components/FieldSelect';
import { precomputed } from 'shared/components/Validation/rules';

import { LabelsInput } from 'teleport/components/LabelsInput';

import { SectionBox, SectionProps } from './sections';
import { MetadataModel } from './standardmodel';
import { MetadataModel, roleVersionOptions } from './standardmodel';
import { MetadataValidationResult } from './validation';

export const MetadataSection = ({
Expand Down Expand Up @@ -55,14 +57,25 @@ export const MetadataSection = ({
onChange({ ...value, description: e.target.value })
}
/>
<Text typography="body3" mb={1}>
Labels
</Text>
<LabelsInput
disableBtns={isProcessing}
labels={value.labels}
setLabels={labels => onChange?.({ ...value, labels })}
rule={precomputed(validation.fields.labels)}
<Box mb={3}>
<Text typography="body3" mb={1}>
Labels
</Text>
<LabelsInput
disableBtns={isProcessing}
labels={value.labels}
setLabels={labels => onChange?.({ ...value, labels })}
Copy link
Contributor

Choose a reason for hiding this comment

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

onChange is optional? Why?

rule={precomputed(validation.fields.labels)}
/>
</Box>
<FieldSelect
label="Version"
isDisabled={isProcessing}
options={roleVersionOptions}
value={value.version}
onChange={version => onChange({ ...value, version })}
// rule={precomputed(validation.fields.verbs)}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// rule={precomputed(validation.fields.verbs)}

dead code

Copy link
Contributor

Choose a reason for hiding this comment

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

remove comment

mb={0}
/>
</SectionBox>
);
33 changes: 26 additions & 7 deletions web/packages/teleport/src/Roles/RoleEditor/StandardEditor/Resources.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ import selectEvent from 'react-select-event';
import { render, screen, userEvent } from 'design/utils/testing';
import { Validator } from 'shared/components/Validation';

import { RoleVersion } from 'teleport/services/resources';

import {
AppAccessSection,
DatabaseAccessSection,
Expand All @@ -32,6 +34,7 @@ import {
import {
AppAccess,
DatabaseAccess,
defaultRoleVersion,
KubernetesAccess,
newResourceAccess,
ServerAccess,
Expand All @@ -50,7 +53,7 @@ describe('ServerAccessSection', () => {
render(
<StatefulSection<ServerAccess, ResourceAccessValidationResult>
component={ServerAccessSection}
defaultValue={newResourceAccess('node')}
defaultValue={newResourceAccess('node', defaultRoleVersion)}
onChange={onChange}
validatorRef={v => {
validator = v;
Expand Down Expand Up @@ -104,13 +107,16 @@ describe('ServerAccessSection', () => {
});

describe('KubernetesAccessSection', () => {
const setup = () => {
const setup = (roleVersion: RoleVersion = 'v7') => {
const onChange = jest.fn();
let validator: Validator;
render(
<StatefulSection<KubernetesAccess, ResourceAccessValidationResult>
component={KubernetesAccessSection}
defaultValue={newResourceAccess('kube_cluster')}
defaultValue={{
...newResourceAccess('kube_cluster', defaultRoleVersion),
roleVersion,
}}
onChange={onChange}
validatorRef={v => {
validator = v;
Expand Down Expand Up @@ -169,8 +175,10 @@ describe('KubernetesAccessSection', () => {
expect.objectContaining({ value: 'create' }),
expect.objectContaining({ value: 'delete' }),
],
roleVersion: 'v7',
},
],
roleVersion: 'v7',
} as KubernetesAccess);
});

Expand Down Expand Up @@ -228,12 +236,20 @@ describe('KubernetesAccessSection', () => {
});

test('validation', async () => {
const { user, validator } = setup();
const { user, validator } = setup('v6');
await user.click(screen.getByRole('button', { name: 'Add a Label' }));
await user.click(screen.getByRole('button', { name: 'Add a Resource' }));
await selectEvent.select(screen.getByLabelText('Kind'), 'Service');
await user.clear(screen.getByLabelText('Name'));
await user.clear(screen.getByLabelText('Namespace'));
await selectEvent.select(screen.getByLabelText('Verbs'), [
'All verbs',
'create',
]);
act(() => validator.validate());
expect(
screen.getByText('Only pods are allowed for role version v6')
).toBeVisible();
expect(
screen.getByPlaceholderText('label key')
).toHaveAccessibleDescription('required');
Expand All @@ -243,6 +259,9 @@ describe('KubernetesAccessSection', () => {
expect(screen.getByLabelText('Namespace')).toHaveAccessibleDescription(
'Namespace is required for resources of this kind'
);
expect(
screen.getByText('Mixing "All verbs" with other options is not allowed')
).toBeVisible();
});
});

Expand All @@ -253,7 +272,7 @@ describe('AppAccessSection', () => {
render(
<StatefulSection<AppAccess, ResourceAccessValidationResult>
component={AppAccessSection}
defaultValue={newResourceAccess('app')}
defaultValue={newResourceAccess('app', defaultRoleVersion)}
onChange={onChange}
validatorRef={v => {
validator = v;
Expand Down Expand Up @@ -359,7 +378,7 @@ describe('DatabaseAccessSection', () => {
render(
<StatefulSection<DatabaseAccess, ResourceAccessValidationResult>
component={DatabaseAccessSection}
defaultValue={newResourceAccess('db')}
defaultValue={newResourceAccess('db', defaultRoleVersion)}
onChange={onChange}
validatorRef={v => {
validator = v;
Expand Down Expand Up @@ -425,7 +444,7 @@ describe('WindowsDesktopAccessSection', () => {
render(
<StatefulSection<WindowsDesktopAccess, ResourceAccessValidationResult>
component={WindowsDesktopAccessSection}
defaultValue={newResourceAccess('windows_desktop')}
defaultValue={newResourceAccess('windows_desktop', defaultRoleVersion)}
onChange={onChange}
validatorRef={v => {
validator = v;
Expand Down
11 changes: 9 additions & 2 deletions web/packages/teleport/src/Roles/RoleEditor/StandardEditor/Resources.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,8 @@ import { Mark } from 'design/Mark';
import Text, { H4 } from 'design/Text';
import FieldInput from 'shared/components/FieldInput';
import { FieldMultiInput } from 'shared/components/FieldMultiInput/FieldMultiInput';
import FieldSelect, {
import {
FieldSelect,
FieldSelectCreatable,
} from 'shared/components/FieldSelect';
import { MenuButton, MenuItem } from 'shared/components/MenuAction';
Expand Down Expand Up @@ -321,7 +322,10 @@ export function KubernetesAccessSection({
onClick={() =>
onChange?.({
...value,
resources: [...value.resources, newKubernetesResourceModel()],
resources: [
...value.resources,
newKubernetesResourceModel(value.roleVersion),
],
})
}
>
Expand Down Expand Up @@ -351,6 +355,7 @@ function KubernetesResourceView({
}) {
const { kind, name, namespace, verbs } = value;
const theme = useTheme();
console.log('ROLE VERSION', value.roleVersion);
Copy link
Contributor

Choose a reason for hiding this comment

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

remove console

return (
<Box
border={1}
Expand Down Expand Up @@ -378,6 +383,7 @@ function KubernetesResourceView({
isDisabled={isProcessing}
options={kubernetesResourceKindOptions}
value={kind}
rule={precomputed(validation.kind)}
onChange={k => onChange?.({ ...value, kind: k })}
/>
<FieldInput
Expand Down Expand Up @@ -412,6 +418,7 @@ function KubernetesResourceView({
isDisabled={isProcessing}
options={kubernetesVerbOptions}
value={verbs}
rule={precomputed(validation.verbs)}
onChange={v => onChange?.({ ...value, verbs: v })}
mb={0}
/>
Expand Down
20 changes: 20 additions & 0 deletions web/packages/teleport/src/Roles/RoleEditor/StandardEditor/StandardEditor.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,8 +138,10 @@ test('edits metadata', async () => {
const onSave = (r: Role) => (role = r);
render(<TestStandardEditor onSave={onSave} />);
await user.type(screen.getByLabelText('Description'), 'foo');
await selectEvent.select(screen.getByLabelText('Version'), 'v6');
await user.click(screen.getByRole('button', { name: 'Create Role' }));
expect(role.metadata.description).toBe('foo');
expect(role.version).toBe('v6');
});

test('edits resource access', async () => {
Expand All @@ -159,6 +161,24 @@ test('edits resource access', async () => {
expect(role.spec.allow.logins).toEqual(['{{internal.logins}}', 'ec2-user']);
});

test('propagates version info to Kubernetes resource access', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe rename this test to "triggers v6 validation for kubernetes resource."

I initially got confused otherwise since the test only says propagates version info but we are interested in validation triggered at the end.

const user = userEvent.setup();
const onSave = jest.fn();
render(<TestStandardEditor onSave={onSave} />);
await selectEvent.select(screen.getByLabelText('Version'), 'v6');
await user.click(screen.getByRole('tab', { name: 'Resources' }));
await user.click(
screen.getByRole('button', { name: 'Add New Resource Access' })
);
await user.click(screen.getByRole('menuitem', { name: 'Kubernetes' }));
await user.click(screen.getByRole('button', { name: 'Add a Resource' }));
await selectEvent.select(screen.getByLabelText('Kind'), 'Job');
await user.click(screen.getByRole('button', { name: 'Create Role' }));

// Validation should have failed on a Job resource and role v6.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we assert that some validation error message appears on the screen?

expect(onSave).not.toHaveBeenCalled();
});

const getAllMenuItemNames = () =>
screen.queryAllByRole('menuitem').map(m => m.textContent);

Expand Down
Loading
Loading