[DC] Meraki Device Connector #290

edulop91 · 2019-08-28T19:03:20Z

No description provided.

…alues

…rror

Replacing underscore with actual description Co-Authored-By: Eduardo Lopez <elopez@chanzuckerberg.com>

…alues

…rror

Replacing underscore with actual description Co-Authored-By: Eduardo Lopez <elopez@chanzuckerberg.com>

…Alert into aku/meraki_connector

This reverts commit 26d1589.

sfc-gh-gbutzi · 2019-09-06T17:27:51Z

src/connectors/meraki.py

+
+
+def connect(connection_name, options):
+    landing_table_client = f'data.meraki_{connection_name}_connection_client'


The table names have to end in _CONNECTION or they're not considered a connection table (i.e. the connection runner won't pick them up and call the ingest method on the module for the table). So you'd want these to be f'data.meraki_{connection_name}_client_connection' or '_device_connection'.

sfc-gh-gbutzi · 2019-09-06T17:29:58Z

src/connectors/meraki.py

+    return None
+
+
+def ingest(table_name, options):


So your connect method creates two tables, both of which I think you want to be connection tables. But the ingest method populates both of them every time it gets called, which means that you're going to be duplicating data in your tables; you'll populate both tables when the connection runner finds the client_connection table, and then you'll populate both tables again a second later when the connection runner finds the device_connection table.

For each call to the ingest method, it should populate one table: the table which ingest is called for.

sfc-gh-afedorov · 2019-09-06T20:08:46Z

src/connectors/meraki.py

+    except HTTPError as http_err:
+        log.error(f"Error GET: url={url}")
+        log.error(f"HTTP error occurred: {http_err}")
+        raise http_err


re-raise with just raise, that way the stack trace is maintained rather than being reset to this point

sfc-gh-afedorov · 2019-09-06T20:19:47Z

src/connectors/meraki.py

+                    client.get('mdnsName','None'),
+                    client.get('dhcpHostname','None'),
+                    client.get('ip','None'),
+                    parse_number(client.get('vlan','None')),


not sure what the client value is, but agreed this looks buggy. right now it does:

client.get('vlan', 'None') or None

are you sure this is what you want?

sfc-gh-afedorov · 2019-09-06T20:20:26Z

src/connectors/meraki.py

+                    client.get('ip','None'),
+                    parse_number(client.get('vlan','None')),
+                    client.get('switchport','None'),
+                    parse_number(client.get('usage','None').get('sent','None')),


You can't .get on a string (i.e. 'None'), so this will raise an exception if 'usage' not in client

sfc-gh-afedorov · 2019-09-06T20:21:23Z

src/connectors/meraki.py

