Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: environment variable interpolation #30

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

tangowithfoxtrot
Copy link
Contributor

@tangowithfoxtrot tangowithfoxtrot commented Jan 26, 2025

Allow ${VAR_INTERPOLATION} in config file. Addresses #23.

Example

radarr:
  host: "${RADARR_HOST}" # with optional quotes
  port: 7878
  api_token: ${RADARR_API_KEY} # or without optional quotes
  use_ssl: false

Copy link

codecov bot commented Jan 26, 2025

Codecov Report

Attention: Patch coverage is 42.85714% with 28 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/app/mod.rs 42.85% 28 Missing ⚠️
Files with missing lines Coverage Δ
src/app/mod.rs 68.46% <42.85%> (-6.54%) ⬇️

src/app/mod.rs Outdated
pub port: Option<u16>,
#[serde(deserialize_with = "deserialize_optional_env_var")]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a first attempt at getting variable interpolation working. It does work, but it's breaking optional config values. I keep getting the following error:

Error: radarr: missing field `uri` at line 2 column 3

This occurs with any other optional config value, like ssl_cert_paths as well.

I'm having trouble figuring out where that's coming from or how to make the values actually optional again 😅 @Dark-Alex-17, any ideas? It doesn't seem to be coming from the validate() function, so it seems like something is using the config values before they're validated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aside from breaking optional values, I'm not sure if there's a less verbose way to generalize deserializing the various types of config values (String, Option<String>, and Option<u16>).

For now, I had to make separate deserializers for each, but I wonder if there's a way to generalize them, without doing something icky and just treating them all as String or Option<String> 🤔

Copy link
Owner

Choose a reason for hiding this comment

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

I have something working that might address these concerns. Mind if I push to your branch?

Copy link
Owner

@Dark-Alex-17 Dark-Alex-17 Jan 26, 2025

Choose a reason for hiding this comment

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

In short though:

  • To address the concern about security, I created a regex to remove all non-essential characters (including whitespace) from all variable values, not just those interpolated from environment variables. When I tested this, even valid Rust code was rendered unreadable and unusable so I can confirm it does pretty good mitigation of any code injection attacks

  • To address the issue of not being able to start with empty optional values, I added #[serde(default, ...)] to the Option fields which allows Serde to construct the default None value when it loads from the YAML file and when there's no value for the corresponding field.

  • I tried writing a macro to simplify the need for dedicated deserializers for Option fields but alas, even after all my tinkering and Googling, what you've come up with truly is the best way to achieve this.

  • And I also added a new crate, veil, so that debugging is easier; It allows you to derive the Debug trait on a struct but mark which fields you want redacted on output. So, I naturally redacted the api_token field. Then I simply added a new line to main.rs that debugs the config that was loaded to help with debugging issues both here, and in the future. I actually just put this in a separate commit to main to make life better for all.

  • I also added a few tests to make sure it was working the way I wanted. The only other tests that would need to be written are for the Option deserializers.

  • I also tested this out with YAML fields that have multiple environment variables and it worked perfectly; e.g. radarr.uri: http://${RADARR_HOST}:${RADARR_PORT}/something

What are your thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To address the concern about security, I created a regex to remove all non-essential characters (including whitespace) from all variable values, not just those interpolated from environment variables. When I tested this, even valid Rust code was rendered unreadable and unusable so I can confirm it does pretty good mitigation of any code injection attacks

That's a very interesting and defensive approach. I like it!

To address the issue of not being able to start with empty optional values, I added #[serde(default, ...)] ...

🙌 TIL about serde default. Great find! That was way simpler than I thought it might be.

I tried writing a macro to simplify the need for dedicated deserializers for Option fields but alas, even after all my tinkering and Googling, what you've come up with truly is the best way to achieve this.

That's a relief 😌 I had no idea how else to make that nicer and haven't delved into macros yet. lol.

I also tested this out with YAML fields that have multiple environment variables and it worked perfectly; e.g. radarr.uri: http://${RADARR_HOST}:${RADARR_PORT}/something

Good thinking. Combining the vars didn't occur to me. I'm glad it works.

I also added a few tests to make sure it was working the way I wanted. The only other tests that would need to be written are for the Option deserializers.

I'll take a look at these next.

Thanks for your help!

@Dark-Alex-17
Copy link
Owner

Sorry about the main branch thing. Not entirely sure what's going on right now since it's working locally and that supposed missing } is present in the app_tests.rs file...

@tangowithfoxtrot
Copy link
Contributor Author

No worries. I'm pretty sure I'm just having git skill issues somewhere. lol.

@tangowithfoxtrot tangowithfoxtrot changed the title feat: Environment variable interpolation feat: environment variable interpolation Jan 26, 2025
@Dark-Alex-17
Copy link
Owner

Dark-Alex-17 commented Jan 27, 2025

Hey, I've been there. Even as a pro, there's a reason the initial commit for Git literally calls it the, "information manager from hell", along with this explanation:

GIT - the stupid content tracker

"git" can mean anything, depending on your mood.

  • random three-letter combination that is pronounceable, and not actually used by any common UNIX command. The fact that it is a mispronounciation of "get" may or may not be relevant.
  • stupid. contemptible and despicable. simple.
    Take your pick from the dictionary of slang.
  • "global information tracker": you're in a good mood, and it actually works for you. Angels sing, and a light suddenly fills the room.
  • "goddamn idiotic truckload of sh*t": when it breaks

So. You're in good company 😁

@Dark-Alex-17 Dark-Alex-17 added the enhancement New feature or request label Jan 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants