Fix NO_PROXY environment variable setting

3 files changed, 86 insertions, 34 deletions

diff --git a/inventory/byo/hosts.origin.example b/inventory/byo/hosts.origin.example
index e6bc6c829..7f005a931 100644
--- a/inventory/byo/hosts.origin.example
+++ b/inventory/byo/hosts.origin.example
@@ -740,6 +740,10 @@ openshift_master_identity_providers=[{'name': 'htpasswd_auth', 'login': 'true',
 # Global Proxy Configuration
 # These options configure HTTP_PROXY, HTTPS_PROXY, and NOPROXY environment
 # variables for docker and master services.
+#
+# Hosts in the openshift_no_proxy list will NOT use any globally
+# configured HTTP(S)_PROXYs. openshift_no_proxy accepts domains
+# (.example.com), and hosts (example.com), and IP addresses.
 #openshift_http_proxy=http://USER:PASSWORD@IPADDR:PORT
 #openshift_https_proxy=https://USER:PASSWORD@IPADDR:PORT
 #openshift_no_proxy='.hosts.example.com,some-host.com'
@@ -747,7 +751,11 @@ openshift_master_identity_providers=[{'name': 'htpasswd_auth', 'login': 'true',
 # Most environments don't require a proxy between openshift masters, nodes, and
 # etcd hosts. So automatically add those hostnames to the openshift_no_proxy list.
 # If all of your hosts share a common domain you may wish to disable this and
-# specify that domain above.
+# specify that domain above instead.
+#
+# For example, having hosts with FQDNs: m1.ex.com, n1.ex.com, and
+# n2.ex.com, one would simply add '.ex.com' to the openshift_no_proxy
+# variable (above) and set this value to False
 #openshift_generate_no_proxy_hosts=True
 #
 # These options configure the BuildDefaults admission controller which injects
diff --git a/inventory/byo/hosts.ose.example b/inventory/byo/hosts.ose.example
index 928da40fa..a6d2cbb41 100644
--- a/inventory/byo/hosts.ose.example
+++ b/inventory/byo/hosts.ose.example
@@ -740,6 +740,10 @@ openshift_master_identity_providers=[{'name': 'htpasswd_auth', 'login': 'true',
 # Global Proxy Configuration
 # These options configure HTTP_PROXY, HTTPS_PROXY, and NOPROXY environment
 # variables for docker and master services.
+#
+# Hosts in the openshift_no_proxy list will NOT use any globally
+# configured HTTP(S)_PROXYs. openshift_no_proxy accepts domains
+# (.example.com), hosts (example.com), and IP addresses.
 #openshift_http_proxy=http://USER:PASSWORD@IPADDR:PORT
 #openshift_https_proxy=https://USER:PASSWORD@IPADDR:PORT
 #openshift_no_proxy='.hosts.example.com,some-host.com'
@@ -747,7 +751,11 @@ openshift_master_identity_providers=[{'name': 'htpasswd_auth', 'login': 'true',
 # Most environments don't require a proxy between openshift masters, nodes, and
 # etcd hosts. So automatically add those hostnames to the openshift_no_proxy list.
 # If all of your hosts share a common domain you may wish to disable this and
-# specify that domain above.
+# specify that domain above instead.
+#
+# For example, having hosts with FQDNs: m1.ex.com, n1.ex.com, and
+# n2.ex.com, one would simply add '.ex.com' to the openshift_no_proxy
+# variable (above) and set this value to False
 #openshift_generate_no_proxy_hosts=True
 #
 # These options configure the BuildDefaults admission controller which injects
diff --git a/roles/openshift_facts/library/openshift_facts.py b/roles/openshift_facts/library/openshift_facts.py
index 04b5dc86b..0a9f3cd9b 100755
--- a/roles/openshift_facts/library/openshift_facts.py
+++ b/roles/openshift_facts/library/openshift_facts.py
@@ -1643,38 +1643,74 @@ def set_proxy_facts(facts):
     if 'common' in facts:
         common = facts['common']
 
-        # No openshift_no_proxy settings detected, empty list for now
-        if 'no_proxy' not in common:
-            common['no_proxy'] = []
-
-        # _no_proxy settings set. It is just a simple string, not a
-        # list or anything
-        elif 'no_proxy' in common and isinstance(common['no_proxy'], string_types):
-            # no_proxy is now a list of all the comma-separated items
-            # in the _no_proxy value
-            common['no_proxy'] = common['no_proxy'].split(",")
-
-            # at this point common['no_proxy'] is a LIST datastructure. It
-            # may be empty, or it may contain some hostnames or ranges.
-
-            # We always add local dns domain, the service domain, and
-            # ourselves, no matter what (if you are setting any
-            # NO_PROXY values)
-            common['no_proxy'].append('.svc')
-            common['no_proxy'].append('.' + common['dns_domain'])
-            common['no_proxy'].append(common['hostname'])
-
-        # You are also setting system proxy vars, openshift_http_proxy/openshift_https_proxy
-        if 'http_proxy' in common or 'https_proxy' in common:
-            # You want to generate no_proxy hosts and it's a boolean value
-            if 'generate_no_proxy_hosts' in common and safe_get_bool(common['generate_no_proxy_hosts']):
-                # And you want to set up no_proxy for internal hostnames
-                if 'no_proxy_internal_hostnames' in common:
-                    # Split the internal_hostnames string by a comma
-                    # and add that list to the overall no_proxy list
-                    common['no_proxy'].extend(common['no_proxy_internal_hostnames'].split(','))
-
-        common['no_proxy'] = ','.join(sort_unique(common['no_proxy']))
+        ######################################################################
+        # We can exit early now if we don't need to set any proxy facts
+        proxy_params = ['no_proxy', 'https_proxy', 'http_proxy']
+        # If any of the known Proxy Params (pp) are defined
+        proxy_settings_defined = any(
+            [True for pp in proxy_params if pp in common]
+        )
+
+        if not proxy_settings_defined:
+            return facts
+
+        # As of 3.6 if ANY of the proxy parameters are defined in the
+        # inventory then we MUST add certain domains to the NO_PROXY
+        # environment variable.
+
+        ######################################################################
+
+        # Spot to build up some data we may insert later
+        raw_no_proxy_list = []
+
+        # Automatic 3.6 NO_PROXY additions if a proxy is in use
+        svc_cluster_name = ['.svc', '.' + common['dns_domain'], common['hostname']]
+
+        # auto_hosts: Added to NO_PROXY list if any proxy params are
+        # set in the inventory. This a list of the FQDNs of all
+        # cluster hosts:
+        auto_hosts = common['no_proxy_internal_hostnames'].split(',')
+
+        # custom_no_proxy_hosts: If you define openshift_no_proxy in
+        # inventory we automatically add those hosts to the list:
+        if 'no_proxy' in common:
+            custom_no_proxy_hosts = common['no_proxy'].split(',')
+        else:
+            custom_no_proxy_hosts = []
+
+        # This should exist no matter what. Defaults to true.
+        if 'generate_no_proxy_hosts' in common:
+            generate_no_proxy_hosts = safe_get_bool(common['generate_no_proxy_hosts'])
+
+        ######################################################################
+
+        # You set a proxy var. Now we are obliged to add some things
+        raw_no_proxy_list = svc_cluster_name + custom_no_proxy_hosts
+
+        # You did not turn openshift_generate_no_proxy_hosts to False
+        if generate_no_proxy_hosts:
+            raw_no_proxy_list.extend(auto_hosts)
+
+        ######################################################################
+
+        # Was anything actually added? There should be something by now.
+        processed_no_proxy_list = sort_unique(raw_no_proxy_list)
+        if processed_no_proxy_list != list():
+            common['no_proxy'] = ','.join(processed_no_proxy_list)
+        else:
+            # Somehow we got an empty list. This should have been
+            # skipped by now in the 'return' earlier. If
+            # common['no_proxy'] is DEFINED it will cause unexpected
+            # behavior and bad templating. Ensure it does not
+            # exist. Even an empty list or string will have undesired
+            # side-effects.
+            del common['no_proxy']
+
+        ######################################################################
+        # In case you were wondering, because 'common' is a reference
+        # to the object facts['common'], there is no need to re-assign
+        # it.
+
     return facts