Allowing URI path prefixes in server URLs #110

jgnagy · 2019-04-04T20:43:56Z

Some Kubernetes platforms (like Rancher 2) proxy kubernetes API requests through a reverse-proxy. The kubectl command (as well as other kubernetes tools that use it) work properly with this approach. This PR brings k8s-client closer to the behavior users expect from tools like these.

I experienced this issue while trying to use mortar and tracked the issue down to how this library was handling proxied requests like these.

While the testing that I added is very far from exhaustive, it does at least ensure that when provided a server with a URI path, it is handled correctly from a Transport perspective. I also verified that I can now see resources in my Rancher cluster.

Hopefully this can be reviewed and released so I can pester the mortar maintainers to pull it in at that level.

jgnagy · 2019-04-04T20:54:08Z

Looks like the build in Travis is only failing because of bundler 2.x vs 1.x differences in 2.5. It certainly isn't caused by anything I did, but if you need me to try and resolve it I'm happy to.

kke · 2019-04-05T07:33:15Z

I have made a bunch of rubocop / dry-types / travis fixes that were included in #112

It should remove the unrelated changes from this PR.

jnummelin · 2019-04-05T08:20:27Z

Let's get #112 merged first to make master branch more stable

kke · 2019-04-05T08:28:06Z

#112 merged, please rebase / merge master.

jgnagy · 2019-04-08T14:13:16Z

It looks like #112 removed rake from the gemspec which will make testing/linting a little more difficult, FYI (I just ran bundle exec rubocop and bundle exec rspec).

jgnagy · 2019-04-08T14:29:24Z

Merged in from master, retested, and pushed.

jakolehm

LGTM

jakolehm · 2019-04-30T08:06:07Z

@kke PTAL

kke · 2019-04-30T08:38:39Z

lib/k8s/transport.rb

    # @param auth_token [String] optional Authorization: Bearer token
    # @param options [Hash] @see Excon.new
    def initialize(server, auth_token: nil, **options)
      @server = server
+      @path_prefix = URI.parse(server).path


I think this needs to change the way @server is initialized too.

uri = URI.parse(server) @server = #{uri.protocol}://#{uri.host}:#{uri.port}" @path_prefix = uri.path

kke · 2019-04-30T08:51:00Z

lib/k8s/transport.rb

