allow user to ask for FQDN as public hostname (SOC-9616) #1901
Conversation
![hound[bot]](https://avatars.githubusercontent.com/in/3598?s=60&v=4){kind=small}
| if public_name.nil? || public_name.empty? | ||
| if use_ssl | ||
| if use_ssl || want_fqdn |
There was a problem hiding this comment.
Style/ConditionalAssignment: Use the return of the conditional for variable assignment and comparison.
| @@ -33,7 +33,8 @@ def self.get_host_for_admin_url(node, use_cluster = false) | |||
| end | |||
| end | |||
|
|
|||
| def self.get_host_for_public_url(node, use_ssl, use_cluster = false) | |||
| def self.get_host_for_public_url(node, use_ssl, use_cluster = false, | |||
There was a problem hiding this comment.
Metrics/CyclomaticComplexity: Cyclomatic complexity for get_host_for_public_url is too high. [7/6]
Metrics/PerceivedComplexity: Perceived complexity for get_host_for_public_url is too high. [9/7]
|
@rsalevsky do we plan to fix travis? Currently failing because of CVE-2019-5477 (see https://travis-ci.org/crowbar/crowbar-core/jobs/574040609#L1570 ) |
There was a problem hiding this comment.
can you please explain this a bit more? if you set the public name on the cluster or the host then that one should be picked?
I am not necessarily adding another option, but it seems incorrect to me to do that only for keystone/websso then. we should generalize the solution (for example by simply always returning the hostname rather than ip addresses).
Yes, and AFAICS this PR doesn't change that behavior. It only changes what is returned when no public name is set.
IIRC we specifically introduced the behavior to return IP addresses for the endpoints by default, to be able to work when no public name is set and the crowbar generated hostnames are not resolveable externally (see: crowbar/barclamp-glance@22d3c19 for some very old history). Not sure if that is really a good reason still (or if it ever was :) ). But we might break existing setups if we set fqdn for the public endpoints everywhere now. That's why I hinted in crowbar/crowbar-openstack#2188 (comment) at making this optional. |
Probably won't break as we are already returning the FQDN if SSL is enabled. That is unless we do a bunch of other hacks to DNS if SSL is enabled. |
right, which is why I was wondering why we need to make it extra complicated. But I do accept and agree with ralfs comment that this is on a maintenance branch so we should be careful with changing default behavior in a maintenance update. so I'll no longer object. would still be nice to capture this in the commit message rather than in a comment on the PR.. |
There was a problem hiding this comment.
@guangyee Would you mind addressing @dirkmueller's last comment and putting some of the reasoning into the commit message. I then this would be ready for merging.
Currently CrowbarHelper.get_host_for_public_url() indiscriminately return the IP instead of FQDN in the situation where the node's public_name is not set and SSL is not being used. This behavior was introduced in crowbar/barclamp-glance@22d3c19 to address certain situations where the crowbar generated hostnames are not resolveable externally. For SSL, we need the FQDN or hostname for certificate validation. However, for WebSSO, certain identity providers (i.e. Google) requires FQDN to match the authorized domain, regardless of whether SSL is used. In theory, we could change the behavior of get_host_for_public_url() to always return the FQDN instead of IP as we *should be* supporting external name resolution by now, but doing so may risk breaking backward compatibility. Therefore, this patch introduce an option to allow users to ask for FQDN regardless whether SSL is used without breaking backward compatibility.
|
@rhafer thanks for the reminder. I've updated the commit msg. |
Currently CrowbarHelper.get_host_for_public_url() indiscriminately
return the IP instead of FQDN in the situation where the
node's public_name is not set and SSL is not being used.
This behavior was introduced in
crowbar/barclamp-glance@22d3c19
to address certain situations where the crowbar generated hostnames are not
resolveable externally. For SSL, we need the FQDN or hostname for
certificate validation. However, for WebSSO, certain identity providers
(i.e. Google) requires FQDN to match the authorized domain, regardless of
whether SSL is used.
In theory, we could change the behavior of get_host_for_public_url() to
always return the FQDN instead of IP as we should be supporting
external name resolution by now, but doing so may risk breaking
backward compatibility. Therefore, this patch introduce an option to allow
users to ask for FQDN regardless whether SSL is used without breaking
backward compatibility.