Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #15

Merged
merged 8 commits into from
Mar 3, 2020
Merged

Fixes #15

merged 8 commits into from
Mar 3, 2020

Conversation

reginafcompton
Copy link
Contributor

@reginafcompton reginafcompton commented Mar 3, 2020

Overview

This PR handles the following Clubhouse cards:

Getting and using tokens

With this PR, the process for getting a token and hitting a secure endpoint unfolds like so:

  1. Request a token
  2. Store the token in a tmp file
  3. If the token cannot be requested, then store an "invalid token" in the tmp file
  4. Read the token from the tmp file, and use it to GET the MCI and DR APIs

Handling database connections

This PR also insures that the metrics collector properly closes database connections. This includes two major changes:

  1. Instantiate the database engine ONCE, i.e., at the beginning of the audit function. This differs from the original solution, which instantiate an engine every time the script executed.

The typical usage of create_engine() is once per particular database URL, held globally for the lifetime of a single application process....the Engine is most efficient when created just once at the module level of an application, not per-object or per-function call. (SQLAlchemy Docs)

  1. Explicitly open a data base connection and close it after running a query:
    connection = engine.connect()
    result = connection.execute(f"SELECT COUNT(*) from {endpoint}")
    count, = result.fetchone()
    connection.close()

Copy link
Contributor

@gregmundy gregmundy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The solution seems reasonable, although I would think you'd want to save yourself some overhead and simply maintain the token in memory since the application doesn't actually exit. Even if the container died or restarted, the temp file wouldn't persist and you'd have to retrieve a new token anyway.

The concern I have here is that by writing it to a temp file is that you're exposing the token outside of the application (meaning I could log in to a node and see the contents of that file). But, if someone from the outside were logged in to our k8s cluster, we'd have much problems. This becomes more dangerous if you're running the logger API outside of k8s (which is possible). No need to fix right now, but may need to re-think that solution in the future.

@reginafcompton reginafcompton merged commit 199dd23 into master Mar 3, 2020
@reginafcompton reginafcompton deleted the fixes branch March 3, 2020 20:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants