Skip to content
This repository has been archived by the owner on Nov 5, 2021. It is now read-only.

Add AWS EC2 metadata sysvars #315

Closed
wants to merge 3 commits into from
Closed

Conversation

kohend
Copy link
Contributor

@kohend kohend commented Nov 14, 2019

Set sysvars relevant for EC2 instances when running on Amazon's EC2

Copy link
Contributor

@manugarg manugarg left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the pull request. Please find my comments inline.

sysvars/sysvars_ec2.go Outdated Show resolved Hide resolved
@@ -110,6 +110,10 @@ func Init(ll *logger.Logger, userVars map[string]string) error {
if err := gceVars(sysVars); err != nil {
return err
}
} else {
if err := getEC2Meta(sysVars); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

This will fail on all non-AWS environments? Is there a way to run getEC2Meta only while on AWS?

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like there is no way, short of looking at the hypervisor uuid file. In that case, I think we should log the error and ignore it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will not fail, when the metadata is not available it sets a boolean sysvar and returns nil.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See line 31 in the ec2 file

sysvars/sysvars_ec2.go Outdated Show resolved Hide resolved
sysvars/sysvars_ec2.go Outdated Show resolved Hide resolved
sysvars/sysvars_ec2.go Outdated Show resolved Hide resolved
sysvars/sysvars_ec2.go Outdated Show resolved Hide resolved
sysvars/sysvars_ec2.go Outdated Show resolved Hide resolved
return fmt.Errorf("Could not get instance document %v", err)
}

sysVars["EC2_METADATA_Availabile"] = "true"
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of adding all the variables by default, I'd start with a few that are useful for probing (also, some of this information may be security risk, e.g. DevpayProductCodes, AccountID).

Can you start with only: AvailabilityZone, Region, InstanceID, PrivateIP and may be ImageID?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed sensitive info, added ramdisk and architecture to the list

@kohend
Copy link
Contributor Author

kohend commented Nov 17, 2019

Replied inline and changed

@manugarg
Copy link
Contributor

Just a heads up, I'll committing this using the process described here:
https://github.com/google/cloudprober/blob/master/CONTRIBUTING.md#source-of-truth-sot-and-commit-process

@manugarg
Copy link
Contributor

@kohend I've committed this change through PR #318.

Thanks once again for the contribution! :)

@manugarg manugarg closed this Nov 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants