THREESCALE-10924: Remove duplicated config .yml files, keep only those in config/ #3766

mayorova · 2024-04-25T14:14:02Z

What this PR does / why we need it:

We currently have multiple places where .yml configuration files are set:

config - the default folder
config/examples - these are not added to the container image, but for development they need to be copied to config
openshift/system/config - these are copied to the container image by Dockerfile commands.

This causes some inconveniences:

whenever the config changes, we need to update all the files
we sometimes need to remember to copy the updated files from example to local versions again

This can be avoided by having a single set of configs. Normally, they would not collide, because we use development and test for development, and production typically is only used in the container image. And even if we wanted to run production locally, the values are normally configured through env variables, so it's the matter of adjusting your .env file.

Which issue(s) this PR fixes

https://issues.redhat.com/browse/THREESCALE-10924

Verification steps

See these comments:
#3766 (comment)
#3766 (comment)

Special notes for your reviewer:

mayorova · 2024-04-25T14:17:37Z

config/application.rb

@@ -253,7 +253,7 @@ def cache_store_config
    email_sanitizer_configs = (three_scale.delete(:email_sanitizer) || {}).symbolize_keys
    config.three_scale.email_sanitizer.merge!(email_sanitizer_configs)

-    config.three_scale.merge!(three_scale.slice!(:force_ssl, :access_code))
+    config.three_scale.merge!(three_scale.slice!(:force_ssl))


I think there was actually a bug here. This access_code is retrieved here:

porta/lib/developer_portal/lib/access_code_protection.rb

Line 42 in 3f78b0f

access_code.presence || ThreeScale.config.access_code

But with this code, it was resulting in nil. The correct way to get the value would be Rails.application.config.access_code. But I thought it would be better to place it under three_scale specific config.

mayorova · 2024-04-25T14:23:30Z

config/zync.yml

+  <<: *default
+  endpoint:
+
+production:


This file is overwritten by 3scale operator: https://github.com/3scale/3scale-operator/blob/87c3f3b9d18b2fb625d05fa6c2cbb60b3c397d0c/pkg/3scale/amp/component/system.go#L1227-L1235

Need to figure out:

do we need it? can't we use the built-in zync.yml file?

if we have one already in config, would operator override it?

do we need it? can't we use the built-in zync.yml file?

I updated the file in b8fdb59 a bit, to apply the timeouts that are set in the operator. We can add ZYNC_ENDPOINT to the operator and use the default file instead. Possibly the idea of having the zync.yml file set in operator is that this way it is available as a ConfigMap, and can be easily changed. On the other hand, I don't think that this file is supposed to be updated easily.

if we have one already in config, would operator override it?

The extra files defined by the operator are mounted at /opt/system-extra-configs, which are then symlinked to config directory via https://github.com/3scale/porta/blob/cleanup-config-files-remove-dups/openshift/system/entrypoint.sh#L17

As we use ln -sf there shouldn't be a problem with overriding the existing file with the new content (set by operator).

mayorova · 2024-04-25T14:43:03Z

openshift/system/config/prometheus.yml

@@ -1,3 +0,0 @@
-production:


This file is not used. It was initially added in 809e5de, but then its loading was removed in 79d15ba

mayorova · 2024-04-26T14:54:22Z

Differences between the current and the target configs, caused by the refactoring of the config files (this is with the assumption that .env file is in a specific way):

development:

web_hooks.sanitized_url - changed from "http://localhost/ping" to "http://127.0.0.1/"
three_scale.access_code added (was not existing before, due to a bug, explained here)
three_scale.core.fake_server: false added (was not present before, the behavior is the same)
three_scale.currencies - added a hash with a list of values, was empty hash before (2 default currencies were used in this case) - I think there is no reason to NOT have the full currencies list in development
three_scale.noreply_email - changed from "no-reply@example.net" to "no-reply@example.com"
three_scale.notification_email - changed from "3scale Notification <no-reply@example.com>" to "example.com Notification <no-reply@example.com>"
three_scale.segment.write_key - changed from "null" to true - we don't really use it in development, and as it is disabled anyway, the value doesn't matter.
three_scale.service_discovery.client_id - was "3scale", now is nil, shouldn't matter as it is disabled.
smtp_settings - usd to be empty, now they are complete.
database - pool changed from 25 to 5

