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

Kms cluster encryption #7153

Merged

Conversation

a2batic
Copy link
Contributor

@a2batic a2batic commented Nov 8, 2020

Merged after: #7062
screencapture-localhost-9000-k8s-ns-openshift-storage-clusterserviceversions-ocs-operator-v4-6-0-ocs-openshift-io-v1-StorageCluster-new-2020-11-18-17_17_46
Screenshot from 2020-11-18 17-18-01

@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 8, 2020
@a2batic a2batic marked this pull request as draft November 8, 2020 20:06
@openshift-ci-robot openshift-ci-robot added the component/ceph Related to ceph-storage-plugin label Nov 8, 2020
@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 8, 2020
@a2batic a2batic marked this pull request as ready for review November 18, 2020 13:22
@a2batic a2batic changed the title [WIP] Kms cluster encryption Kms cluster encryption Nov 18, 2020
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 18, 2020
@a2batic a2batic force-pushed the kms-cluster-encryption branch 3 times, most recently from cb38464 to 6dcb79e Compare November 23, 2020 20:21
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 23, 2020
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 23, 2020
Copy link
Contributor

@afreen23 afreen23 left a comment

Choose a reason for hiding this comment

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

High level review for now

Comment on lines 16 to 22
import { KMSProviders } from '../../constants/ocs-install';
import './kms-config.scss';
import * as _ from 'lodash';
import { advancedKMSModal } from '../modals/advanced-kms-modal/advanced-kms-modal';
import { setDispatch, parseURL } from './utils';
import { PencilAltIcon } from '@patternfly/react-icons';
import { global_palette_blue_300 as blueInfoColor } from '@patternfly/react-tokens/dist/js/global_palette_blue_300';
Copy link
Contributor

Choose a reason for hiding this comment

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

Fix import hierarchy

