::Kubeclient::Client::handle_uri assumes k8s api mounted at / for paths longer than 2 components

[dirkd]

Hi,

when trying to use kubeclient (v3.0.0 via Gitlab's Kubernetes integration) with a Rancher k8s cluster (both in v1.6 and v2) there's a problem regarding kubeclients assumptions (rancher/rancher#13058).

The following code assumes that as soon as there's more than two path components the second one (third entry in components after splitting) has to be the k8s api_group the user wants set:

https://github.com/abonas/kubeclient/blob/e37a8029275a6032a6f9b641b3800601ff1a2a0b/lib/kubeclient/common.rb#L149-L150

This works as long as the k8s apiserver is mounted at the /-route on the host providing the apiserver. As Rancher provides a common endpoint for multi-cluster environments it mounts the k8s apiserver at a deeper point of the path (to differentiate the environment) which breaks kubeclients assumptions on URL structure.

[cben]

Great find!
I think this is strongly related to #284 and we could to solve both at once.
Currently user gives kubeclient 2 parts e.g.: 'https://host:port/apis/batch', 'v1' (to take one API group as a running example)
Unfortunately that's neither a convenient way for user to split it (this is what #284 argues) — nor for kubeclient!

Looking at the code, what kubeclient needs in addition to the combined path https://host:port/apis/batch/v1 is knowing the batch/v1 part because it also has (had?) to be sent in apiVersion due to kubernetes/kubernetes#6439.
What you see here is the silliness of kubeclient trying to extract the batch part from url, to recombine with /v1! https://github.com/abonas/kubeclient/blob/e37a8029275a6032a6f9b641b3800601ff1a2a0b/lib/kubeclient/common.rb#L324

Proposal

UPDATE: I no longer thing we need a new interface, I think we can solve it compatibly.

Kubeclient::Client.new('http://localhost:8080', path: 'apis', api_version: 'batch/v1')

• Naming is hard, improvements welcome.

• I do NOT want to overload existing positional args with new meaning, would require too much guessing. Named hash arguments are good way to distinguish new interface from old.

• Why explicit param for apis/ portion? It usually could be guessed — api/v1, apis/... for everything else (though at some point api/v2 will come along?). But we already know one other case: Openshift extensions are under oapi/v1! So let's keep it explicit.

• Can we combine base URL and path into one param? Probably. But at some point we'd like to somehow allow Client object to access multiple path and/or apiVersion (Allow different resources to use different apiVersion #208) so it separating it seems more future-proof.

• Terminology: code currently uses @api_version for just the v1 portion, I think that's only internal and better use the term to mean what goes in apiVersion (batch/v1)?

  • The term "api group" is ambigous, could mean /apis/batch/v1 or batch/v1 so I think better avoided.
  • EDIT: Config::Context.api_version already means v1 not batch/v1, so that'd be confusing too :-(

• TODO: does Config::Context need extension? Does the idiom for constructing Client from Context need to change? I see that idiom duplicated in tons of projects on github, better add a method for that.

[ kubernetes doc: https://kubernetes.io/docs/reference/api-overview/#api-groups ]

[cben]

Additionally, I'm not sure extracting apiVersion from the last path segment works at all, apparently apiVersion != groupVersion ?

$ curl -k https://$MASTER:8443/apis/authentication.k8s.io/v1 -H "Authorization: Bearer "(cat ~/$MASTER.token)
{
  "kind": "APIResourceList",
  "apiVersion": "v1",
  "groupVersion": "authentication.k8s.io/v1",
  "resources": [
    {
      "name": "tokenreviews",
      "singularName": "",
      "namespaced": false,
      "kind": "TokenReview",
      "verbs": [
        "create"
      ]
    }
  ]
}
$ curl -k https://$MASTER:8443/apis/authentication.k8s.io/v1beta1 -H "Authorization: Bearer "(cat ~/$MASTER.token)
{
  "kind": "APIResourceList",
  "apiVersion": "v1",
  "groupVersion": "authentication.k8s.io/v1beta1",
  "resources": [
    {
      "name": "tokenreviews",
      "singularName": "",
      "namespaced": false,
      "kind": "TokenReview",
      "verbs": [
        "create"
      ]
    }
  ]
}⏎

$ curl -s -k https://$MASTER:8443/version{,/openshift} -H "Authorization: Bearer "(cat ~/$MASTER.token) | jq .gitVersion
"v1.7.6+a08f5eeb62"
"v3.7.1+c2ce2c0-1"

[juwalter]

Ran into the same issue as @dirkd many thanks to suggestion re: nginx proxy at rancher/rancher#13058 (comment) got it working (Rancher 1.6; Gitlab 10.7)

[juwalter]

(btw - helm.sh has it working (and kubectl obviously) - not sure what they are doing - but I guess it might be a safe bet to follow their lead)

[Jul13nT]

Any update on this issue ? It prevents me using Gitlab's Kubernetes integration with my cluster 😞 I don't have enough knowledge to try a reverse proxy as suggested...

[cben]

Thanks for the nag. I got carried away thinking of future APIs, perfect is the enemy of good...

• Any opinions on API proposal above?
• Note what I propose would require projects to change way they construct Kubeclient::Client (if they want to work with api not at /) — is that acceptable?
  • If you prefer a no-code-changes-needed fix, please invent one :-) I'm not sure how to do it, worried it would break existing behavior...
• Help me figure out whether / how new params require changes to Kubeclient::Config[::Context]? And to idiom for constructing Client from Config?
• PRs welcome!

[cben]

Hmm, we already accidentally(?) have 2 interfaces :-(

1. README mostly documents constructing with endpoint ending in /api or /apis.
2. However in few places (Kubeclient::Client.new(uri), Kubeclient::Client.new('https://kubernetes.default.svc', ...)) it shows constructing with the bare domain without /api.
3. Also, when constructing from config.context.api_endpoint (as README also documents), that's set from server: in kubeconfig yaml and typically has the bare domain, without /api.

We didn't really notice the inconsistency because bare domain works too!
But only for api/v1 resources, omitting /apis with other api groups fails:

[3] pry(main)> c = Kubeclient::Client.new(context.api_endpoint, "autoscaling/v1", auth_options: context.auth_options, ssl_options: context.ssl_options)
=> #<Kubeclient::Client:0x0055b774ee75b0
 @api_endpoint=#<URI::HTTPS https://internal-api.bpaskinc.origin-gce.dev.openshift.com:8443/api>,
 @api_group="",
 @api_version="autoscaling/v1",
...
[4] pry(main)> RestClient.log = 'stderr'; c.discover
RestClient.get "https://internal-api.bpaskinc.origin-gce.dev.openshift.com:8443/api/autoscaling/v1", "Accept"=>"*/*", "User-Agent"=>"rest-client/2.1.0.rc1 (linux-gnu x86_64) ruby/2.4.1p111"
# => 404 NotFound | application/json 174 bytes, 0.63s
Kubeclient::ResourceNotFoundError: the server could not find the requested resource

With /apis it works.

[7] pry(main)> c = Kubeclient::Client.new(context.api_endpoint + "/apis", "autoscaling/v1", auth_options: context.auth_options, ssl_options: context.ssl_options)
[8] pry(main)> c.discover
RestClient.get "https://internal-api.bpaskinc.origin-gce.dev.openshift.com:8443/apis/autoscaling/v1", "Accept"=>"*/*", "User-Agent"=>"rest-client/2.1.0.rc1 (linux-gnu x86_64) ruby/2.4.1p111"
# => 200 OK | application/json 468 bytes, 0.63s
=> true

EDIT: Oh, wait, now I mixed another way to initialize! According to current docs, I'm not supposed to pass version "autoscaling/v1" (though that worked?), I'm supposed to split it:

[13] pry(main)> c = Kubeclient::Client.new(context.api_endpoint + "/apis/autoscaling", "v1", auth_options: context.auth_options, ssl_options: context.ssl_options)
[10] pry(main)> c.discover
RestClient.get "https://internal-api.bpaskinc.origin-gce.dev.openshift.com:8443/apis/autoscaling/v1", "Accept"=>"*/*", "User-Agent"=>"rest-client/2.1.0.rc1 (linux-gnu x86_64) ruby/2.4.1p111"
# => 200 OK | application/json 468 bytes, 0.75s

Welp...

[cben]

Anybody knows how the official kubernetes client libs express these params?
This might be a good time to borrow from https://github.com/kubernetes-client/ruby :-)

[dplummer]

I'm interested to see this fixed. In the meantime this is the workaround I'm using:

config = Kubeclient::Config.read("#{ENV['HOME']}/.kube/config")
client = Kubeclient::Client.new(
  config.context.api_endpoint + "/apis",
  "v1beta1",
  ssl_options: config.context.ssl_options,
  auth_options: config.context.auth_options
)
client.instance_variable_set(:@api_group, "extensions")
client.instance_variable_get(:@api_endpoint).path += "/extensions"
client

[cben]

Simpler Idea: get groupVersion from discovery ?

Stop caring how exactly endpoint / version are split in constructor.
Join them anyway into single URL, do discovery on that path, take groupVersion from that.

(well, with compatibility special casing for bare domain without /api)

[chasebolt]

great gem! i am also running into this issue with gitlab. would appreciate it if someone can get a fix ironed out.

[cben]

I started a small project to help understand backward-compat, and possibly for future pseudo-integration testing: https://github.com/cben/kubernetes-discovery-samples
It has scraped discovery responses, as static files, from k8s versions 1.1–1.10 (well actually from matching openshift versions because these were easier).
You can even access it via GitHub Pages as a fake endpoint!

c = Kubeclient::Client.new("https://cben.github.io/kubernetes-discovery-samples/openshift-origin-v3.10.0/api", "v1")
c.discover  # => true
c.methods  # worked!  defined stuff like get_pods, patch_persistent_volume...

Of course you can't actually run any actions.

OK, now what can I learn from it? :-)

• I see groupVersion was always available, already in https://cben.github.io/kubernetes-discovery-samples/openshift-origin-v1.1.1/api/v1/

[danielhass]

Any news on this one?

[krishofmans]

Indeed, I'm interested as well. This breaks a lot of gitlab functionality, it would be nice to have sane api url handling.

[cben]

Sorry, I'm aware this is high priority, more or less know what to do but not finding the time :-(. Did I mention PRs always welcome? ;-)
Specifically, if anybody has time to write tests for existing functionality and/or desired functionality, that would help a lot!
Forms that already work, and don't yet:

• Client.new('https://host:6443/api', 'v1')
• Client.new('https://host:6443/apis/batch', 'v1')
• Client.new('https://host:6443/oapi', 'v1') — for openshift
  (UPDATE: these are obsolete. gone since openshift 4.0, in favor of separate standard /apis/... groups. These groups worked since openshift 3.5 or 3.6, so even code that needs to support say 3.11 probably doesn't need to touch /oapi.)
• Client.new('https://host:6443', 'v1') — 👍 this is what we document for initialization from Config context.api_endpoint.
• Client.new('https://host:6443', 'batch/v1') — 👍 allowing this would solve dealing with api/v1 vs apis/extensions/v1beta1 is annoying since they are split between url and version #284
• [?] Client.new('https://host:6443/apis', 'batch/v1') — partially works?! undocumented, not fully intended.

(See also above #318 (comment) about the multiple forms that already work)

And the formats we might like for this issue (not sure we need all, and not sure none of these work):

• Client.new('https://host:6443/SUB/PATH/api', 'v1')
• Client.new('https://host:6443/SUB/PATH/apis/batch', 'v1')
• Client.new('https://host:6443/SUB/PATH/oapi', 'v1') — not critical
• Client.new('https://host:6443/SUB/PATH', 'v1') — 👍
• Client.new('https://host:6443/SUB/PATH', 'batch/v1') — 👍 dealing with api/v1 vs apis/extensions/v1beta1 is annoying since they are split between url and version #284 together with this issue.
• Client.new('https://host:6443/SUB/PATH/apis', 'batch/v1') — not sure if desired.

In the future I'd love the ones marked 👍 to become the recommended ways.
The "endpoint" would be exactly what kubeconfig calls endpoint, and "version" would be exactly the apiVersion. api/ vs apis/ can be autodetected. (openshift's oapi/ is maybe exception, not a blocker for improving normal k8s usage, I have some other ideas for it.)
But for now we should have backward compatibility with other working forms.

Again, PRs for any subset of this — even just tests for just what works now — would help!

[Sadarex]

Any news on this one? It's still not possible to use a Rancher Kubernetes Cluster with Gitlab.

[cben]

Should be fixed by #457 (thanks T4cC0re!), released in kubeclient 4.9.1. Please test.

[pmalek-sumo]

Hi all,

I'm not sure if that's the correct place to ask for this but I feel that the PR that got merged that closed this issue changed how URLs are generated (at least for our use case).

I'm pretty confident that our use case falls under (above mentioned)

Client.new('https://host:6443/apis', 'batch/v1') — partially works?! undocumented, not fully intended.

---

As of now we're using version v4.9.0 and we're creating kubeclient instances like so:

https://github.com/SumoLogic/sumologic-kubernetes-collection/blob/6648b63c3f6a20698867f5bbb179ea9293ccf3db/fluent-plugin-enhance-k8s-metadata/lib/sumologic/kubernetes/connector.rb#L26-L44

With kubernetes URL (for the needs of our tests) set to http://localhost:8080 and versions set to

• v1 to e.g get pods with /api/v1/namespaces/{name}/pods
• or 'apps/v1 and extensions/v1beta1 for group APIs (for instance to get /apis/extensions/v1beta1/namespaces/{namespace})

So with 4.9.0 we were getting

Kubeclient::Client.new with URL: http://localhost:8080/api and apiVersion: v1
api version v1
api group
api endpoint http://localhost:8080/api

Kubeclient::Client.new with URL: http://localhost:8080/apis and apiVersion: apps/v1
api version apps/v1
api group
api endpoint http://localhost:8080/apis

Kubeclient::Client.new with URL: http://localhost:8080/apis and apiVersion: extensions/v1beta1
api version extensions/v1beta1
api group
api endpoint http://localhost:8080/apis

which makes calls like http://localhost:8080/apis/extensions/v1beta1/namespaces/sumologic/replicasets/{name} but with 4.9.1 we're getting (mark the additional api at the end of the group apis):

Kubeclient::Client.new with URL: http://localhost:8080/api and apiVersion: v1
api version v1
api group
api endpoint http://localhost:8080/api

Kubeclient::Client.new with URL: http://localhost:8080/apis and apiVersion: apps/v1
api version apps/v1
api group
api endpoint http://localhost:8080/apis/api

Kubeclient::Client.new with URL: http://localhost:8080/apis and apiVersion: extensions/v1beta1
api version extensions/v1beta1
api group
api endpoint http://localhost:8080/apis/api

which makes calls like http://localhost:8080/apis/api/extensions/v1beta1/namespaces/sumologic/replicasets/{name}

So the question is how to create the client in this case? The tests added in the PR that fixed this issue don't include a case like this.

---

EDIT

I've tried the approach listed in https://github.com/abonas/kubeclient#usage for the group api

so that I call it like this (and get the values below accordingly):

Kubeclient::Client.new with URL: http://localhost:8080/api and apiVersion: v1
api version v1
api group
api endpoint http://localhost:8080/api

Kubeclient::Client.new with URL: http://localhost:8080/apis/apps and apiVersion: v1
api version v1
api group apps/
api endpoint http://localhost:8080/apis/apps

Kubeclient::Client.new with URL: http://localhost:8080/apis/extensions and apiVersion: v1beta1
api version v1beta1
api group extensions/
api endpoint http://localhost:8080/apis/extensions

but then a couple of assertions tests running just fine with 4.9.0 fail e.g. pod metadata's owners are empty but it should be populated - https://github.com/SumoLogic/sumologic-kubernetes-collection/blob/656c3b4a1904b66c749b5f3f57797c886d18ca83/fluent-plugin-enhance-k8s-metadata/test/sumologic/kubernetes/test_reader.rb#L44

[cben]

The latter approach from your EDIT is correct.
(I agree it's silly, #284, but that's want currently vsupposed to work)

Can you give more info on your test?
Is the test doing real network requests, or stubbing them in some way?
What is the full response you get from fetch_pod_metadata?
Try also using httplog gem to record details.

[pmalek-sumo]

@cben Thanks for the prompt response!

We're stubbing all of the requests here and we've noticed a problem because real requests are disabled and hence we were getting webmock errors

WebMock::NetConnectNotAllowedError: Real HTTP connections are disabled

An example of a problematic test that fails when upgraded to 4.9.1 can be found here

fetch_pod_metadata for 4.9.1 returns

{"pod_labels"=>{"pod_labels"=>{"k8s-app"=>"kube-dns", "pod-template-hash"=>"1967608236"}}, "node"=>"ip-172-20-90-197.us-west-1.compute.internal", "owners"=>{}}

and for 4.9.0

{"pod_labels"=>{"pod_labels"=>{"k8s-app"=>"kube-dns", "pod-template-hash"=>"1967608236"}}, "node"=>"ip-172-20-90-197.us-west-1.compute.inte│CLIENT: create with URL: http://localhost:8080/apis and apiVersion: extensions/v1beta1
rnal", "owners"=>{"replicaset"=>"kube-dns-5fbcb4d67b", "deployment"=>"kube-dns"}}

As for the httplog I'll look into that, thanks.

[pmalek-sumo]

@cben I've managed to fix the problem described above with the approach that you pointed out as correct - a bit troublesome but it works. Thanks.