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.

Sign up for GitHub

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

Jump to bottom

Bugfix/k8s context #324

Merged
merged 4 commits into from
Oct 13, 2021
Merged

Bugfix/k8s context #324

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6b65124
only use context when specified
johrstrom Oct 12, 2021
0b2fb1c
static stuff actually works
johrstrom Oct 12, 2021
c01a75e
could bugs and some tests
johrstrom Oct 13, 2021
2abc321
fix gke tests
johrstrom Oct 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
82 changes: 38 additions & 44 deletions lib/ood_core/job/adapters/kubernetes/batch.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,32 +11,25 @@ class OodCore::Job::Adapters::Kubernetes::Batch
class Error < StandardError; end
class NotFoundError < StandardError; end

attr_reader :config_file, :bin, :cluster, :mounts
attr_reader :all_namespaces, :using_context, :helper
attr_reader :config_file, :bin, :cluster, :context, :mounts
attr_reader :all_namespaces, :helper
attr_reader :username_prefix, :namespace_prefix
attr_reader :auto_supplemental_groups

def initialize(options = {})
options = options.to_h.symbolize_keys

@config_file = options.fetch(:config_file, default_config_file)
@config_file = options.fetch(:config_file, Batch.default_config_file)
@bin = options.fetch(:bin, '/usr/bin/kubectl')
@cluster = options.fetch(:cluster, 'open-ondemand')
@context = options.fetch(:context, nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

@johrstrom Should this default to @cluster to maintain past behavior? I think forcing a context makes sense since not having forced context can cause a lot of problems and lead to sites easily having weird behavior. If we don't force a context then this will require a Puppet change so OSC side doesn't break. It would still require a Puppet change to expose all options but not required if we have sane default.

@mounts = options.fetch(:mounts, []).map { |m| m.to_h.symbolize_keys }
@all_namespaces = options.fetch(:all_namespaces, false)
@username_prefix = options.fetch(:username_prefix, '')
@namespace_prefix = options.fetch(:namespace_prefix, '')
@auto_supplemental_groups = options.fetch(:auto_supplemental_groups, false)

@using_context = false
@helper = OodCore::Job::Adapters::Kubernetes::Helper.new

begin
make_kubectl_config(options)
rescue
# FIXME could use a log here
# means you couldn't 'kubectl set config'
end
end

def resource_file(resource_type = 'pod')
Expand Down Expand Up @@ -117,6 +110,32 @@ def delete(id)
safe_call("delete", "configmap", configmap_name(id))
end

class << self
def default_config_file
(ENV['KUBECONFIG'] || "#{Dir.home}/.kube/config")
end

def default_auth
{
type: 'managed'
}.symbolize_keys
end

def default_server
{
endpoint: 'https://localhost:8080',
cert_authority_file: nil
}.symbolize_keys
end

def configure_kube!(config)
k = self.new(config)
# TODO: probably shouldn't be using send here
k.send(:set_cluster, config.fetch(:server, default_server).to_h.symbolize_keys)
k.send(:configure_auth, config.fetch(:auth, default_auth).to_h.symbolize_keys)
end
end

private

def safe_call(verb, resource, id)
Expand Down Expand Up @@ -250,27 +269,6 @@ def namespace
"#{namespace_prefix}#{username}"
end

def context
cluster
end

def default_config_file
(ENV['KUBECONFIG'] || "#{Dir.home}/.kube/config")
end

def default_auth
{
type: 'managaged'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

managed was misspelled here. fixed in this pr.

}.symbolize_keys
end

def default_server
{
endpoint: 'https://localhost:8080',
cert_authority_file: nil
}.symbolize_keys
end

def formatted_ns_cmd
"#{namespaced_cmd} -o json"
end
Expand All @@ -281,7 +279,7 @@ def namespaced_cmd

def base_cmd
base = "#{bin} --kubeconfig=#{config_file}"
base << " --context=#{context}" if using_context
base << " --context=#{context}" if context?
base
end

Expand Down Expand Up @@ -309,11 +307,6 @@ def pod_info_from_json(pod)
nil
end

def make_kubectl_config(config)
set_cluster(config.fetch(:server, default_server).to_h.symbolize_keys)
configure_auth(config.fetch(:auth, default_auth).to_h.symbolize_keys)
end

def configure_auth(auth)
type = auth.fetch(:type)
return if managed?(type)
Expand All @@ -326,8 +319,8 @@ def configure_auth(auth)
end
end

def use_context
@using_context = true
def context?
!@context.nil?
end

def managed?(type)
Expand Down Expand Up @@ -359,23 +352,24 @@ def set_gke_credentials(auth)
# gke cluster name can probably can differ from what ood calls the cluster
cmd = "gcloud container clusters get-credentials #{locale} #{cluster}"
env = { 'KUBECONFIG' => config_file }
call(cmd, env)
call(cmd, env: env)
end

def set_context
cmd = "#{base_cmd} config set-context #{cluster}"
# can't really use base_cmd, bc it may use --context flag
cmd = "#{bin} --kubeconfig=#{config_file} config set-context #{context}"
cmd << " --cluster=#{cluster} --namespace=#{namespace}"
cmd << " --user=#{k8s_username}"

call(cmd)
use_context
end

def set_cluster(config)
server = config.fetch(:endpoint)
cert = config.fetch(:cert_authority_file, nil)

cmd = "#{base_cmd} config set-cluster #{cluster}"
# shouldn't use context here either
cmd = "#{bin} --kubeconfig=#{config_file} config set-cluster #{cluster}"
cmd << " --server=#{server}"
cmd << " --certificate-authority=#{cert}" unless cert.nil?

Expand Down
126 changes: 77 additions & 49 deletions spec/job/adapters/kubernetes/batch_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,40 +74,29 @@
config_file: '~/kube.config',
bin: '/usr/bin/wontwork',
cluster: 'test-cluster',
context: 'ood-test-cluster',
mounts: mounts,
all_namespaces: true,
namespace_prefix: 'user-',
username_prefix: 'dev',
username_prefix: 'dev-',
server: {
endpoint: 'https://some.k8s.host',
cert_authority_file: '/etc/some.cert'
},
auth: {
type: 'oidc'
},
auto_supplemental_groups: true
}
}

