Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace rest_client with faraday #466

Merged
merged 9 commits into from
Dec 11, 2020
3 changes: 2 additions & 1 deletion kubeclient.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,9 @@ Gem::Specification.new do |spec|
spec.add_development_dependency('mocha', '~> 1.5')
spec.add_development_dependency 'openid_connect', '~> 1.1'

spec.add_dependency 'faraday', '~> 1.1'
spec.add_dependency 'faraday_middleware', '~> 1.0'
spec.add_dependency 'jsonpath', '~> 1.0'
spec.add_dependency 'rest-client', '~> 2.0'
spec.add_dependency 'recursive-open-struct', '~> 1.1', '>= 1.1.1'
spec.add_dependency 'http', '>= 3.0', '< 5.0'
end
1 change: 0 additions & 1 deletion lib/kubeclient.rb
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
# frozen_string_literal: true

require 'json'
require 'rest-client'

require_relative 'kubeclient/aws_eks_credentials'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should not be necessary and can mess with require hacks, so prefer to avoid

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what you refer to with this? Is it all the require => require_relative changes? It's just been merged on master, so needs to reverted if it's not the best idea after all.

@grosser can you point me to where I can read more about this?

require_relative 'kubeclient/common'
Expand Down
126 changes: 68 additions & 58 deletions lib/kubeclient/common.rb
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
# frozen_string_literal: true

require 'faraday'
require 'faraday_middleware'
require 'json'
require 'rest-client'
require 'net/http'

module Kubeclient
# Common methods
Expand Down Expand Up @@ -121,16 +123,21 @@ def discovery_needed?(method_sym)

def handle_exception
yield
rescue RestClient::Exception => e
rescue Faraday::Error => e
err_message = build_http_error_message(e)
response_code = e.response ? (e.response[:status] || e.response&.env&.status) : nil
error_klass = (response_code == 404 ? ResourceNotFoundError : HttpError)
raise error_klass.new(response_code, err_message, e.response)
end

def build_http_error_message(e)
json_error_msg =
begin
JSON.parse(e.response || '') || {}
rescue JSON::ParserError
JSON.parse(e.response[:body] || '') || {}
rescue StandardError
andrzej-stencel marked this conversation as resolved.
Show resolved Hide resolved
{}
end
err_message = json_error_msg['message'] || e.message
error_klass = e.http_code == 404 ? ResourceNotFoundError : HttpError
raise error_klass.new(e.http_code, err_message, e.response)
json_error_msg['message'] || e.message || ''
end

def discover
Expand Down Expand Up @@ -285,27 +292,35 @@ def self.underscore_entity(entity_name)
.downcase
end

def create_rest_client(path = nil)
path ||= @api_endpoint.path
def create_http_client(url = nil)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please mark this, and everything faraday-specific, private. (feel free to re-order them if you want)

