demarches-simplifiees / demarches-simplifiees.fr

Dématérialiser et simplifier les démarches administratives
https://www.demarches-simplifiees.fr
GNU Affero General Public License v3.0
190 stars 89 forks source link

Endiguer l’expansion des variables d'environnement #6934

Open akarzim opened 2 years ago

akarzim commented 2 years ago

Résumé

À un moment il faut qu’on arrêter l’expansion des variables d'environnement... J'ai l'impression en plus que beaucoup de ces textes ont vocation à être traduits. Donc l'usage des variables d'environnent n'est pas possible. J'ai l'impression que la solution c'est :

  • passer ces valeurs dans i18n. Sur une instance, surcharger les locales.
  • passer le gros des variables d'environnement de configuration dans un fichier de configuration d'instance.

Originally posted by @tchak in https://github.com/betagouv/demarches-simplifiees.fr/issues/6878#issuecomment-1028173970

mots-clés : convention, env, rails, variable

tickets liés : #6935, #6936

Actuellement

Voici l'usage qui en est fait actuellement :

  1. une variable d'environnement est créée et déclarée idéalement dans le fichier config/env.example ou, si elle est optionnelle, dans config/env.example.optional ;
  2. une constante globale éponyme est déclarée dans un initializer ;
  3. cette constante est utilisée dans d'autres initializers et dans le reste du code de l'application.

Constats

  1. les variables d'environnement sont parfois utilisées directement dans le code, sans passer par une constante ;
  2. certaines constantes ne sont pas dépendantes de variables d'environnement, mais contiennent des valeurs en dur ;
  3. parfois, des chaînes de caractères sont directement utilisées dans le code, sans passer par une constante ou une variable d'environnement ;
  4. l'ordre de résolution des initializers a une importance car certains peuvent dépendre de constantes déclarées dans d'autres ;
  5. les constantes sont dispersées dans différents initializers, tantôt par sémantique (ex: toutes les URLs) tantôt par domaine (Contacts, Passwords, Watermark, etc.).

Comportement attendu

Rails nous met à disposition un ensemble d'outils pour répondre aux besoins de configuration de l'application. Cela passe notamment par :

source: https://guides.rubyonrails.org/v6.1/configuring.html#custom-configuration source: https://guides.rubyonrails.org/v6.1/security.html#environmental-security

Proposition

Adopter les conventions de Rails pour configurer l'application DS apporterait plusieurs avantages : Rails serait responsable du bon chargement de la configuration de l'application (plus besoin d'ordonner les initializers) ; et nous pourrions, au sein d'un même fichier YAML, embrasser d'un seul regard la configuration d'une brique applicative pour l'ensemble des environnements Rails (ex: un fichier sentry.yml avec la configuration de Sentry dont les valeurs par défaut pourraient différer d'un environnement à l'autre). Un exemple de ce à quoi cela ressemblerait peut être observé sur le commit 74c31f2ebd4fb321c943ae0bdad5eacdd71aa59f traitant de la configuration de FranceConnect.

Cela reste un gros chantier, sur lequel il faudra évidemment se coordonner si on l'entreprend, mais apporterait beaucoup à la stabilité de l'application et faciliterait la paramétrabilité by design de l'applicaiton.


Note 1 : Au sujet du chiffrement des secrets, voir le ticket #6936.

Note 2 : Au sujet des variables d'environnement qui tiennent davantage de l’internationalisation, voir le ticket #6935.


voir : PR à venir / @adullact & @synbioz

  • L'ADULLACT a mandaté le prestataire @synbioz pour développer plusieurs fonctionnalités (tickets et PR à venir).
  • C'est dans ce cadre que @synbioz nous propose certains correctifs annexes comme celui-ci.
  • Si c'est nécessaire, @akarzim et @jobygoude de @synbioz pourront interagir avec l'équipe @betagouv sur ce ticket et sur la PR (répondre aux commentaires, pousser des commits…).

trackingAdullactContrib

kemenaran commented 2 years ago

Je suis carrément chaud pour séparer les différentes configs dans des fichiers YAML séparés (et là où c'est pertinent mettre la conf dans Rails.application.x.config.*).

kemenaran commented 2 years ago

On pourrait même commencer par un branding.yml, qui contiendrait les éléments de branding non-localisables (nom de l'app, du logo, etc)

tchak commented 2 years ago

Pour moi c'est oui pour un branding.yml chargé via config_for. Je serai aussi pour dans le futur ne plus avoir de globales et ne plus avoir d'accès au ENV dans le code de l'app. Mais j'ai l'impression que je suis assez seul là-dessus.

akarzim commented 2 years ago

Je serai aussi pour dans le futur ne plus avoir de globales et ne plus avoir d'accès au ENV dans le code de l'app. Mais j'ai l'impression que je suis assez seul là-dessus.

@tchak je te soutiens à 100% sur cette approche !

kemenaran commented 2 years ago

Je suis en train de penser à un système comme ce que fait Mattermost :

Avantages :

Inconvénients

Vous en pensez quoi ?

akarzim commented 2 years ago

Ça m'a l'air prometteur !

Si je comprends bien, pour que la surcharge puisse se faire de manière transparente, les YAML ressembleraient plutôt à ceci :

# branding.yml
ds:
  app_name: "<%= ENV.fetch('DS_APP_NAME', 'demarches-simplifiees.fr') %>"
  contact_email: "<%= ENV.fetch('DS_CONTACT_EMAIL', 'contact@example.org') %>"
  # etc…

Concernant la configuration, il est tout à fait possible de mettre des conditions et de surcharger Rails.config.x.* au besoin, je ne vois pas de point de blocage de ce côté là. Tu as un exemple en tête ?

kemenaran commented 2 years ago

Non, l'idée ça serait d'overrider les variables automatiquement.

Un truc du genre :

# config/initializers/read_env.rb

# For each env var that begins by `DS_`, override the matching setting in Rails.application.config.x.*
ENV.filter { |name, _| name.has_prefix?('DS_') }.each do |name, value|
  Rails.application.config.set('x.' + name.lowercase) = value
end

(En vrai il faudrait plutôt faire dans le sens inverse : énumérer les clefs de la config, et ensuite voir s'il y a une variable d'env correspondante. Mais c'est l'idée.)

akarzim commented 2 years ago

Ah oui, c'est une approche un peu plus « méta » mais ça marche aussi, je vois l'idée.

Ce qui serait parfait, en peaufinant cette approche, ce serait de pouvoir gérer proprement :

Par exemple, ici la variable d'env vaut enabled mais la clé de configuration est bien un booléen :

config.x.france_connect.enabled = ENV.fetch("DS_FRANCE_CONNECT_ENABLED", "enabled") == "enabled"

Il faudrait ainsi pouvoir être en mesure de lever une exception si une variable d'environnement prend une valeur inattendue. Une piste pourrait être de se tourner vers l'une des gems dry-rb, comme dry-types par exemple, pour décrire les clés et valeurs acceptées, et faire de la coercition quand nécessaire.