PXP-7805 Push audit logs to an AWS SQS #923
Conversation
10e3759 to
cd0a2e9
Compare
|
The style in this PR agrees with This formatting comment was generated automatically by a script in uc-cdis/wool. |
Pull Request Test Coverage Report for Build 11210
💛 - Coveralls |
| # `region` are required. Fields `aws_access_key_id` and | ||
| # `aws_secret_access_key` are optional. | ||
| PUSH_AUDIT_LOGS_CONFIG: | ||
| type: aws_sqs |
There was a problem hiding this comment.
defaulting to this means that existing deployments with ENABLE_AUDIT_LOGS on will now error until they flip this to api, right? We may want to note that in DEPLOYMENT CHANGES. I know you note something about enabling them, but may want to explicitly mentioned that even for deployments that DON'T want this, they have to update the cfg (unless you change this default)
fence/blueprints/data/indexd.py
Outdated
| resource_paths = indexed_file.index_document.get("authz", []) | ||
| if not resource_paths: | ||
| # fall back on ACL | ||
| resource_paths = indexed_file.index_document.get("acl", []) | ||
| if not protocol and indexed_file.indexed_file_locations: | ||
| if not protocol and len(indexed_file.indexed_file_locations) > 0: |
There was a problem hiding this comment.
shouldn't the truthy check above already handle this?
There was a problem hiding this comment.
which check? if the indexd record urls field is empty, we need to check this here - we record no protocol, and then the presigned URL generation function errors 'there are no locations'
There was a problem hiding this comment.
sorry, I meant the previous code. like why are we switching from indexed_file.indexed_file_locations to len(indexed_file.indexed_file_locations) > 0. For situations where urls are empty the previous code should have been False.
There was a problem hiding this comment.
woops, yeah i'm not sure what happened there 😆
fence/__init__.py
Outdated
| return request_url | ||
|
|
||
|
|
||
| @app.after_request |
There was a problem hiding this comment.
is this called after every request? 😬 I'm a little nervous of the overhead of this. I like the idea of consolidating and making it cleaner, but could we isolate this cleaning logic to only functions where we know we're capturing audit logs?
Maybe make a custom decorator and only decorate functions we know we want to audit log? Also only do the decorator logic if audit logs are enabled.
I also want to make sure that if someone doesn't want ANY logging, it shouldn't impact performance
There was a problem hiding this comment.
good point, i'll look into that
There was a problem hiding this comment.
Maybe something like this conditional decorator based on if audit logs are enabled. And then this function basically becomes a decorator only used when it's enabled and only on functions where we know we want to audit log.
| """ | ||
| Attempt to parse the request for token to authenticate the user. fallback to | ||
| populated information about an anonymous user. | ||
| By default, cast `sub` to str. Use `sub_type` to override this behavior. |
There was a problem hiding this comment.
why not just always cast to string?
There was a problem hiding this comment.
I didn't look too much into it, but authutils.current_token["sub"] is a string and flask.g.user.id is an int 🤔
I'm casting authutils.current_token["sub"] to int here because audit-service expects an int, to match the type of sub in the fence DB.
| raise InternalError("Unable to create audit log") | ||
|
|
||
| def create_audit_log(self, category, data): |
There was a problem hiding this comment.
You have another function called create_audit_log in a different module. Probably fine but might be confusing. Pending if/how we do the decorator/performance improvements
| @@ -479,6 +479,8 @@ AUDIT_SERVICE: 'http://audit-service' | |||
| ENABLE_AUDIT_LOGS: | |||
| presigned_url: false | |||
| login: false | |||
| PUSH_AUDIT_LOGS_CONFIG: | |||
| type: api | |||
There was a problem hiding this comment.
we should also write some tests that monkey patch this and add others for aws_sqs and mock out the AWS call for better coverage of the new feature
There was a problem hiding this comment.
Yeah the current tests are testing "type: api", so the audit_service_client module without the SQS part. Tbh I wasn't sure how to test it since everything would be mocked. But i'll spend some more time on that
There was a problem hiding this comment.
well just the boto call out to sqs would need to be mocked, you could check that it actually gets called with expected data
There was a problem hiding this comment.
oh, I see you added some 🎊
fence/resources/audit/client.py
Outdated
| ) | ||
|
|
||
| def ping(self): | ||
| max_tries = 3 |
There was a problem hiding this comment.
can you use backoff lib for this to remain consistent? we're using it elsewhere
fence/fence/resources/openid/ras_oauth2.py
Line 118 in b85118f
There was a problem hiding this comment.
you can override the exception/handling of default if you need to
fence/resources/audit/client.py
Outdated
| self.ping() | ||
| self.validate_config() | ||
| else: | ||
| logger.warn("NOT enabling audit logs") |
There was a problem hiding this comment.
.warn is deprecated I believe. you should use .warning
fence/resources/audit/client.py
Outdated
| f"Audit logs are enabled but audit-service is unreachable at {status_url}: {r.text}" | ||
| ) | ||
|
|
||
| def validate_config(self): |
There was a problem hiding this comment.
Don't hate me. I'm tempted to ask if we get some docstrings for all these public functions. Granted I think most are self explanatory. But for consistency, eh 🤷
Jira Ticket: PXP-7805
goes with uc-cdis/audit-service#2 and uc-cdis/cloud-automation#1603
New Features
Improvements
Deployment changes
gen3 sqs info $(gen3 api safe-name audit-sqs)), seefence/fence/config-default.yaml
Lines 632 to 636 in fec3dd9
kubectl delete secret fence-configandgen3 kube-setup-fence