kubernetes: verify endpoint api version #2696
Conversation
|
@miq-bot add_label providers/containers |
|
<gemfile_checker />@JPrause @jvlcek Gemfile changes detected in commit simon3z@2fafd53. Please review. |
|
<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush. |
simon3z
force-pushed
the
verify-endpoint-api
branch
from
2fafd53
to
34be397
Compare
|
<gemfile_checker />@JPrause @jvlcek Gemfile changes detected in commit simon3z@34be397. Please review. |
simon3z
force-pushed
the
verify-endpoint-api
branch
from
34be397
to
ba66585
Compare
simon3z
changed the title
|
<gemfile_checker />@JPrause @jvlcek Gemfile changes detected in commit simon3z@ba66585. Please review. |
simon3z
force-pushed
the
verify-endpoint-api
branch
from
ba66585
to
8ed2fb7
Compare
|
<gemfile_checker />@JPrause @jvlcek Gemfile changes detected in commit simon3z@8ed2fb7. Please review. |
simon3z
force-pushed
the
verify-endpoint-api
branch
from
8ed2fb7
to
7fced07
Compare
|
<gemfile_checker />@JPrause @jvlcek Gemfile changes detected in commit simon3z@7fced07. Please review. |
|
@Fryguy can you review/merge this? |
| # TODO: support real authentication using certificates | ||
| true | ||
| with_provider_connection(options) do |connection| | ||
| unless (connection.api['versions'] || []).include? api_version |
There was a problem hiding this comment.
So, again, this should not live here...it should be in the kubeclient gem itself in the api_valid? method, which I added there. kubeclient already knows what it "should" connect to, so it should be responsible for validating it.
There was a problem hiding this comment.
@Fryguy kubeclient supports all the APIs, it's ManageIQ being picky on which one supports, so the validation should be here. For as much as the api_valid? method is involved... the api would be valid but ManageIQ wouldn't be able to parse the objects.
There was a problem hiding this comment.
The reason I think it's kubeclient's responsibility is that it's passed as a parameter to Kubeclient.new. Therefore we are telling kubeclient which version we need, and it's own validator should verify that the version is available and accessible. Thus, if we hypothetically connected to a kubernetes instance that did not have the version we gave to kubeclient, then kubeclient should tell us that (ideally on the api_valid? method). If we were not passing the api_version in, then I'd agree with you.
simon3z
force-pushed
the
verify-endpoint-api
branch
from
7fced07
to
0eb4cdc
Compare
|
<gemfile_checker />@JPrause @jvlcek Gemfile changes detected in commit simon3z@0eb4cdc. Please review. |
| end | ||
| with_provider_connection(options) do |connection| | ||
| unless (connection.api['versions'] || []).include? self.class.api_version | ||
| raise MiqException::MiqHostError, \ |
There was a problem hiding this comment.
No need for the line continuator...in fact, please keep it on one line.
This will prevent worker cycling when the Kubernetes endpoint is not available.
Signed-off-by: Federico Simoncelli <fsimonce@redhat.com>
| true | ||
| end | ||
| with_provider_connection(options) do |connection| | ||
| unless (connection.api['versions'] || []).include? self.class.api_version |
There was a problem hiding this comment.
Prefer using Hash#fetch with default vs. || here.
unless connection.api.fetch('versions', []).include? self.class.api_versionSigned-off-by: Federico Simoncelli <fsimonce@redhat.com>
simon3z
force-pushed
the
verify-endpoint-api
branch
from
0eb4cdc
to
b5600d8
Compare
|
Checked commits simon3z@51a2ddc .. simon3z@b5600d8 with rubocop 0.27.1 vmdb/app/models/ems_kubernetes.rb
vmdb/spec/models/ext_management_system_spec.rb
|
|
@Fryguy this should be ready to be merged. |
|
@blomquisg @Fryguy ping? |
kubernetes: verify endpoint api version
This PR is based on #2156 and it should be queued behind that. /cc @Fryguy
There are still a couple of things that I am working on and I still need to verify it (maybe there's an api_version name collision, etc... we'll see when #2156 will be merged).