<p>Connected to external key management service: {kms.name}</p>
{encryption.advanced && (
<p className="ocs-install-wizard__review-encryption">
Connect to external key management service: {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.

The KMS connection is done after clicking create, this should be better

Suggested change
Connect to external key management service: {kms.name}
External key management service: {kms.name}

requestData.spec.security = {
kms: {
connectionDetailsConfigMap: 'ocs-vault-connection-details',
tokenSecretName: 'ocs-kms-vault-token',
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer to make ocs-kms-vault-token a constant, since used at multiple places.

Comment on lines 61 to 58
export const maxFileUploadSize = 4000000;
export const fileSizeErrorMsg = 'Maximum file size exceeded. File limit is 4MB.';
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be better to prefix names with kms- to understand what its for ?

@@ -71,7 +77,7 @@ export const CreateInternalCluster: React.FC<CreateInternalClusterProps> = ({ ma
name: 'Configure',
id: CreateStepsSC.CONFIGURE,
component: <Configure state={state} dispatch={dispatch} mode={mode} />,
enableNext: state.encryption.hasHandled && hasConfiguredNetwork,
enableNext: state.encryption.hasHandled && hasConfiguredNetwork && state.kms.hasHandled,
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove any restriction from individual steps, until we push validation based wizard.

Suggested change
enableNext: state.encryption.hasHandled && hasConfiguredNetwork && state.kms.hasHandled,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was decided to have Next button blocked wrt validations in the wizard.
@cloudbehl @yuvalgalanti

Copy link
Contributor

Choose a reason for hiding this comment

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

yes that's what was planned.

Copy link
Contributor

Choose a reason for hiding this comment

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

We have navs still clickable, I thnk we should block them as well.
(Possibly in another PR)

Choose a reason for hiding this comment

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

Connect to external key management service: {kms.name}
</p>
)}
<p>Encryption Level: {getEncryptionType(encryption)}</p>
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
<p>Encryption Level: {getEncryptionType(encryption)}</p>
<p>Encryption Level: {getEncryptionLevel(encryption)}</p>

<p className="ocs-install-wizard__review-encryption">Enable Encryption</p>
{encryption.advanced && kms.hasHandled && (
<p className="ocs-install-wizard__review-encryption">
Connect to external key management service: {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.

Suggested change
Connect to external key management service: {kms.name}
External key management service: {kms.name}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As per UX, its Connected since its connected after Create button click, so changed it to Connect.

<p className="ocs-install-wizard__review-encryption">Enable Encryption</p>
{encryption.advanced && kms.hasHandled && (
<p>Connected to external key management service: {kms.name}</p>
{encryption.advanced && (
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not kms.hasHandled here ?

              {encryption.advanced && (

Copy link
Contributor Author

@a2batic a2batic Nov 29, 2020

Choose a reason for hiding this comment

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

I have removed at the other place too, it's not required.


if (!checked) {
kmsObject = initialState.kms;
setDispatch(ActionType.SET_KMS_ENCRYPTION, kmsObject, 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 would prefer to call it following, just to avoid confusion with root state dispatcher

Suggested change
setDispatch(ActionType.SET_KMS_ENCRYPTION, kmsObject, mode, dispatch);
setKms(ActionType.SET_KMS_ENCRYPTION, kmsObject, mode, dispatch);

&__form-body {
padding: var(--pf-global--spacer--md) 0 !important;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

missing new line

Comment on lines 28 to 38
const setServiceName = (name: string) => {
setDispatch(ActionType.SET_KMS_ENCRYPTION, { ...kms, name }, 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.

Suggested change
const setServiceName = (name: string) => {
setDispatch(ActionType.SET_KMS_ENCRYPTION, { ...kms, name }, mode, dispatch);
};
const setServiceName = (name: string) => setDispatch(ActionType.SET_KMS_ENCRYPTION, { ...kms, name }, mode, dispatch);

Comment on lines 48 to 86
const validateAddressMessage = () => {
if (kms.address === '') {
return 'This is a required field';
}
return 'Please enter a URL';
};
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 validateAddressMessage = () => {
if (kms.address === '') {
return 'This is a required field';
}
return 'Please enter a URL';
};
const validateAddressMessage = () => (kms.address === '') ?
'This is a required field' : 'Please enter a URL';

Comment on lines 55 to 59
const validatePort = () => {
return _.isNaN(Number(kms.port)) || kms.port < 0 || !kms.port
? ValidatedOptions.error
: ValidatedOptions.default;
};
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 validatePort = () => {
return _.isNaN(Number(kms.port)) || kms.port < 0 || !kms.port
? ValidatedOptions.error
: ValidatedOptions.default;
};
const validatePort = () => _.isNaN(Number(kms.port)) || kms.port < 0 || !kms.port
? ValidatedOptions.error
: ValidatedOptions.default;

Comment on lines 84 to 96
const openAdvancedModal = () => {
return advancedKMSModal({
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 openAdvancedModal = () => {
return advancedKMSModal({
const openAdvancedModal = () => advancedKMSModal({

Comment on lines 30 to 31
import { SecretModel } from '@console/internal/models';
import { SecretKind } from '@console/internal/module/k8s/types';
Copy link
Contributor

Choose a reason for hiding this comment

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

Fix import order.

export const AdvancedKMSModal = withHandlePromise((props: AdvancedKMSModalProps) => {
const { close, cancel, errorMessage, inProgress, state, dispatch, mode } = props;
const { kms } = state;
const [backendPath, setBackendPath] = React.useState(kms.backend || '');
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need another state here that is derived from another state? Can't we use dispatch and update the state over there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The advance modal has a Save button, on clicking which the data has to be added in the state, to locally store data, I have added local states.

};

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.

Why not use PF form?

Copy link
Contributor

Choose a reason for hiding this comment

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

Then, we need to use PF form Modal as well. Console Modal's CSS breaks in PF4 form.

@@ -66,7 +66,11 @@
padding-left: var(--pf-global--spacer--lg);

&__form-body {
padding: var(--pf-global--spacer--md);
padding: var(--pf-global--spacer--md) !important;
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 have a comment on why we are overriding using important?

@a2batic a2batic force-pushed the kms-cluster-encryption branch 3 times, most recently from 813c34c to e36ddc2 Compare November 29, 2020 13:55
>
<FormSelect
value={kmsProvider}
onChange={(e) => setKMSProvider(e)}
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
onChange={(e) => setKMSProvider(e)}
onChange={setKMSProvider}

id="kms-provider-name"
name="kms-provider-name"
aria-label="kms-provider-name"
isDisabled
Copy link
Contributor

Choose a reason for hiding this comment

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

we are not setting the isDisabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For 4.7, it will always be disabled.

className="co-m-pane__form ocs-install-encryption__form-body"
className="ocs-install-encryption__form-body"
helperTextInvalid="This is a required field"
validated={kms.name.valid ? ValidatedOptions.default : ValidatedOptions.error}
Copy link
Contributor

Choose a reason for hiding this comment

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

create a generic function and use it for all fields. Like isValid(name)

import { InternalClusterAction } from '../ocs-install/internal-mode/reducer';
import { KMSConfig, KMSConfigMap } from '../ocs-install/types';

export const parseURL = (url: string) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can be put in a shared folder. Will do in next PR.

@a2batic
Copy link
Contributor Author

a2batic commented Dec 1, 2020

@bipuladh will address this comment(#7153 (comment)) with the follow up PR: #7330. Please review the rest.

@a2batic
Copy link
Contributor Author

a2batic commented Dec 1, 2020

@afreen23 @cloudbehl please review.

@a2batic
Copy link
Contributor Author

a2batic commented Dec 1, 2020

/retest

<FormSelect
value={kmsProvider}
onChange={setKMSProvider}
id="kms-provider-name"
Copy link
Contributor

Choose a reason for hiding this comment

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

id needs to match with FormGroup

Suggested change
id="kms-provider-name"
id="kms-provider"

onChange={setAddress}
className="ocs-install-kms__form-address--padding"
type="url"
id="kms-address"
Copy link
Contributor

Choose a reason for hiding this comment

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

here as well.

Suggested change
id="kms-address"
id="kms-service-address"

Comment on lines 141 to 175
<FormGroup
fieldId="kms-service-address-port"
label="Port"
className="ocs-install-kms__form-port ocs-install-encryption__form-body--small-padding"
helperTextInvalid={validatePortMessage()}
validated={isValid(kms.port.valid)}
isRequired
>
<TextInput
value={kms.port.value}
onChange={setAddressPort}
type="text"
id="kms-address-port"
name="kms-address-port"
isRequired
validated={isValid(kms.port.valid)}
/>
</FormGroup>
</div>
<FormGroup
fieldId="kms-service-token"
label="Token"
className="ocs-install-encryption__form-body"
helperTextInvalid="This is a required field"
validated={isValid(kms.token.valid)}
isRequired
>
<TextInput
value={kms.token.value}
onChange={setToken}
type="password"
id="kms-token"
name="kms-token"
isRequired
validated={isValid(kms.token.valid)}
Copy link
Contributor

Choose a reason for hiding this comment

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

Fix id names

}
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [kms.name]);
const serviceName = (name: string) => {
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 serviceName = (name: string) => {
const setServiceName = (name: string) => {

Comment on lines 98 to 120
if (caCertificate && caCertificate !== '') {
kmsAdvanced.caCert = caSecret;
kmsAdvanced.caCertFile = caCertificateFile;
} else {
kmsAdvanced.caCert = null;
kmsAdvanced.caCertFile = '';
}

if (clientCertificate && clientCertificate !== '') {
kmsAdvanced.clientCert = clientCertSecret;
kmsAdvanced.clientCertFile = clientCertificateFile;
} else {
kmsAdvanced.clientCert = null;
kmsAdvanced.clientCertFile = '';
}

if (clientKey && clientCertificate !== '') {
kmsAdvanced.clientKey = clientKeySecret;
kmsAdvanced.clientKeyFile = clientKeyFile;
} else {
kmsAdvanced.clientKey = null;
kmsAdvanced.clientKeyFile = '';
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Does not setting value helps here ?
Because anyways you are setting state. I think you can avoid the guard here

if (encryption.advanced && kms.hasHandled) {
promises.push(...kmsResources(kms));
}
await Promise.all(promises).then(() => k8sCreate(OCSServiceModel, storageCluster));
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand its not the part of PR, but this is a minor :)

Suggested change
await Promise.all(promises).then(() => k8sCreate(OCSServiceModel, storageCluster));
await Promise.all(promises).then(() => k8sCreate(StorageClusterModel, storageCluster));

Copy link
Contributor

Choose a reason for hiding this comment

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

No worries to take in another PR, this needs fix at various places.

};

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.

Then, we need to use PF form Modal as well. Console Modal's CSS breaks in PF4 form.

@@ -15,6 +15,8 @@ export const requestedCapacityTooltip =
'The backing storage requested will be higher as it will factor in the requested capacity, replica factor, and fault tolerant costs associated with the requested capacity.';
export const encryptionTooltip =
'The storage cluster encryption level can be set to include all components under the cluster (including storage class and PVs) or to include only storage class encryption. PV encryption can use an auth token that will be used with the KMS configuration to allow multi-tenancy.';
export const vaultNamespaceTooltip =
'Vault enterprise namespaces are isolated environments that functionally exist as "Vaults within a Vault." They have separate login paths and support creating and managing data isolated to their namespace.';
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
'Vault enterprise namespaces are isolated environments that functionally exist as "Vaults within a Vault." They have separate login paths and support creating and managing data isolated to their namespace.';
'Vault enterprise namespaces are isolated environments that functionally exist as "Vaults within a Vault". They have separate login paths and support creating and managing data isolated to their namespace.';

Comment on lines 144 to 151
if (kmsEnable) {
requestData.spec.encryption = {
...requestData.spec.encryption,
kms: {
enable: true,
},
};
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Just for consistency, can you use Object.assign as well similar to arbiter and network?

@@ -38,6 +39,7 @@ export type InternalClusterAction =
| { type: ActionType.SET_ENCRYPTION; payload: EncryptionType }
// KMS action
| { type: ActionType.SET_KMS_ENCRYPTION; payload: KMSConfig }
| { type: ActionType.CLEAR_KMS_STATE; payload?: KMSConfig }
Copy link
Contributor

Choose a reason for hiding this comment

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

Why payload optional ? Can't we pass empty params to the same action , used to set kms ActionType.SET_KMS_ENCRYPTION ?

};

return (
<Form onSubmit={submit} key="pool-form-modal">
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: pool-form-modal

Suggested change
<Form onSubmit={submit} key="pool-form-modal">
<Form onSubmit={submit} key="">

Comment on lines 57 to 60
export const kmsMaxFileUploadSize = 4000000;
export const kmsFileSizeErrorMsg = 'Maximum file size exceeded. File limit is 4MB.';
export const KMSConfigMapName = 'ocs-kms-connection-details';
export const KMSSecretName = 'ocs-kms-token';
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets be consistent with names.

Suggested change
export const kmsMaxFileUploadSize = 4000000;
export const kmsFileSizeErrorMsg = 'Maximum file size exceeded. File limit is 4MB.';
export const KMSConfigMapName = 'ocs-kms-connection-details';
export const KMSSecretName = 'ocs-kms-token';
export const KMSMaxFileUploadSize = 4000000;
export const KMSFileSizeErrorMsg = 'Maximum file size exceeded. File limit is 4MB.';
export const KMSConfigMapName = 'ocs-kms-connection-details';
export const KMSSecretName = 'ocs-kms-token';

Signed-off-by: Kanika Murarka <kmurarka@redhat.com>
@a2batic
Copy link
Contributor Author

a2batic commented Dec 2, 2020

/test e2e-gcp-console

Copy link
Contributor

@afreen23 afreen23 left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 2, 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-merge-robot openshift-merge-robot merged commit d73d98e into openshift:master Dec 2, 2020
@spadgett spadgett added this to the v4.7 milestone Dec 9, 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

8 participants