Add support for verifying the Kubernetes connection #2156
Conversation
| @@ -20,7 +27,8 @@ def self.raw_connect(hostname, port, api_version) | |||
| def self.raw_api_endpoint(hostname, port) | |||
| require 'uri' | |||
|
|
|||
| uri = URI::HTTP.build(:path => "/api", :port => port.to_i) | |||
| port &&= port.to_i | |||
| uri = URI::HTTP.build(:path => "/api", :port => port) | |||
There was a problem hiding this comment.
This was a bug...if the user did not enter a port, it should no be passed. This code was formerly passing 0
There was a problem hiding this comment.
uri = URI::HTTP.build(:path => "/api", :port => port.try(:to_i))
There was a problem hiding this comment.
uri = URI::HTTP.build(:path => "/api", :port => port.try(:to_i))
I like this syntax!
There was a problem hiding this comment.
This was a bug...if the user did not enter a port, it should no be passed. This code was formerly passing
good point, great catch
There was a problem hiding this comment.
port.try(:to_i)
Interesting. I thought that since nil.to_i == 0, then nil.try(:to_i) would also be 0, but I just tested and it's nil. Pretty cool...I'll change it.
There was a problem hiding this comment.
Cool. Looks like this still works with rails 4.0's try.
NilClass#try is defined as def try ; nil ; end
Object#try has changed in rails 4.0 (as brandon has pointed out) to now have something like a responds_to? in it.
You are raising a good point, and I've started thinking about this. b. User can point it mistakenly on any server, that's correct, but getting not expected content will not harm, as we will be searching for entities that are of an interest for us (for example, replication controllers) and if they won't be there, then they won't be there. It will also require some validation action "are you k8s" on k8s side and it might be problematic to add it there (or looking forward - openshift might not be interested to identify itself as k8s for that matter) c. How does it behave with rhev? what is validated there? d. It does make me wonder though, why not build the endpoint in the kubeclient (adding the "/api" part in kubeclient that is) and provide only the version and the host/port in the manageiq provider side, what do you think? |
so it means there's no retry? how can it be "restarted/refreshed" to make it understand the endpoint became available? |
Well, that stinks. Many APIs have a root request that will give you basic information like version. Does k8s have something like this? For now I used endpoints, and I'm basically testing that it doesn't blow up with 401 or 404 errors. Is endpoints an acceptable thing to call? It sounded like something that would be relatively small.
For fun, I pointed it to google.com and it blew up with
We actually had a similar problem with discovery on RHEV in #1929, where we were just looking to see if it responded on a certain port and /api path. As you can imagine, there were false positives, so we changed it to look for a file called "engine.ssh.key.txt". That is for discovery though. For verification, we just hit /api and look for
I think this is a great idea. The less a caller needs to know the better. I could see the Kubeclient.new taking either a full URI like it does now, or take parts (hostname and an optional port and path), and build its own URI. Would it make sense to make the version optional as well, defaulting to the most commonly used one? |
This is what i pinged @h-kataria about in the initial post. We would need a verify button. Additionally there is a scheduled task that verifies all connections every hour or so. We can probably change that if needed. @jrafanie This reminds me. I feel like the task should check good connections every hour, but should check bad connection more frequently. Like every 5 minutes? Generally if a connection goes bad, you want it back ASAP if it gets restored. Thoughts? |
this is due to the following issue, I'd be happy to discuss the proper ways to solve it on kubeclient repo:
OK I'll take a note to refactor the calls for less information to be passed. |
How do I access that via Kubeclient? Is there a |
no, there's no such method, we might need to add it. I'll open an issue and link it here. |
I haven't looked at what verification does. Please open an issue and reference #1929 as we shouldn't just assume anything that responds to a very generic rest api endpoint with valid credentials is actually talking to rhevm/ovirt. |
Same here, we should open an issue to get that done, it sounds simple to add a new schedule to check currently |
|
Maybe we should layout all of the current problems with authentication/verify_credentials in one github issue with bullet items so we don't lose them. I'm still trying to get #1990 to completion though we keep coming up with cleaner implementation ideas. 😺 |
|
<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush. |
|
Thanks @jrafanie. I'm just waiting on the changes for kubeclient before
|
Fryguy
changed the title
Fryguy
force-pushed
the
fix_kubernetes_connection_validation
branch
from
622033b
to
6ea4a2a
Compare
|
Updated with using the proposed Kubeclient#valid? method from ManageIQ/kubeclient#54 . So, this PR now depends on that change getting in and released. It will need a version bump of the gem as well. |
Fryguy
changed the title
Fryguy
force-pushed
the
fix_kubernetes_connection_validation
branch
from
6ea4a2a
to
ab41a13
Compare
|
Changed method name to api_valid on the kubeclient side, so updated here as well. |
new kubeclient is released (0.1.8) |
|
@h-kataria Can you comment on my question to you in the original post? |
Fryguy
force-pushed
the
fix_kubernetes_connection_validation
branch
from
ab41a13
to
4221115
Compare
Fryguy
changed the title
|
Updated PR with the changes to kubeclient. Just waiting on @h-kataria on how to proceed with respect to the UI. @h-kataria In addition, I see that port is missing in the UI. I think that's a mistake. |
|
<gemfile_checker />@JPrause @jvlcek Gemfile changes detected in commit Fryguy@4221115. Please review. |
|
@Fryguy About the missing "port" field, we provided just a skeleton of Containers UI, i think @erinboyd might be enhancing that screen to add any missing fields on that screen. Regarding verification of credentials, currently Container Provider screen does have Credentials box so user can enter credentials and validate/save them as they would for any other type of Provider. Are we removing credentials box out of the Container Provider screen? if yes, then we would need a "Validate" button on that screen next to one of the other required fields such as Hostname or IPAddress. |
|
@h-kataria Oh I see. So, then I think this is good to merge, and we'll have to verify that the verify button stays in the UI changes made by @erinboyd |
Fryguy
changed the title
|
@Fryguy @h-kataria @abonas I removed the credentials box from the new provider screen...so will we need an additional button that validates the connection manually? Is this what you are saying? |
|
@erinboyd @abonas If you have removed Credentials box from Containers Provider screen then yes you do need add a validate button on that screen somewhere next to IP Address/Hostname box depending upon whichever is a required field, and have it do similar thing that other Validate button does in Credentials box |
|
@h-kataria okay. I will do this. |
|
<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush. |
This will prevent worker cycling when the Kubernetes endpoint is not available.
Fryguy
force-pushed
the
fix_kubernetes_connection_validation
branch
from
4221115
to
4427ddd
Compare
|
@abonas I've rebased and fix the conflicts. |
|
Checked commit Fryguy@4427ddd with rubocop 0.27.1 vmdb/app/models/ems_kubernetes.rb
vmdb/spec/models/ext_management_system_spec.rb
|
|
cc @simon3z |
| def verify_credentials(_auth_type = nil, _options = {}) | ||
| # TODO: support real authentication using certificates | ||
| true | ||
| with_provider_connection(options, &:api_valid?) |
There was a problem hiding this comment.
Validation here should be a check that remote endpoint supports the API that we know about (v1beta3 / future v1.0).
I'll send a separate patch about that.
There was a problem hiding this comment.
isn't that the job of the api_valid? method on kubeclient?
|
We need a real validation about the api, I'll send a patch. In general 👍 💯 |
|
@Fryguy, legit travis failures? |
|
@blomquisg @Fryguy I based my patches on this PR, and I fixed the travis failure. I kept @Fryguy 's authorship on the patch, obviously, and I added a real validation here: #2696 Can you try review/merge it? Thanks. |
| end | ||
|
|
||
| def authentication_status_ok?(_type = nil) | ||
| private |
There was a problem hiding this comment.
Adding private here you mask other public methods (all_computer_system_ids, aggregate_logical_cpus, aggregate_memory).
There was a problem hiding this comment.
Doh, must have happened when I rebased.
|
Subsumed into #2696 |
This will prevent worker cycling when the Kubernetes endpoint is not
available.
@abonas Please review.
Even though there are no credentials I need an authentications record to keep track of the verification status. So, I created a dummy record for now. I figure we can change that record into a certificate based record when we get to certificates.
@h-kataria This brings up the question of "verifying" the connection in the UI, which will now be needed, in case the connection goes bad. The user will want to go into the EMS and "revalidate" it to make it active again. However since there are no credentials, I'm not sure how you want to present this in the UI. cc @jrafanie
@abonas I this this PR would be better served with a method on Kubeclient called
verify?or something similar. You can see what I did for Foreman here. What do you think? Basically, the method tries to connect and does a lightweight action. Additionally it verifies that the content it gets back is what is expected. This is important since you could technically hit any https://example.com/api and it could come back ok, so we want to verify it's the right one.