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?

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.

jlledom · 2024-05-07T08:34:51Z

config/oauth2.yml

@@ -14,4 +14,5 @@ test:
    client_secret: fake_secret

 production:
-  <<: *default
+  github:


Does this disable adding Github as an external auth provider?

This is for "3scale-branded" GitHub integration, that is used in SaaS *

This has always been disabled in on-premises: https://github.com/3scale/porta/blob/99b24fb6622653c0f040e36b5530bd1b55e8f64b/openshift/system/config/oauth2.yml

in SaaS we have a global GitHub client, that we allow the providers to use for authentication, without the need to set up their own GitHub OAuth application. They can still use their own GitHub apps, this setting doesn't (or at least shouldn't) prevent that.

Then I think it should be disabled by default. With current configuration it's enabled by for development, but with empty id and key:

base: &default github: enabled: <%= ENV.fetch('GITHUB_ENABLED', '1') == '1' %> client_id: <%= ENV['GITHUB_CLIENT_ID'] %> client_secret: <%= ENV['GITHUB_CLIENT_SECRET'] %> development: <<: *default

IMO it should be like this:

enabled: <%= ENV.fetch('GITHUB_ENABLED', '0') == '1' %>

Yep, makes sense 👍

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

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

config/segment.yml

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


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 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 added 10 commits May 8, 2024 15:02

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

be406a6

Remove prometheus.yml

40f82c1

Cleanup database.yml

b483f84

Fix service discovery config

626e827

Remove obsolete entries from .dockerignore and .gitignore

f99095e

Fix some issues

209a530

Remove references to config/examples

e29aa3b

Simlify the test as per comment

3f097f7

Fix configs as per comments

c078fe5

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

config/segment.yml

enabled: false

stub: false

write_key: 'null'

enabled: <%= ENV['SEGMENT_WRITE_KEY'].present? %>

Copy link

Contributor

akostadinov May 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!

jlledom approved these changes May 9, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

mayorova commented Apr 25, 2024 •

edited

mayorova Apr 25, 2024

mayorova Apr 25, 2024

mayorova Apr 25, 2024 •

edited

mayorova commented Apr 26, 2024 •

edited

mayorova commented Apr 29, 2024 •

edited

mayorova Apr 29, 2024

mayorova Apr 29, 2024 •

edited

mayorova May 3, 2024

jlledom May 7, 2024

mayorova May 7, 2024

jlledom May 8, 2024 •

edited

jlledom May 7, 2024

mayorova May 7, 2024

jlledom May 8, 2024

mayorova May 8, 2024

akostadinov May 8, 2024

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

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

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

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