Security groups request ids by fisherma91 · Pull Request #935 · softlayer/softlayer-python

fisherma91 · 2018-02-01T19:54:50Z

This PR adds the following features to SoftLayer Python:

Support for the new return values of SoftLayer_Network_SecurityGroup.addRules(), SoftLayer_Network_SecurityGroup.editRules(), SoftLayer_Network_SecurityGroup.removeRules(), SoftLayer_Network_SecurityGroup. attachNetworkComponents() and SoftLayer_Network_SecurityGroup. detachNetworkComponents()
Add abstraction for Event_Log. getAllEventObjectNames()
Add the ability to search for Event_Log objects by type, date range, and requestId (Value now returned by the modified SoftLayer_Network_SecurityGroup methods mentioned above)

The python api will now output a table with requestIDs for specific calls to security groups apis

Fix unit tests Fix code style issues

Add new return format

Add functionality to get event logs to slcli

Fix Security Group unit tests

Refactor Event Log Code Fix Unit Tests Add Unit Tests Fix Code Styling

Remove unneeded code left over from refactoring Fix incorrect package name

Code Styling changes

Change public facing name to Audit Logs Add functionality to get event log types

Fix ordering of test expecations to be Actual then Expected

Add functionality to filter by eventName

Add functionality to filter by dates

Add request id search functionality

Fix fixtures from merge

coveralls · 2018-02-01T19:58:41Z

Coverage increased (+0.2%) to 87.152% when pulling 6333687 on fisherma91:security-groups-request-ids into 23dab1e on softlayer:master.

Fix tox issues

rlrossiter

I did a first pass through the event log side of things, and I think there could be a little shuffling to organize what functions are where to make it a little more usable.

From an interface standpoint, I think you could have 2 functions on the event log manager publicly exposed, get_logs_by_event() and get_logs_by_object() (or break it apart in any way that makes sense) that would contain most of the filter building that currently exists within the CLI.

We can then add a function to either the event log manager that then gets the audit logs for SG operations. Then the sg CLI could have an extra command add it that does the request log querying instead of building SG functionality into the event log CLI.

rlrossiter · 2018-02-02T16:47:55Z

SoftLayer/CLI/event_log/get.py

