From 9c9a781d840e197957257f3552f6bbcaa73113ef Mon Sep 17 00:00:00 2001 From: James Mead Date: Tue, 22 Aug 2023 17:12:51 +0100 Subject: [PATCH 1/2] Fail fast if GOVUK_ENVIRONMENT_NAME missing in production env In #2319 we fixed an issue which was triggered because this env var was not defined in the build-and-publish-image job in the deploy GH action. This job uses the shared-build-and-push-image job [1] which builds a Docker image [2] using Signon's Dockerfile. This Dockerfile is based on [3] the alphagov/govuk-ruby-base image which sets the RAILS_ENV env var to "production" [4]. The Dockerfile also includes a command [5] to run the assets:precompile task. At this point the Rails app boots up and the code in config/initializers/govuk_admin_template.rb is executed. Before the fix this resulted in a call to String#titleize on GovukEnvironment.name [6] which was nil, due to the combination of RAILS_ENV being set to "production" and GOVUK_ENVIRONMENT_NAME not being set, which raised a NoMethodError exception. While #2319 fixed the specific scenario described above, we realised that there might be other processes where RAILS_ENV is set to "production" and GOVUK_ENVIRONMENT_NAME is not set. So it's conceivable that there are scenarios where GovukEnvironment.name could be nil and the app might misbheave without raising an exception (e.g. in generating email copy) which is actually what was happening before the fix in #2311. It seems better to find out about such issues as soon as possible, so this commit reverts the changes in config/initializers/govuk_admin_template.rb and changes GovukEnvironment.name to fail fast if GOVUK_ENVIRONMENT_NAME is not defined. This also has the advantage that if the env var is missing, the exception & stack trace should make it a lot clearer what the problem is. With this change in place, in order not to fall foul of the problem we fixed in #2319, we need to set the GOVUK_ENVIRONMENT_NAME env var in the Dockerfile which is used in the build-and-publish-image job mentioned above. The actual value of GOVUK_ENVIRONMENT_NAME is not important here; it's just important that it is set to something so the Rails app can boot up successfully and be used to precompile the assets. The value of GovukAdminTemplate.environment_label which is set in config/initializers/govuk_admin_template.rb is only used to set the text of the main heading in the govuk_admin_template layout which is irrelevant in the precompiling of assets. So we've followed the pattern established by DEVISE_PEPPER & DEVISE_SECRET_KEY [7] and set GOVUK_ENVIRONMENT_NAME to "unused". Note that I've chosen to use ClimateControl to set the value of GOVUK_ENVIRONMENT_NAME in tests, because it provides a more realistic ENV in the test and makes the test less implementation-specific. We should probably be using ClimateControl in all tests instead of stubbing methods on ENV as per these recommendations [8], but I plan to tackle that separately. [1]: https://github.com/alphagov/govuk-infrastructure/blob/6f63a5722279252a8a24e66a09607e7900ffe0c0/.github/workflows/build-and-push-image.yml#L54 [2]: https://github.com/alphagov/govuk-infrastructure/blob/6f63a5722279252a8a24e66a09607e7900ffe0c0/.github/workflows/build-and-push-image.yml#L140 [3]: https://github.com/alphagov/signon/blob/746b473e7470dd4e969b38883d9233a0602c53a7/Dockerfile#L2 [4]: https://github.com/alphagov/govuk-ruby-images/blob/f15874ab79fbdaa53cd5d4dd2fd92086efe0d5a6/base.Dockerfile#L99 [5]: https://github.com/alphagov/signon/blob/746b473e7470dd4e969b38883d9233a0602c53a7/Dockerfile#L16 [6]: https://github.com/alphagov/signon/blob/9bc8214b1b34297fd344300e97c48570fe1cc6b7/config/initializers/govuk_admin_template.rb#L11 [7]: https://github.com/alphagov/signon/blob/746b473e7470dd4e969b38883d9233a0602c53a7/Dockerfile#L8-L9 [8]: https://docs.publishing.service.gov.uk/manual/conventions-for-rails-applications.html#testing-utilities --- Dockerfile | 3 ++- app/models/govuk_environment.rb | 2 +- config/initializers/govuk_admin_template.rb | 2 +- test/models/govuk_environment_test.rb | 13 ++++++++++--- 4 files changed, 14 insertions(+), 6 deletions(-) diff --git a/Dockerfile b/Dockerfile index fe7fc7548..e8247182d 100644 --- a/Dockerfile +++ b/Dockerfile @@ -6,7 +6,8 @@ ARG builder_image=ghcr.io/alphagov/govuk-ruby-builder:$ruby_version FROM $builder_image AS builder ENV DEVISE_PEPPER=unused \ - DEVISE_SECRET_KEY=unused + DEVISE_SECRET_KEY=unused \ + GOVUK_ENVIRONMENT_NAME=unused WORKDIR $APP_HOME COPY Gemfile* .ruby-version ./ diff --git a/app/models/govuk_environment.rb b/app/models/govuk_environment.rb index 11ec64bf8..4f7298e38 100644 --- a/app/models/govuk_environment.rb +++ b/app/models/govuk_environment.rb @@ -3,7 +3,7 @@ def self.name if Rails.env.development? || Rails.env.test? "development" else - ENV["GOVUK_ENVIRONMENT_NAME"] + ENV.fetch("GOVUK_ENVIRONMENT_NAME") end end diff --git a/config/initializers/govuk_admin_template.rb b/config/initializers/govuk_admin_template.rb index 8bd10ed96..cdb9fc963 100644 --- a/config/initializers/govuk_admin_template.rb +++ b/config/initializers/govuk_admin_template.rb @@ -8,5 +8,5 @@ c.disable_google_analytics = false end -GovukAdminTemplate.environment_label = (GovukEnvironment.name || "development").titleize +GovukAdminTemplate.environment_label = GovukEnvironment.name.titleize GovukAdminTemplate.environment_style = GovukEnvironment.production? ? "production" : "preview" diff --git a/test/models/govuk_environment_test.rb b/test/models/govuk_environment_test.rb index 1dbbb9c79..50cec20fe 100644 --- a/test/models/govuk_environment_test.rb +++ b/test/models/govuk_environment_test.rb @@ -28,11 +28,18 @@ class GovukEnvironmentTest < ActionMailer::TestCase setup do Rails.env.stubs(:development?).returns(false) Rails.env.stubs(:test?).returns(false) - ENV.stubs(:[]).with("GOVUK_ENVIRONMENT_NAME").returns("govuk-environment-name") end - should "return value of GOVUK_ENVIRONMENT_NAME env var" do - assert_equal "govuk-environment-name", GovukEnvironment.name + should "return value of GOVUK_ENVIRONMENT_NAME if it is set" do + ClimateControl.modify(GOVUK_ENVIRONMENT_NAME: "govuk-environment-name") do + assert_equal "govuk-environment-name", GovukEnvironment.name + end + end + + should "fail fast if GOVUK_ENVIRONMENT_NAME is not set" do + ClimateControl.modify(GOVUK_ENVIRONMENT_NAME: nil) do + assert_raises(KeyError) { GovukEnvironment.name } + end end end end From ee23540902a3750f743bc1379f851047ad3b08c4 Mon Sep 17 00:00:00 2001 From: James Mead Date: Wed, 23 Aug 2023 11:04:25 +0100 Subject: [PATCH 2/2] Remove documentation re INSTANCE_NAME env var This should have been included in https://github.com/alphagov/signon/pull/2315 when we stopped using INSTANCE_NAME anywhere in the app. --- docs/environment-variables.md | 4 ---- 1 file changed, 4 deletions(-) diff --git a/docs/environment-variables.md b/docs/environment-variables.md index acb315e96..b9f0b8f1b 100644 --- a/docs/environment-variables.md +++ b/docs/environment-variables.md @@ -40,10 +40,6 @@ Used to configure `GovukAdminTemplate` and in `Healthcheck::ApiTokens#expiring_t * `GOVUK_ENVIRONMENT_NAME` -Used in various bits of logic to detect the production instance of the app and used to display various environment-specific strings. - -* `INSTANCE_NAME` - ## Splunk Used to stream event logs to Splunk for analysis of Signon usage patterns and anomalies.