@@ -0,0 +1,181 @@
+"""Meraki


Let's make this name a little more descriptive -- there are a lot of things we could potentially collect from Meraki, and the titles should be distinct.

Co-Authored-By: Eduardo Lopez <elopez@chanzuckerberg.com>

sfc-gh-afedorov · 2019-09-07T00:17:44Z

src/connectors/meraki_devices.py

+                    log.error(e)
+                    continue
+
+                db.insert(


sorry I wasn't more clear on this. could you please replace this db.insert expression with --

db.insert( landing_table, values=[( timestamp, client, client.get('id'), client.get('mac'), client.get('description'), client.get('mdnsName'), client.get('dhcpHostname'), client.get('ip'), client.get('switchport'), client.get('vlan') or None, client.get('usage', {}).get('sent') or None, client.get('usage', {}).get('recv') or None, serial_number, ) for client in clients], select=db.derive_insert_select(LANDING_TABLE_COLUMNS_CLIENT), columns=db.derive_insert_columns(LANDING_TABLE_COLUMNS_CLIENT) )

I think it will do the same thing but more succinctly.

to clarify x or y will return y if x if "falsy", i.e. if bool(x) is False, and dict's .get returns None as the default value if the key is not present

also, are you sure vlan needs to be treated differently than ip or switchport? this seems off.

Sometimes client.get('vlan') will return '' instead of None, which cannot be inserted into the database since the VLAN column requires a number. I like your suggested change though! I just can't treat vlan the same way as ip because that '' value is invalid in our case.

Got it, interesting. If you're sure that the others never return '' then lgtm. Adding a comment like --

client.get('vlan') or None, # vlan sometimes set to ''

might help to future folks, as well

I've never seen that occur with usage:recv or usage:sent, but I think it'll be best if we used the same approach for those instances too. Thank you for this feedback!

sfc-gh-afedorov · 2019-09-07T00:21:30Z

src/connectors/meraki_devices.py

+                values=[(
+                    timestamp,
+                    device,
+                    device.get('serial',None),


please remove all of these ,None, as they're vestigial (None is the default value when key is not present), and per pep8, the comma in param lists needs to be followed by a space or newline.

sfc-gh-afedorov · 2019-09-07T00:22:39Z

src/connectors/meraki_devices.py

+    comment = yaml_dump(module='meraki_devices', **options)
+
+    db.create_table(name=landing_table_client,
+                    cols=LANDING_TABLE_COLUMNS_CLIENT, comment=comment)


add a newline before "comment" please to adhere to PEP8

sfc-gh-afedorov · 2019-09-07T00:52:20Z

@edulop91 could you give me push access to this repo so I can help push minor cleanup a little faster than explaining it in comments? I'll still comment on major things that have value to see but whitespace stuff seems it might become tedious.

sfc-gh-afedorov

looking close. seeing a couple of minor bugs and a few more fixes. looks like I can edit in GitHub so don't hesitate to ask questions if you have any.

sfc-gh-afedorov · 2019-09-07T00:35:53Z

src/connectors/meraki_devices.py

+
+def ingest(table_name, options):
+    ingest_type = ''
+    if (table_name.endswith('CONNECTION_CLIENT')) or (table_name.endswith('connection_client')):


what Greg was meaning to explain here earlier is that no table that ends with anything other than "_CONNECTION" will ever be passed into the ingest function. the runner that runs these ingest function is pretty bare-bones, check it out here.

it's not super apparent, but it also guarantees that the connections will be upper case, so you here you can do something like

ingest_type = 'client' if table_name.endswith('_CLIENT_CONNECTION') else 'device'

but you do also need to make sure that the tables are created ending in _CONNECTION above so that the runner knows they are "connection tables", i.e. should be populated by the connector runners.

I think I know the answer already, but does the table name have to be all-caps? (I'm guessing yes)

It depends. If you're working with the string in python, then yes; python is case-sensitive and so the strings '_connection' and '_CONNECTION' are not identical.

If you're working with the table in sql, then no; sql is generally case insensitive (more precisely, it will uppercase everything you say unless you explicitly tell it not to) and it will view select * from data.meraki_connection the same as SELECT * FROM DATA.MERAKI_CONNECTION.

Note that you can get around this by putting something in double quotes inside sql; that is,

select * from "DATA"."MERAKI_CONNECTION" is not the same as select * from "data"."meraki_connection"; one of those will look for a table with an uppercase name and one will look for a table with a lowercase name.

Does that answer your question? Happy to clarify if the above is confusing!

would it be annoying if I put:
ingest_type = 'client' if (table_name.endswith('_CLIENT_CONNECTION')or table_name.endswith('_client_connection')) else 'device' ?

What situation are you trying to protect against? Snowflake defaults to uppercase names, so you have to explicitly specify that you want an object to have a lowercase name by putting it in double-quotes. We don't do that in the connectors - do you have reason to think that somebody might create a connection table manually and explicitly name it "meraki_client_connection", and still want the connector to work with it?

Ohh, I think I understand. Thank you for clarifying! I'm not worried about lower-case confusion anymore.

sfc-gh-afedorov · 2019-09-07T00:36:18Z

src/connectors/meraki_devices.py

+
+
+def connect(connection_name, options):
+    landing_table_client = f'data.meraki_devices_{connection_name}_connection_client'


change the suffix here to _client_connection

sfc-gh-afedorov · 2019-09-07T00:36:30Z

src/connectors/meraki_devices.py

+
+def connect(connection_name, options):
+    landing_table_client = f'data.meraki_devices_{connection_name}_connection_client'
+    landing_table_device = f'data.meraki_devices_{connection_name}_connection_device'


change the suffix here to _device_connection

sfc-gh-afedorov · 2019-09-07T00:40:59Z

src/connectors/meraki_devices.py

+    ('DHCP_HOSTNAME', 'VARCHAR(256)'),
+    ('IP', 'VARCHAR(256)'),
+    ('SWITCHPORT', 'VARCHAR(256)'),
+    ('VLAN', 'INT'), 


here and in a couple of other places, there are stray spaces before the end of the line, which violates pep8 standards. could you set your text editor to remove extra spaces at the end of each line on save? to do it in Sublime, you can add this to preferences —

"trim_trailing_white_space_on_save": true,

happy to look up in other editors if you're not sure how

sfc-gh-afedorov · 2019-09-07T00:41:33Z

src/connectors/meraki_devices.py

+
+from datetime import datetime
+
+import snowflake


please remove unused import -- pyflakes will help you spot these, as well. let me know if you'd like help setting it up with your text editor.

Do you know if flake8 works for this purpose?

sfc-gh-afedorov · 2019-09-07T00:49:34Z

src/connectors/meraki_devices.py

+    whitelist = options['network_id_whitelist']
+
+    for network in whitelist:
+


remove extra blank line

sfc-gh-afedorov · 2019-09-07T00:55:04Z

src/connectors/meraki_devices.py

+def connect(connection_name, options):
+    landing_table_client = f'data.meraki_devices_{connection_name}_connection_client'
+    landing_table_device = f'data.meraki_devices_{connection_name}_connection_device'
+    options['network_id_whitelist'] = options.get('network_id_whitelist', '').split(',')


this will return a Union[str, list] when I think you'd be better off with just a list, i.e. put [] in the second param of get.

also, up to you if you're comfortable, but I would have made this a change to connectors_runner.py and added the list type to be parsed there next to json. later on, we can also add a special UI element.

I guess I have a few questions. This function "returns" a dictionary with a message but it creates a table and grants roles. What are you referring to when you say "This will return a Union[str, list]"? Are you referring to options?

Would adding the list type in connectors_runner.py parse the user input as a list? When we tested this particular input, we got it as a string, like 'id1, id2, id3'.

How does this look? @andrey-snowflake
@greg-snowflake #306

Perfect, thanks.

Err, had a bit of a think-o. The "return" type I was thinking of was of the expression

options.get('network_id_whitelist', '').split(',')

but since it splits the empty string, it's always a list.

sfc-gh-afedorov · 2019-09-07T00:55:33Z

src/connectors/meraki_devices.py

+    db.execute(f'GRANT INSERT, SELECT ON {landing_table_client} TO ROLE {SA_ROLE}')
+
+    db.create_table(name=landing_table_device,
+                    cols=LANDING_TABLE_COLUMNS_DEVICE, comment=comment)


ditto -- it's easier to read if each param gets its own line

kuannie1 · 2019-09-09T22:44:08Z

@greg-snowflake @andrey-snowflake: I think I'm done making changes. How does this look now? Given my pull request #306

kuannie1 and others added 30 commits August 27, 2019 09:25

first commit

c001f5e

meraki connector skeleton (needs custom updating)

1d8a45b

rebase

b0e4801

meraki logo

b30174b

changed secrets

24f0664

a connector that runs. next: making more tables and double checking v…

92b01f0

…alues

create two tables, has numeric value empty string is not recognized e…

179325b

…rror

removed None

f509053

deleted unnecessary functions and included db.py functions

d777a21

debug

706624a

ingesting meraki client & device info and inserting into landing table

81f4a11

Update src/connectors/meraki.py

327ae3f

Replacing underscore with actual description Co-Authored-By: Eduardo Lopez <elopez@chanzuckerberg.com>

first commit

fd457c3

meraki connector skeleton (needs custom updating)

fcc2520

rebase

2a8c475

meraki logo

bb027a5

changed secrets

d0eb88a

a connector that runs. next: making more tables and double checking v…

a881d9b

…alues

create two tables, has numeric value empty string is not recognized e…

e413652

…rror

removed None

d7bb036

deleted unnecessary functions and included db.py functions

47859d9

debug

db03993

ingesting meraki client & device info and inserting into landing table

e7487e1

Update src/connectors/meraki.py

b154502

Replacing underscore with actual description Co-Authored-By: Eduardo Lopez <elopez@chanzuckerberg.com>

Merge branch 'aku/meraki_connector' of github.com:chanzuckerberg/Snow…

d39979e

…Alert into aku/meraki_connector

made some successful changes but pushing before I break anything else

f78dc8a

combined table variables

4bf02cb

fixed try except network

a0f99f4

fixed syntax

409b2d0

deleting redundant pictures

f7d259d

Revert "unfinished meraki file"

6260259

This reverts commit 26d1589.

sfc-gh-gbutzi reviewed Sep 6, 2019

View reviewed changes

sfc-gh-afedorov requested changes Sep 6, 2019

View reviewed changes

kuannie1 and others added 6 commits September 6, 2019 13:26

only 1 table in ingest()

9d8b261

Update src/connectors/__init__.py

af7c6c4

Co-Authored-By: Eduardo Lopez <elopez@chanzuckerberg.com>

Eduardo's suggestions from code review

a386650

Co-Authored-By: Eduardo Lopez <elopez@chanzuckerberg.com>

Apply suggestions from code review

c020007

Co-Authored-By: Eduardo Lopez <elopez@chanzuckerberg.com>

renaming meraki to meraki_devices and code review suggestions

75e301e

andrey's feedback

a8f2f38

sfc-gh-afedorov reviewed Sep 7, 2019

View reviewed changes

sfc-gh-afedorov added 4 commits September 6, 2019 17:57

Delete cisco_umbrella.png

7ae202a

Merge branch 'v1.8.6' into aku/meraki_connector

a0fb70c

Update __init__.py

c09881b

Update meraki_devices.py

383b997

sfc-gh-afedorov requested changes Sep 7, 2019

View reviewed changes

kuannie1 and others added 2 commits September 9, 2019 11:21

followed pep8 standards

b466ccc

Merge branch 'v1.8.6' into aku/meraki_connector

b147f05

kuannie1 and others added 2 commits September 9, 2019 16:17

meraki works with WebUI

158ccd8

Update meraki_devices.py

78acd49

sfc-gh-afedorov approved these changes Sep 10, 2019

View reviewed changes

sfc-gh-afedorov merged commit ce826ea into snowflakedb:v1.8.6 Sep 10, 2019

sfc-gh-afedorov pushed a commit that referenced this pull request Sep 10, 2019

[DC] Adds Meraki Device Data Connector (#290)

c67f8ee

sfc-gh-afedorov mentioned this pull request Sep 10, 2019

v1.8.6 More Connectors, New Handlers, and Baseline fixes #308

Merged

sfc-gh-afedorov pushed a commit that referenced this pull request Sep 18, 2019

[DC] Adds Meraki Device Data Connector (#290)

ae435bb



		def connect(connection_name, options):
		landing_table_client = f'data.meraki_{connection_name}_connection_client'



		def connect(connection_name, options):
		landing_table_client = f'data.meraki_devices_{connection_name}_connection_client'

		whitelist = options['network_id_whitelist']

		for network in whitelist:

[DC] Meraki Device Connector #290

[DC] Meraki Device Connector #290

Conversation

edulop91 commented Aug 28, 2019

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

sfc-gh-afedorov Sep 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kuannie1 Sep 9, 2019 • 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

sfc-gh-afedorov commented Sep 7, 2019

sfc-gh-afedorov left a 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

kuannie1 Sep 9, 2019 • 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

kuannie1 Sep 9, 2019 • edited Loading

Choose a reason for hiding this comment

kuannie1 Sep 9, 2019 • edited Loading

Choose a reason for hiding this comment

kuannie1 Sep 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kuannie1 commented Sep 9, 2019

sfc-gh-afedorov Sep 7, 2019 •

edited

Loading

kuannie1 Sep 9, 2019 •

edited

Loading

kuannie1 Sep 9, 2019 •

edited

Loading

kuannie1 Sep 9, 2019 •

edited

Loading

kuannie1 Sep 9, 2019 •

edited

Loading

kuannie1 Sep 9, 2019 •

edited

Loading