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

feat(HMS-1245): UI AWS permission check #292

Closed
wants to merge 1 commit into from
Closed
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
11 changes: 10 additions & 1 deletion src/API/index.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import axios from 'axios';
import { AZURE_PROVIDER } from '../constants';
import { AWS_PROVIDER, AZURE_PROVIDER } from '../constants';
import { imageBuilderURL, provisioningUrl } from './helpers';

const typesUrlForProvider = (provider, region) => {
Expand Down Expand Up @@ -72,3 +72,12 @@ export const fetchLaunchTemplates = async (sourceID, region) => {
} = await axios.get(provisioningUrl(`sources/${sourceID}/launch_templates?region=${region}`));
return data;
};

export const checkPermissions = async (provider, sourceID, region) => {
switch (provider) {
case AWS_PROVIDER: {
const { data } = await axios.get(provisioningUrl(`sources/${sourceID}/validate_permissions?region=${region}`));
return data;
}
}
};
1 change: 1 addition & 0 deletions src/API/queryKeys.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,3 +4,4 @@ export const PUBKEYS_QUERY_KEY = 'pubkeys';
export const instanceTypesQueryKeys = (region) => ['instanceTypes', region];
export const IMAGE_REGIONS_KEY = 'image_region';
export const TEMPLATES_KEY = 'templates';
export const PERMISSION_CHECK_KEY = 'permissions';
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
import PropTypes from 'prop-types';
import React from 'react';
import { Form, FormGroup, Popover, Title, Button } from '@patternfly/react-core';
import { Form, FormGroup, Popover, Title, Button, FormAlert, Alert } from '@patternfly/react-core';
import { HelpIcon } from '@patternfly/react-icons';
import { useQuery } from 'react-query';

import { AWS_PROVIDER } from '../../../../constants';
import { imageProps } from '../../helpers';
Expand All @@ -11,13 +12,23 @@ import InstanceTypesSelect from '../../../InstanceTypesSelect';
import RegionsSelect from '../../../RegionsSelect';
import { useWizardContext } from '../../../Common/WizardContext';
import TemplatesSelect from '../../../TemplateSelect';
import { checkPermissions } from '../../../../API';

