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.

Sign up for GitHub

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

Jump to bottom

[CA-849] Trace auth calls #434

Merged
merged 18 commits into from
May 29, 2020
Merged

[CA-849] Trace auth calls #434

merged 18 commits into from
May 29, 2020

Conversation

andy7i
Copy link
Contributor

@andy7i andy7i commented May 14, 2020 •
edited
Loading

Ticket: https://broadworkbench.atlassian.net/browse/CA-849
Goal: trace auth calls.
Follow on work from Sam tracing work: #421 (comment)

This PR includes:
Adds tracing for auth calls
Adds SamRequestContextDirectives
Adds a new directive withNewTraceSpan
Light plumbing

Example trace:
Screen Shot 2020-05-27 at 7 01 27 PM

PR checklist

  • I've followed the instructions if I've made any changes to the API, especially if they're breaking changes
  • I've updated the RC_XXX release ticket with any manual steps required to release this change
  • I've updated the FISMA documentation if I've made any security-related changes, including auth, encryption, or auditing

@coveralls
Copy link

coveralls commented May 20, 2020 •
edited
Loading

Coverage Status

Coverage decreased (-0.6%) to 75.478% when pulling 7dc4b2b on aj--trace-ic--ca-849 into 8e80892 on develop.

@andy7i
Copy link
Contributor Author

andy7i commented May 21, 2020

jenkins retest -- fiab-start failed with an unrelated issue

10:46:02 [ERROR] POST https://us-central1-broad-workbench-tos-qa.cloudfunctions.net/tos/v1/user/response failed with status 400
10:46:02 "Error writing user response: TermsOfService fiab-automation-upward-dragon/1 does not exist."

@andy7i
Copy link
Contributor Author

andy7i commented May 21, 2020

jenkins retest - fiab-start failed with:

10:58:34 ERROR: for consent-mongo-1  sandbox b00909f34cd031047318b0de30de5d0c8080968113c5f4f3612dd88fc4972a37 not found
10:58:34 
10:58:34 ERROR: for agora-mongo  sandbox 254229972b9b9b4f520468e19a0035484e3cdb2d74f41f09df7e5f83af4964f5 not found
10:58:34 
10:58:34 ERROR: for bond-app  sandbox 3ba27453366fe5691bb083878e45d6fb6a385a2ca6b4ab0c8c2b008d395e5e84 not found

@andy7i andy7i changed the title [CA-849] Trace IC (WIP) [CA-849] Trace IC May 21, 2020
@andy7i andy7i marked this pull request as ready for review May 21, 2020 15:24
@andy7i
Copy link
Contributor Author

andy7i commented May 21, 2020

jenkins retest - fiab-start failed with:

12:26:00 ERROR: for consent-mongo-1  sandbox b00909f34cd031047318b0de30de5d0c8080968113c5f4f3612dd88fc4972a37 not found
12:26:00 
12:26:00 ERROR: for agora-mongo  sandbox 254229972b9b9b4f520468e19a0035484e3cdb2d74f41f09df7e5f83af4964f5 not found
12:26:00 
12:26:00 ERROR: for bond-app  sandbox 3ba27453366fe5691bb083878e45d6fb6a385a2ca6b4ab0c8c2b008d395e5e84 not found

@andy7i
Copy link
Contributor Author

andy7i commented May 22, 2020 •
edited
Loading

jenkins retest - leo test aborted after being stuck for 2 hours:

docker ps (before kill running containers older than 2 days):
CONTAINER ID        IMAGE                                                          COMMAND                  CREATED             STATUS                          PORTS               NAMES
13ceb7adc504        gcr.io/broad-dsp-gcr-public/leonardo-cluster-cleanup:initial   "/bin/sh"                2 days ago          Up 2 days                                           sql-client-container
4192782a9dae        owasp/zap2docker-weekly                                        "zap-baseline.py -t …"   3 weeks ago         Up 3 weeks (health: starting)                       friendly_einstein
Build timed out (after 120 minutes). Marking the build as aborted.

@andy7i
Copy link
Contributor Author

andy7i commented May 22, 2020

jenkins retest - rawls tests passed but then timed out

Tests Passed

Error response from daemon: No such container: automation-s2gyocwy
Test container already stopped
Build timed out (after 120 minutes). Marking the build as aborted.

https://fc-jenkins.dsp-techops.broadinstitute.org/job/rawls-fiab-test-runner/22496/console

@andy7i
WIP - (re)implementing tracing directive
6d4049a
@andy7i
definitely reimplemented tracing directive
fa97f97
@andy7i
Revert "definitely reimplemented tracing directive"
3a15d8f 
This reverts commit fa97f97
@andy7i
WIP - break security and use private methods in TracingDirective
e957f3c 
unresolved error:
```
SamModelDirectives.scala:58:39: value & is not a member of Object
[error]         recordSuccess.invoke(newSpan) & recordException.invoke(newSpan) & provide(newSamRequestContext)
[error]                                       ^
```
@andy7i
Revert "WIP - break security and use private methods in TracingDirect…
238434e 
…ive"

This reverts commit e957f3c
@andy7i
Revert "Revert "definitely reimplemented tracing directive""
a81b2bd 
This reverts commit 3a15d8f
@andy7i
minor text changes
b974c6b
@andy7i
created new SamRequestContextDirectives and unfortunately copied the …
241ea48 
…private trace directives from TraceDirectives. The other option would be to change the accessibility (which is modeled in a prior commit) but may run counter to SecurityManager.
@andy7i andy7i changed the title [CA-849] Trace IC [CA-849] Trace auth calls May 28, 2020
@andy7i andy7i requested review from gpolumbo-broad and dvoet and removed request for gpolumbo-broad May 28, 2020 15:29
@andy7i
Copy link
Contributor Author

andy7i commented May 28, 2020

jenkins retest - quay issues

dvoet
dvoet reviewed May 29, 2020
View reviewed changes
src/main/scala/org/broadinstitute/dsde/workbench/sam/api/SamRequestContextDirectives.scala Outdated
* parentSpan.
* @param samRequestContext the existing samRequestContext.
*/
def withNewTraceSpan(spanName: String, samRequestContext: SamRequestContext): Directive1[SamRequestContext] =
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would call this withChildTraceSpan

dvoet
dvoet approved these changes May 29, 2020
View reviewed changes
Copy link
Collaborator

@dvoet dvoet left a comment

Choose a reason for hiding this comment

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

naming nit then 👍

@andy7i andy7i merged commit 352225d into develop May 29, 2020
@andy7i andy7i deleted the aj--trace-ic--ca-849 branch May 29, 2020 21:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dvoet dvoet dvoet approved these changes

@gpolumbo-broad gpolumbo-broad gpolumbo-broad approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@andy7i @coveralls @dvoet @gpolumbo-broad