Files to compare:
devmaster.txt
devcleaned.txt

production (with configs copied from openshift/system/config:

three_scale.access_code added (was not existing before, due to a bug, explained here)
three_scale.bugsnag_api_key and three_scale.bugsnag_release_stage added. The API key is nil if env var is not set, so the behavior is the same (i.e. Bugsnag not enabled)
three_scale.daily_weekly_reports_pref has value false, previously was missing (because is not in the settings.yml file in openshift. The outcome is the same.
three_scale.error_reporting_stages - was empty, now is set to ["production"]
rolling_updates.billable_contracts and rolling_updates.provider_sso were missing, and now are set to nil, so the behavior is the same.
zync config was missing (as it's overwritten by operator), and now it is set to some localhost default values. In theory it should be rewritten by the operator still - but need to check.
domain_substitution -was empty, now has config enabled: false

Files to compare:
prodmaster.txt
prodcleaned.txt

config/web_hooks.yml

mayorova · 2024-04-29T12:22:25Z

Script that I used to print the configs:

#!/usr/bin/ruby

config = {}

%i(web_hooks three_scale zync domain_substitution paperclip_defaults backend_client redis s3).each do |key|
  config[key] = Rails.configuration.send(key).sort.to_h
end

config[:cache_store] = Rails.configuration.cache_store

config[:smtp_settings] = Rails.configuration.action_mailer.smtp_settings

config[:zync] = {}
%i(endpoint authentication connect_timeout send_timeout receive_timeout skip_non_oidc_applications).each do |key|
  config[:zync][key] = Rails.configuration.zync.send(key)
end

config[:database] = ActiveRecord::Base.configurations[Rails.env]

pp config; nil

Example commands:

bundle exec rails runner /path/to/script.rb > devmaster.txt
RAILS_ENV=production bundle exec rails runner /path/to/script.rb > prodmaster.txt

mayorova · 2024-04-29T12:24:41Z

config/core.yml

  username: <%= ENV.fetch('CONFIG_INTERNAL_API_USER', nil) %>
  password: <%= ENV.fetch('CONFIG_INTERNAL_API_PASSWORD', nil) %>

 development:
  <<: *default
+  # fake server is useful when you don't run a real backend apisonator
+  # fake_server: 'http://localhost:3000/internal/'


OK, this might be a potential place that a developer might want to change locally. We can add an env var for this.

mayorova · 2024-04-29T12:28:56Z

config/database.yml

@@ -0,0 +1,42 @@
+base: &default
+<% case ENV['DATABASE_URL'].to_s


So, this might be controversial...
This changed file does the ENV['DATABASE_URL'].to_s 3 times 😬 (well, I guess at the execution time it will be 2.
But to be honest, for me it's much easier to understand this one than the original one: https://github.com/3scale/porta/blob/539780953ebc6eddebcc1450a842e9d3198ea571/config/examples/database.yml

But maybe I'm wrong, and the original is actually better. Opinions welcome!

I changed my mind 🙃

I will revert the format of this file to the original one.

test/unit/three_scale/middleware/cors_test.rb

features/old/cms/email_templates.feature

config/web_hooks.yml

jlledom · 2024-05-07T08:13:53Z

config/segment.yml

@@ -1,7 +1,7 @@
 base: &default
  enabled: false
  stub: false
-  write_key: 'null'
+  write_key: true


What is this for?

This is for Segment analytics

porta/app/views/provider/_analytics.html.erb

Line 7 in 99b24fb

analytics.load("<%= segment.write_key %>");

We use it in SaaS, on-premises it's disabled. This true value was in openshift configs, but the value actually doesn't matter, cause as long as it's disabled, it won't be used.

We can actually add env vars here, so we can later reuse this config in SaaS. But not sure whether to leave it for another PR.

If it's disabled by default in all environments, how they enable it in SaaS? I guess they need to create a configmap for this file and mount it. I think it would be better to add an env variable to enable it, and leave it disabled by default. Anyway, better to do it in another PR I think.

config/oauth2.yml

akostadinov · 2024-05-08T14:22:48Z

config/segment.yml

-  enabled: false
-  stub: false
-  write_key: 'null'
+  enabled: <%= ENV['SEGMENT_WRITE_KEY'].present? %>


mayorova · 2024-06-03T12:34:10Z

config/database.yml

@@ -3,7 +3,8 @@ when /^oracle/ %>
 base: &default
  adapter: oracle_enhanced
  url: <%= ENV['DATABASE_URL'] %>
-  pool: <%= ENV.fetch('RAILS_MAX_THREADS', 25) %>
+  encoding: utf8


production config uses these settings, and I don't see any reason not to use them in test/dev:

porta/openshift/system/config/database.yml

Lines 17 to 18 in 6d213ec

encoding: utf8

pool: <%= ENV.fetch('RAILS_MAX_THREADS', 5) %>

mayorova · 2024-06-03T12:34:35Z

config/database.yml


 <% when /^postgresql/ %>
 base: &default
  adapter: postgresql
  url: <%= ENV['DATABASE_URL'] %>
-  pool: <%= ENV.fetch('RAILS_MAX_THREADS', 25) %>
-  encoding: unicode
+  encoding: utf8


sames as https://github.com/3scale/porta/pull/3766/files#r1624383012

mayorova · 2024-06-03T12:34:47Z

config/database.yml

  encoding: utf8
+  pool: <%= ENV.fetch('RAILS_MAX_THREADS', 5) %>


same as https://github.com/3scale/porta/pull/3766/files#r1624383012

Shouldn't this be utf8mb3 or utf8mb3_bin like in schema.rb? Just asking, I don't really know the difference between encoding, charset and collation.

porta/db/schema.rb

Line 15 in 43dcd03

create_table "access_tokens", charset: "utf8mb3", collation: "utf8mb3_bin", force: :cascade do |t|

mayorova · 2024-06-03T12:36:59Z

test/unit/three_scale/middleware/cors_test.rb

+    cors_config = YAML.load_file(Rails.root.join("config/cors.yml")).deep_symbolize_keys
+    rails_envs = %i[development test production]
+    rails_envs.each do |rails_env|
+      stub_config = cors_config[rails_env]


Removed enabled: true as it doesn't make difference.

The base branch was changed.

codecov · 2024-06-04T11:36:34Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.22%. Comparing base (ffc3d01) to head (c0ef05e).
Report is 31 commits behind head on master.

❗ Current head c0ef05e differs from pull request most recent head b8fdb59

Please upload reports for the commit b8fdb59 to get more accurate results.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3766      +/-   ##
==========================================
- Coverage   94.29%   94.22%   -0.08%     
==========================================
  Files        2774     2771       -3     
  Lines       92333    91291    -1042     
==========================================
- Hits        87068    86017    -1051     
- Misses       5265     5274       +9

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

akostadinov · 2024-06-05T06:14:39Z

.circleci/config.yml

-use-example-config-files: &use-example-config-files
-  run:
-    name: Copy example config files into place to be used by tests
-    command: cp config/examples/*.yml config/


Wait, why do we remove this?

Because there's no config/examples anymore, this PR removes it and moves its files to config.

@akostadinov @jlledom if you prefer, I can move them back to config/examples, so the only thing the PR will do is get rid of the duplicates in openshift/system/config by unifying the files

I prefer returning the examples until we have something to deal with setting credentials in a relatively secure way.

akostadinov · 2024-06-05T06:15:47Z

.dockerignore

I don't understand why are these lines removed.

Before, the Dockerfile copied them from config/examples, so the ones under config had to be ignored, then overwritten by those under config/examples.

Now they have been moved from config/examples so the ones under config are already the good ones and we can keep them.

akostadinov · 2024-06-05T06:16:18Z

.gitignore

Same here, these ignores should stay.

No, now we want to track them in git as we tracked those under config/examples before, since they are essentially the same files.

akostadinov · 2024-06-05T06:16:44Z

Dockerfile

@@ -19,7 +19,6 @@ ENV DISABLE_SPRING="true" \
 WORKDIR /opt/system/

 ADD . ./
-ADD config/examples/*.yml config/


We need to keep this.

No, config/examples doesn't exist anymore and its files are already under config

bin/setup

akostadinov · 2024-06-05T06:19:34Z

config/amazon_s3.yml

These files contain secrets. The should be git ignored and dockerignored. To prevent leaking them into git or into the container image builds.

This one doesn't contain secrets:

https://github.com/3scale/porta/blob/cleanup-config-files-remove-dups/config/amazon_s3.yml

And it was already in git before, but under config/examples

it contains secret_access_key, at least locally is supposed to

akostadinov · 2024-06-05T06:22:24Z

I think we shouldn't change the structure of config/examples. These should be the files we do local modifications in our local dev envs. Such that when we do local modifications, these don't show up in git diff and don't leak into github or a locally built container image.

jlledom · 2024-06-05T08:38:23Z

I think we shouldn't change the structure of config/examples. These should be the files we do local modifications in our local dev envs. Such that when we do local modifications, these don't show up in git diff and don't leak into github or a locally built container image.

I think I expressed this concern before but I don't find the comment. Anyway, this PRs aims to leave the files under config immutable and use ENV variables to set our local configs from the .env file.

akostadinov · 2024-06-05T09:02:45Z

It's not so good idea to keep passwords in env variables because they are visible to all local processes from this or any other user.

mayorova · 2024-06-05T09:03:35Z

I think we shouldn't change the structure of config/examples. These should be the files we do local modifications in our local dev envs. Such that when we do local modifications, these don't show up in git diff and don't leak into github or a locally built container image.

I think I expressed this concern before but I don't find the comment. Anyway, this PRs aims to leave the files under config immutable and use ENV variables to set our local configs from the .env file.

@akostadinov @jlledom Yes, I understand your concerns. But what @jlledom says is exactly the reasoning.

I actually found it pretty annoying many times that when some PR changes some configs, you need to copy the .yml files from examples to configs again and again, because they are not "in sync" with the branch.

But I also agree that sometimes we want to test stuff locally, and we don't want necessarily to affect the "default" configs.

My idea was to be able to configure most of the things (all would be impossible) via environment variables, and .env always stays local. Now, to figure out which settings are commonly modified for local development might take some time, and we may want to add env vars gradually.

But if you don't like this approach, I'm fine with just moving all the configs back to config/examples again, no problem. In the Dockerfile builds we'll just copy them from config/examples rather than from openshift/system/config.

akostadinov · 2024-06-05T09:07:50Z

At least for anything with passwords, need to be in examples. The rest I guess makes more sense to stay as defaults.

jlledom · 2024-06-05T09:30:03Z

It's not so good idea to keep passwords in env variables because they are visible to all local processes from this or any other user.

I don't thinks that's much of a problem. I have a few passwords stored in my .env and all of them are for fake/test/development/staging environments, I never needed to use any production credentials.

akostadinov · 2024-06-05T09:36:20Z

Any cloud credentials can equally well be abused and costs incurred.

mayorova · 2024-06-05T10:04:29Z

Well, regarding the passwords, in the image the passwords would need to go in env vars anyway, so if we want to use the default configs for both local development and images, they will need to be in env vars in any case.

I think if these are stored in .env in the project dir, they would be read by dotenv only when the Rails process is started, I don't think they are shared with other processes in this case.

I am more concerned about such files as settings.yml where we sometimes tweak onpremises: true, or force_ssl: and secure_cookie: (which are currently hardcoded to true for production, which is good for openshift, but not good for local env - here for example we might bring back FORCE_SSL env var with a default value).

Or maybe domain_substitution.yml which sometimes is tweaked when we run against the staging DB.

Or potentially rolling_updates.yml, however I don't think we ever touch it nowadays, and always just apply the defaults.

Probably in core.yml we could add an env var for fake_server.

So, we can maybe put settings.yml, core.yml and domain_substitution.yml back to examples, or we can add env vars for the values that will potentially change.

I'd vote for env vars way. I think it's easier, and we can even just have multiple sets of .env files with different setups (e.g. "production mode pointing to staging", "local saas" etc.), while config files always stay the same.

akostadinov · 2024-06-05T12:03:28Z

I see your concerns. Just an idea, this might make sense to be implemented with a global override configuration file. Where one can put any configration value and it will override the defaults. Just for dev and test environment. It would not be trivial to implement but would probably cover all use cases.

Well, regarding the passwords, in the image the passwords would need to go in env vars anyway, so if we want to use the default configs for both local development and images, they will need to be in env vars in any case.

It is alright to have env vars read by the config files. Just we locally change it to a static values for sensitive content.

I think if these are stored in .env in the project dir, they would be read by dotenv only when the Rails process is started, I don't think they are shared with other processes in this case.

Check /proc/<PID>/environ, e.g. /proc/406847/environ, you can read the environment of any running process by any other running process on your system.

akostadinov · 2024-06-06T11:08:20Z

three_scale.segment.write_key

Why should it be true? nil makes more sense for something that is not used.

mayorova · 2024-06-06T12:00:02Z

three_scale.segment.write_key

Why should it be true? nil makes more sense for something that is not used.

Yeah, sorry, that comment is outdated, since then I update to taking the value from an env var: https://github.com/3scale/porta/pull/3766/files#diff-3027cb92d1f25cc230471a202a313a8e4ffdc445147f6530c29dbc6e7b7e3ef8R3

So, if the env var is not set, it will be nil. true was confusing indeed, the only reason I even used this value because it was like that in the current openshift config:

porta/openshift/system/config/segment.yml

Line 4 in 505d6cc

write_key: true

It was failing because of the validation error: "From does not match the domain of your outbound email" errors.add(field, :wrong_domain) unless email.join.split('@').last == domain Caused by the change of the default no-reply email in settings from example.net to example.com

See: https://github.com/3scale/3scale-operator/blob/3ea4ffa77883f7ea365f83e2d138bb5b8ec5158c/pkg/3scale/amp/component/system.go#L1226-L1236

mayorova · 2024-06-07T12:00:15Z

@akostadinov @jlledom I restored b9dd022

You can compare the contents of the config files in production mode (so, what will be applied in the image - note that the operator will overwrite some files):

master, with openshift/system/config files copied to config
openshift-master.txt
this branch, with both openshift/system/config and config/examples files copied to config
openshift-cleanup.txt

Diff with some common parts removed:

mayorova marked this pull request as draft April 25, 2024 14:14

mayorova commented Apr 25, 2024

View reviewed changes

mayorova force-pushed the cleanup-config-files-remove-dups branch from 517082c to fcb147f Compare April 25, 2024 14:59

mayorova commented Apr 29, 2024

View reviewed changes

config/web_hooks.yml Outdated Show resolved Hide resolved

mayorova commented Apr 29, 2024

View reviewed changes

mayorova marked this pull request as ready for review April 29, 2024 12:35

jlledom reviewed May 7, 2024

View reviewed changes

mayorova mentioned this pull request May 7, 2024

THREESCALE-8404: Add TLS and ACL support for Redis #3572

Merged

jlledom previously approved these changes May 8, 2024

View reviewed changes

mayorova dismissed jlledom’s stale review via c078fe5 May 8, 2024 13:02

mayorova force-pushed the cleanup-config-files-remove-dups branch from a70cde4 to c078fe5 Compare May 8, 2024 13:02

akostadinov reviewed May 8, 2024

View reviewed changes

jlledom previously approved these changes May 9, 2024

View reviewed changes

mayorova dismissed jlledom’s stale review via a021f68 June 3, 2024 11:51

mayorova force-pushed the cleanup-config-files-remove-dups branch 3 times, most recently from 85275af to c0ef05e Compare June 3, 2024 12:32

mayorova commented Jun 3, 2024

View reviewed changes

mayorova changed the base branch from master to dependabot/bundler/rails-6.1.7.7 June 3, 2024 12:35

mayorova commented Jun 3, 2024

View reviewed changes

jlledom previously approved these changes Jun 3, 2024

View reviewed changes

Base automatically changed from dependabot/bundler/rails-6.1.7.7 to master June 4, 2024 09:01

akostadinov reviewed Jun 5, 2024

View reviewed changes

bin/setup Outdated Show resolved Hide resolved

akostadinov reviewed Jun 5, 2024

View reviewed changes

mayorova force-pushed the cleanup-config-files-remove-dups branch from 837f6ea to f706804 Compare June 7, 2024 11:24

mayorova added 8 commits June 7, 2024 13:26

Remove duplicated config .yml files, keep only in config/

0260dff

Remove prometheus.yml

cadd97c

Cleanup database.yml

b88aa12

Remove obsolete entries from .dockerignore and .gitignore

2ecd679

Remove references to config/examples

73d8796

Align zync.yml with operator-set content

8c4cf60

See: https://github.com/3scale/3scale-operator/blob/3ea4ffa77883f7ea365f83e2d138bb5b8ec5158c/pkg/3scale/amp/component/system.go#L1226-L1236

Restore config/examples folder and use these configs for OpenShift image

b9dd022

mayorova force-pushed the cleanup-config-files-remove-dups branch from f706804 to b9dd022 Compare June 7, 2024 11:26

josemigallas approved these changes Jun 7, 2024

View reviewed changes

		@@ -0,0 +1,42 @@
		base: &default
		<% case ENV['DATABASE_URL'].to_s

	encoding: utf8
	pool: <%= ENV.fetch('RAILS_MAX_THREADS', 5) %>

THREESCALE-10924: Remove duplicated config .yml files, keep only those in config/ #3766

Are you sure you want to change the base?

THREESCALE-10924: Remove duplicated config .yml files, keep only those in config/ #3766

Conversation

mayorova commented Apr 25, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mayorova Apr 25, 2024 • edited

Choose a reason for hiding this comment

mayorova commented Apr 26, 2024 • edited

mayorova commented Apr 29, 2024 • edited

Choose a reason for hiding this comment

mayorova Apr 29, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlledom May 8, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jun 4, 2024 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akostadinov commented Jun 5, 2024

jlledom commented Jun 5, 2024

akostadinov commented Jun 5, 2024

mayorova commented Jun 5, 2024

akostadinov commented Jun 5, 2024

jlledom commented Jun 5, 2024

akostadinov commented Jun 5, 2024

mayorova commented Jun 5, 2024

akostadinov commented Jun 5, 2024

akostadinov commented Jun 6, 2024

mayorova commented Jun 6, 2024

mayorova commented Jun 7, 2024 • edited

mayorova commented Apr 25, 2024 •

edited

mayorova Apr 25, 2024 •

edited

mayorova commented Apr 26, 2024 •

edited

mayorova commented Apr 29, 2024 •

edited

mayorova Apr 29, 2024 •

edited

jlledom May 8, 2024 •

edited

codecov bot commented Jun 4, 2024 •

edited

mayorova commented Jun 7, 2024 •

edited