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 Encryption workflow for storage cluster creation #7062

Merged
merged 1 commit into from Nov 19, 2020

Conversation

a2batic
Copy link
Contributor

@a2batic a2batic commented Nov 1, 2020

@openshift-ci-robot openshift-ci-robot added component/ceph Related to ceph-storage-plugin approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Nov 1, 2020
Comment on lines 39 to 59
<>
<FormGroup
fieldId="kms-service-name"
label="Service Name"
className="co-m-pane__form ocs-install-encryption__form-body"
isRequired
>
<TextInput
value={serviceName}
onChange={getServiceName}
isRequired
type="text"
id="kms-service-name"
name="kms-service-name"
/>
</FormGroup>
</>
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
<>
<FormGroup
fieldId="kms-service-name"
label="Service Name"
className="co-m-pane__form ocs-install-encryption__form-body"
isRequired
>
<TextInput
value={serviceName}
onChange={getServiceName}
isRequired
type="text"
id="kms-service-name"
name="kms-service-name"
/>
</FormGroup>
</>
<FormGroup
fieldId="kms-service-name"
label="Service Name"
className="co-m-pane__form ocs-install-encryption__form-body"
isRequired
>
<TextInput
value={serviceName}
onChange={getServiceName}
isRequired
type="text"
id="kms-service-name"
name="kms-service-name"
/>
</FormGroup>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment on lines 227 to 230
case CreateStepsSC.CONFIGURE:
return !state.hasEncryptionHandled;
case CreateStepsSC.REVIEWANDCREATE:
return state.nodes.length < MINIMUM_NODES || !getName(state.storageClass);
Copy link
Contributor

Choose a reason for hiding this comment

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

We want to disable the create storage cluster step instead.
And for configure step, we dont need to disable but show a warning and also reflect the same warning on review step.
This is the current convention for wizard flow.

Suggested change
case CreateStepsSC.CONFIGURE:
return !state.hasEncryptionHandled;
case CreateStepsSC.REVIEWANDCREATE:
return state.nodes.length < MINIMUM_NODES || !getName(state.storageClass);
case CreateStepsSC.REVIEWANDCREATE:
return state.nodes.length < MINIMUM_NODES || !getName(state.storageClass) || !state.hasEncryptionHandled;

match: RouterMatch<{ appName: string; ns: string }>;
setIsNewSCToBeCreated?: React.Dispatch<boolean>;
setHasNoProvSC?: React.Dispatch<boolean>;
};
Copy link
Contributor

Choose a reason for hiding this comment

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

We dont want to delete this file for now, in order to allow backporting of bugs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ack

Comment on lines 124 to 129
<Alert
variant="danger"
className="co-alert ocs-install-encryption__form-alert"
title="Select at least 1 encryption level or disable encryption"
isInline
/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Use ValidationMessage for consistency.

export const ValidationMessage: React.FC<ValidationMessageProps> = ({ className, validation }) => {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ack

Comment on lines 60 to 61
enableNext: state.hasEncryptionHandled,
},
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
enableNext: state.hasEncryptionHandled,
},
},

Same here, we want to disable storage cluster creation instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

And put relevant validations/warnings on review and the corresponding step.

className="ocs-install-encryption__form-checkbox"
/>
<Checkbox
id="pv-encryption"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we calling it pv encryption when on UI it represents the storage class encryption ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ya, internally its encryption PVs, so I kept PV encryption. Will change it to storage class encyption for better understanding.

setDispatch(ActionType.SET_ENCRYPTION_VALID, true);
}
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [clusterWideChecked, pvChecked, encryptionChecked]);
Copy link
Contributor

Choose a reason for hiding this comment

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

I cannot follow , why we need to do this ? can you elaborate ?

import { KMSConfigure } from '../../kms-config/kms-config';
import './install-wizard.scss';

const PVEncryptionLabel: React.FC = () => (
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
const PVEncryptionLabel: React.FC = () => (
const StorageClassEncryptionLabel: React.FC = () => (


const toggleEncryption = (checked: boolean) =>
dispatch({ type: ActionType.SET_ENABLE_ENCRYPTION, payload: checked });

return (
<Form>
Copy link
Contributor

Choose a reason for hiding this comment

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

if you are setting isRequired then either you put manual validation or set browser defaults by setting following:

Suggested change
<Form>
<Form noValidate={false}>

fieldId="kms-service-name"
label="Service Name"
className="co-m-pane__form ocs-install-encryption__form-body"
isRequired
Copy link
Contributor

Choose a reason for hiding this comment

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

Please set a custom validation since PF Form disables vanilla html or enable the latter by setting noValidate=false

@a2batic a2batic force-pushed the cluster-encryption branch 2 times, most recently from 0515e44 to 56098b4 Compare November 5, 2020 09:26
@a2batic a2batic mentioned this pull request Nov 8, 2020
Signed-off-by: Kanika Murarka <kmurarka@redhat.com>
Comment on lines +30 to +34
if (!kms.name) {
setDispatch(ActionType.SET_KMS_ENCRYPTION, { ...kms, hasHandled: false }, mode, dispatch);
} else {
setDispatch(ActionType.SET_KMS_ENCRYPTION, { ...kms, hasHandled: true }, mode, dispatch);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this would suffice.

Suggested change
if (!kms.name) {
setDispatch(ActionType.SET_KMS_ENCRYPTION, { ...kms, hasHandled: false }, mode, dispatch);
} else {
setDispatch(ActionType.SET_KMS_ENCRYPTION, { ...kms, hasHandled: true }, mode, dispatch);
}
setDispatch(ActionType.SET_KMS_ENCRYPTION, { ...kms, hasHandled: !kms.name }, mode, dispatch);

<TextInput
value={kms.name}
onChange={getServiceName}
isRequired
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Shift isRequired at the end.

@a2batic
Copy link
Contributor Author

a2batic commented Nov 18, 2020

@cloudbehl ack, will add these changes in KMS related PR #7153

Comment on lines +29 to +36
React.useEffect(() => {
if (!kms.name) {
setDispatch(ActionType.SET_KMS_ENCRYPTION, { ...kms, hasHandled: false }, mode, dispatch);
} else {
setDispatch(ActionType.SET_KMS_ENCRYPTION, { ...kms, hasHandled: true }, mode, dispatch);
}
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [kms.name]);
Copy link
Contributor

Choose a reason for hiding this comment

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

If this needs change on "kms name" , then move it to the "onChnage" handler for kms name

<FormGroup
fieldId="kms-service-name"
label="Service Name"
className="co-m-pane__form ocs-install-encryption__form-body"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:

Suggested change
className="co-m-pane__form ocs-install-encryption__form-body"
className="co-m-pane__form ocs-install-encryption__form-group"

@afreen23
Copy link
Contributor

/lgtm

okay to take #7062 (comment)
in the other PR

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 19, 2020
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: a2batic, afreen23

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@a2batic
Copy link
Contributor Author

a2batic commented Nov 19, 2020

/retest

@openshift-merge-robot openshift-merge-robot merged commit bd3450e into openshift:master Nov 19, 2020
@spadgett spadgett added this to the v4.7 milestone Nov 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/ceph Related to ceph-storage-plugin lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants