Add put/get/delete alert_source APIs. #59

sfzeng · 2020-05-18T02:46:33Z

What this PR does / why we need it:
This PR is to add PUT/GET/DELETE alert source APIs.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

Special notes for your reviewer:
Currently, the alert source is considered as a host with a specific ip, it may be changed to a host list in the future.
For SNMPv3, the security model may looks like as below:

So when snmp version is set as SNMPv3, we need to check those parameters. Check Table 1 from the following link for details about SNMPv3 Security Mechanisms: https://www.cisco.com/c/en/us/td/docs/ios-xml/ios/snmp/configuration/xe-3se/3850/snmp-xe-3se-3850-book/nm-snmp-snmpv3.html
Release note:

dolphin/db/sqlalchemy/api.py

dolphin/db/sqlalchemy/models.py

dolphin/exception.py

dolphin/utils.py

dolphin/api/v1/alert.py

wisererik · 2020-05-18T09:48:10Z

dolphin/api/v1/router.py

@@ -49,6 +50,21 @@ def _setup_routes(self, mapper):
                       controller=self.resources['access_info'],
                       action="update",
                       conditions={"method": ["PUT"]})
+
+        self.resources['alert_sources'] = alert.create_resource()
+        mapper.connect("storages", "/storages/{id}/alert_sources",


plz don't use underscore in the url. suggest name: alert-source

wisererik · 2020-05-18T09:58:26Z

dolphin/api/v1/alert.py

+            auth_enable_str = alert_source.get('auth_enable', None)
+            engine_id = alert_source.get('engine_id', None)
+            if not user_name or not auth_enable_str or not engine_id:
+                msg = "username, auth_enable and engine_id are required if snmp version is SNMPv3."


First letter should be upper case in message.

… instead of underscore in the url

sushanthakumar · 2020-05-18T13:13:31Z

dolphin/api/schemas/alert.py

+        'version': parameter_types.snmp_version,
+        'community_string': {'type': 'string', 'minLength': 1, 'maxLength': 255},
+        'username': {'type': 'string', 'minLength': 1, 'maxLength': 255},
+        'password': {'type': 'string', 'minLength': 1, 'maxLength': 255},


We can change to auth_password or auth_key

Changed to auth_key

sushanthakumar · 2020-05-18T13:18:54Z

dolphin/api/v1/alert.py

+            auth_enabled_str = alert_source.get('auth_enabled', None)
+            engine_id = alert_source.get('engine_id', None)
+            if not user_name or not auth_enabled_str or not engine_id:
+                msg = "If snmp version is SNMPv3, then username, auth_enabled and engine_id are required."


Few lines looks to be beyond page border

My IDE(Pycharm) shows, all lines are fine, which IDE you used?

Anyway, I cut those long lines.

As per Python coding guidelines, the max char for a line is 79..So maybe you can set the line wrap/length accordingly

sushanthakumar · 2020-05-18T13:19:54Z

dolphin/api/v1/router.py

@@ -19,6 +19,7 @@
 from dolphin.api.v1 import access_info
 from dolphin.api.v1 import pools
 from dolphin.api.v1 import volumes
+from dolphin.api.v1 import alert


Optimize import

sushanthakumar · 2020-05-18T13:28:20Z

dolphin/api/validation/parameter_types.py

@@ -172,3 +172,22 @@ def valid_char(char):
    'minimum': 0, 'maximum': 65535
 }

