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

Leo/Sam auth: handle Sam API exceptions in SamAuthProvider #138

Merged
merged 9 commits into from
Jan 29, 2018
Merged

Leo/Sam auth: handle Sam API exceptions in SamAuthProvider #138

merged 9 commits into from
Jan 29, 2018

Conversation

rtitle
Copy link
Collaborator

@rtitle rtitle commented Jan 25, 2018 •
edited
Loading

I made a LeoAuthProviderHelper and ServiceAccountProviderHelper which wrap LeoAuthProvider and ServiceAccountProvider respectively and recover all thrown exceptions to LeoException. I was thinking this would be a better approach than assuming the providers are doing exception handling themselves (because they may not be written by us).

Also added some specs to make sure exception handling is working. I don't think additional auto tests are needed for this. I haven't run auto tests yet. :)

Have you read CONTRIBUTING.md lately? If not, do that first.

I, the developer opening this PR, do solemnly pinky swear that:

  • I've documented my API changes in Swagger

In all cases:

  • Get two thumbsworth of review and PO signoff if necessary
  • Verify all tests go green
  • Squash and merge; you can delete your branch after this
  • Test this change deployed correctly and works on dev environment after deployment

rtitle
rtitle commented Jan 25, 2018
View reviewed changes
src/main/scala/org/broadinstitute/dsde/workbench/leonardo/Boot.scala
@@ -86,20 +87,6 @@ object Boot extends App with LazyLogging {
}
}

private def constructServiceAccountProvider(className: String, config: Config): ServiceAccountProvider = {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moved to the *Helper classes.

rtitle
rtitle commented Jan 25, 2018
View reviewed changes
src/main/scala/org/broadinstitute/dsde/workbench/leonardo/service/LeonardoService.scala Outdated
authProvider.canSeeAllClustersInProject(userInfo.userEmail, googleProject) flatMap {
authProvider.canSeeAllClustersInProject(userInfo.userEmail, googleProject).recover { case NonFatal(e) =>
logger.warn(s"Sam returned an exception for resource ${googleProject.value}. Filtering out from list results.", e)
false
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

"list clusters" should not throw exceptions for individual clusters: it should filter them out.

@rtitle
Copy link
Collaborator Author

rtitle commented Jan 26, 2018 •
edited
Loading

fiab-start is failing:

Pulling leonardo-app (broadinstitute/leonardo:rt-gawb-3107)...
manifest for broadinstitute/leonardo:rt-gawb-3107 not found
Build step 'Execute shell' marked build as failure

EDIT: just needed to rebase

@rtitle rtitle requested a review from helgridly January 26, 2018 14:20
helgridly
helgridly approved these changes Jan 29, 2018
View reviewed changes
Copy link
Contributor

@helgridly helgridly left a comment

Choose a reason for hiding this comment

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

nice; this will help with #159 too

src/main/scala/org/broadinstitute/dsde/workbench/leonardo/service/LeonardoService.scala Outdated
@@ -207,12 +206,18 @@ class LeonardoService(protected val dataprocConfig: DataprocConfig,
clustersByProject = clusterList.groupBy(_.googleProject)
visibleClusters <- clustersByProject.toList.flatTraverse[Future, Cluster] { case (googleProject, clusters) =>
val clusterList = clusters.toList
authProvider.canSeeAllClustersInProject(userInfo.userEmail, googleProject) flatMap {
authProvider.canSeeAllClustersInProject(userInfo.userEmail, googleProject).recover { case NonFatal(e) =>
logger.warn(s"The auth provider returned an exception for resource ${googleProject.value}. Filtering out from list results.", e)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: note that this is a call to canSeeAllClustersInProject in this text

jmthibault79
jmthibault79 reviewed Jan 29, 2018
View reviewed changes
src/test/scala/org/broadinstitute/dsde/workbench/leonardo/auth/LeoAuthProviderHelperSpec.scala
Future.failed(ClusterNotFoundException(googleProject, name1))
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a comment about why this test and the next have exceptions

jmthibault79
jmthibault79 approved these changes Jan 29, 2018
View reviewed changes
Copy link
Contributor

@jmthibault79 jmthibault79 left a comment

Choose a reason for hiding this comment

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

Looks good

@rtitle
Copy link
Collaborator Author

rtitle commented Jan 29, 2018

fiab-test-leonardo passed; fiab-test is failing due to infrastructure issues.

@rtitle
Copy link
Collaborator Author

rtitle commented Jan 29, 2018

jenkins retest

2 similar comments
@rtitle
Copy link
Collaborator Author

rtitle commented Jan 29, 2018

jenkins retest

@rtitle
Copy link
Collaborator Author

rtitle commented Jan 29, 2018

jenkins retest

@rtitle rtitle merged commit 99bacf5 into develop Jan 29, 2018
@rtitle rtitle deleted the rt-gawb-3107 branch January 29, 2018 21:23
akarukappadath pushed a commit that referenced this pull request Jul 11, 2018
@rtitle @akarukappadath
Leo/Sam auth: handle Sam API exceptions in SamAuthProvider (#138)
6724fce 
* Add error handling to providers

* Add tests

* Clean up

* Add logging

* Exception recovery not needed on SamAuthProvider

* Better specs and error handling

* Add comment

* Log text nit

* Add comment to specs
akarukappadath pushed a commit that referenced this pull request Oct 22, 2018
@rtitle @akarukappadath
Leo/Sam auth: handle Sam API exceptions in SamAuthProvider (#138)
cd55fee 
* Add error handling to providers

* Add tests

* Clean up

* Add logging

* Exception recovery not needed on SamAuthProvider

* Better specs and error handling

* Add comment

* Log text nit

* Add comment to specs
Qi77Qi pushed a commit that referenced this pull request Jun 25, 2019
@rtitle @Qi77Qi
Leo/Sam auth: handle Sam API exceptions in SamAuthProvider (#138)
0964a23 
* Add error handling to providers

* Add tests

* Clean up

* Add logging

* Exception recovery not needed on SamAuthProvider

* Better specs and error handling

* Add comment

* Log text nit

* Add comment to specs
jchanbroad pushed a commit that referenced this pull request Jun 27, 2019
@rtitle @jchanbroad
Leo/Sam auth: handle Sam API exceptions in SamAuthProvider (#138)
78b54a2 
* Add error handling to providers

* Add tests

* Clean up

* Add logging

* Exception recovery not needed on SamAuthProvider

* Better specs and error handling

* Add comment

* Log text nit

* Add comment to specs
jdcanas pushed a commit that referenced this pull request Jul 10, 2019
@rtitle
Leo/Sam auth: handle Sam API exceptions in SamAuthProvider (#138)
a5f1ee5 
* Add error handling to providers

* Add tests

* Clean up

* Add logging

* Exception recovery not needed on SamAuthProvider

* Better specs and error handling

* Add comment

* Log text nit

* Add comment to specs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@helgridly helgridly helgridly approved these changes

@jmthibault79 jmthibault79 jmthibault79 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
3 participants
@rtitle @helgridly @jmthibault79