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

Cross account describe ASGs #527

Merged
merged 4 commits into from
Jun 5, 2015

Conversation

amanya
Copy link
Contributor

@amanya amanya commented May 28, 2015

The motivation of this patch is to make Eureka capable of knowing the state of auto scaling groups even if they are from microservices running in accounts other than the one where Eureka is running.

In that case, it will do a a cross-account assume role to get permission to describe the ASGs.

Here is a walkthrough from Amazon that explains how to setup the cross-account IAM role delegation:

The role must be called ListAutoScalingGroups and the policy must be like this:

{
    "Version": "2012-10-17",
    "Statement": [
        {
            "Action": [
                "autoscaling:DescribeAutoScalingGroups"
            ],
            "Effect": "Allow",
            "Resource": "*"
        }
    ]
} 

@cloudbees-pull-request-builder

NetflixOSS » eureka » eureka-pull-requests #239 SUCCESS
This pull request looks good

@cloudbees-pull-request-builder

NetflixOSS » eureka » eureka-pull-requests #241 SUCCESS
This pull request looks good

for (InstanceInfo instanceInfo : app.getInstances()) {
String thisAsgName = instanceInfo.getASGName();
if (thisAsgName != null && thisAsgName.equals(asgName)) {
return ((AmazonInfo) instanceInfo.getDataCenterInfo()).get(MetaDataKey.accountId);
Copy link
Contributor

Choose a reason for hiding this comment

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

The accountId metadata field was added to eureka-client relatively recently, so for backwards compatibility, it is possible in existing deployments to have older clients that do not register this field. This will return null here and cause an NPE upstream. Can you please add a null check here and let it fall through instead?

@qiangdavidliu
Copy link
Contributor

@amanya thank you for the pull request, this would be a very useful feature for Eureka to have. I added a couple of minor comments that I hope you can take a quick look at.

Additionally, have you been able to verify this code change? We have mockito as a dependency and it should be able to write a quick unit test for this mocking out the AWS SDK with mockito.

@cloudbees-pull-request-builder

NetflixOSS » eureka » eureka-pull-requests #260 SUCCESS
This pull request looks good

@amanya
Copy link
Contributor Author

amanya commented Jun 4, 2015

@qiangdavidliu thanks for your comments :)

I think I've managed to solve the first two issues, but for making the tests, I'll need to use a tool like Powermockito due to the static dependencies of the AwsAsgUtil class, right?

@qiangdavidliu
Copy link
Contributor

Thanks @amanya . We can merge this in and I'll take a look at what kind of testing we can do for this.

qiangdavidliu added a commit that referenced this pull request Jun 5, 2015
@qiangdavidliu qiangdavidliu merged commit 0854300 into Netflix:master Jun 5, 2015
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

3 participants