From: Alfonso Martínez Date: Thu, 3 Jun 2021 10:25:45 +0000 (+0200) Subject: mgr/dashboard: simplify object locking fields in 'Bucket Creation' form X-Git-Tag: v16.2.5~82^2 X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=refs%2Fpull%2F41777%2Fhead;p=ceph.git mgr/dashboard: simplify object locking fields in 'Bucket Creation' form - UI: The object locking retention period must be expressed in days. Years value from API (as API keeps param parity with RGW API) are converted to days. - API: add more validation and increase test coverage. Fixes: https://tracker.ceph.com/issues/49885 Signed-off-by: Alfonso Martínez (cherry picked from commit 09394a19913b8159a7608a594e29886eaae7570e) --- diff --git a/src/pybind/mgr/dashboard/controllers/rgw.py b/src/pybind/mgr/dashboard/controllers/rgw.py index bd395f47315a..12a65613dd16 100644 --- a/src/pybind/mgr/dashboard/controllers/rgw.py +++ b/src/pybind/mgr/dashboard/controllers/rgw.py @@ -201,8 +201,8 @@ class RgwBucket(RgwRESTController): retention_period_days, retention_period_years): rgw_client = RgwClient.instance(owner, daemon_name) return rgw_client.set_bucket_locking(bucket_name, mode, - int(retention_period_days), - int(retention_period_years)) + retention_period_days, + retention_period_years) @staticmethod def strip_tenant_from_bucket_name(bucket_name): diff --git a/src/pybind/mgr/dashboard/frontend/cypress/integration/rgw/buckets.po.ts b/src/pybind/mgr/dashboard/frontend/cypress/integration/rgw/buckets.po.ts index 6950227aee97..a17e73e80d0e 100644 --- a/src/pybind/mgr/dashboard/frontend/cypress/integration/rgw/buckets.po.ts +++ b/src/pybind/mgr/dashboard/frontend/cypress/integration/rgw/buckets.po.ts @@ -42,7 +42,6 @@ export class BucketsPageHelper extends PageHelper { this.selectLockMode('Compliance'); cy.get('#lock_mode').should('have.class', 'ng-valid'); cy.get('#lock_retention_period_days').type('3'); - cy.get('#lock_retention_period_years').type('0'); } // Click the create button and wait for bucket to be made diff --git a/src/pybind/mgr/dashboard/frontend/src/app/ceph/rgw/rgw-bucket-form/rgw-bucket-form.component.html b/src/pybind/mgr/dashboard/frontend/src/app/ceph/rgw/rgw-bucket-form/rgw-bucket-form.component.html index be54ee84efb5..be46e01524ea 100644 --- a/src/pybind/mgr/dashboard/frontend/src/app/ceph/rgw/rgw-bucket-form/rgw-bucket-form.component.html +++ b/src/pybind/mgr/dashboard/frontend/src/app/ceph/rgw/rgw-bucket-form/rgw-bucket-form.component.html @@ -273,31 +273,8 @@ *ngIf="bucketForm.showError('lock_retention_period_days', frm, 'pattern')" i18n>The entered value must be a positive integer. Retention period requires either Days or Years. - - - - -
- -
- - The entered value must be a positive integer. - Retention period requires either Days or Years. + *ngIf="bucketForm.showError('lock_retention_period_days', frm, 'lockDays')" + i18n>Retention Days must be a positive integer.
diff --git a/src/pybind/mgr/dashboard/frontend/src/app/ceph/rgw/rgw-bucket-form/rgw-bucket-form.component.spec.ts b/src/pybind/mgr/dashboard/frontend/src/app/ceph/rgw/rgw-bucket-form/rgw-bucket-form.component.spec.ts index aa02c4cc0304..574d807215f2 100644 --- a/src/pybind/mgr/dashboard/frontend/src/app/ceph/rgw/rgw-bucket-form/rgw-bucket-form.component.spec.ts +++ b/src/pybind/mgr/dashboard/frontend/src/app/ceph/rgw/rgw-bucket-form/rgw-bucket-form.component.spec.ts @@ -318,29 +318,17 @@ describe('RgwBucketFormComponent', () => { }); describe('object locking', () => { - const setDaysAndYears = (fn: (name: string) => void) => { - ['lock_retention_period_days', 'lock_retention_period_years'].forEach(fn); - }; - const expectPatternLockError = (value: string) => { formHelper.setValue('lock_enabled', true, true); - setDaysAndYears((name: string) => { - formHelper.setValue(name, value); - formHelper.expectError(name, 'pattern'); - }); + formHelper.setValue('lock_retention_period_days', value); + formHelper.expectError('lock_retention_period_days', 'pattern'); }; - const expectValidLockInputs = (enabled: boolean, mode: string, days: string, years: string) => { + const expectValidLockInputs = (enabled: boolean, mode: string, days: string) => { formHelper.setValue('lock_enabled', enabled); formHelper.setValue('lock_mode', mode); formHelper.setValue('lock_retention_period_days', days); - formHelper.setValue('lock_retention_period_years', years); - [ - 'lock_enabled', - 'lock_mode', - 'lock_retention_period_days', - 'lock_retention_period_years' - ].forEach((name) => { + ['lock_enabled', 'lock_mode', 'lock_retention_period_days'].forEach((name) => { const control = component.bucketForm.get(name); expect(control.valid).toBeTruthy(); expect(control.errors).toBeNull(); @@ -360,15 +348,13 @@ describe('RgwBucketFormComponent', () => { expect(control.disabled).toBeTruthy(); }); - it('should have the "eitherDaysOrYears" error', () => { + it('should have the "lockDays" error', () => { formHelper.setValue('lock_enabled', true); - setDaysAndYears((name: string) => { - const control = component.bucketForm.get(name); - control.updateValueAndValidity(); - expect(control.value).toBe(0); - expect(control.invalid).toBeTruthy(); - formHelper.expectError(control, 'eitherDaysOrYears'); - }); + const control = component.bucketForm.get('lock_retention_period_days'); + control.updateValueAndValidity(); + expect(control.value).toBe(0); + expect(control.invalid).toBeTruthy(); + formHelper.expectError(control, 'lockDays'); }); it('should have the "pattern" error [1]', () => { @@ -380,11 +366,16 @@ describe('RgwBucketFormComponent', () => { }); it('should have valid values [1]', () => { - expectValidLockInputs(true, 'Governance', '0', '1'); + expectValidLockInputs(true, 'Governance', '1'); }); it('should have valid values [2]', () => { - expectValidLockInputs(false, 'Compliance', '100', '0'); + expectValidLockInputs(false, 'Compliance', '2'); + }); + + it('should convert retention years to days', () => { + expect(component['getLockDays']({ lock_retention_period_years: 1000 })).toBe(365242); + expect(component['getLockDays']({ lock_retention_period_days: 5 })).toBe(5); }); }); }); diff --git a/src/pybind/mgr/dashboard/frontend/src/app/ceph/rgw/rgw-bucket-form/rgw-bucket-form.component.ts b/src/pybind/mgr/dashboard/frontend/src/app/ceph/rgw/rgw-bucket-form/rgw-bucket-form.component.ts index da3fb9f3e0b1..b1e3a4400a70 100644 --- a/src/pybind/mgr/dashboard/frontend/src/app/ceph/rgw/rgw-bucket-form/rgw-bucket-form.component.ts +++ b/src/pybind/mgr/dashboard/frontend/src/app/ceph/rgw/rgw-bucket-form/rgw-bucket-form.component.ts @@ -63,15 +63,13 @@ export class RgwBucketFormComponent extends CdForm implements OnInit { createForm() { const self = this; - const eitherDaysOrYears = CdValidators.custom('eitherDaysOrYears', () => { + const lockDaysValidator = CdValidators.custom('lockDays', () => { if (!self.bucketForm || !_.get(self.bucketForm.getRawValue(), 'lock_enabled')) { return false; } - const years = self.bucketForm.getValue('lock_retention_period_years'); - const days = self.bucketForm.getValue('lock_retention_period_days'); - return (days > 0 && years > 0) || (days === 0 && years === 0); + const lockDays = Number(self.bucketForm.getValue('lock_retention_period_days')); + return !Number.isInteger(lockDays) || lockDays === 0; }); - const lockPeriodDefinition = [0, [CdValidators.number(false), eitherDaysOrYears]]; this.bucketForm = this.formBuilder.group({ id: [null], bid: [null, [Validators.required], this.editing ? [] : [this.bucketNameValidator()]], @@ -83,8 +81,7 @@ export class RgwBucketFormComponent extends CdForm implements OnInit { 'mfa-token-pin': [''], lock_enabled: [{ value: false, disabled: this.editing }], lock_mode: ['COMPLIANCE'], - lock_retention_period_days: lockPeriodDefinition, - lock_retention_period_years: lockPeriodDefinition + lock_retention_period_days: [0, [CdValidators.number(false), lockDaysValidator]] }); } @@ -135,6 +132,7 @@ export class RgwBucketFormComponent extends CdForm implements OnInit { // the Angular react framework will throw an error if there is no // field for a given key. let value: object = _.pick(bidResp, _.keys(defaults)); + value['lock_retention_period_days'] = this.getLockDays(bidResp); value['placement-target'] = bidResp['placement_rule']; value['versioning'] = bidResp['versioning'] === RgwBucketVersioning.ENABLED; value['mfa-delete'] = bidResp['mfa_delete'] === RgwBucketMfaDelete.ENABLED; @@ -184,8 +182,7 @@ export class RgwBucketFormComponent extends CdForm implements OnInit { values['mfa-token-serial'], values['mfa-token-pin'], values['lock_mode'], - values['lock_retention_period_days'], - values['lock_retention_period_years'] + values['lock_retention_period_days'] ) .subscribe( () => { @@ -210,8 +207,7 @@ export class RgwBucketFormComponent extends CdForm implements OnInit { values['placement-target'], values['lock_enabled'], values['lock_mode'], - values['lock_retention_period_days'], - values['lock_retention_period_years'] + values['lock_retention_period_days'] ) .subscribe( () => { @@ -362,4 +358,12 @@ export class RgwBucketFormComponent extends CdForm implements OnInit { getMfaDeleteStatus() { return this.isMfaDeleteEnabled ? RgwBucketMfaDelete.ENABLED : RgwBucketMfaDelete.DISABLED; } + + private getLockDays(bucketData: object): number { + if (bucketData['lock_retention_period_years'] > 0) { + return Math.floor(bucketData['lock_retention_period_years'] * 365.242); + } + + return bucketData['lock_retention_period_days']; + } } diff --git a/src/pybind/mgr/dashboard/frontend/src/app/shared/api/rgw-bucket.service.spec.ts b/src/pybind/mgr/dashboard/frontend/src/app/shared/api/rgw-bucket.service.spec.ts index 32c88b8c9475..e457c2ab1227 100644 --- a/src/pybind/mgr/dashboard/frontend/src/app/shared/api/rgw-bucket.service.spec.ts +++ b/src/pybind/mgr/dashboard/frontend/src/app/shared/api/rgw-bucket.service.spec.ts @@ -41,20 +41,20 @@ describe('RgwBucketService', () => { it('should call create', () => { service - .create('foo', 'bar', 'default', 'default-placement', false, 'COMPLIANCE', '10', '0') + .create('foo', 'bar', 'default', 'default-placement', false, 'COMPLIANCE', '5') .subscribe(); const req = httpTesting.expectOne( - `api/rgw/bucket?bucket=foo&uid=bar&zonegroup=default&placement_target=default-placement&lock_enabled=false&lock_mode=COMPLIANCE&lock_retention_period_days=10&lock_retention_period_years=0&${RgwHelper.DAEMON_QUERY_PARAM}` + `api/rgw/bucket?bucket=foo&uid=bar&zonegroup=default&placement_target=default-placement&lock_enabled=false&lock_mode=COMPLIANCE&lock_retention_period_days=5&${RgwHelper.DAEMON_QUERY_PARAM}` ); expect(req.request.method).toBe('POST'); }); it('should call update', () => { service - .update('foo', 'bar', 'baz', 'Enabled', 'Enabled', '1', '223344', 'GOVERNANCE', '0', '1') + .update('foo', 'bar', 'baz', 'Enabled', 'Enabled', '1', '223344', 'GOVERNANCE', '10') .subscribe(); const req = httpTesting.expectOne( - `api/rgw/bucket/foo?${RgwHelper.DAEMON_QUERY_PARAM}&bucket_id=bar&uid=baz&versioning_state=Enabled&mfa_delete=Enabled&mfa_token_serial=1&mfa_token_pin=223344&lock_mode=GOVERNANCE&lock_retention_period_days=0&lock_retention_period_years=1` + `api/rgw/bucket/foo?${RgwHelper.DAEMON_QUERY_PARAM}&bucket_id=bar&uid=baz&versioning_state=Enabled&mfa_delete=Enabled&mfa_token_serial=1&mfa_token_pin=223344&lock_mode=GOVERNANCE&lock_retention_period_days=10` ); expect(req.request.method).toBe('PUT'); }); diff --git a/src/pybind/mgr/dashboard/frontend/src/app/shared/api/rgw-bucket.service.ts b/src/pybind/mgr/dashboard/frontend/src/app/shared/api/rgw-bucket.service.ts index 03fe5ca017dc..c1c02629cdd5 100644 --- a/src/pybind/mgr/dashboard/frontend/src/app/shared/api/rgw-bucket.service.ts +++ b/src/pybind/mgr/dashboard/frontend/src/app/shared/api/rgw-bucket.service.ts @@ -41,8 +41,7 @@ export class RgwBucketService { placementTarget: string, lockEnabled: boolean, lock_mode: 'GOVERNANCE' | 'COMPLIANCE', - lock_retention_period_days: string, - lock_retention_period_years: string + lock_retention_period_days: string ) { return this.rgwDaemonService.request((params: HttpParams) => { return this.http.post(this.url, null, { @@ -55,7 +54,6 @@ export class RgwBucketService { lock_enabled: String(lockEnabled), lock_mode, lock_retention_period_days, - lock_retention_period_years, daemon_name: params.get('daemon_name') } }) @@ -72,8 +70,7 @@ export class RgwBucketService { mfaTokenSerial: string, mfaTokenPin: string, lockMode: 'GOVERNANCE' | 'COMPLIANCE', - lockRetentionPeriodDays: string, - lockRetentionPeriodYears: string + lockRetentionPeriodDays: string ) { return this.rgwDaemonService.request((params: HttpParams) => { params = params.append('bucket_id', bucketId); @@ -84,7 +81,6 @@ export class RgwBucketService { params = params.append('mfa_token_pin', mfaTokenPin); params = params.append('lock_mode', lockMode); params = params.append('lock_retention_period_days', lockRetentionPeriodDays); - params = params.append('lock_retention_period_years', lockRetentionPeriodYears); return this.http.put(`${this.url}/${bucket}`, null, { params: params }); }); } diff --git a/src/pybind/mgr/dashboard/services/rgw_client.py b/src/pybind/mgr/dashboard/services/rgw_client.py index 598d05ee39df..25d1ed5ddac1 100644 --- a/src/pybind/mgr/dashboard/services/rgw_client.py +++ b/src/pybind/mgr/dashboard/services/rgw_client.py @@ -15,7 +15,7 @@ from ..settings import Settings from ..tools import build_url, dict_contains_path, dict_get, json_str_to_object try: - from typing import Any, Dict, List, Optional, Tuple + from typing import Any, Dict, List, Optional, Tuple, Union except ImportError: pass # For typing only @@ -608,12 +608,11 @@ class RgwClient(RestClient): @RestClient.api_put('/{bucket_name}?object-lock') def set_bucket_locking(self, - bucket_name, - mode, - retention_period_days, - retention_period_years, - request=None): - # type: (str, str, int, int, Optional[object]) -> None + bucket_name: str, + mode: str, + retention_period_days: Optional[Union[int, str]] = None, + retention_period_years: Optional[Union[int, str]] = None, + request: Optional[object] = None) -> None: """ Places the locking configuration on the specified bucket. The locking configuration will be applied by default to every new @@ -631,6 +630,14 @@ class RgwClient(RestClient): # pylint: disable=unused-argument # Do some validations. + try: + retention_period_days = int(retention_period_days) if retention_period_days else 0 + retention_period_years = int(retention_period_years) if retention_period_years else 0 + if retention_period_days < 0 or retention_period_years < 0: + raise ValueError + except (TypeError, ValueError): + msg = "Retention period must be a positive integer." + raise DashboardException(msg=msg, component='rgw') if retention_period_days and retention_period_years: # https://docs.aws.amazon.com/AmazonS3/latest/API/archive-RESTBucketPUTObjectLockConfiguration.html msg = "Retention period requires either Days or Years. "\ @@ -640,6 +647,9 @@ class RgwClient(RestClient): msg = "Retention period requires either Days or Years. "\ "You must specify at least one." raise DashboardException(msg=msg, component='rgw') + if not isinstance(mode, str) or mode.upper() not in ['COMPLIANCE', 'GOVERNANCE']: + msg = "Retention mode must be either COMPLIANCE or GOVERNANCE." + raise DashboardException(msg=msg, component='rgw') # Generate the XML data like this: # diff --git a/src/pybind/mgr/dashboard/tests/test_rgw_client.py b/src/pybind/mgr/dashboard/tests/test_rgw_client.py index edc10071f352..b01076c8b19e 100644 --- a/src/pybind/mgr/dashboard/tests/test_rgw_client.py +++ b/src/pybind/mgr/dashboard/tests/test_rgw_client.py @@ -103,6 +103,49 @@ class RgwClientTest(TestCase, KVStoreMockMixin): self.assertEqual(['realm1', 'realm2'], instance.get_realms()) self.assertEqual([], instance.get_realms()) + def test_set_bucket_locking_error(self): + instance = RgwClient.admin_instance() + test_params = [ + ('COMPLIANCE', 'null', None, 'must be a positive integer'), + ('COMPLIANCE', None, 'null', 'must be a positive integer'), + ('COMPLIANCE', -1, None, 'must be a positive integer'), + ('COMPLIANCE', None, -1, 'must be a positive integer'), + ('COMPLIANCE', 1, 1, 'You can\'t specify both at the same time'), + ('COMPLIANCE', None, None, 'You must specify at least one'), + ('COMPLIANCE', 0, 0, 'You must specify at least one'), + (None, 1, 0, 'must be either COMPLIANCE or GOVERNANCE'), + ('', 1, 0, 'must be either COMPLIANCE or GOVERNANCE'), + ('FAKE_MODE', 1, 0, 'must be either COMPLIANCE or GOVERNANCE') + ] + for params in test_params: + mode, days, years, error_msg = params + with self.assertRaises(DashboardException) as cm: + instance.set_bucket_locking( + bucket_name='test', + mode=mode, + retention_period_days=days, + retention_period_years=years + ) + self.assertIn(error_msg, str(cm.exception)) + + @patch('dashboard.rest_client._Request', Mock()) + def test_set_bucket_locking_success(self): + instance = RgwClient.admin_instance() + test_params = [ + ('Compliance', '1', None), + ('Governance', 1, None), + ('COMPLIANCE', None, '1'), + ('GOVERNANCE', None, 1), + ] + for params in test_params: + mode, days, years = params + self.assertIsNone(instance.set_bucket_locking( + bucket_name='test', + mode=mode, + retention_period_days=days, + retention_period_years=years + )) + class RgwClientHelperTest(TestCase): def test_parse_frontend_config_1(self):