-      File.join('/', *path)
+      path_components = ['/', *path]
+      # Only add the path_prefix if it isn't there already...
+      path_components.insert(0, path_prefix) unless [*path].join.match?(/^#{path_prefix}/)


[*path] is redundant, it will just return a copy of the same array:

> path = ['a', 'b'] > [*path] => ['a', 'b']

[*path].join.match will not add slashes to the path:

['a', 'b', 'c'].join => 'abc' ['a', 'b', 'c'].join('/') => 'a/b/c'

Also the regex is unnecessary, it could just be path.join('/').start_with?(path_prefix)

Maybe something like this:

def path(*path) result = path.join('/') result = File.join(path_prefix, path) unless result.start_with?(path_prefix) result.start_with?('/') ? result : "/#{result}" end

But is it actually necessary to skip adding the prefix if path already includes it?

That would force you to use a prefix that is not a kube api endpoint path. If the prefix is /api, maybe it should just generate /api/api. If you don't want that, just don't add the prefix to your server uri.

That being said, I think the whole method can then be:

def path(*path) File.join(path_prefix, *path) end

if the path_prefix is set to / in the initializer in case the uri path was empty.

Using this one-line path yielded what looks to be a recursive issue with the construction of the path containing a prefix:

2.5.3 :001 > client = K8s::Client.config(K8s::Config.load_file(ENV['KUBECONFIG'])) => #<K8s::Client:0x00007fd2e01d15b8 @transport=#<K8s::Transport:0x00007fd2e01d1ae0 @server="https://REDACTED.example.com:443", @path_prefix="/k8s/clusters/c-vkf6c", @auth_token="... 2.5.3 :002 > client.api.resource('pods', namespace: 'cattle-system').list.each do |pod| 2.5.3 :003 > puts "namespace=#{pod.metadata.namespace} pod: #{pod.metadata.name}" 2.5.3 :004?> end Traceback (most recent call last): 16: from /Users/jlg438/.rvm/gems/ruby-2.5.3/gems/bundler-1.17.3/lib/bundler/vendor/thor/lib/thor.rb:387:in `dispatch' 15: from /Users/jlg438/.rvm/gems/ruby-2.5.3/gems/bundler-1.17.3/lib/bundler/vendor/thor/lib/thor/invocation.rb:126:in `invoke_command' 14: from /Users/jlg438/.rvm/gems/ruby-2.5.3/gems/bundler-1.17.3/lib/bundler/vendor/thor/lib/thor/command.rb:27:in `run' 13: from /Users/jlg438/.rvm/gems/ruby-2.5.3/gems/bundler-1.17.3/lib/bundler/cli.rb:463:in `exec' 12: from /Users/jlg438/.rvm/gems/ruby-2.5.3/gems/bundler-1.17.3/lib/bundler/cli/exec.rb:28:in `run' 11: from /Users/jlg438/.rvm/gems/ruby-2.5.3/gems/bundler-1.17.3/lib/bundler/cli/exec.rb:74:in `kernel_load' 10: from /Users/jlg438/.rvm/gems/ruby-2.5.3/gems/bundler-1.17.3/lib/bundler/cli/exec.rb:74:in `load' 9: from exe/console:8:in `<top (required)>' 8: from (irb):2 7: from /Users/jlg438/Development/ruby/k8s-client/lib/k8s/api_client.rb:73:in `resource' 6: from /Users/jlg438/Development/ruby/k8s-client/lib/k8s/api_client.rb:61:in `find_api_resource' 5: from /Users/jlg438/Development/ruby/k8s-client/lib/k8s/api_client.rb:54:in `api_resources' 4: from /Users/jlg438/Development/ruby/k8s-client/lib/k8s/api_client.rb:46:in `api_resources!' 3: from /Users/jlg438/Development/ruby/k8s-client/lib/k8s/transport.rb:326:in `get' 2: from /Users/jlg438/Development/ruby/k8s-client/lib/k8s/transport.rb:250:in `request' 1: from /Users/jlg438/Development/ruby/k8s-client/lib/k8s/transport.rb:228:in `parse_response' K8s::Error::NotFound (GET /k8s/clusters/c-vkf6c/k8s/clusters/c-vkf6c/api/v1 => HTTP 404 Not Found: {"paths"=>["/apis", "/apis/", "/apis/apiextensions.k8s.io", "/apis/apiextensions.k8s.io/v1beta1", "/health z", "/healthz/etcd", "/healthz/log", "/healthz/ping", "/healthz/poststarthook/generic-apiserver-start-informers", "/healthz/poststarthook/start-apiextensions-controllers", "/healthz/poststarthook/start-api extensions-informers", "/metrics", "/openapi/v2", "/swagger-2.0.0.json", "/swagger-2.0.0.pb-v1", "/swagger-2.0.0.pb-v1.gz", "/swagger.json", "/swaggerapi", "/version"]})

Notice the GET /k8s/clusters/c-vkf6c/k8s/clusters/c-vkf6c/api/v1.

Using the "Maybe something like this" works better and all the tests still pass:

2.5.3 :001 > client = K8s::Client.config(K8s::Config.load_file(ENV['KUBECONFIG'])) => #<K8s::Client:0x00007fd0520055a8 @transport=#<K8s::Transport:0x00007fd052005aa8 @server="https://REDACTED.example.com:443", @path_prefix="/k8s/clusters/c-vkf6c", @auth_token="... 2.5.3 :002 > client.api.resource('pods', namespace: 'cattle-system').list.each do |pod| 2.5.3 :003 > puts "namespace=#{pod.metadata.namespace} pod: #{pod.metadata.name}" 2.5.3 :004?> end namespace=cattle-system pod: cattle-cluster-agent-78c9cd6ddb-wdf4c namespace=cattle-system pod: cattle-node-agent-4b9zl namespace=cattle-system pod: cattle-node-agent-88r5s namespace=cattle-system pod: cattle-node-agent-8dqrd ...

kke

The [*path].join.match?(/^#{path_prefix}/) will never match a prefix with / slashes in it.

jgnagy · 2019-05-07T17:23:09Z

@kke, please review my comments and the additional commit. I made the initializer change you mentioned for @server, though this appears to now conflict what a more recent change for auth. Should be fairly easy to resolve but I will defer to you on how to do so.

kke · 2019-05-08T08:35:21Z

There was some sort of recursion going on

      result = path.join('/')
      result = File.join(path_prefix, path) unless result.start_with?(path_prefix)
                                                     # ^^^ the original array, should've been "result" probably.
      result.start_with?('/') ? result : "/#{result}"

I resolved the conflict and made it a bit prettier now.

kke · 2019-05-08T08:38:23Z

lib/k8s/transport.rb

-      @server = server
+      uri = URI.parse(server)
+      @server = "#{uri.scheme}://#{uri.host}:#{uri.port}"
+      @path_prefix = File.join('/', uri.path, '/') # add leading and/or trailing slashes


irb(main):001:0> File.join('/', '', '/') => "/" irb(main):004:0> File.join('/', '/', '/') => "/" irb(main):003:0> File.join('/', '/foo/', '/') => "/foo/" irb(main):005:0> File.join('/', '/foo', '/') => "/foo/"

The prefix now always has either / or a path with leading and trailing /s.

kke · 2019-05-08T08:45:47Z

@jakolehm seems fine to me now, ptala.

jakolehm

LGTM

rbq · 2019-08-14T09:40:09Z

@jgnagy How do you configure the client for Rancher? I'm using my existing ~/.kube/config, which creates a client with @path_prefix="/k8s/clusters/c-blahblah/", but then any requests fails with the path in there twice:

GET /k8s/clusters/c-blahblah/k8s/clusters/c-blahblah/api/v1 => HTTP 404 Not Found`

kke · 2019-08-14T10:32:49Z

@rbq what does your request look like? (client.api('v1').resource('xyz').get) ?)

rbq · 2019-08-14T10:57:14Z

@kke I tried different things, e.g. the example from the README. Right now my script looks like this:

#!/usr/bin/env ruby
require 'bundler/inline'

gemfile do
  source 'https://rubygems.org'
  gem 'k8s-client'
end

config = K8s::Config.load_file File.expand_path '~/.kube/config'
client = K8s::Client.config config

client.api('v1').resource('ingresses', namespace: 'default').list().each do |i|
  puts "namespace=#{i.metadata.namespace} name=#{i.metadata.name}"
end

rbq · 2019-08-14T15:02:29Z

Oh, and the rest of the exception, in case that's useful in any way:

Traceback (most recent call last):
	7: from ./index.rb:12:in `<main>'
	6: from […]/gems/k8s-client-0.10.2/lib/k8s/api_client.rb:73:in `resource'
	5: from […]/gems/k8s-client-0.10.2/lib/k8s/api_client.rb:61:in `find_api_resource'
	4: from […]/gems/k8s-client-0.10.2/lib/k8s/api_client.rb:54:in `api_resources'
	3: from […]/gems/k8s-client-0.10.2/lib/k8s/api_client.rb:46:in `api_resources!'
	2: from […]/gems/k8s-client-0.10.2/lib/k8s/transport.rb:362:in `get'
	1: from […]/gems/k8s-client-0.10.2/lib/k8s/transport.rb:286:in `request'
[…]/gems/k8s-client-0.10.2/lib/k8s/transport.rb:264:in `parse_response': ⏎
GET /k8s/clusters/c-blahblah/k8s/clusters/c-blahblah/api/v1 => HTTP 404 ⏎
Not Found: {"paths"=>["/apis", "/apis/", "/apis/apiextensions.k8s.io", ⏎
"/apis/apiextensions.k8s.io/v1beta1", "/healthz", "/healthz/etcd", ⏎
"/healthz/log", "/healthz/ping", "/healthz/poststarthook/crd-informer-synced", ⏎
"/healthz/poststarthook/generic-apiserver-start-informers", ⏎
"/healthz/poststarthook/start-apiextensions-controllers", ⏎
"/healthz/poststarthook/start-apiextensions-informers", "/metrics", ⏎
"/openapi/v2", "/version"]} (K8s::Error::NotFound)

jgnagy · 2019-08-14T16:57:19Z

@rbq, my original PR for this didn't have this issue, but as I mentioned in this comment, the PR was modified by @kke to change the behavior. I have not been able to use this k8s-client with Rancher given how k8s-client handles this proxying. My team just drops to shell commands for kubectl for most things at this point, though I hope this is resolved eventually.

kke · 2019-08-15T11:50:43Z

@rbq / @jgnagy can you try with the branch of #158 ?

#!/usr/bin/env ruby
require 'bundler/inline'

gemfile do
  source 'https://rubygems.org'
  gem 'k8s-client', github: 'kontena/k8s-client', branch: 'fix/prefix_path'
end

config = K8s::Config.load_file File.expand_path '~/.kube/config'
client = K8s::Client.config config

client.api('v1').resource('ingresses', namespace: 'default').list().each do |i|
  puts "namespace=#{i.metadata.namespace} name=#{i.metadata.name}"
end

rbq · 2019-08-15T11:59:45Z

@kke The branch works (but unfortuanately my script doesn't, since Ingress is not actually a v1 resource ;).

jgnagy added 5 commits April 4, 2019 12:44

Adding support for a path prefix

90efbde

Locking dry-types to 0.14.x to prevent breaking

7e6a03b

Appeasing rubocop

6d193aa

Adding some tests and better path_prefix logic

eae4402

Switching Array#prepend to #insert (2.4 support)

198a5db

jnummelin requested a review from kke April 5, 2019 05:00

Merging in from upstream master

d0b41cc

jakolehm added the enhancement New feature or request label Apr 10, 2019

jakolehm approved these changes Apr 10, 2019

View reviewed changes

kke reviewed Apr 30, 2019

View reviewed changes

kke suggested changes Apr 30, 2019

View reviewed changes

Making suggested changes

9791ce6

Kimmo Lehto added 2 commits May 8, 2019 10:20

Merge branch 'master' into feature/allow-path-prefixes

d694683

Beautify a bit

b0bec29

kke reviewed May 8, 2019

View reviewed changes

kke approved these changes May 8, 2019

View reviewed changes

jakolehm approved these changes May 8, 2019

View reviewed changes

kke merged commit b9d7aa3 into kontena:master May 8, 2019

kke mentioned this pull request May 9, 2019

Bump version to 0.10.0 #118

Merged

kke mentioned this pull request May 9, 2019

Use k8s-client 0.10 kontena/mortar#100

Closed

This was referenced Aug 15, 2019

Server addresses with path-prefix do not work #157

Closed

Fix path prefix handling in server address #158

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allowing URI path prefixes in server URLs #110

Allowing URI path prefixes in server URLs #110

jgnagy commented Apr 4, 2019

jgnagy commented Apr 4, 2019

kke commented Apr 5, 2019

jnummelin commented Apr 5, 2019

kke commented Apr 5, 2019

jgnagy commented Apr 8, 2019

jgnagy commented Apr 8, 2019

jakolehm left a comment

jakolehm commented Apr 30, 2019

kke Apr 30, 2019 •

edited

Loading

kke Apr 30, 2019

kke Apr 30, 2019

jgnagy May 7, 2019

kke left a comment

jgnagy commented May 7, 2019

kke commented May 8, 2019 •

edited

Loading

kke May 8, 2019

kke May 8, 2019

kke commented May 8, 2019

jakolehm left a comment

rbq commented Aug 14, 2019

kke commented Aug 14, 2019

rbq commented Aug 14, 2019

rbq commented Aug 14, 2019

jgnagy commented Aug 14, 2019

kke commented Aug 15, 2019

rbq commented Aug 15, 2019

Allowing URI path prefixes in server URLs #110

Allowing URI path prefixes in server URLs #110

Conversation

jgnagy commented Apr 4, 2019

jgnagy commented Apr 4, 2019

kke commented Apr 5, 2019

jnummelin commented Apr 5, 2019

kke commented Apr 5, 2019

jgnagy commented Apr 8, 2019

jgnagy commented Apr 8, 2019

jakolehm left a comment

Choose a reason for hiding this comment

jakolehm commented Apr 30, 2019

kke Apr 30, 2019 • edited Loading

Choose a reason for hiding this comment

kke Apr 30, 2019

Choose a reason for hiding this comment

kke Apr 30, 2019

Choose a reason for hiding this comment

jgnagy May 7, 2019

Choose a reason for hiding this comment

kke left a comment

Choose a reason for hiding this comment

jgnagy commented May 7, 2019

kke commented May 8, 2019 • edited Loading

kke May 8, 2019

Choose a reason for hiding this comment

kke May 8, 2019

Choose a reason for hiding this comment

kke commented May 8, 2019

jakolehm left a comment

Choose a reason for hiding this comment

rbq commented Aug 14, 2019

kke commented Aug 14, 2019

rbq commented Aug 14, 2019

rbq commented Aug 14, 2019

jgnagy commented Aug 14, 2019

kke commented Aug 15, 2019

rbq commented Aug 15, 2019

kke Apr 30, 2019 •

edited

Loading

kke commented May 8, 2019 •

edited

Loading