Don't pass empty lists of certificates to the oVirt SDK

[jhernand]

The oVirt SDK accepts lists of trusted CA certificates. But when an
empty list, or a list containing just 'nil' elements is passed, the
result is that no certificate is trusted, not even the system default
trusted certificate. To avoid that this patch changes the oVirt provider
so that it passes 'nil' instead of empty lists.

[jhernand]

@miq-bot assign @oourfali

@borod108 @pkliczewski please review.

[miq-bot]

Checked commit https://github.com/jhernand/manageiq/commit/118b7ec52fb9336d7b7af59115da6442fd4db1a2 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 0 offenses detected
Everything looks good. 🍰

[jhernand]

@durandom please review.

[chessbyte]

@oourfali the changes in this PR looks trivial. Please give your 👍 so we can merge.

[oourfali]

Looks good to me.

[durandom]

so options[:ca_certs] is not an array already? Just read the code and it seems to be a concatenated String of certs... well :)

:ca_certs => options[:ca_certs].present? ? Array(options[:ca_certs]) : nil

because

[8] pry(main)> Array(nil)
=> []
[9] pry(main)> Array([])
=> []
[10] pry(main)> [].present?
=> false
[11] pry(main)> ''.present?
=> false
[12] pry(main)> nil.present?
=> false

[jhernand]

options[:ca_certs] is the content of the endpoints.certificate_authority column, and that can be nil or a string containing one or multiple concatenated certificates. The SDK accepts a list, to makes things simple for users that may have those multiple certificates already split into multiple strings. Each item of that list can also contain multiple certificates concatenated in a single string. The only problem is that when the list [] or [nil] the behaviour of the SDK is to trust no certificate, which is not what we want, what we want in that case is to trust the default system certificates. The way to achieve that in hte SDK is to pass nil.

[agrare]

@jhernand can options[:ca_certs] have more than one cert? It is plural so it sounds like it :)
In that case you'd end up with [[cert1, cert2]], how about ca_certs = Array(ca_certs) if ca_certs

[jhernand]

options[:ca_certs] is the content of the endpoints.certificate_authority column. That is a single string that may contain multiple concatenated certificates, that is why the name is plural, but the content is either nil or one string.

[agrare]

Sorry if you've already covered this but if it is a comma separated string should you split it up here? It looks like OvirtSDK4::Connection is expecting an array of strings https://github.com/oVirt/ovirt-engine-sdk-ruby/blob/master/sdk/lib/ovirtsdk4/connection.rb#L132-L136

[jhernand]

No problem. The content of that database is one string contaning one ore more certificates, in PEM format. For example:

-----BEGIN CERTIFICATE----
The first certificate.
-----END CERTIFICATE-----
-----BEGIN CERTIFICATE-----
The second certificate.
-----END CERTIFICATE-----
...

For user convenience, the oVirt SDK accepts them in separate strings, which would require us to split them here, but not using command but the BEGIN/END marks. Fortunately, also for user conveninence, the oVirt SDK also accepts multiple certificates in the same string, so we don't need to split anything here. The only thing that the SDK doesn't accept is [nil] (well, it accepts it, but it doesn't have the meaning that we need: it trusts no CA instead of trusting any CA), so that is the only case that we need to check here.

[agrare]

Got it thanks :)

[durandom]

👍 from me

@jhernand your version is good too - mine is a matter of taste and has a bit more safeguards

[durandom]

@miq-bot assign @agrare

@agrare pls merge

[jhernand]

Thanks @agrare.