From 39965aaef98ca56c6b94116a13e02042f4d0a005 Mon Sep 17 00:00:00 2001 From: sathish Date: Thu, 24 Oct 2019 14:27:07 -0700 Subject: [PATCH 1/4] Project Specific env in group --- .../environment_variable_groups_controller.rb | 5 +- .../env/app/models/environment_variable.rb | 21 +++++-- ...ronment_variable_groups_controller_test.rb | 55 ++++++++++++++++++- .../test/models/environment_variable_test.rb | 10 ++++ 4 files changed, 84 insertions(+), 7 deletions(-) diff --git a/plugins/env/app/controllers/environment_variable_groups_controller.rb b/plugins/env/app/controllers/environment_variable_groups_controller.rb index 8732bd054e..042a5465c2 100644 --- a/plugins/env/app/controllers/environment_variable_groups_controller.rb +++ b/plugins/env/app/controllers/environment_variable_groups_controller.rb @@ -54,7 +54,10 @@ def preview @project = Project.find(params[:project_id]) end - @groups = SamsonEnv.env_groups(Deploy.new(project: @project), deploy_groups, preview: true) + options = {project_specific: params[:project_specific]} + options.merge!(params[:preview] ? {preview: true, resolve_secrets: true} : {resolve_secrets: false}) + + @groups = SamsonEnv.env_groups(Deploy.new(project: @project), deploy_groups, **options) respond_to do |format| format.html diff --git a/plugins/env/app/models/environment_variable.rb b/plugins/env/app/models/environment_variable.rb index 0bcc6956a9..9ee86752c9 100644 --- a/plugins/env/app/models/environment_variable.rb +++ b/plugins/env/app/models/environment_variable.rb @@ -25,7 +25,7 @@ class << self # preview parameter can be used to not raise an error, # but return a value with a helpful message # also used by an external plugin - def env(deploy, deploy_group, preview: false, resolve_secrets: true) + def env(deploy, deploy_group, preview: false, resolve_secrets: true, project_specific: nil) env = {} if deploy_group && deploy.project.config_service? @@ -36,7 +36,7 @@ def env(deploy, deploy_group, preview: false, resolve_secrets: true) env.merge! env_vars_from_repo(env_repo_name, deploy.project, deploy_group) end - env.merge! env_vars_from_db(deploy, deploy_group) + env.merge! env_vars_from_db(deploy, deploy_group, project_specific: project_specific) resolve_dollar_variables(env) resolve_secrets(deploy.project, deploy_group, env, preview: preview) if resolve_secrets @@ -73,11 +73,24 @@ def config_service_location(project, deploy_group, display:) private - def env_vars_from_db(deploy, deploy_group) + def env_vars_from_db(deploy, deploy_group, project_specific: nil) + # Project Specific: + # nil => project env + groups env + # true => project env + # false => groups env + project_envs = + if project_specific.to_s == "true" + deploy.project.environment_variables + elsif project_specific.to_s == "false" + deploy.project.environment_variable_groups.flat_map(&:environment_variables) + else + deploy.project.nested_environment_variables + end + variables = deploy.environment_variables + (deploy.stage&.environment_variables || []) + - deploy.project.nested_environment_variables + project_envs variables.sort_by!(&:priority) variables.each_with_object({}) do |ev, all| all[ev.name] = ev.value if !all[ev.name] && ev.matches_scope?(deploy_group) diff --git a/plugins/env/test/controllers/environment_variable_groups_controller_test.rb b/plugins/env/test/controllers/environment_variable_groups_controller_test.rb index 22aa95097a..41fa825a5c 100644 --- a/plugins/env/test/controllers/environment_variable_groups_controller_test.rb +++ b/plugins/env/test/controllers/environment_variable_groups_controller_test.rb @@ -46,6 +46,17 @@ def self.it_destroys } ) end + + let!(:other_env_group) do + EnvironmentVariableGroup.create!( + name: "OtherG1", + environment_variables_attributes: { + 0 => {name: "X", value: "Y"}, + 1 => {name: "Y", value: "Z", scope_type_and_id: "DeployGroup-#{deploy_group.id}"} + } + ) + end + let(:other_project) do p = project.dup p.name = 'xxxxx' @@ -128,7 +139,8 @@ def unauthorized_env_group end it "calls env with preview" do - EnvironmentVariable.expects(:env).with(anything, anything, preview: true).times(3) + EnvironmentVariable.expects(:env). + with(anything, anything, project_specific: nil, resolve_secrets: false).times(3) get :preview, params: {group_id: env_group.id} assert_response :success end @@ -136,7 +148,7 @@ def unauthorized_env_group describe "a json GET to #preview" do it "succeeds" do - get :preview, params: {group_id: env_group.id, project_id: project.id}, format: :json + get :preview, params: {group_id: env_group.id, project_id: project.id, preview: true}, format: :json assert_response :success json_response = JSON.parse response.body json_response['groups'].sort.must_equal [ @@ -160,6 +172,45 @@ def unauthorized_env_group get :preview, params: {group_id: env_group.id, project_id: project.id, deploy_group: "pod23"}, format: :json end end + + describe "project_specific" do + before do + EnvironmentVariable.create!(parent: project, name: 'B', value: 'b') + ProjectEnvironmentVariableGroup.create!(environment_variable_group: other_env_group, project: project) + end + it "renders only project env" do + get :preview, params: {project_id: project.id, project_specific: true}, format: :json + assert_response :success + json_response = JSON.parse response.body + json_response['groups'].sort.must_equal [ + [".pod-100", {"B" => "b"}], + [".pod1", {"B" => "b"}], + [".pod2", {"B" => "b"}] + ] + end + + it "renders only groups env" do + get :preview, params: {project_id: project.id, project_specific: false}, format: :json + assert_response :success + json_response = JSON.parse response.body + json_response['groups'].sort.must_equal [ + [".pod-100", {"Y" => "Z", "X" => "Y"}], + [".pod1", {"X" => "Y"}], + [".pod2", {"X" => "Y"}] + ] + end + + it "renders without project_specific" do + get :preview, params: {project_id: project.id, project_specific: nil}, format: :json + assert_response :success + json_response = JSON.parse response.body + json_response['groups'].sort.must_equal [ + [".pod-100", {"B" => "b", "Y" => "Z", "X" => "Y"}], + [".pod1", {"B" => "b", "X" => "Y"}], + [".pod2", {"B" => "b", "X" => "Y"}] + ] + end + end end end diff --git a/plugins/env/test/models/environment_variable_test.rb b/plugins/env/test/models/environment_variable_test.rb index 4d93027965..3bec567137 100644 --- a/plugins/env/test/models/environment_variable_test.rb +++ b/plugins/env/test/models/environment_variable_test.rb @@ -243,6 +243,16 @@ def fake_response(response) ) end + it "includes only project specific environment variables" do + EnvironmentVariable.env(deploy, nil, project_specific: true). + must_equal("PROJECT" => "PROJECT") + end + + it "includes only project groups environment variables" do + EnvironmentVariable.env(deploy, nil, project_specific: false). + must_equal("X" => "Y", "Y" => "Z") + end + describe "secrets" do before do create_secret 'global/global/global/foobar' From e2acc6219593edb8a3267c9e8882d7b0130d613d Mon Sep 17 00:00:00 2001 From: sathish Date: Thu, 24 Oct 2019 14:45:05 -0700 Subject: [PATCH 2/4] Updated loofah because of vulnerability --- Gemfile.lock | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Gemfile.lock b/Gemfile.lock index 5f345f3c7b..cd9a4784fd 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -264,7 +264,7 @@ GEM connection_pool (2.2.1) crack (0.4.3) safe_yaml (~> 1.0.0) - crass (1.0.4) + crass (1.0.5) dalli (2.7.6) ddtrace (0.14.2) msgpack @@ -344,7 +344,7 @@ GEM railties (>= 4) request_store (~> 1.0) logstash-event (1.2.02) - loofah (2.2.3) + loofah (2.3.1) crass (~> 1.0.2) nokogiri (>= 1.5.9) mail (2.7.1) From 45cb6770e3202435d53e2df647298971402f19f1 Mon Sep 17 00:00:00 2001 From: sathish Date: Thu, 24 Oct 2019 15:53:58 -0700 Subject: [PATCH 3/4] Code review changes --- .../environment_variable_groups_controller.rb | 2 +- .../env/app/decorators/project_decorator.rb | 14 +++++++-- .../env/app/models/environment_variable.rb | 17 ++--------- .../test/decorators/project_decorator_test.rb | 29 +++++++++++++++++-- 4 files changed, 41 insertions(+), 21 deletions(-) diff --git a/plugins/env/app/controllers/environment_variable_groups_controller.rb b/plugins/env/app/controllers/environment_variable_groups_controller.rb index 042a5465c2..a947699234 100644 --- a/plugins/env/app/controllers/environment_variable_groups_controller.rb +++ b/plugins/env/app/controllers/environment_variable_groups_controller.rb @@ -55,7 +55,7 @@ def preview end options = {project_specific: params[:project_specific]} - options.merge!(params[:preview] ? {preview: true, resolve_secrets: true} : {resolve_secrets: false}) + options.merge!(params[:preview].to_s == "false" ? {resolve_secrets: false} : {preview: true, resolve_secrets: true}) @groups = SamsonEnv.env_groups(Deploy.new(project: @project), deploy_groups, **options) diff --git a/plugins/env/app/decorators/project_decorator.rb b/plugins/env/app/decorators/project_decorator.rb index c578a9b66d..bcdc7d92ec 100644 --- a/plugins/env/app/decorators/project_decorator.rb +++ b/plugins/env/app/decorators/project_decorator.rb @@ -24,8 +24,18 @@ def serialized_environment_variables EnvironmentVariable.serialize(nested_environment_variables, @env_scopes) end - def nested_environment_variables - [self, *environment_variable_groups].flat_map(&:environment_variables) + def nested_environment_variables(project_specific: nil) + # Project Specific: + # nil => project env + groups env + # true/"true" => project env + # false/"false" => groups env + if project_specific.to_s == "true" + environment_variables + elsif project_specific.to_s == "false" + environment_variable_groups.flat_map(&:environment_variables) + else + [self, *environment_variable_groups].flat_map(&:environment_variables) + end end private diff --git a/plugins/env/app/models/environment_variable.rb b/plugins/env/app/models/environment_variable.rb index 9ee86752c9..b7ad1c0f26 100644 --- a/plugins/env/app/models/environment_variable.rb +++ b/plugins/env/app/models/environment_variable.rb @@ -73,24 +73,11 @@ def config_service_location(project, deploy_group, display:) private - def env_vars_from_db(deploy, deploy_group, project_specific: nil) - # Project Specific: - # nil => project env + groups env - # true => project env - # false => groups env - project_envs = - if project_specific.to_s == "true" - deploy.project.environment_variables - elsif project_specific.to_s == "false" - deploy.project.environment_variable_groups.flat_map(&:environment_variables) - else - deploy.project.nested_environment_variables - end - + def env_vars_from_db(deploy, deploy_group, **args) variables = deploy.environment_variables + (deploy.stage&.environment_variables || []) + - project_envs + deploy.project.nested_environment_variables(**args) variables.sort_by!(&:priority) variables.each_with_object({}) do |ev, all| all[ev.name] = ev.value if !all[ev.name] && ev.matches_scope?(deploy_group) diff --git a/plugins/env/test/decorators/project_decorator_test.rb b/plugins/env/test/decorators/project_decorator_test.rb index dd8299137c..c2b06ed951 100644 --- a/plugins/env/test/decorators/project_decorator_test.rb +++ b/plugins/env/test/decorators/project_decorator_test.rb @@ -5,11 +5,10 @@ describe Project do let(:project) { projects(:test) } + let(:group) { EnvironmentVariableGroup.create!(name: "Foo", environment_variables_attributes: [env_attributes]) } + let(:env_attributes) { {name: "A", value: "B", scope_type_and_id: "Environment-#{environments(:production)}"} } describe "auditing" do - let(:group) { EnvironmentVariableGroup.create!(name: "Foo", environment_variables_attributes: [env_attributes]) } - let(:env_attributes) { {name: "A", value: "B", scope_type_and_id: "Environment-#{environments(:production)}"} } - it "does not audit when var did not change" do project.update_attributes!(name: 'Bar') project.audits.map(&:audited_changes).must_equal([{'name' => ['Foo', 'Bar']}]) @@ -56,4 +55,28 @@ refute project.audits.last end end + + describe "nested_environment_variables" do + let(:group_env) { project.environment_variable_groups.flat_map(&:environment_variables) } + + before do + @project_env = EnvironmentVariable.create!(parent: project, name: 'B', value: 'b') + ProjectEnvironmentVariableGroup.create!(environment_variable_group: group, project: project) + end + + it "includes only project specific environment variables" do + project.nested_environment_variables(project_specific: true). + must_equal [@project_env] + end + + it "includes only project groups environment variables" do + project.nested_environment_variables(project_specific: false). + must_equal group_env + end + + it "includes both project and groups environment variables" do + project.nested_environment_variables. + must_equal group_env.unshift(@project_env) + end + end end From 8cf53efe00b8b69c5ae05908019eb51c4de6950f Mon Sep 17 00:00:00 2001 From: sathish Date: Thu, 24 Oct 2019 16:06:22 -0700 Subject: [PATCH 4/4] Fixed test cases for preview --- Gemfile.lock | 1 + .../environment_variable_groups_controller_test.rb | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/Gemfile.lock b/Gemfile.lock index cd9a4784fd..6d78f00985 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -587,6 +587,7 @@ GEM PLATFORMS ruby + x86_64-darwin-18 DEPENDENCIES active_hash diff --git a/plugins/env/test/controllers/environment_variable_groups_controller_test.rb b/plugins/env/test/controllers/environment_variable_groups_controller_test.rb index 41fa825a5c..fe1cb17818 100644 --- a/plugins/env/test/controllers/environment_variable_groups_controller_test.rb +++ b/plugins/env/test/controllers/environment_variable_groups_controller_test.rb @@ -140,7 +140,7 @@ def unauthorized_env_group it "calls env with preview" do EnvironmentVariable.expects(:env). - with(anything, anything, project_specific: nil, resolve_secrets: false).times(3) + with(anything, anything, project_specific: nil, preview: true, resolve_secrets: true).times(3) get :preview, params: {group_id: env_group.id} assert_response :success end @@ -148,7 +148,7 @@ def unauthorized_env_group describe "a json GET to #preview" do it "succeeds" do - get :preview, params: {group_id: env_group.id, project_id: project.id, preview: true}, format: :json + get :preview, params: {group_id: env_group.id, project_id: project.id, preview: false}, format: :json assert_response :success json_response = JSON.parse response.body json_response['groups'].sort.must_equal [