-
Notifications
You must be signed in to change notification settings - Fork 133
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
Aws cross account #121
base: master
Are you sure you want to change the base?
Aws cross account #121
Conversation
How is this different than #120? Can you provide a short description of the problem this change solves? |
@brharrington Both #120 and #121 are same. Only differences between these two pull requests are |
edda.region=us-west-1 | ||
edda.region=us-east-1 | ||
edda.aws.assumeRoleArn= | ||
edda.aws.assumerole.enabled=true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't set this as the default/example
|
||
override def doCrawl()(implicit req: RequestId) = | ||
backoffRequest { ctx.awsClient.ec2(true).describeAddresses(request).getAddresses.asScala.map( | ||
item => Record(item.getPublicIp, ctx.beanMapper(item))) }.toSeq ++ (if(assumeRoleEnabled) doNonAssumeCrawl() else Seq.empty[Record]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this logic doesn't look correct, if assumeRoleEnabled == true then doNonAssumeCrawl()?
this structuring is pretty awkward, feels like the conditional should be handled up front or pushed down outside of this context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When assumeRoleEnabled = true
, two kind of Crawl()
is done.
doCrawl()
pulls data from account where edda is hosteddoNonAssumeCrawl()
pulls data from account of ARN given
Code can be optimized bit more. I shall do it and push back
I started trying to review this but I don't think it's a reasonable approach. The AssumeRole functionality shouldn't bleed into every crawl implementation IMO. |
No description provided.