ec2_tags.py prints stacktrace when run outside of EC2

[frioux]

I am not really a python programmer but this is crazy to me, and my coworker who does claim to be a snake lord was confused too.

Basically, ec2_tags.py raises an exception that is uncatchable unless you import urllib2.exceptions.URLError. The even weirder thing is that the exception is catchable when the code is run directly from python; only when run via salt does this issue manifest itself.

This patch (not a PR because I'm not confident in it enough) fixed it, but I have no idea why:

diff --git a/salt/_grains/ec2_tags.py b/salt/_grains/ec2_tags.py
index 1f03d99..0b4ab8d 100644
--- a/salt/_grains/ec2_tags.py
+++ b/salt/_grains/ec2_tags.py
@@ -53,6 +53,7 @@ from distutils.version import StrictVersion
 import boto.ec2
 import boto.utils
 import salt.log
+import urllib2.exceptions.URLError

 log = logging.getLogger(__name__)

Stage this hunk [y,n,q,a,d,/,j,J,g,e,?]? n
@@ -68,9 +69,11 @@ def _get_instance_info():


 def _on_ec2():
-    m = boto.utils.get_instance_metadata(timeout=0.1, num_retries=1)
-    return bool(m)
-
+    try:
+        m = boto.utils.get_instance_metadata(timeout=0.1, num_retries=1)
+        return bool(m)
+    except:
+        return False

 def _get_credentials():
     creds = AWS_CREDENTIALS.copy()

[jfindlay]

@frioux, this seems good to me. What is the exception that was being raised? It is likely that there are subtle import differences between the plain python context and the salt context.

[jfindlay]

Good programming practice suggests you should only except the exception you expect

    try:
        m = boto.utils.get_instance_metadata(timeout=0.1, num_retries=1)
        return bool(m)
    except urllib2.exceptions.URLError:
        return False

so that the except statement does not hide other, unrelated problems.

[blast-hardcheese]

Ah, saw the comment was updated-- ignore this.

[frioux]

@jfindlay the reason I did it the way I did was to make it clear that I am bewildered that not importing the package lets exceptions through. The more correct way makes sense for deployment.

[frioux]

If you can explain it I'd love it :)

[frioux]

Should I submit a PR or will one of you guys edit the original?

[jfindlay]

@blast-hardcheese, but have you no expression on my excellent alliteration?

@frioux, when you say that it lets the exception through do you mean it results in a stack trace whereas not importing the exception does not result in a stack trace?

[frioux]

I was wrong. After getting some help from a friend, I found that boto is internally using boto.log.exception. I think the solution will either be to fix boto (maybe with a flag?) or to not use boto to check, like ec2_info.py does.

[spazm]

The import affected the result only because that import line is incorrect and throwing an ImportError, which was silently swallowed by salt.

1. incorrect import, raises ImportError:

   import urllib2.exceptions.URLError 
   

2. corrected import, compiles and runs.
   get_instance_metadata internally catches all errors and reports via boto.log.exception(...), printing a stack trace

   import urllib2
   
   try:
       m = boto.utils.get_instance_metadata(timeout=0.1, num_retries=1)
       return bool(m)
   except urllib2.URLError:
       # we never get here, get_instance_metadata doesn't raise
       return False
   

[blast-hardcheese]

@jfindlay Most definitely! // your expert elocution // commands much respect!

[nergdron]

note that it's now mid-2019, and this error is still present. I've just checked and @spazm's code does fix the problem, so it'd be really great to get that merged in.