+boolean = {
+    'type': ['string'],
+    'enum': [True, 'True', 'TRUE', 'true', False, 'False', 'FALSE', 'false'],


Do we really need to support many combinations, lets check the usual practice

I think yes, I check cinder, it used the same way.

sushanthakumar · 2020-05-18T13:29:41Z

dolphin/db/api.py

+    return IMPL.alert_source_update(context, storage_id, values)
+
+
+def alert_source_get(context, storage_id):


We need to fetch alert source when we want to get storage id from source ip. So we need one alert source get all api with filter, do we have it already?

This PR only include the user side functuion, that will be done in the next PR, otherwise PR will became too big.

sushanthakumar · 2020-05-18T13:36:45Z

dolphin/api/v1/alert.py

+        if version.lower() == 'snmpv3':
+            user_name = alert_source.get('username', None)
+            auth_enabled_str = alert_source.get('auth_enabled', None)
+            engine_id = alert_source.get('engine_id', None)


If user selects v3 and gives just username and enginid, will it be success. Is it better to consider default auth_enabled_str as false

In that case, it will failed and the error message will tell the user auth_enabled is required. Cauz IMO, SNMPv3 is mainly for security, if auth is not enabled, then that will not be safe, so we'd better let the user to explicitly enable or disable authentication.

sushanthakumar · 2020-05-18T13:39:47Z

dolphin/api/v1/alert.py

+            else:
+                alert_source = db.alert_source_create(ctx, alert_source)
+        except exception.StorageNotFound as e:
+            msg = (_("Alert source cannot be created or updated for a non-exist storage %s.") % id)


non-exist can be changed to non-existing

sushanthakumar · 2020-05-18T13:45:02Z

dolphin/api/v1/alert.py

+            raise exc.HTTPBadRequest(explanation=msg)
+        except exception.InvalidInput as e:
+            msg = _('Failed to put alert_source: {0}'.format(e))
+            raise exc.HTTPBadRequest(explanation=msg)


Any chance of getting general exception here?

The test show, those exception will be handled in wsgi level.

…t long lines to be shorter.

kumarashit

LGTM

dolphin/api/v1/alert.py

dolphin/utils.py

sushanthakumar · 2020-05-19T03:56:18Z

LGTM

sushanthakumar · 2020-05-19T08:00:52Z

dolphin/api/v1/alert.py

-                          "auth_protocol and auth_key is required."
+                    msg = "If snmp version is SNMPv3 and security_level is " \
+                          "AuthPriv or AuthNoPriv, auth_protocol and auth_key" \
+                          " is required."


just change to "are required"

sushanthakumar · 2020-05-19T08:29:22Z

LGTM

kumarashit

LGTM

Add put/get/delete alert_source APIs.

1049b99

kumarashit reviewed May 18, 2020

View reviewed changes

address some issues from comments

712d4b0

wisererik reviewed May 18, 2020

View reviewed changes

Make the first letter of exception message be upper case and use dash…

ab5c038

… instead of underscore in the url

sushanthakumar reviewed May 18, 2020

View reviewed changes

Rename password to be auth_key, update some exception message, and cu…

bdb3cd6

…t long lines to be shorter.

kumarashit previously approved these changes May 19, 2020

View reviewed changes

dolphin/api/v1/alert.py Show resolved Hide resolved

dolphin/utils.py Outdated Show resolved Hide resolved

Use security_level instead of auth_eanble and enryption_enable

e94464a

sfzeng dismissed kumarashit’s stale review via e94464a May 19, 2020 07:56

sushanthakumar reviewed May 19, 2020

View reviewed changes

wisererik previously approved these changes May 19, 2020

View reviewed changes

fix typo

0aa1e41

sfzeng dismissed wisererik’s stale review via 0aa1e41 May 19, 2020 08:20

kumarashit approved these changes May 19, 2020

View reviewed changes

wisererik approved these changes May 19, 2020

View reviewed changes

wisererik merged commit a27aac7 into sodafoundation:master May 19, 2020

PravinRanjan10 mentioned this pull request May 11, 2020

Not correct behaviour of log info message #46

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add put/get/delete alert_source APIs. #59

Add put/get/delete alert_source APIs. #59

sfzeng commented May 18, 2020 •

edited

Loading

wisererik May 18, 2020

sfzeng May 18, 2020

wisererik May 18, 2020

sfzeng May 18, 2020

sushanthakumar May 18, 2020

sfzeng May 19, 2020

sushanthakumar May 18, 2020

sfzeng May 19, 2020

sfzeng May 19, 2020

kumarashit May 19, 2020

sushanthakumar May 18, 2020

sfzeng May 19, 2020

sushanthakumar May 18, 2020 •

edited

Loading

sfzeng May 19, 2020

sushanthakumar May 18, 2020 •

edited

Loading

sfzeng May 19, 2020 •

edited

Loading

sushanthakumar May 18, 2020

sfzeng May 19, 2020

sushanthakumar May 18, 2020

sfzeng May 19, 2020

sushanthakumar May 18, 2020

sfzeng May 19, 2020

kumarashit left a comment

sushanthakumar commented May 19, 2020

sushanthakumar May 19, 2020

sfzeng May 19, 2020

sushanthakumar commented May 19, 2020

kumarashit left a comment

		return IMPL.alert_source_update(context, storage_id, values)


		def alert_source_get(context, storage_id):

Add put/get/delete alert_source APIs. #59

Add put/get/delete alert_source APIs. #59

Conversation

sfzeng commented May 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sushanthakumar May 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sushanthakumar May 18, 2020 • edited Loading

Choose a reason for hiding this comment

sfzeng May 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kumarashit left a comment

Choose a reason for hiding this comment

sushanthakumar commented May 19, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sushanthakumar commented May 19, 2020

kumarashit left a comment

Choose a reason for hiding this comment

sfzeng commented May 18, 2020 •

edited

Loading

sushanthakumar May 18, 2020 •

edited

Loading

sushanthakumar May 18, 2020 •

edited

Loading

sfzeng May 19, 2020 •

edited

Loading