Merge pull request #28942 from ricardoasmarques/validate-iscsi-controls

mgr/dashboard: Validate iSCSI controls min/max value

Reviewed-by: Ricardo Dias <rdias@suse.com>
Reviewed-by: Tiago Melo <tmelo@suse.com>

src/pybind/mgr/dashboard/controllers/iscsi.py

@@ -223,7 +223,7 @@ class IscsiTarget(RESTController):
             raise DashboardException(msg='Target already exists',
                                      code='target_already_exists',
                                      component='iscsi')
-        IscsiTarget._validate(target_iqn, portals, disks, groups)
+        IscsiTarget._validate(target_iqn, target_controls, portals, disks, groups)
         IscsiTarget._create(target_iqn, target_controls, acl_enabled, portals, disks, clients,
                             groups, 0, 100, config)
 
@@ -245,7 +245,7 @@ class IscsiTarget(RESTController):
             raise DashboardException(msg='Target IQN already in use',
                                      code='target_iqn_already_in_use',
                                      component='iscsi')
-        IscsiTarget._validate(new_target_iqn, portals, disks, groups)
+        IscsiTarget._validate(new_target_iqn, target_controls, portals, disks, groups)
         config = IscsiTarget._delete(target_iqn, config, 0, 50, new_target_iqn, target_controls,
                                      portals, disks, clients, groups)
         IscsiTarget._create(new_target_iqn, target_controls, acl_enabled, portals, disks, clients,
@@ -412,7 +412,7 @@ class IscsiTarget(RESTController):
         return False
 
     @staticmethod
-    def _validate(target_iqn, portals, disks, groups):
+    def _validate(target_iqn, target_controls, portals, disks, groups):
         if not target_iqn:
             raise DashboardException(msg='Target IQN is required',
                                      code='target_iqn_required',
@@ -430,6 +430,26 @@ class IscsiTarget(RESTController):
                                      code='portals_required',
                                      component='iscsi')
 
+        # 'target_controls_limits' was introduced in ceph-iscsi > 3.2
+        # When using an older `ceph-iscsi` version these validations will
+        # NOT be executed beforehand
+        if 'target_controls_limits' in settings:
+            for target_control_name, target_control_value in target_controls.items():
+                limits = settings['target_controls_limits'].get(target_control_name)
+                if limits is not None:
+                    min_value = limits.get('min')
+                    if min_value is not None and target_control_value < min_value:
+                        raise DashboardException(msg='Target control {} must be >= '
+                                                     '{}'.format(target_control_name, min_value),
+                                                 code='target_control_invalid_min',
+                                                 component='iscsi')
+                    max_value = limits.get('max')
+                    if max_value is not None and target_control_value > max_value:
+                        raise DashboardException(msg='Target control {} must be <= '
+                                                     '{}'.format(target_control_name, max_value),
+                                                 code='target_control_invalid_max',
+                                                 component='iscsi')
+
         for portal in portals:
             gateway_name = portal['host']
             try:
@@ -449,6 +469,26 @@ class IscsiTarget(RESTController):
             IscsiTarget._validate_image(pool, image, backstore, required_rbd_features,
                                         unsupported_rbd_features)
 
+            # 'disk_controls_limits' was introduced in ceph-iscsi > 3.2
+            # When using an older `ceph-iscsi` version these validations will
+            # NOT be executed beforehand
+            if 'disk_controls_limits' in settings:
+                for disk_control_name, disk_control_value in disk['controls'].items():
+                    limits = settings['disk_controls_limits'][backstore].get(disk_control_name)
+                    if limits is not None:
+                        min_value = limits.get('min')
+                        if min_value is not None and disk_control_value < min_value:
+                            raise DashboardException(msg='Disk control {} must be >= '
+                                                         '{}'.format(disk_control_name, min_value),
+                                                     code='disk_control_invalid_min',
+                                                     component='iscsi')
+                        max_value = limits.get('max')
+                        if max_value is not None and disk_control_value > max_value:
+                            raise DashboardException(msg='Disk control {} must be <= '
+                                                         '{}'.format(disk_control_name, max_value),
+                                                     code='disk_control_invalid_max',
+                                                     component='iscsi')
+
         initiators = []
         for group in groups:
             initiators = initiators + group['members']

src/pybind/mgr/dashboard/frontend/src/app/ceph/block/iscsi-target-form/iscsi-target-form.component.ts

@@ -30,7 +30,9 @@ export class IscsiTargetFormComponent implements OnInit {
   modalRef: BsModalRef;
   minimum_gateways = 1;
   target_default_controls: any;
+  target_controls_limits: any;
   disk_default_controls: any;
+  disk_controls_limits: any;
   backstores: string[];
   default_backstore: string;
   unsupported_rbd_features: any;
@@ -124,7 +126,9 @@ export class IscsiTargetFormComponent implements OnInit {
       // iscsiService.settings()
       this.minimum_gateways = data[3].config.minimum_gateways;
       this.target_default_controls = data[3].target_default_controls;
+      this.target_controls_limits = data[3].target_controls_limits;
       this.disk_default_controls = data[3].disk_default_controls;
+      this.disk_controls_limits = data[3].disk_controls_limits;
       this.backstores = data[3].backstores;
       this.default_backstore = data[3].default_backstore;
       this.unsupported_rbd_features = data[3].unsupported_rbd_features;
@@ -666,7 +670,8 @@ export class IscsiTargetFormComponent implements OnInit {
   targetSettingsModal() {
     const initialState = {
       target_controls: this.targetForm.get('target_controls'),
-      target_default_controls: this.target_default_controls
+      target_default_controls: this.target_default_controls,
+      target_controls_limits: this.target_controls_limits
     };
 
     this.modalRef = this.modalService.show(IscsiTargetIqnSettingsModalComponent, { initialState });
@@ -677,6 +682,7 @@ export class IscsiTargetFormComponent implements OnInit {
       imagesSettings: this.imagesSettings,
       image: image,
       disk_default_controls: this.disk_default_controls,
+      disk_controls_limits: this.disk_controls_limits,
       backstores: this.getValidBackstores(this.getImageById(image))
     };
 

src/pybind/mgr/dashboard/frontend/src/app/ceph/block/iscsi-target-image-settings-modal/iscsi-target-image-settings-modal.component.html

@@ -5,54 +5,67 @@
   </ng-container>
 
   <ng-container class="modal-content">
-    <div class="modal-body">
-      <p class="alert-warning"
-         i18n>Changing these parameters from their default values is usually not necessary.</p>
+    <form name="settingsForm"
+          class="form"
+          #formDir="ngForm"
+          [formGroup]="settingsForm"
+          novalidate>
+      <div class="modal-body">
+        <p class="alert-warning"
+           i18n>Changing these parameters from their default values is usually not necessary.</p>
 
-      <!-- BACKSTORE -->
-      <div class="form-group row">
-        <div class="col-sm-12">
-          <label class="col-form-label"
-                 i18n>Backstore</label>
-          <select id="backstore"
-                  name="backstore"
-                  class="form-control custom-select"
-                  [(ngModel)]="model.backstore"
-                  [disabled]="backstores.length == 1">
-            <option *ngFor="let bs of backstores"
-                    [value]="bs">{{ bs | iscsiBackstore }}</option>
-          </select>
+        <!-- BACKSTORE -->
+        <div class="form-group row">
+          <div class="col-sm-12">
+            <label class="col-form-label"
+                   i18n>Backstore</label>
+            <select id="backstore"
+                    name="backstore"
+                    class="form-control custom-select"
+                    formControlName="backstore">
+              <option *ngFor="let bs of backstores"
+                      [value]="bs">{{ bs | iscsiBackstore }}</option>
+            </select>
+          </div>
+        </div>
+
+        <!-- CONTROLS -->
+        <ng-container *ngFor="let bs of backstores">
+          <ng-container *ngIf="settingsForm.value['backstore'] === bs">
+            <div class="form-group row"
+                 *ngFor="let setting of disk_default_controls[bs] | keyvalue">
+              <div class="col-sm-12">
+                <label class="col-form-label"
+                       for="{{ setting.key }}">{{ setting.key }}</label>
+                <input type="number"
+                       class="form-control"
+                       [formControlName]="setting.key">
+                <span class="invalid-feedback"
+                      *ngIf="settingsForm.showError(setting.key, formDir, 'min')">
+                  <ng-container i18n>Must be greater than or equal to {{ disk_controls_limits[bs][setting.key]['min'] }}.</ng-container>
+                </span>
+                <span class="invalid-feedback"
+                      *ngIf="settingsForm.showError(setting.key, formDir, 'max')">
+                  <ng-container i18n>Must be less than or equal to {{ disk_controls_limits[bs][setting.key]['max'] }}.</ng-container>
+                </span>
+                <span class="form-text text-muted">{{ helpText[setting.key]?.help }}</span>
+              </div>
+            </div>
+          </ng-container>
+        </ng-container>
+      </div>
+
+      <div class="modal-footer">
+        <div class="button-group text-right">
+          <cd-submit-button i18n
+                            [form]="settingsForm"
+                            (submitAction)="save()">Confirm</cd-submit-button>
+          <cd-back-button [back]="modalRef.hide"
+                          name="Cancel"
+                          i18n-name>
+          </cd-back-button>
         </div>
       </div>
-
-      <!-- CONTROLS -->
-      <ng-container *ngFor="let bs of backstores">
-        <ng-container *ngIf="model.backstore === bs">
-          <div class="form-group row"
-               *ngFor="let setting of disk_default_controls[bs] | keyvalue">
-            <div class="col-sm-12">
-              <label class="col-form-label"
-                     for="{{ setting.key }}">{{ setting.key }}</label>
-              <input type="number"
-                     class="form-control"
-                     [(ngModel)]="model[bs][setting.key]">
-              <span class="form-text text-muted">{{ helpText[setting.key]?.help }}</span>
-            </div>
-          </div>
-        </ng-container>
-      </ng-container>
-    </div>
-
-    <div class="modal-footer">
-      <div class="button-group text-right">
-        <button class="btn btn-secondary"
-                (click)="save()"
-                i18n>Confirm</button>
-        <cd-back-button [back]="modalRef.hide"
-                        name="Cancel"
-                        i18n-name>
-        </cd-back-button>
-      </div>
-    </div>
+    </form>
   </ng-container>
 </cd-modal>

src/pybind/mgr/dashboard/frontend/src/app/ceph/block/iscsi-target-image-settings-modal/iscsi-target-image-settings-modal.component.spec.ts

@@ -1,6 +1,6 @@
 import { HttpClientTestingModule } from '@angular/common/http/testing';
 import { ComponentFixture, TestBed } from '@angular/core/testing';
-import { FormsModule } from '@angular/forms';
+import { ReactiveFormsModule } from '@angular/forms';
 import { RouterTestingModule } from '@angular/router/testing';
 
 import { BsModalRef } from 'ngx-bootstrap/modal';
@@ -15,7 +15,7 @@ describe('IscsiTargetImageSettingsModalComponent', () => {
 
   configureTestBed({
     declarations: [IscsiTargetImageSettingsModalComponent],
-    imports: [SharedModule, FormsModule, HttpClientTestingModule, RouterTestingModule],
+    imports: [SharedModule, ReactiveFormsModule, HttpClientTestingModule, RouterTestingModule],
     providers: [BsModalRef, i18nProviders]
   });
 
@@ -44,16 +44,17 @@ describe('IscsiTargetImageSettingsModalComponent', () => {
     expect(component).toBeTruthy();
   });
 
-  it('should fill the model', () => {
-    expect(component.model).toEqual({
+  it('should fill the form', () => {
+    expect(component.settingsForm.value).toEqual({
       backstore: 'backstore:1',
-      'backstore:1': {},
-      'backstore:2': {}
+      foo: null,
+      bar: null,
+      baz: null
     });
   });
 
   it('should save changes to imagesSettings', () => {
-    component.model['backstore:1'] = { foo: 1234 };
+    component.settingsForm.controls['foo'].setValue(1234);
     expect(component.imagesSettings).toEqual({
       'rbd/disk_1': { backstore: 'backstore:1', 'backstore:1': {} }
     });

src/pybind/mgr/dashboard/frontend/src/app/ceph/block/iscsi-target-image-settings-modal/iscsi-target-image-settings-modal.component.ts

@@ -1,9 +1,11 @@
 import { Component, OnInit } from '@angular/core';
+import { FormControl, Validators } from '@angular/forms';
 
 import * as _ from 'lodash';
 import { BsModalRef } from 'ngx-bootstrap/modal';
 
 import { IscsiService } from '../../../shared/api/iscsi.service';
+import { CdFormGroup } from '../../../shared/forms/cd-form-group';
 
 @Component({
   selector: 'cd-iscsi-target-image-settings-modal',
@@ -14,9 +16,10 @@ export class IscsiTargetImageSettingsModalComponent implements OnInit {
   image: string;
   imagesSettings: any;
   disk_default_controls: any;
+  disk_controls_limits: any;
   backstores: any;
 
-  model: any;
+  settingsForm: CdFormGroup;
   helpText: any;
 
   constructor(public modalRef: BsModalRef, public iscsiService: IscsiService) {}
@@ -24,18 +27,48 @@ export class IscsiTargetImageSettingsModalComponent implements OnInit {
   ngOnInit() {
     this.helpText = this.iscsiService.imageAdvancedSettings;
 
-    this.model = _.cloneDeep(this.imagesSettings[this.image]);
+    const fg = {
+      backstore: new FormControl(this.imagesSettings[this.image]['backstore'])
+    };
     _.forEach(this.backstores, (backstore) => {
-      this.model[backstore] = this.model[backstore] || {};
+      const model = this.imagesSettings[this.image][backstore] || {};
+      _.forIn(this.disk_default_controls[backstore], (_value, key) => {
+        const validators = [];
+        if (this.disk_controls_limits && key in this.disk_controls_limits[backstore]) {
+          if ('min' in this.disk_controls_limits[backstore][key]) {
+            validators.push(Validators.min(this.disk_controls_limits[backstore][key]['min']));
+          }
+          if ('max' in this.disk_controls_limits[backstore][key]) {
+            validators.push(Validators.max(this.disk_controls_limits[backstore][key]['max']));
+          }
+        }
+        fg[key] = new FormControl(model[key], {
+          validators: validators
+        });
+      });
     });
+
+    this.settingsForm = new CdFormGroup(fg);
   }
 
   save() {
-    const backstore = this.model.backstore;
+    const backstore = this.settingsForm.controls['backstore'].value;
     const settings = {};
-    _.forIn(this.model[backstore], (value, key) => {
-      if (!(value === '' || value === null)) {
-        settings[key] = value;
+    _.forIn(this.settingsForm.controls, (control, key) => {
+      if (
+        !(control.value === '' || control.value === null) &&
+        key in this.disk_default_controls[this.settingsForm.value['backstore']]
+      ) {
+        settings[key] = control.value;
+        // If one setting belongs to multiple backstores, we have to update it in all backstores
+        _.forEach(this.backstores, (currentBackstore) => {
+          if (currentBackstore !== backstore) {
+            const model = this.imagesSettings[this.image][currentBackstore] || {};
+            if (key in model) {
+              this.imagesSettings[this.image][currentBackstore][key] = control.value;
+            }
+          }
+        });
       }
     });
     this.imagesSettings[this.image]['backstore'] = backstore;

src/pybind/mgr/dashboard/frontend/src/app/ceph/block/iscsi-target-iqn-settings-modal/iscsi-target-iqn-settings-modal.component.html

@@ -20,6 +20,14 @@
                    *ngIf="!isRadio(setting.key)"
                    type="number"
                    [formControlName]="setting.key">
+            <span class="invalid-feedback"
+                  *ngIf="settingsForm.showError(setting.key, formDir, 'min')">
+              <ng-container i18n>Must be greater than or equal to {{ target_controls_limits[setting.key]['min'] }}.</ng-container>
+            </span>
+            <span class="invalid-feedback"
+                  *ngIf="settingsForm.showError(setting.key, formDir, 'max')">
+              <ng-container i18n>Must be less than or equal to {{ target_controls_limits[setting.key]['max'] }}.</ng-container>
+            </span>
 
             <ng-container *ngIf="isRadio(setting.key)">
               <br>

src/pybind/mgr/dashboard/frontend/src/app/ceph/block/iscsi-target-iqn-settings-modal/iscsi-target-iqn-settings-modal.component.ts

@@ -1,5 +1,5 @@
 import { Component, OnInit } from '@angular/core';
-import { FormControl } from '@angular/forms';
+import { FormControl, Validators } from '@angular/forms';
 
 import * as _ from 'lodash';
 import { BsModalRef } from 'ngx-bootstrap/modal';
@@ -15,6 +15,7 @@ import { CdFormGroup } from '../../../shared/forms/cd-form-group';
 export class IscsiTargetIqnSettingsModalComponent implements OnInit {
   target_controls: FormControl;
   target_default_controls: any;
+  target_controls_limits: any;
 
   settingsForm: CdFormGroup;
   helpText: any;
@@ -26,7 +27,16 @@ export class IscsiTargetIqnSettingsModalComponent implements OnInit {
     this.helpText = this.iscsiService.targetAdvancedSettings;
 
     _.forIn(this.target_default_controls, (_value, key) => {
-      fg[key] = new FormControl(this.target_controls.value[key]);
+      const validators = [];
+      if (this.target_controls_limits && key in this.target_controls_limits) {
+        if ('min' in this.target_controls_limits[key]) {
+          validators.push(Validators.min(this.target_controls_limits[key]['min']));
+        }
+        if ('max' in this.target_controls_limits[key]) {
+          validators.push(Validators.max(this.target_controls_limits[key]['max']));
+        }
+      }
+      fg[key] = new FormControl(this.target_controls.value[key], { validators: validators });
     });
 
     this.settingsForm = new CdFormGroup(fg);