before(:each) do
allow(Open3).to receive(:capture3).with(
{},
"/usr/bin/kubectl --kubeconfig=#{ENV['HOME']}/.kube/config " \
"config set-cluster open-ondemand --server=https://localhost:8080",
stdin_data: ""
).and_return(['', '', success])

@basic_batch = described_class.new({})
allow(@basic_batch).to receive(:username).and_return('testuser')
allow(@basic_batch).to receive(:helper).and_return(helper)
end

let(:configured_batch){
allow(Open3).to receive(:capture3).with(
{},
"/usr/bin/wontwork --kubeconfig=~/kube.config " \
"config set-cluster test-cluster --server=https://some.k8s.host " \
"--certificate-authority=/etc/some.cert",
stdin_data: ""
).and_return(['', '', success])

batch = described_class.new(config)
allow(batch).to receive(:username).and_return('testuser')
allow(batch).to receive(:helper).and_return(helper)
Expand Down Expand Up @@ -263,6 +252,7 @@
expect(@basic_batch.config_file).to eq("#{ENV['HOME']}/.kube/config")
expect(@basic_batch.bin).to eq('/usr/bin/kubectl')
expect(@basic_batch.mounts).to eq([])
expect(@basic_batch.context).to eq(nil)
end

it "does not throw an error when it can't configure" do
Expand Down Expand Up @@ -352,7 +342,7 @@ def build_script(opts = {})
# make sure template get's passed into command correctly
allow(Open3).to receive(:capture3).with(
{},
"/usr/bin/wontwork --kubeconfig=~/kube.config " \
"/usr/bin/wontwork --kubeconfig=~/kube.config --context=ood-test-cluster " \
"--namespace=user-testuser -o json create -f -",
stdin_data: pod_yml_from_all_configs.to_s
).and_return(['', '', success])
Expand Down Expand Up @@ -919,7 +909,7 @@ def build_script(opts = {})
it "throws error up the stack with --all-namespaces" do
allow(Open3).to receive(:capture3).with(
{},
"/usr/bin/wontwork --kubeconfig=~/kube.config get pods -o json --all-namespaces",
"/usr/bin/wontwork --kubeconfig=~/kube.config --context=ood-test-cluster get pods -o json --all-namespaces",
stdin_data: ""
).and_return(['', errmsg, failure])

Expand All @@ -942,13 +932,6 @@ def build_script(opts = {})

describe "#info" do
def info_batch(id, file)
allow(Open3).to receive(:capture3).with(
{},
"/usr/bin/kubectl --kubeconfig=#{ENV['HOME']}/.kube/config " \
"config set-cluster open-ondemand --server=https://localhost:8080",
stdin_data: ""
).and_return(['', '', success])

batch = described_class.new({})
allow(batch).to receive(:username).and_return('testuser')
allow(batch).to receive(:helper).and_return(helper)
Expand Down Expand Up @@ -978,13 +961,6 @@ def info_batch(id, file)
end

def info_batch_full(id)
allow(Open3).to receive(:capture3).with(
{},
"/usr/bin/kubectl --kubeconfig=#{ENV['HOME']}/.kube/config " \
"config set-cluster open-ondemand --server=https://localhost:8080",
stdin_data: ""
).and_return(['', '', success])

batch = described_class.new({})
allow(batch).to receive(:username).and_return('testuser')
allow(DateTime).to receive(:now).and_return(past)
Expand Down Expand Up @@ -1012,13 +988,6 @@ def info_batch_full(id)
end

def not_found_batch(id)
allow(Open3).to receive(:capture3).with(
{},
"/usr/bin/kubectl --kubeconfig=#{ENV['HOME']}/.kube/config " \
"config set-cluster open-ondemand --server=https://localhost:8080",
stdin_data: ""
).and_return(['', '', success])

batch = described_class.new({})
allow(batch).to receive(:username).and_return('testuser')
allow(batch).to receive(:helper).and_return(helper)
Expand Down Expand Up @@ -1097,18 +1066,77 @@ def not_found_batch(id)
end
end

describe '#set_context' do
it 'generates correct command' do
expected_cmd = "/usr/bin/kubectl --kubeconfig=#{ENV['HOME']}/.kube/config config set-context open-ondemand --cluster=open-ondemand --namespace=testuser --user=testuser"
expect(@basic_batch).to receive(:call).with(expected_cmd)
@basic_batch.send(:set_context)
describe '#configure_kube!' do
it 'generates correct default commands' do
expected_cmd = "/usr/bin/kubectl --kubeconfig=#{ENV['HOME']}/.kube/config config set-cluster open-ondemand --server=https://localhost:8080"
expect_any_instance_of(Batch).to receive(:call).with(expected_cmd)
Batch.configure_kube!({})
end

it 'generates correct default auth commands' do
expected_cmd = "/usr/bin/kubectl --kubeconfig=#{ENV['HOME']}/.kube/config config set-cluster open-ondemand --server=https://localhost:8080"
expect_any_instance_of(Batch).to receive(:call).with(expected_cmd)
Batch.configure_kube!({ :auth => Batch.default_auth })
end

it 'generates sets oidc context' do
set_cluster = "/usr/bin/wontwork --kubeconfig=~/kube.config config set-cluster test-cluster " \
"--server=https://some.k8s.host --certificate-authority=/etc/some.cert"
set_context = "/usr/bin/wontwork --kubeconfig=~/kube.config config set-context ood-test-cluster " \
"--cluster=test-cluster --namespace=user-jessie --user=dev-jessie"

expect_any_instance_of(Batch).to receive(:call).with(set_cluster)
expect_any_instance_of(Batch).to receive(:username).and_return('jessie')
expect_any_instance_of(Batch).to receive(:username).and_return('jessie')
expect_any_instance_of(Batch).to receive(:call).with(set_context)
Batch.configure_kube!(config)
end

it 'generates correct command when username prefix defined' do
allow(@basic_batch).to receive(:username_prefix).and_return('dev-')
expected_cmd = "/usr/bin/kubectl --kubeconfig=#{ENV['HOME']}/.kube/config config set-context open-ondemand --cluster=open-ondemand --namespace=testuser --user=dev-testuser"
expect(@basic_batch).to receive(:call).with(expected_cmd)
@basic_batch.send(:set_context)
it 'runs the correct gke commands' do
cfg = {
:auth => {
:type => 'gke',
:region => 'ohio',
:svc_acct_file => '~/.gke/acct.key',
},
:cluster => 'gke-cluster-oakley',
:config_file => '~/.gke/oakley.config',
}

set_cluster = "/usr/bin/kubectl --kubeconfig=~/.gke/oakley.config config set-cluster gke-cluster-oakley " \
"--server=https://localhost:8080"
auth_activate = "gcloud auth activate-service-account --key-file=~/.gke/acct.key"
ctr_cluster = "gcloud container clusters get-credentials --region=ohio gke-cluster-oakley"
env = env = { 'KUBECONFIG' => '~/.gke/oakley.config' }

expect_any_instance_of(Batch).to receive(:call).with(set_cluster)
expect_any_instance_of(Batch).to receive(:call).with(auth_activate)
expect_any_instance_of(Batch).to receive(:call).with(ctr_cluster, env: env)
Batch.configure_kube!(cfg)
end

# same as the test above, only using --zone instead of --region
it 'zone works in gke' do
cfg = {
:auth => {
:type => 'gke',
:zone => 'ohio',
:svc_acct_file => '~/.gke/acct.key',
},
:cluster => 'gke-cluster-oakley',
:config_file => '~/.gke/oakley.config',
}

set_cluster = "/usr/bin/kubectl --kubeconfig=~/.gke/oakley.config config set-cluster gke-cluster-oakley " \
"--server=https://localhost:8080"
auth_activate = "gcloud auth activate-service-account --key-file=~/.gke/acct.key"
ctr_cluster = "gcloud container clusters get-credentials --zone=ohio gke-cluster-oakley"
env = env = { 'KUBECONFIG' => '~/.gke/oakley.config' }

expect_any_instance_of(Batch).to receive(:call).with(set_cluster)
expect_any_instance_of(Batch).to receive(:call).with(auth_activate)
expect_any_instance_of(Batch).to receive(:call).with(ctr_cluster, env: env)
Batch.configure_kube!(cfg)
end
end
end