const AccountCustomizationsAWS = ({ setStepValidated, image }) => {
const [{ chosenSource, chosenRegion, chosenInstanceType }, setWizardContext] = useWizardContext();
const { data: missingPermissions } = useQuery(
[`permissions`, `${chosenRegion}-${chosenSource}`],
() => checkPermissions(image.provider, chosenSource, chosenRegion),
{
select: (perm) => perm.missing_entities,
enabled: !!chosenRegion && !!chosenSource,
}
);
const [validations, setValidation] = React.useState({
sources: chosenSource ? 'success' : 'default',
sources: chosenSource ? ((missingPermissions || []).length == 0 ? 'success' : 'warning') : 'default',
types: chosenInstanceType ? 'success' : 'default',
amount: 'success',
region: 'default',
Copy link
Member

@amirfefer amirfefer Sep 6, 2023

Choose a reason for hiding this comment

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

Keep in mind that the default validation means that the field is just set and is not validated yet. we use this validations state for the entire form validation, if one field is marked as default or error the form is not validated, and the Next button will be disabled.

});

const onRegionChange = ({ region, imageID }) => {
Expand All @@ -34,18 +45,52 @@ const AccountCustomizationsAWS = ({ setStepValidated, image }) => {
setStepValidated(!errorExists);
}, [validations]);

React.useEffect(() => {
if ((missingPermissions || []).length != 0) {
avitova marked this conversation as resolved.
Show resolved Hide resolved
if (validations.sources !== 'error') {
setValidation((prevValidations) => ({
...prevValidations,
sources: 'warning',
}));
}
if (validations.region !== 'error') {
setValidation((prevValidations) => ({
...prevValidations,
region: 'warning',
}));
}
avitova marked this conversation as resolved.
Show resolved Hide resolved
} else {
setValidation((prevValidations) => ({
...prevValidations,
sources: chosenSource ? 'success' : 'default',
region: 'default',
}));
}
}, [missingPermissions]);

return (
<Form>
<Title ouiaId="account_custom_title" headingLevel="h1" size="xl">
Account and customizations | Amazon
</Title>
<FormGroup
label="Select account"
validated={validations.sources}
helperTextInvalid="Please pick a value"
isRequired
fieldId="aws-select-source"
>
{(missingPermissions || []).length != 0 && (
<FormAlert>
<Alert isExpandable isInline variant="warning" title={`Launch might fail due to missing permissions.`}>
<>
<p>
Check if <a href="https://console.aws.amazon.com/iam/">policies</a> in your {image.provider} account for the selected region are set
as{' '}
<a href="https://github.com/RHEnVision/provisioning-backend/blob/main/docs/configure-amazon-role.md#service-account-policy">
Copy link
Member

Choose a reason for hiding this comment

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

I guess it would be better to link to formal red hat docs, not dev docs

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree. We do not have the list in the RedHat docs, though. 🤔 And it is one more place we'd have to change in case of changed expected policy. We could use this link but that is quite a lot of steps to only access needed permission list.

Copy link
Member

Choose a reason for hiding this comment

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

Could we ... in theory... have the permission list extracted to a file in the backend and load it in code here on the build time - or figure out how to load it to documentation on build time?

We can also play with it as next step tho. I'd either link Sources directly with "Make sure you're following the steps while creating source", or link the official sources documentation with similar message 🤷

Copy link
Member

Choose a reason for hiding this comment

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

Linking to developer docs is not terrible, just not profesional, but given we are explaining the process quite deeply there I'm mostly worried customers might get confused by all the info there :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I am not sure how we'd want to show the long list here. The link seems like the only sensible option, but I am open if you see a way to show it. We could have one more expandable section with the required permissions if that makes sense.

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking of Popup, but I agree it's not great, let's just link the official doc, for now. Or split up the developer docs to User and Maintainer oriented docs 🤔 but honestly I think the best way is to link the official docs and as follow-up figure out how to pull the information there directly from our repo

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you suggest we hardcode the list of permissions to the official docs for now? Yeah, we could do that, and add it to the [https://github.com/RHEnVision/provisioning-backend/blob/main/CONTRIBUTING.md#basic-guidelines-for-code-contributions](list of places to change).
At the moment, we need to load the permission list to:

  • Go file
  • Javascript file
  • yaml
  • markdown

And we can try to eliminate some of these. 🤔

Copy link
Member

@ezr-ondrej ezr-ondrej Sep 12, 2023

Choose a reason for hiding this comment

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

I'm suggesting to split the IAM policy: RHEnVision/provisioning-backend#682 and then include it on build time here.

But I'm more then happy to leave it to a follow-up :)

Copy link
Member

Choose a reason for hiding this comment

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

Contributing guide is perfect first step for sure :)

our documentation
</a>{' '}
recommends. Following permissions might be missing:
</p>
<p>{(missingPermissions || []).join(', ')}</p>
</>
</Alert>
</FormAlert>
)}
<FormGroup label="Select account" validated="warning" helperTextInvalid="Please pick a value" isRequired fieldId="aws-select-source">
<SourcesSelect
image={image}
setValidation={(validation) =>
Expand All @@ -54,6 +99,7 @@ const AccountCustomizationsAWS = ({ setStepValidated, image }) => {
sources: validation,
}))
}
validated={validations.sources}
/>
</FormGroup>
<FormGroup
Expand All @@ -76,7 +122,13 @@ const AccountCustomizationsAWS = ({ setStepValidated, image }) => {
</Popover>
}
>
<RegionsSelect provider={AWS_PROVIDER} onChange={onRegionChange} composeID={image.id} currentRegion={chosenRegion} />
<RegionsSelect
provider={AWS_PROVIDER}
onChange={onRegionChange}
composeID={image.id}
currentRegion={chosenRegion}
validated={validations.region}
/>
</FormGroup>
<FormGroup
label="Select instance type"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ const AccountCustomizationsAzure = ({ setStepValidated, image }) => {
sources: validation,
}))
}
validated={validations.sources}
/>
</FormGroup>
<FormGroup
Expand All @@ -75,7 +76,13 @@ const AccountCustomizationsAzure = ({ setStepValidated, image }) => {
</Popover>
}
>
<RegionsSelect provider={AZURE_PROVIDER} currentRegion={wizardContext.chosenRegion} onChange={onRegionChange} composeID={image.id} />
<RegionsSelect
provider={AZURE_PROVIDER}
currentRegion={wizardContext.chosenRegion}
onChange={onRegionChange}
composeID={image.id}
validated={'default'}
avitova marked this conversation as resolved.
Show resolved Hide resolved
/>
</FormGroup>
<FormGroup
label="Select instance size"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ const AccountCustomizationsGCP = ({ setStepValidated, image }) => {
sources: validation,
}))
}
validated={validations.sources}
/>
</FormGroup>
<FormGroup
Expand All @@ -76,7 +77,13 @@ const AccountCustomizationsGCP = ({ setStepValidated, image }) => {
</Popover>
}
>
<RegionsSelect provider={GCP_PROVIDER} onChange={onRegionChange} composeID={image.id} currentRegion={wizardContext.chosenRegion} />
<RegionsSelect
provider={GCP_PROVIDER}
onChange={onRegionChange}
composeID={image.id}
currentRegion={wizardContext.chosenRegion}
validated={'default'}
avitova marked this conversation as resolved.
Show resolved Hide resolved
/>
</FormGroup>
<FormGroup
label="Select machine type"
Expand Down
4 changes: 3 additions & 1 deletion src/Components/RegionsSelect/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import { IMAGE_REGIONS_KEY } from '../../API/queryKeys';
import { fetchImageClones, fetchImageCloneStatus } from '../../API';
import { defaultRegionByProvider } from '../Common/helpers';