As already discussed, breaking people that used undocumented .create_rest_client / .rest_client is fine 👍.
Perhaps in future we'll expose some or parts of it — this is tempting with Faraday. Maybe we'll keep it abstracted (#389). Maybe we'll instead convenience methods on Config (#417). For now making it private keeps our options open...

naming: currently, variables and methods called http_... mostly referred to the http gem (as opposed to rest-client). But we'll get rid of http and anyway that was confusing usage, I'm happy to reclaim it to just mean the HTTP protocol 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having .http_client public makes it super-easy to replace the Faraday adapter with net/http/persistent by the users of kubeclient. I can now do this like this:

client = Kubeclient::Client.new(...)
client.http_client.adapter(:net_http_persistent)

This way I get HTTP connection persistence without having it in upstream Kubeclient.

Yes I would really like to be able to get HTTP connection persistence with Kubeclient 5.0, that's the whole point, or the business driver if you will :)

url = "#{@api_endpoint}/#{@api_version}" if url.nil?
options = {
ssl_ca_file: @ssl_options[:ca_file],
ssl_cert_store: @ssl_options[:cert_store],
verify_ssl: @ssl_options[:verify_ssl],
ssl_client_cert: @ssl_options[:client_cert],
ssl_client_key: @ssl_options[:client_key],
proxy: @http_proxy_uri,
max_redirects: @http_max_redirects,
user: @auth_options[:username],
password: @auth_options[:password],
open_timeout: @timeouts[:open],
read_timeout: @timeouts[:read]
request: {
open_timeout: @timeouts[:open],
read_timeout: @timeouts[:read]
},
ssl: {
ca_file: @ssl_options[:ca_file],
cert_store: @ssl_options[:cert_store],
client_cert: @ssl_options[:client_cert],
client_key: @ssl_options[:client_key],
verify: @ssl_options[:verify_ssl]
}
}
RestClient::Resource.new(@api_endpoint.merge(path).to_s, options)

Faraday.new(url, options) do |connection|
if @auth_options[:username] && @auth_options[:password]
connection.basic_auth(@auth_options[:username], @auth_options[:password])
end
connection.use(FaradayMiddleware::FollowRedirects, limit: @http_max_redirects)
cben marked this conversation as resolved.
Show resolved Hide resolved
connection.response(:raise_error)
end
end

def rest_client
@rest_client ||= begin
create_rest_client("#{@api_endpoint.path}/#{@api_version}")
def http_client
@http_client ||= begin
create_http_client
end
end

Expand Down Expand Up @@ -355,8 +370,7 @@ def get_entities(entity_type, resource_name, options = {})

ns_prefix = build_namespace_prefix(options[:namespace])
response = handle_exception do
rest_client[ns_prefix + resource_name]
.get({ 'params' => params }.merge(@headers))
http_client.get(ns_prefix + resource_name, params, @headers)
end
format_response(options[:as] || @as, response.body, entity_type)
end
Expand All @@ -368,8 +382,7 @@ def get_entities(entity_type, resource_name, options = {})
def get_entity(resource_name, name, namespace = nil, options = {})
ns_prefix = build_namespace_prefix(namespace)
response = handle_exception do
rest_client[ns_prefix + resource_name + "/#{name}"]
.get(@headers)
http_client.get("#{ns_prefix}#{resource_name}/#{name}", nil, @headers)
end
format_response(options[:as] || @as, response.body)
end
Expand All @@ -380,15 +393,9 @@ def delete_entity(resource_name, name, namespace = nil, delete_options: {})
ns_prefix = build_namespace_prefix(namespace)
payload = delete_options_hash.to_json unless delete_options_hash.empty?
response = handle_exception do
rs = rest_client[ns_prefix + resource_name + "/#{name}"]
RestClient::Request.execute(
rs.options.merge(
method: :delete,
url: rs.url,
headers: { 'Content-Type' => 'application/json' }.merge(@headers),
payload: payload
)
)
http_client.delete("#{ns_prefix}#{resource_name}/#{name}", nil, json_headers) do |request|
request.body = payload
end
end
format_response(@as, response.body)
end
Expand All @@ -406,30 +413,29 @@ def create_entity(entity_type, resource_name, entity_config)
hash[:kind] = entity_type
hash[:apiVersion] = @api_group + @api_version
response = handle_exception do
rest_client[ns_prefix + resource_name]
.post(hash.to_json, { 'Content-Type' => 'application/json' }.merge(@headers))
http_client.post(ns_prefix + resource_name, hash.to_json, json_headers)
end
format_response(@as, response.body)
end

def update_entity(resource_name, entity_config)
name = entity_config[:metadata][:name]
ns_prefix = build_namespace_prefix(entity_config[:metadata][:namespace])
params = entity_config.to_h.to_json
response = handle_exception do
rest_client[ns_prefix + resource_name + "/#{name}"]
.put(entity_config.to_h.to_json, { 'Content-Type' => 'application/json' }.merge(@headers))
http_client.put("#{ns_prefix}#{resource_name}/#{name}", params, json_headers)
end
format_response(@as, response.body)
end

def patch_entity(resource_name, name, patch, strategy, namespace)
ns_prefix = build_namespace_prefix(namespace)
response = handle_exception do
rest_client[ns_prefix + resource_name + "/#{name}"]
.patch(
patch.to_json,
{ 'Content-Type' => "application/#{strategy}+json" }.merge(@headers)
)
http_client.patch(
"#{ns_prefix}#{resource_name}/#{name}",
patch.to_json,
{ 'Content-Type' => "application/#{strategy}+json" }.merge(@headers)
)
end
format_response(@as, response.body)
end
Expand All @@ -438,11 +444,11 @@ def apply_entity(resource_name, resource, field_manager:, force: true)
name = "#{resource[:metadata][:name]}?fieldManager=#{field_manager}&force=#{force}"
ns_prefix = build_namespace_prefix(resource[:metadata][:namespace])
response = handle_exception do
rest_client[ns_prefix + resource_name + "/#{name}"]
.patch(
resource.to_json,
{ 'Content-Type' => 'application/apply-patch+yaml' }.merge(@headers)
)
http_client.patch(
"#{ns_prefix}#{resource_name}/#{name}",
resource.to_json,
{ 'Content-Type' => 'application/apply-patch+yaml' }.merge(@headers)
)
end
format_response(@as, response.body)
end
Expand Down Expand Up @@ -474,8 +480,7 @@ def get_pod_log(pod_name, namespace,

ns = build_namespace_prefix(namespace)
handle_exception do
rest_client[ns + "pods/#{pod_name}/log"]
.get({ 'params' => params }.merge(@headers))
http_client.get("#{ns}pods/#{pod_name}/log", params, @headers).body
end
end

Expand Down Expand Up @@ -506,16 +511,15 @@ def proxy_url(kind, name, port, namespace = '')
@entities[kind.to_s].resource_name
end
ns_prefix = build_namespace_prefix(namespace)
rest_client["#{ns_prefix}#{entity_name_plural}/#{name}:#{port}/proxy"].url
"#{@api_endpoint}/#{@api_version}/#{ns_prefix}#{entity_name_plural}/#{name}:#{port}/proxy"
end

def process_template(template)
ns_prefix = build_namespace_prefix(template[:metadata][:namespace])
response = handle_exception do
rest_client["#{ns_prefix}processedtemplates"]
.post(template.to_h.to_json, { 'Content-Type' => 'application/json' }.merge(@headers))
http_client.post("#{ns_prefix}processedtemplates", template.to_h.to_json, json_headers)
end
JSON.parse(response)
JSON.parse(response.body)
andrzej-stencel marked this conversation as resolved.
Show resolved Hide resolved
end

def api_valid?
Expand All @@ -526,7 +530,9 @@ def api_valid?
end

def api
response = handle_exception { create_rest_client.get(@headers) }
response = handle_exception do
create_http_client(@api_endpoint.to_s).get(nil, nil, @headers).body
end
JSON.parse(response)
end

Expand Down Expand Up @@ -602,7 +608,7 @@ def load_entities
end

def fetch_entities
JSON.parse(handle_exception { rest_client.get(@headers) })
JSON.parse(handle_exception { http_client.get(nil, nil, @headers).body })
end

def bearer_token(bearer_token)
Expand Down Expand Up @@ -666,5 +672,9 @@ def http_options(uri)

options.merge(@socket_options)
end

def json_headers
{ 'Content-Type' => 'application/json' }.merge(@headers)
end
end
end
6 changes: 4 additions & 2 deletions lib/kubeclient/http_error.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,10 @@ def initialize(error_code, message, response) # rubocop:disable Lint/MissingSupe

def to_s
string = "HTTP status code #{@error_code}, #{@message}"
if @response.is_a?(RestClient::Response) && @response.request
string += " for #{@response.request.method.upcase} #{@response.request.url}"
if @response && @response[:request]
request_method = @response[:request][:method]&.to_s&.upcase
request_path = @response[:request][:url_path]
string += " for #{request_method} #{request_path}"
end
string
end
Expand Down
Loading