From 01de647f2ee3ac3b218afa89581126166c7a829d Mon Sep 17 00:00:00 2001 From: treydock Date: Fri, 19 Mar 2021 10:36:27 -0400 Subject: [PATCH] Add some helper environment variable to both main and init container (#252) Add some helper environment variable to both main and init container. This will set the default environment variables USER, UID, HOME, GROUP and GID appropriately from the web host. I.e., HOME is set to the actual users HOME. This also introduces a breaking change in that we now use maps instead of arrays to define environment variables. --- lib/ood_core/job/adapters/kubernetes/batch.rb | 30 ++++++-- .../job/adapters/kubernetes/helper.rb | 15 ++-- .../job/adapters/kubernetes/resources.rb | 5 +- .../adapters/kubernetes/templates/pod.yml.erb | 21 ++++-- .../output/k8s/pod_yml_from_all_configs.yml | 29 ++++++++ .../output/k8s/pod_yml_from_defaults.yml | 29 ++++++++ .../fixtures/output/k8s/pod_yml_no_mounts.yml | 31 ++++++++- spec/job/adapters/kubernetes/batch_spec.rb | 54 +++++---------- spec/job/adapters/kubernetes/helper_spec.rb | 69 +++++++++++++------ 9 files changed, 209 insertions(+), 74 deletions(-) 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