diff --git a/lib/ood_core/job/adapters/kubernetes/batch.rb b/lib/ood_core/job/adapters/kubernetes/batch.rb index 4ba4e868c..94ecd6c3c 100644 --- a/lib/ood_core/job/adapters/kubernetes/batch.rb +++ b/lib/ood_core/job/adapters/kubernetes/batch.rb @@ -153,26 +153,48 @@ def k8s_username username_prefix.nil? ? username : "#{username_prefix}-#{username}" end + def user + @user ||= Etc.getpwnam(username) + end + + def home_dir + user.dir + end + def run_as_user - Etc.getpwnam(username).uid + user.uid end def run_as_group - Etc.getpwnam(username).gid + user.gid end def fs_group run_as_group end + def group + Etc.getgrgid(run_as_group).name + end + + def default_env + { + USER: username, + UID: run_as_user, + HOME: home_dir, + GROUP: group, + GID: run_as_group, + } + end + # helper to template resource yml you're going to submit and # create an id. def generate_id_yml(script) native_data = script.native - container = helper.container_from_native(native_data[:container]) + container = helper.container_from_native(native_data[:container], default_env) id = generate_id(container.name) configmap = helper.configmap_from_native(native_data, id) - init_containers = helper.init_ctrs_from_native(native_data[:init_containers]) + init_containers = helper.init_ctrs_from_native(native_data[:init_containers], container.env) spec = OodCore::Job::Adapters::Kubernetes::Resources::PodSpec.new(container, init_containers: init_containers) all_mounts = native_data[:mounts].nil? ? mounts : mounts + native_data[:mounts] diff --git a/lib/ood_core/job/adapters/kubernetes/helper.rb b/lib/ood_core/job/adapters/kubernetes/helper.rb index e7fd75080..c54e18271 100644 --- a/lib/ood_core/job/adapters/kubernetes/helper.rb +++ b/lib/ood_core/job/adapters/kubernetes/helper.rb @@ -38,18 +38,21 @@ def info_from_json(pod_json: nil, service_json: nil, secret_json: nil, ns_prefix # # @param container [#to_h] # the input container hash + # @param default_env [#to_h] + # Default env to merge with defined env # @return [OodCore::Job::Adapters::Kubernetes::Resources::Container] - def container_from_native(container) + def container_from_native(container, default_env) + env = container.fetch(:env, {}).to_h.symbolize_keys OodCore::Job::Adapters::Kubernetes::Resources::Container.new( container[:name], container[:image], command: parse_command(container[:command]), port: container[:port], - env: container.fetch(:env, []), + env: default_env.merge(env), memory: container[:memory], cpu: container[:cpu], working_dir: container[:working_dir], - restart_policy: container[:restart_policy] + restart_policy: container[:restart_policy], ) end @@ -93,13 +96,15 @@ def configmap_from_native(native, id) # @param native_data [#to_h] # the native data to parse. Expected key init_ctrs and for that # key to be an array of hashes. + # @param default_env [#to_h] + # Default env to merge with defined env # @return [Array] # the array of init containers - def init_ctrs_from_native(ctrs) + def init_ctrs_from_native(ctrs, default_env) init_ctrs = [] ctrs&.each do |ctr_raw| - ctr = container_from_native(ctr_raw) + ctr = container_from_native(ctr_raw, default_env) init_ctrs.push(ctr) end diff --git a/lib/ood_core/job/adapters/kubernetes/resources.rb b/lib/ood_core/job/adapters/kubernetes/resources.rb index 84f3aebdf..055b67540 100644 --- a/lib/ood_core/job/adapters/kubernetes/resources.rb +++ b/lib/ood_core/job/adapters/kubernetes/resources.rb @@ -15,7 +15,7 @@ class Container :restart_policy, :supplemental_groups def initialize( - name, image, command: [], port: nil, env: [], memory: "4Gi", cpu: "1", + name, image, command: [], port: nil, env: {}, memory: "4Gi", cpu: "1", working_dir: "", restart_policy: "Never", supplemental_groups: [] ) raise ArgumentError, "containers need valid names and images" unless name && image @@ -24,7 +24,7 @@ def initialize( @image = image @command = command.nil? ? [] : command @port = port&.to_i - @env = env.nil? ? [] : env + @env = env.nil? ? {} : env @memory = memory.nil? ? "4Gi" : memory @cpu = cpu.nil? ? "1" : cpu @working_dir = working_dir.nil? ? "" : working_dir @@ -44,7 +44,6 @@ def ==(other) restart_policy == other.restart_policy && supplemental_groups == other.supplemental_groups end - end class PodSpec diff --git a/lib/ood_core/job/adapters/kubernetes/templates/pod.yml.erb b/lib/ood_core/job/adapters/kubernetes/templates/pod.yml.erb index 470f594eb..beebd24ec 100644 --- a/lib/ood_core/job/adapters/kubernetes/templates/pod.yml.erb +++ b/lib/ood_core/job/adapters/kubernetes/templates/pod.yml.erb @@ -39,13 +39,15 @@ spec: <%- unless spec.container.working_dir.empty? -%> workingDir: "<%= spec.container.working_dir %>" <%- end -%> - <%- unless spec.container.env.empty? -%> env: - <%- spec.container.env.each do |env| -%> - - name: <%= env[:name] %> - value: "<%= env[:value] %>" + - name: POD_NAME + valueFrom: + fieldRef: + fieldPath: metadata.name + <%- spec.container.env.each_pair do |name, value| -%> + - name: <%= name %> + value: "<%= value %>" <%- end # for each env -%> - <%- end # unless env is nil -%> <%- unless spec.container.command.empty? -%> command: <%- spec.container.command.each do |cmd| -%> @@ -83,6 +85,15 @@ spec: <%- spec.init_containers.each do |ctr| -%> - name: "<%= ctr.name %>" image: "<%= ctr.image %>" + env: + - name: POD_NAME + valueFrom: + fieldRef: + fieldPath: metadata.name + <%- ctr.env.each_pair do |name, value| -%> + - name: <%= name %> + value: "<%= value %>" + <%- end # for each env -%> command: <%- ctr.command.each do |cmd| -%> - "<%= cmd %>" diff --git a/spec/fixtures/output/k8s/pod_yml_from_all_configs.yml b/spec/fixtures/output/k8s/pod_yml_from_all_configs.yml index 02e996e9e..19bb4f651 100644 --- a/spec/fixtures/output/k8s/pod_yml_from_all_configs.yml +++ b/spec/fixtures/output/k8s/pod_yml_from_all_configs.yml @@ -26,8 +26,20 @@ spec: imagePullPolicy: IfNotPresent workingDir: "/my/home" env: + - name: POD_NAME + valueFrom: + fieldRef: + fieldPath: metadata.name + - name: USER + value: "testuser" + - name: UID + value: "1001" - name: HOME value: "/my/home" + - name: GROUP + value: "testgroup" + - name: GID + value: "1002" - name: PATH value: "/usr/bin:/usr/local/bin" command: @@ -60,6 +72,23 @@ spec: initContainers: - name: "init-1" image: "busybox:latest" + env: + - name: POD_NAME + valueFrom: + fieldRef: + fieldPath: metadata.name + - name: USER + value: "testuser" + - name: UID + value: "1001" + - name: HOME + value: "/my/home" + - name: GROUP + value: "testgroup" + - name: GID + value: "1002" + - name: PATH + value: "/usr/bin:/usr/local/bin" command: - "/bin/ls" - "-lrt" diff --git a/spec/fixtures/output/k8s/pod_yml_from_defaults.yml b/spec/fixtures/output/k8s/pod_yml_from_defaults.yml index edb3a0517..1538f6c58 100644 --- a/spec/fixtures/output/k8s/pod_yml_from_defaults.yml +++ b/spec/fixtures/output/k8s/pod_yml_from_defaults.yml @@ -26,8 +26,20 @@ spec: imagePullPolicy: IfNotPresent workingDir: "/my/home" env: + - name: POD_NAME + valueFrom: + fieldRef: + fieldPath: metadata.name + - name: USER + value: "testuser" + - name: UID + value: "1001" - name: HOME value: "/my/home" + - name: GROUP + value: "testgroup" + - name: GID + value: "1002" - name: PATH value: "/usr/bin:/usr/local/bin" command: @@ -56,6 +68,23 @@ spec: initContainers: - name: "init-1" image: "busybox:latest" + env: + - name: POD_NAME + valueFrom: + fieldRef: + fieldPath: metadata.name + - name: USER + value: "testuser" + - name: UID + value: "1001" + - name: HOME + value: "/my/home" + - name: GROUP + value: "testgroup" + - name: GID + value: "1002" + - name: PATH + value: "/usr/bin:/usr/local/bin" command: - "/bin/ls" - "-lrt" diff --git a/spec/fixtures/output/k8s/pod_yml_no_mounts.yml b/spec/fixtures/output/k8s/pod_yml_no_mounts.yml index aa70dae23..7d6c44c71 100644 --- a/spec/fixtures/output/k8s/pod_yml_no_mounts.yml +++ b/spec/fixtures/output/k8s/pod_yml_no_mounts.yml @@ -26,8 +26,20 @@ spec: imagePullPolicy: IfNotPresent workingDir: "/my/home" env: + - name: POD_NAME + valueFrom: + fieldRef: + fieldPath: metadata.name + - name: USER + value: "testuser" + - name: UID + value: "1001" - name: HOME - value: "/my/home" + value: "/home/testuser" + - name: GROUP + value: "testgroup" + - name: GID + value: "1002" - name: PATH value: "/usr/bin:/usr/local/bin" command: @@ -54,6 +66,23 @@ spec: initContainers: - name: "init-1" image: "busybox:latest" + env: + - name: POD_NAME + valueFrom: + fieldRef: + fieldPath: metadata.name + - name: USER + value: "testuser" + - name: UID + value: "1001" + - name: HOME + value: "/home/testuser" + - name: GROUP + value: "testgroup" + - name: GID + value: "1002" + - name: PATH + value: "/usr/bin:/usr/local/bin" command: - "/bin/ls" - "-lrt" diff --git a/spec/job/adapters/kubernetes/batch_spec.rb b/spec/job/adapters/kubernetes/batch_spec.rb index ef197557b..a58501ed5 100644 --- a/spec/job/adapters/kubernetes/batch_spec.rb +++ b/spec/job/adapters/kubernetes/batch_spec.rb @@ -6,6 +6,7 @@ Batch = OodCore::Job::Adapters::Kubernetes::Batch Helper = OodCore::Job::Adapters::Kubernetes::Helper K8sJobInfo = OodCore::Job::Adapters::Kubernetes::K8sJobInfo + User = Struct.new(:dir, :uid, :gid, keyword_init: true) let(:helper) { helper = Helper.new @@ -275,16 +276,10 @@ def build_script(opts = {}) image: 'ruby:2.5', command: 'rake spec', port: 8080, - env: [ - { - name: 'HOME', - value: '/my/home' - }, - { - name: 'PATH', - value: '/usr/bin:/usr/local/bin' - } - ], + env: { + HOME: '/my/home', + PATH: '/usr/bin:/usr/local/bin' + }, memory: '6Gi', cpu: '4', working_dir: '/my/home', @@ -311,8 +306,8 @@ def build_script(opts = {}) allow(configured_batch).to receive(:generate_id).with('rspec-test').and_return('rspec-test-123') allow(configured_batch).to receive(:username).and_return('testuser') - allow(configured_batch).to receive(:run_as_user).and_return(1001) - allow(configured_batch).to receive(:run_as_group).and_return(1002) + allow(configured_batch).to receive(:user).and_return(User.new(dir: '/home/testuser', uid: 1001, gid: 1002)) + allow(configured_batch).to receive(:group).and_return('testgroup') # make sure it get's templated right, also helpful in debugging bc # it'll show a better diff than the test below. @@ -339,16 +334,10 @@ def build_script(opts = {}) image: 'ruby:2.5', command: 'rake spec', port: 8080, - env: [ - { - name: 'HOME', - value: '/my/home' - }, - { - name: 'PATH', - value: '/usr/bin:/usr/local/bin' - } - ], + env: { + 'HOME' => '/my/home', + 'PATH' => '/usr/bin:/usr/local/bin' + }, memory: '6Gi', cpu: '4', working_dir: '/my/home', @@ -375,8 +364,8 @@ def build_script(opts = {}) allow(@basic_batch).to receive(:generate_id).with('rspec-test').and_return('rspec-test-123') allow(@basic_batch).to receive(:username).and_return('testuser') - allow(@basic_batch).to receive(:run_as_user).and_return(1001) - allow(@basic_batch).to receive(:run_as_group).and_return(1002) + allow(@basic_batch).to receive(:user).and_return(User.new(dir: '/home/testuser', uid: 1001, gid: 1002)) + allow(@basic_batch).to receive(:group).and_return('testgroup') # make sure it get's templated right, also helpful in debugging bc # it'll show a better diff than the test below. @@ -403,16 +392,9 @@ def build_script(opts = {}) image: 'ruby:2.5', command: 'rake spec', port: 8080, - env: [ - { - name: 'HOME', - value: '/my/home' - }, - { - name: 'PATH', - value: '/usr/bin:/usr/local/bin' - } - ], + env: { + PATH: '/usr/bin:/usr/local/bin' + }, memory: '6Gi', cpu: '4', working_dir: '/my/home', @@ -432,8 +414,8 @@ def build_script(opts = {}) allow(@basic_batch).to receive(:generate_id).with('rspec-test').and_return('rspec-test-123') allow(@basic_batch).to receive(:username).and_return('testuser') - allow(@basic_batch).to receive(:run_as_user).and_return(1001) - allow(@basic_batch).to receive(:run_as_group).and_return(1002) + allow(@basic_batch).to receive(:user).and_return(User.new(dir: '/home/testuser', uid: 1001, gid: 1002)) + allow(@basic_batch).to receive(:group).and_return('testgroup') # make sure it get's templated right, also helpful in debugging bc # it'll show a better diff than the test below. diff --git a/spec/job/adapters/kubernetes/helper_spec.rb b/spec/job/adapters/kubernetes/helper_spec.rb index e65f291f1..b3e80050f 100644 --- a/spec/job/adapters/kubernetes/helper_spec.rb +++ b/spec/job/adapters/kubernetes/helper_spec.rb @@ -234,10 +234,9 @@ image: 'ruby:2.5', command: 'rake spec', port: 8080, - env: [ - name: 'HOME', - value: '/over/here' - ], + env: { + 'HOME' => '/over/here', + }, memory: '12Gi', cpu: '6', working_dir: '/over/there', @@ -245,14 +244,24 @@ } } + let(:default_env) { + { + HOME: '/home/test', + UID: 1000, + } + } + it "correctly parses a full container" do - expect(helper.container_from_native(ctr_hash)).to eq( + expect(helper.container_from_native(ctr_hash, default_env)).to eq( Kubernetes::Resources::Container.new( 'ruby-test-container', 'ruby:2.5', port: 8080, command: ['rake', 'spec'], - env: [{ name: 'HOME', value: '/over/here' }], + env: { + HOME: '/over/here', + UID: 1000, + }, memory: '12Gi', cpu: '6', working_dir: '/over/there', @@ -264,12 +273,15 @@ it "correctly parses container with no port" do ctr_hash.delete(:port) - expect(helper.container_from_native(ctr_hash)).to eq( + expect(helper.container_from_native(ctr_hash, default_env)).to eq( Kubernetes::Resources::Container.new( 'ruby-test-container', 'ruby:2.5', command: ['rake', 'spec'], - env: [{ name: 'HOME', value: '/over/here' }], + env: { + HOME: '/over/here', + UID: 1000, + }, memory: '12Gi', cpu: '6', working_dir: '/over/there', @@ -281,12 +293,15 @@ it "correctly parses container with no command" do ctr_hash.delete(:command) - expect(helper.container_from_native(ctr_hash)).to eq( + expect(helper.container_from_native(ctr_hash, default_env)).to eq( Kubernetes::Resources::Container.new( 'ruby-test-container', 'ruby:2.5', port: 8080, - env: [{ name: 'HOME', value: '/over/here' }], + env: { + HOME: '/over/here', + UID: 1000, + }, memory: '12Gi', cpu: '6', working_dir: '/over/there', @@ -298,11 +313,15 @@ it "correctly parses container with no env" do ctr_hash.delete(:env) - expect(helper.container_from_native(ctr_hash)).to eq( + expect(helper.container_from_native(ctr_hash, default_env)).to eq( Kubernetes::Resources::Container.new( 'ruby-test-container', 'ruby:2.5', port: 8080, + env: { + HOME: '/home/test', + UID: 1000, + }, command: ['rake', 'spec'], memory: '12Gi', cpu: '6', @@ -315,12 +334,15 @@ it "correctly parses container with working directory" do ctr_hash.delete(:working_dir) - expect(helper.container_from_native(ctr_hash)).to eq( + expect(helper.container_from_native(ctr_hash, default_env)).to eq( Kubernetes::Resources::Container.new( 'ruby-test-container', 'ruby:2.5', port: 8080, - env: [{ name: 'HOME', value: '/over/here' }], + env: { + HOME: '/over/here', + UID: 1000, + }, command: ['rake', 'spec'], memory: '12Gi', cpu: '6', @@ -332,13 +354,16 @@ it "correctly parses container with no restart_policy" do ctr_hash.delete(:restart_policy) - expect(helper.container_from_native(ctr_hash)).to eq( + expect(helper.container_from_native(ctr_hash, default_env)).to eq( Kubernetes::Resources::Container.new( 'ruby-test-container', 'ruby:2.5', port: 8080, command: ['rake', 'spec'], - env: [{ name: 'HOME', value: '/over/here' }], + env: { + HOME: '/over/here', + UID: 1000, + }, memory: '12Gi', cpu: '6', working_dir: '/over/there', @@ -348,7 +373,7 @@ it "correctly parses container with no extra fields" do # expected defaults - ctr_hash[:env] = [] + ctr_hash[:env] = {} ctr_hash[:command] = [] ctr_hash.delete(:port) ctr_hash[:memory] = '4Gi' @@ -356,10 +381,14 @@ ctr_hash[:restart_policy] = 'Never' ctr_hash[:working_dir] = '' - expect(helper.container_from_native(ctr_hash)).to eq( + expect(helper.container_from_native(ctr_hash, default_env)).to eq( Kubernetes::Resources::Container.new( 'ruby-test-container', - 'ruby:2.5' + 'ruby:2.5', + env: { + HOME: '/home/test', + UID: 1000, + } ) ) end @@ -367,14 +396,14 @@ it "throws an error when no name is given" do ctr = { image: 'ruby:25' } expect{ - helper.container_from_native(ctr) + helper.container_from_native(ctr, default_env) }.to raise_error(ArgumentError, "containers need valid names and images") end it "throws an error when no name is given" do ctr = { name: 'ruby-test-container' } expect{ - helper.container_from_native(ctr) + helper.container_from_native(ctr, default_env) }.to raise_error(ArgumentError, "containers need valid names and images") end end