Resolve secrets in ERB strings

[wjordan]

Allow configuration values to depend on secrets within interpolated strings (such as db_reader: !Secret referenced within dashboard_db_reader: '<%="#{db_reader}#{dashboard_db_name}"%>'.

For the implementation, I've added an internal string representation of the secret as ${secretkey}, which is matched in the lazy_load_secrets! method enhanced to regex-replace (gsub) all secrets found within string values.

Added a test (test_erb_string_secret) to verify the new functionality, and I also added some extra coverage around json secrets (test_json_secret) and fixed a related bug in how json secrets get passed through the system.

[codecov-io]

Codecov Report

❗ No coverage uploaded for pull request base (staging@2272520). Click here to learn what that means.
The diff coverage is 60%.

@@            Coverage Diff             @@
##             staging   #30228   +/-   ##
==========================================
  Coverage           ?   76.52%           
==========================================
  Files              ?      677           
  Lines              ?    27679           
  Branches           ?        0           
==========================================
  Hits               ?    21181           
  Misses             ?     6498           
  Partials           ?        0

Impacted Files	Coverage Δ
lib/cdo/secrets_config.rb	66% <50%> (ø)
lib/cdo/secrets.rb	61.42% <66.66%> (ø)
lib/cdo/lazy.rb	85.71% <71.42%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2272520...4e330b6. Read the comment docs.

[uponthesun]

• This change explicitly only allows interpolation of config values if those values come from secrets, is that right?

• We should take a minute to discuss possible alternatives, since the more features we add the more complex our mini config language is going to get. What if instead we had these as methods on the CDO object, similar to dashboard_hostname?

  code-dot-org/lib/cdo.rb

  Line 74 in 161ab00

  def dashboard_hostname

[wjordan]

We should take a minute to discuss possible alternatives, since the more features we add the more complex our mini config language is going to get.

I believe this change (basically a bug-fix) is much more narrowly scoped compared to any of the alternatives I considered.

This change explicitly only allows interpolation of config values if those values come from secrets, is that right?

Prior to this change, references to Secret values resolved to nil. With this change, references to Secret values will resolve to the value of the secret just like all other config values.

What if instead we had these as methods on the CDO object, similar to dashboard_hostname?

Adding global CDO methods to resolve values referencing Secrets would mean two ways to reference a config value (ERB or CDO method), depending on whether the value to be referenced is defined as (or might possibly be overridden to be) a Secret. I think this would be more complicated to work with than the change in this PR.

A possibly simpler alternative would be to replace all ERB tags in the config with global CDO methods (get rid of the ERB layer entirely), but that would be an even more challenging task.

More generally, I've been trying to eliminate/refactor all of the legacy global methods in the CDO config for a few reasons:

• Global CDO methods aren't declared in the same config file as the other configuration values, so it becomes more complex to locate config values.
• Global CDO methods don't currently support the configuration-override hierarchy, so e.g., allowing a global method to be optionally customized/overridden by local config would take extra work.
• Global CDO methods get executed on every reference, which means the return value of a method might change from one call to another. Mixing dynamic method calls alongside static configuration properties adds complexity and makes it harder to reason about config behavior in general.

[wjordan]

Happy to followup on any remaining design questions, but I'm going to merge this PR now to fix the specific issue and unblock ongoing work.