Retrieve the right AWS metadata for private hostname#849
Retrieve the right AWS metadata for private hostname#849asfgit merged 1 commit intoapache:masterfrom
Conversation
tbouron
force-pushed
the
fix/aws-local-hostname
branch
from
d7786ce to
2455e05
Compare
There was a problem hiding this comment.
thanks @tbouron, could we avoid the extra boolean parameter if we use something like
response=$(curl --write-out %{http_code} --silent --output /dev/null http://169.254.169.254/latest/meta-data/local-hostname)
if [ $(response) == "200" ] ;
then
echo `curl --silent --retry 20 http://169.254.169.254/latest/meta-data/local-hostname"`; exit"));
else
echo `curl --silent --retry 20 http://169.254.169.254/latest/meta-data/public-hostname"`; exit"));
or something similar?
There was a problem hiding this comment.
@andreaturli I don't think so. http://169.254.169.254/latest/meta-data/local-hostname will always return some data and a 200 as AWS assigns a private IP/host for every single EC2 instance.
http://169.254.169.254/latest/meta-data/public-hostname could return a 404 (if the machine has been provisioned without an elastic IP, not entirely sure though) but in this situation, but that's not what we want
There was a problem hiding this comment.
not sure I follow, @tbouron -- I think it would be easier to read if we have 2 methods
getPrivateAwsHostname and getPublicAwsHostname what do you think?
There was a problem hiding this comment.
@andreaturli In you code example, http://169.254.169.254/latest/meta-data/local-hostname will always return a 200 (as an EC2 instance always as a private IP) so your if statement will always go one way which is not what we want here.
It would may be easier to read (although I'm not sure about that) but will also introduce some code duplication which I would be reluctant do to, especially in this huge file.
There was a problem hiding this comment.
sorry @tbouron, String getHostnameAws(...) is already very confusing, not mentioning the fact that is AWS specific which is another smell, IMHO.
Which hostname is going to return that method?
I think we have the chance to improve this area of the code and I think another (boolean) parameter is not going to help in that respect. wdyt?
There was a problem hiding this comment.
If you are ok to add even more code + duplication then sure, no problem, I'll do that :D
Agree this is a code smell (which has already been identified based on this comment) but I don't have a magical solution to improve this particular area. If you have any idea, I'm opened to suggestions @andreaturli
There was a problem hiding this comment.
@tbouron if we can rely on jclouds for getting the private hostname usingNodeMetadata.getHostname, Brooklyn doesn’t need to calculate a new one especially for nodes created using JcloudsLocation. Notice NodeMetadata.getHostname is nullable so I think JcloudsLocation should manage that or assign the value to host.addresses.private
There was a problem hiding this comment.
@andreaturli I did what you suggested: using NodeMetadata.getHostname or reusing the getPrivateHostnameGeneric() method for private hostname. The public hostname is calculated as before.
tbouron
force-pushed
the
fix/aws-local-hostname
branch
from
2455e05 to
8877368
Compare
| // exceptional situation rather than a pattern to follow. We need a better way to | ||
| // do cloud-specific things. | ||
| if ("aws-ec2".equals(provider) && Boolean.TRUE.equals(lookupAwsHostname)) { | ||
| Maybe<String> result = getHostnameAws(node, sshHostAndPort, userCredentials, setup); |
There was a problem hiding this comment.
looks much easier @tbouron -- can we remove getHostnameAws or is it used elsewhere?
There was a problem hiding this comment.
@andreaturli It is unfortunately used by the DefaultConnectivityResolver class
|
@andreaturli Looks like it works as expected :)
|
|
lgtm thanks @tbouron |
3 participants
In most conditions for AWS deployments (machine sshable) Brooklyn was retrieving the
public-hostnamemetadata for both public AND private hostname sensors.