Switch the xcc port to https if https is enabled. #891
Conversation
deploy/lib/server_config.rb
Outdated
| @@ -2451,7 +2451,8 @@ def xcc | |||
| :http_connection_retry_count => @properties["ml.http.retry-count"].to_i, | |||
| :http_connection_open_timeout => @properties["ml.http.open-timeout"].to_i, | |||
| :http_connection_read_timeout => @properties["ml.http.read-timeout"].to_i, | |||
| :http_connection_retry_delay => @properties["ml.http.retry-delay"].to_i | |||
| :http_connection_retry_delay => @properties["ml.http.retry-delay"].to_i, | |||
| :use_https_for_rest => @properties["ml.ssl-certificate-template"].present? || @properties["ml.use-https-for-rest"] == "true" | |||
There was a problem hiding this comment.
The rest server that gets called by Roxy is typically the server at app-port, to which ssl-cert stuff is typically applied. The xcc server could be a separate server, and it is more difficult to guess if it is running https or not. Easiest way to solve that would be to introduce a use-https-for-xcc property in default.properties, which can be overridden in the other ones. The use_https_for_rest (or better: use_https_for_xcc) would only respond to that property in the case of XCC, unless perhaps the install-xcc property has been set to false, and xcc-port equals app-port (a helper function to verify this would be convenient).
There was a problem hiding this comment.
I wasn't sure whether to use the use-https-for-rest or just the use-https option for xcc, so I went with what #862 mentioned. I agree with you though, it's best to introduce a separate option for xcc in particular. I'll amend the PR.
deploy/lib/xcc.rb
Outdated
| @@ -100,6 +100,7 @@ def initialize(options) | |||
| }) | |||
| @request = {} | |||
| @gmt_offset = Time.now.gmt_offset | |||
| @rest_protocol = "http#{options[:use_https_for_rest] ? 's' : ''}" | |||
deploy/lib/xcc.rb
Outdated
| @@ -176,7 +177,7 @@ def go(url, verb, headers = {}, params = nil, body = nil) | |||
| end | |||
|
|
|||
| def build_load_uri(target_uri, options, commit) | |||
| url = "http://#{@hostname}:#{@port}/insert?" | |||
| url = "#{@rest_protocol}://#{@hostname}:#{@port}/insert?" | |||
|
Unfortunately I can't really test with https enabled easily, but it looks like the changes are made in the right places. I left a few suggestions inline though.. |
When building the url to talk to the xcc port roxy doesn't look whether that should be https or not, it's just hardcoded to http, which makes it impossible to deploy modules. This commit changes that by switching to https if either `use-https-for-xcc` is set to true. By default this option is set to false.
ciprianc
force-pushed
the
fix-issue-862
branch
from
c8d4570
to
571be97
Compare
| @@ -2451,7 +2451,8 @@ def xcc | |||
| :http_connection_retry_count => @properties["ml.http.retry-count"].to_i, | |||
| :http_connection_open_timeout => @properties["ml.http.open-timeout"].to_i, | |||
| :http_connection_read_timeout => @properties["ml.http.read-timeout"].to_i, | |||
| :http_connection_retry_delay => @properties["ml.http.retry-delay"].to_i | |||
| :http_connection_retry_delay => @properties["ml.http.retry-delay"].to_i, | |||
| :use_https_for_xcc => @properties["ml.use-https-for-xcc"] == "true" | |||
There was a problem hiding this comment.
So the similar use-https-for-rest option checks if either use-https-for-rest is true OR ssl-certificate-template is defined. In that case it makes sense because as soon as you define a certificate template it turns the rest server to https when you bootstrap, you don't have to also enable use-https-for-rest. For XCC the xml for the definition is built a bit differently, and defining a certificate-template won't also enable ssl on the xcc port. If I were to have the same OR condition I think it would break backwards compatibility. Let me know what you think.
There was a problem hiding this comment.
Yes, that was exactly the concern I had before. Looks perfect to me..
|
I'll merge, so that our internal CI can run the self-test with it. |
When building the url to talk to the xcc port roxy doesn't look whether that should be https or not,
it's just hardcoded to http, which makes it impossible to deploy modules. This commit changes that
by switching to https if either
use-https-for-restis set to true orssl-certificate-templateexists in the config.
First attempt at fixing #862