+
+
+@click.command()
+@click.option('--date_min', '-d',


For consistency, all of these options should be --date-min (dash in middle instead of underscore). As for the args, click automatically changes it for you (so --date-min becomes date_min)

rlrossiter · 2018-02-02T16:49:39Z

SoftLayer/CLI/event_log/get.py

+@click.option('--obj_id', '-i',
+              help="The id of the object we want to get audit logs for")
+@click.option('--request_id', '-r',
+              help="The request id we want to look for. If this is set, we will ignore all other arguments.")


I'd rather throw an error if they're used together instead of ignoring all other arguments. That way it isn't silently doing something the user may not be aware of

rlrossiter · 2018-02-02T16:52:54Z

SoftLayer/CLI/event_log/get.py

+    """Get Audit Logs"""
+    mgr = SoftLayer.EventLogManager(env.client)
+
+    if request_id is not None:


So down here you'd add a check for if request_id and any(date_min, date_max, obj_event, ....) and you'd raise a CLIAbort on it

rlrossiter · 2018-02-02T16:57:57Z

SoftLayer/CLI/event_log/get.py

+@click.option('--obj_type', '-t',
+              help="The type of the object we want to get audit logs for")
+@click.option('--utc_offset', '-z',
+              help="UTC Offset for seatching with dates. The default is -0500")


I'm nervous to force people into -0500. I'd lean more towards if they don't specify it, just leave it at 0.

rlrossiter · 2018-02-02T16:58:51Z

SoftLayer/CLI/event_log/get.py

+    return filtered_logs
+
+
+def _parse_date(date_string, utc_offset):


I'd move this to a utility and unit test the heck out of it since this looks a little complex

rlrossiter · 2018-02-02T17:00:51Z

SoftLayer/CLI/event_log/get.py

+    if date_min and date_max:
+        request_filter['eventCreateDate'] = {
+            'operation': 'betweenDate',
+            'options': [


There are utils for building date filters https://github.com/softlayer/softlayer-python/blob/master/SoftLayer/utils.py#L111. Not sure if that can be used for these as well

These won't work for this case, because of where the Event_Log entries are stored.

We need to have a different filter format for it to work.

rlrossiter · 2018-02-02T18:26:40Z

SoftLayer/CLI/event_log/get.py

+    env.fout(table)
+
+
+def _build_filter(date_min, date_max, obj_event, obj_id, obj_type, utc_offset):


I think this should get moved to the manager instead of doing all of the legwork in the CLI. That way if people want to find certain event logs when using python, they can borrow this filter building.

rlrossiter · 2018-02-02T18:29:08Z

SoftLayer/managers/event_log.py

+        :param dict request_filter: filter dict
+        :returns: List of event logs
+        """
+        results = self.client.call("Event_Log",


From a convention standpoint, usually you can just store the event log service on the manager itself. So you'd add self.event_log = client['Event_Log'] to __init__(), and then in these functions you can just do self.event_log.getAllObjects(filter=request_filter)

change date_min to date-min in click args change date_max to date-max in click args change how the event log client is initialized move filter building code into event log manager set default utc offset to +0000 move date parsing code into utils add ability to get event logs by type add ability to get event logs by name move requestID searching into Security Groups code Overhaul unit tests

rlrossiter

Looks like we're getting close. I tried out most of the slcli audit-log command. I still need to try out the slcli sg audit-log to check that as well, but just wanted to post a few of the comments that I had.

rlrossiter · 2018-02-09T22:11:09Z

tests/managers/network_tests.py

+
+        self.assertEqual(expected, result)
+
+    def test__get_cci_event_logs(self):


This isn't super important, but it looks like you've got two _ between test and get.

rlrossiter · 2018-02-12T16:27:03Z

SoftLayer/CLI/event_log/get.py

+@click.option('--obj_type', '-t',
+              help="The type of the object we want to get audit logs for")
+@click.option('--utc_offset', '-z',
+              help="UTC Offset for seatching with dates. The default is -0000")


searching is misspelled here

rlrossiter · 2018-02-12T16:28:09Z

SoftLayer/CLI/event_log/get.py

+              help='The earliest date we want to search for audit logs in mm/dd/yyyy format.')
+@click.option('--date-max', '-D',
+              help='The latest date we want to search for audit logs in mm/dd/yyyy format.')
+@click.option('--obj_event', '-e',


All of these other options still need to be switched over to use - instead of _ as well

rlrossiter · 2018-02-12T16:32:25Z

SoftLayer/CLI/securitygroup/event_log.py

+@click.argument('request_id')
+@environment.pass_env
+def get_by_request_id(env, request_id):
+    """Search for event logs by request id"""


(venv) ryans-mbp-2:softlayer-python rlrossit$ slcli sg audit-log -h Usage: slcli sg audit-log [OPTIONS] REQUEST_ID Search for event logs by request id

Having it be called sg audit log, with the help text then showing "Search for event logs" is kind of confusing. It looks like throughout most of this PR audit log and event log are being used interchangeably. I'd settle on one of the names, and use it everywhere (file/class names, CLI, etc.)

rlrossiter · 2018-02-12T16:34:03Z

SoftLayer/CLI/securitygroup/interface.py

        raise exceptions.CLIAbort("Could not detach network component")

+    table = formatting.Table(REQUEST_COLUMNS)
+    table.add_row([success['requestId']])


I'd rename the success variable returned from detach, since using it in this situation reads weird. I'd just use ret like all of the other functions are using.

rlrossiter · 2018-02-12T17:56:32Z

SoftLayer/utils.py

    }


+def format_event_log_date(date_string, utc):


So it kinda surprises me that there are no unit tests for the utils module, but we should probably have some tests for these functions.

allmightyspiff · 2018-03-29T20:57:45Z

@fisherma91 have you had a chance to look at Ryan's suggestions?

fisherma91 · 2018-04-03T19:44:22Z

@allmightyspiff I have. I've just been heads down with forward development and haven't been able to work in his feedback.

allmightyspiff · 2019-02-05T21:42:17Z

going to be finished in #1099

fisherma91 and others added 15 commits August 23, 2017 14:59

security-groups-request-ids : Add output for RequestIDs

54f72c1

The python api will now output a table with requestIDs for specific calls to security groups apis

security-groups-request-ids : Add output for RequestIDs

4c0badf

Fix unit tests Fix code style issues

security-groups-request-ids : Add output for RequestIDs

7768a83

Add new return format

security-groups-request-ids : Add output for RequestIDs

d3650de

Add functionality to get event logs to slcli

security-groups-request-ids : Add output for RequestIDs

f4bc42f

Fix Security Group unit tests

security-groups-request-ids : Add output for RequestIDs

d7a736f

Refactor Event Log Code Fix Unit Tests Add Unit Tests Fix Code Styling

security-groups-request-ids : Add output for RequestIDs

d851de6

Remove unneeded code left over from refactoring Fix incorrect package name

security-groups-request-ids : Add output for RequestIDs

4228058

Code Styling changes

security-groups-request-ids : Add output for RequestIDs

9d04a8e

Change public facing name to Audit Logs Add functionality to get event log types

security-groups-request-ids : Add output for RequestIDs

a3406a1

Fix ordering of test expecations to be Actual then Expected

security-groups-request-ids : Add output for RequestIDs

884d117

Add functionality to filter by eventName

security-groups-request-ids : Add output for RequestIDs

7c25d97

Add functionality to filter by dates

security-groups-request-ids : Add output for RequestIDs

265460a

Add request id search functionality

Merge branch 'master' into security-groups-request-ids

a453d6d

security-groups-request-ids : Add output for RequestIDs

01ee771

Fix fixtures from merge

rlrossiter self-requested a review February 1, 2018 19:56

security-groups-request-ids : Add output for RequestIDs

3a72d7f

Fix tox issues

rlrossiter suggested changes Feb 2, 2018

View reviewed changes

rlrossiter suggested changes Feb 12, 2018

View reviewed changes

This was referenced Feb 5, 2019

Finishfish #1098

Closed

Finishfish erick-sapp/softlayer-python#2

Closed

allmightyspiff mentioned this pull request Feb 5, 2019

Security groups request ids #1099

Merged

allmightyspiff closed this Feb 5, 2019

		return filtered_logs


		def _parse_date(date_string, utc_offset):

		env.fout(table)


		def _build_filter(date_min, date_max, obj_event, obj_id, obj_type, utc_offset):


		self.assertEqual(expected, result)

		def test__get_cci_event_logs(self):



		@click.command()
		@click.option('--date_min', '-d',

Comments

Conversation

fisherma91 commented Feb 1, 2018

Uh oh!

coveralls commented Feb 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rlrossiter left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rlrossiter left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

allmightyspiff commented Mar 29, 2018

Uh oh!

fisherma91 commented Apr 3, 2018

Uh oh!

allmightyspiff commented Feb 5, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

coveralls commented Feb 1, 2018 •

edited

Loading