Skip to content

Commit

Permalink
Show only the allowed storage interfaces on Disk-modal
Browse files Browse the repository at this point in the history
Fixes: CNV-2914

Enforce the validations from common and user templates
so the user can choose only from the allowed values in the Disk-modal
at the storage tab.

Depends on: openshift#3909

Signed-off-by: Ido Rosenzwig <irosenzw@redhat.com>
  • Loading branch information
irosenzw committed Jan 23, 2020
1 parent e6e3fed commit 11960d5
Show file tree
Hide file tree
Showing 10 changed files with 121 additions and 25 deletions.
Expand Up @@ -8,6 +8,8 @@ import { getStoragesWithWrappers } from '../../selectors/selectors';
import { iGetStorages } from '../../selectors/immutable/storage';
import { iGetProvisionSource } from '../../selectors/immutable/vm-settings';
import { ProvisionSource } from '../../../../constants/vm/provision-source';
import { TemplateValidations } from '../../../../utils/validations/template/template-validations';
import { getTemplateValidation } from '../../selectors/template';

export const validateStorages = (options: UpdateOptions) => {
const { id, prevState, dispatch, getState } = options;
Expand All @@ -28,6 +30,8 @@ export const validateStorages = (options: UpdateOptions) => {
}

const storages = getStoragesWithWrappers(state, id);
const templateValidation = getTemplateValidation(state, id);
const allowedBusses = (templateValidation || new TemplateValidations()).getAllowedBusses();

const validatedStorages = storages.map(
({
Expand Down Expand Up @@ -56,6 +60,7 @@ export const validateStorages = (options: UpdateOptions) => {
{
usedDiskNames,
usedPVCNames,
allowedBusses,
},
),
};
Expand Down
Expand Up @@ -44,7 +44,7 @@ import { vmSettingsOrder } from '../initial-state/vm-settings-tab-initial-state'
import { TemplateValidations } from '../../../../utils/validations/template/template-validations';
import { combineIntegerValidationResults } from '../../../../utils/validations/template/utils';
import { getValidationUpdate } from './utils';
import { getTemplateValidations } from './utils/templates-validations';
import { getTemplateValidations } from '../../selectors/template';

const validateVm: VmSettingsValidator = (field, options) => {
const { getState, id } = options;
Expand Down Expand Up @@ -103,8 +103,10 @@ const memoryValidation: VmSettingsValidator = (field, options): ValidationObject
if (memValueGB == null || memValueGB === '') {
return null;
}
const { id, getState } = options;
const state = getState();
const memValueBytes = memValueGB * 1024 ** 3;
const validations = getTemplateValidations(options);
const validations = getTemplateValidations(state, id);
if (validations.length === 0) {
validations.push(new TemplateValidations()); // add empty validation for positive integer if it is missing one
}
Expand Down
@@ -1,26 +1,19 @@
import { UpdateOptions } from '../../types';
import {
iGetVmSettingAttribute,
iGetVmSettingValue,
} from '../../../selectors/immutable/vm-settings';
import { iGetLoadedCommonData } from '../../../selectors/immutable/selectors';
import { VMSettingsField, VMWizardProps } from '../../../types';
import { iGetTemplateValidations } from '../../../../../selectors/immutable/template/selectors';
import { iGetTemplateValidations } from '../../../selectors/immutable/template/selectors';
import { TemplateValidations } from '../../../utils/validations/template/template-validations';
import {
iGetRelevantTemplate,
iGetRelevantTemplates,
} from '../../../../../selectors/immutable/template/combined';
import { TemplateValidations } from '../../../../../utils/validations/template/template-validations';
} from '../../../selectors/immutable/template/combined';
import { VMSettingsField, VMWizardProps } from '../types';
import { iGetLoadedCommonData } from './immutable/selectors';
import { iGetVmSettingAttribute, iGetVmSettingValue } from './immutable/vm-settings';

const getValidationsFromTemplates = (templates): TemplateValidations[] =>
templates.map(
(relevantTemplate) => new TemplateValidations(iGetTemplateValidations(relevantTemplate)),
);

export const getTemplateValidations = (options: UpdateOptions): TemplateValidations[] => {
const { getState, id } = options;
const state = getState();

export const getTemplateValidations = (state, id: string): TemplateValidations[] => {
const userTemplateName = iGetVmSettingValue(state, id, VMSettingsField.USER_TEMPLATE);
const os = iGetVmSettingAttribute(state, id, VMSettingsField.OPERATING_SYSTEM);
const flavor = iGetVmSettingAttribute(state, id, VMSettingsField.FLAVOR);
Expand Down Expand Up @@ -54,3 +47,14 @@ export const getTemplateValidations = (options: UpdateOptions): TemplateValidati

return getValidationsFromTemplates(relevantTemplates.toArray());
};

export const getTemplateValidation = (state, id: string): TemplateValidations => {
const templateValidations = getTemplateValidations(state, id);
if (templateValidations && templateValidations.length > 0) {
templateValidations.length > 1 &&
console.warn('WARNING: getTemplateValidation: multiple template validations detected!');
return templateValidations[0];
}

return null;
};
Expand Up @@ -24,6 +24,9 @@ import { DataVolumeWrapper } from '../../../../k8s/wrapper/vm/data-volume-wrappe
import { DiskModal } from '../../../modals/disk-modal';
import { VM_TEMPLATE_NAME_PARAMETER } from '../../../../constants/vm-templates';
import { PersistentVolumeClaimWrapper } from '../../../../k8s/wrapper/vm/persistent-volume-claim-wrapper';
import { TemplateValidations } from '../../../../utils/validations/template/template-validations';
import { DiskBus } from '../../../../constants/vm/storage/disk-bus';
import { getTemplateValidation } from '../../selectors/template';

const VMWizardStorageModal: React.FC<VMWizardStorageModalProps> = (props) => {
const {
Expand All @@ -34,6 +37,7 @@ const VMWizardStorageModal: React.FC<VMWizardStorageModalProps> = (props) => {
useProjects,
addUpdateStorage,
storages,
templateValidations,
...restProps
} = props;
const {
Expand Down Expand Up @@ -80,6 +84,10 @@ const VMWizardStorageModal: React.FC<VMWizardStorageModalProps> = (props) => {
},
];

const allowedBusses: Set<DiskBus> = (
templateValidations || new TemplateValidations()
).getAllowedBusses();

return (
<Firehose resources={resources}>
<DiskModal
Expand All @@ -90,6 +98,7 @@ const VMWizardStorageModal: React.FC<VMWizardStorageModalProps> = (props) => {
onNamespaceChanged={(n) => setNamespace(n)}
usedDiskNames={usedDiskNames}
usedPVCNames={usedPVCNames}
allowedBusses={allowedBusses}
disk={diskWrapper}
volume={volumeWrapper}
dataVolume={dataVolumeWrapper}
Expand Down Expand Up @@ -139,6 +148,7 @@ type VMWizardStorageModalProps = ModalComponentProps & {
isCreateTemplate: boolean;
storages: VMWizardStorageWithWrappers[];
addUpdateStorage: (storage: VMWizardStorage) => void;
templateValidations: TemplateValidations;
};

const stateToProps = (state, { wizardReduxID }) => {
Expand All @@ -148,6 +158,7 @@ const stateToProps = (state, { wizardReduxID }) => {
namespace: iGetCommonData(state, wizardReduxID, VMWizardProps.activeNamespace),
isCreateTemplate: iGetCommonData(state, wizardReduxID, VMWizardProps.isCreateTemplate),
storages: getStoragesWithWrappers(state, wizardReduxID),
templateValidations: getTemplateValidation(state, wizardReduxID),
};
};

Expand Down
Expand Up @@ -76,6 +76,7 @@ export const VmWizardStorageRow: React.FC<VMWizardNicRowProps> = ({
validation={{
name: validations.name || validations.url || validations.container || validations.pvc,
size: validations.size,
diskInterface: validations.diskInterface,
}}
columnClasses={columnClasses}
index={index}
Expand Down
Expand Up @@ -69,11 +69,13 @@ export const DiskModal = withHandlePromise((props: DiskModalProps) => {
handlePromise,
close,
cancel,
allowedBusses = new Set(DiskBus.getAll()),
} = props;
const asId = prefixedID.bind(null, 'disk');
const disk = props.disk || DiskWrapper.EMPTY;
const volume = props.volume || VolumeWrapper.EMPTY;
const dataVolume = props.dataVolume || DataVolumeWrapper.EMPTY;
const validAllowedBusses = allowedBusses || new Set(DiskBus.getAll());

const combinedDisk = new CombinedDisk({
diskWrapper: disk,
Expand Down Expand Up @@ -102,7 +104,12 @@ export const DiskModal = withHandlePromise((props: DiskModalProps) => {
disk.getName() || getSequenceName('disk', usedDiskNames),
);
const [bus, setBus] = React.useState<DiskBus>(
disk.getDiskBus() || (isEditing ? null : DiskBus.VIRTIO),
disk.getDiskBus() ||
(isEditing
? null
: validAllowedBusses.has(DiskBus.VIRTIO)
? DiskBus.VIRTIO
: [...validAllowedBusses][0]),
);
const [storageClassName, setStorageClassName] = React.useState<string>(
combinedDisk.getStorageClassName(),
Expand Down Expand Up @@ -170,13 +177,15 @@ export const DiskModal = withHandlePromise((props: DiskModalProps) => {
size: sizeValidation,
container: containerValidation,
pvc: pvcValidation,
diskInterface: busValidation,
url: urlValidation,
},
isValid,
hasAllRequiredFilled,
} = validateDisk(resultDisk, resultVolume, resultDataVolume, resultPersistentVolumeClaim, {
usedDiskNames,
usedPVCNames,
allowedBusses,
});

const [showUIError, setShowUIError] = useShowErrorToggler(false, isValid, isValid);
Expand Down Expand Up @@ -370,19 +379,34 @@ export const DiskModal = withHandlePromise((props: DiskModalProps) => {
/>
</FormRow>
)}
<FormRow title="Interface" fieldId={asId('interface')} isRequired>
<FormRow
title="Interface"
fieldId={asId('interface')}
isRequired
validation={busValidation}
>
<FormSelect
onChange={React.useCallback((diskBus) => setBus(DiskBus.fromString(diskBus)), [
setBus,
])}
isValid={!isValidationError(busValidation)}
value={asFormSelectValue(bus)}
id={asId('interface')}
isDisabled={inProgress}
>
<FormSelectPlaceholderOption isDisabled placeholder="--- Select Interface ---" />
{DiskBus.getAll().map((b) => (
<FormSelectOption key={b.getValue()} value={b.getValue()} label={b.toString()} />
))}
{!validAllowedBusses.has(bus) && (
<FormSelectOption
key={bus.getValue()}
value={bus.getValue()}
label={`${bus.toString()} --- Invalid ---`}
/>
)}
{[...validAllowedBusses].map((b) =>
validAllowedBusses.has(b) ? (
<FormSelectOption key={b.getValue()} value={b.getValue()} label={b.toString()} />
) : null,
)}
</FormSelect>
</FormRow>
{source.requiresStorageClass() && (
Expand Down Expand Up @@ -444,6 +468,7 @@ export type DiskModalProps = {
vmNamespace: string;
namespace: string;
onNamespaceChanged: (namespace: string) => void;
allowedBusses?: Set<DiskBus>;
usedDiskNames: Set<string>;
usedPVCNames: Set<string>;
} & ModalComponentProps &
Expand Down
14 changes: 14 additions & 0 deletions frontend/packages/kubevirt-plugin/src/utils/validations/common.ts
Expand Up @@ -7,9 +7,11 @@ import {
validateEmptyValue,
ValidationErrorType,
ValidationObject,
joinGrammaticallyListOfItems,
} from '@console/shared';
import { parseURL } from '../url';
import { END_WHITESPACE_ERROR, START_WHITESPACE_ERROR, URL_INVALID_ERROR } from './strings';
import { DiskBus } from '../../constants/vm/storage/disk-bus';

export const isValidationError = (validationObject: ValidationObject) =>
!!validationObject && validationObject.type === ValidationErrorType.Error;
Expand Down Expand Up @@ -81,3 +83,15 @@ export const validateURL = (

return parseURL(value) ? null : asValidationObject(addMissingSubject(URL_INVALID_ERROR, subject));
};

export const validateBus = (value: DiskBus, allowedBusses: Set<DiskBus>): ValidationObject => {
if (allowedBusses && !allowedBusses.has(value)) {
return asValidationObject(
`Invalid interface type. Valid types are: ${joinGrammaticallyListOfItems(
[...allowedBusses].map((b) => b.toString()),
)}`,
ValidationErrorType.Error,
);
}
return null;
};
Expand Up @@ -44,7 +44,7 @@ export class MemoryIntervalValidationResult extends IntervalValidationResult {
}`;
}
if (Number.isFinite(this.max)) {
return `Memory ${verb} be ${this.isMaxInclusive ? 'at most' : 'bellow'} ${
return `Memory ${verb} be ${this.isMaxInclusive ? 'at most' : 'below'} ${
humanizeBinaryBytes(this.max).string
}`;
}
Expand Down
@@ -1,7 +1,7 @@
/* eslint-disable lines-between-class-members */
import * as _ from 'lodash';
import { ValidationErrorType } from '@console/shared/src';
import { ValueEnum } from '../../../constants';
import { ValueEnum, DiskBus } from '../../../constants';
import { CommonTemplatesValidation } from '../../../types/template';
import {
IntervalValidationResult,
Expand All @@ -14,6 +14,7 @@ export class ValidationJSONPath extends ValueEnum<string> {
static readonly MEMORY = new ValidationJSONPath(
'jsonpath::.spec.domain.resources.requests.memory',
);
static readonly BUS = new ValidationJSONPath('jsonpath::.spec.domain.devices.disks[*].disk.bus');
}

export class TemplateValidations {
Expand Down Expand Up @@ -110,6 +111,28 @@ export class TemplateValidations {
});
};

getAllowedBusses = (): Set<DiskBus> => {
const allowedBusses = this.getAllowedEnumValues(
ValidationJSONPath.BUS,
ValidationErrorType.Error,
).map(DiskBus.fromString);

return new Set(allowedBusses.length === 0 ? DiskBus.getAll() : allowedBusses);
};

private getAllowedEnumValues = (
jsonPath: ValidationJSONPath,
type: ValidationErrorType,
): string[] => {
// Empty array means all values are allowed
const relevantValidations = this.getRelevantValidations(jsonPath, type);

return relevantValidations.reduce(
(result: string[], validation: CommonTemplatesValidation) => result.concat(validation.values),
[],
);
};

private getRelevantValidations = (jsonPath: ValidationJSONPath, type: ValidationErrorType) => {
return this.validations.filter(
(validation: CommonTemplatesValidation) =>
Expand Down
15 changes: 13 additions & 2 deletions frontend/packages/kubevirt-plugin/src/utils/validations/vm/disk.ts
Expand Up @@ -6,14 +6,16 @@ import {
ValidationErrorType,
ValidationObject,
} from '@console/shared';
import { validateTrim, validateURL } from '../common';
import { validateTrim, validateURL, validateBus } from '../common';
import { DiskWrapper } from '../../../k8s/wrapper/vm/disk-wrapper';
import { VolumeWrapper } from '../../../k8s/wrapper/vm/volume-wrapper';
import { DataVolumeWrapper } from '../../../k8s/wrapper/vm/data-volume-wrapper';
import { POSITIVE_SIZE_ERROR } from '../strings';
import { StorageUISource } from '../../../components/modals/disk-modal/storage-ui-source';
import { CombinedDisk } from '../../../k8s/wrapper/vm/combined-disk';
import { PersistentVolumeClaimWrapper } from '../../../k8s/wrapper/vm/persistent-volume-claim-wrapper';
import { DiskBus } from '../../../constants/vm/storage/disk-bus';
import { DiskType } from '../../.../../../constants/vm/storage/disk-type';

const validateDiskName = (name: string, usedDiskNames: Set<string>): ValidationObject => {
let validation = validateDNS1123SubdomainValue(name);
Expand Down Expand Up @@ -51,16 +53,19 @@ export const validateDisk = (
{
usedDiskNames,
usedPVCNames,
allowedBusses,
}: {
usedDiskNames?: Set<string>;
usedPVCNames?: Set<string>;
allowedBusses: Set<DiskBus>;
},
): UIDiskValidation => {
const validations = {
name: validateDiskName(disk && disk.getName(), usedDiskNames),
size: null,
url: null,
container: null,
diskInterface: null,
pvc: null,
};
let hasAllRequiredFilled = disk && disk.getName();
Expand Down Expand Up @@ -129,8 +134,13 @@ export const validateDisk = (
addRequired(pvcName);
validations.pvc = validatePVCName(pvcName, usedPVCNames);
}
}

// TODO: implement CDROM disk bus validation
if (disk.getType() === DiskType.DISK) {
addRequired(allowedBusses.has(disk.getDiskBus()));
validations.diskInterface = validateBus(disk.getDiskBus(), allowedBusses);
}
}
return {
validations,
hasAllRequiredFilled: !!hasAllRequiredFilled,
Expand All @@ -144,6 +154,7 @@ export type UIDiskValidation = {
size?: ValidationObject;
url?: ValidationObject;
container?: ValidationObject;
diskInterface?: ValidationObject;
pvc?: ValidationObject;
};
isValid: boolean;
Expand Down

0 comments on commit 11960d5

Please sign in to comment.