const RegionsSelect = ({ provider, currentRegion, composeID, onChange }) => {
const RegionsSelect = ({ provider, currentRegion, composeID, onChange, validated }) => {
const [isOpen, setIsOpen] = React.useState(false);

const {
Expand Down Expand Up @@ -72,6 +72,7 @@ const RegionsSelect = ({ provider, currentRegion, composeID, onChange }) => {
onToggle={onToggle}
onSelect={onSelect}
isDisabled={!MULTIPLE_REGION_SUPPORT.includes(provider)}
validated={validated}
>
{images.map(({ id, region }) => (
<SelectOption aria-label="Region item" key={id} value={region} />
Expand All @@ -85,6 +86,7 @@ RegionsSelect.propTypes = {
composeID: PropTypes.string.isRequired,
onChange: PropTypes.func.isRequired,
currentRegion: PropTypes.string,
validated: PropTypes.string.isRequired,
};

export default RegionsSelect;
16 changes: 12 additions & 4 deletions src/Components/SourcesSelect/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import { imageProps } from '../ProvisioningWizard/helpers';
import { useSourcesForImage } from '../Common/Hooks/sources';
import { useWizardContext } from '../Common/WizardContext';

const SourcesSelect = ({ setValidation, image }) => {
const SourcesSelect = ({ setValidation, image, validated }) => {
const [{ chosenSource }, setWizardContext] = useWizardContext();
const [isOpen, setIsOpen] = React.useState(false);
const [selected, setSelected] = React.useState(null);
Expand All @@ -19,14 +19,18 @@ const SourcesSelect = ({ setValidation, image }) => {
onSuccess: (sources) => {
if (chosenSource) {
setSelected(selectObject(chosenSource, sources.find((source) => source.id === chosenSource).name));
setValidation('success');
if (validated !== 'warning') {
setValidation('success');
}
} else if (sources.length === 1) {
setSelected(selectObject(sources[0].id, sources[0].name));
setWizardContext((prevState) => ({
...prevState,
chosenSource: sources[0].id,
}));
setValidation('success');
if (validated !== 'warning') {
setValidation('success');
}
}
},
});
Expand All @@ -42,7 +46,9 @@ const SourcesSelect = ({ setValidation, image }) => {
...prevState,
chosenSource: selection.id,
}));
setValidation('success');
if (validated !== 'warning') {
setValidation('success');
}
}
setIsOpen(false);
};
Expand Down Expand Up @@ -76,6 +82,7 @@ const SourcesSelect = ({ setValidation, image }) => {
onSelect={onSelect}
placeholderText="Select account"
aria-label="Select account"
validated={validated === 'error' || validated === 'warning' ? validated : 'default'}
>
{sources && selectItemsMapper()}
</Select>
Expand All @@ -85,6 +92,7 @@ const SourcesSelect = ({ setValidation, image }) => {
SourcesSelect.propTypes = {
setValidation: PropTypes.func.isRequired,
image: imageProps,
validated: PropTypes.string.isRequired,
};

export default SourcesSelect;
Loading