lnconfig: Support utilizing Environment Variables in `lnd.conf` for `rpcuser` and `rpcpass` fields. #8310

mohamedawnallah · 2023-12-23T17:48:33Z

Change Description

Added supplyEnvValue function in config.go that returns The value of the specified environment variable, or the default value if provided, or the original input string if no matching variable is found or set.
Called supplyEnvValue function in parseRPCParams function in config.go file where rpcuser and rpcpass fields are being parsed.

Fixes #8295

Steps to Test

Added the test cases for supplyEnvVale in the config_test.go file covering the happy and sad paths.

Pull Request Checklist

Testing

Your PR passes all CI checks.
Tests covering the positive and negative (error paths) are included.
Bug fixes contain tests triggering the bug to prevent regressions.

Code Style and Documentation

The change obeys the Code Documentation and Commenting guidelines, and lines wrap at 80.
Commits follow the Ideal Git Commit Structure.
Any new logging statements use an appropriate subsystem and logging level.
Any new lncli commands have appropriate tags in the comments for the rpc in the proto file.
There is a change description in the release notes, or [skip ci] in the commit message for small changes.

📝 Please see our Contribution Guidelines for further guidance.

guggero · 2024-01-03T10:30:51Z

config.go

+// Returns:
+//   - string: The value of the specified environment variable, or the default value
+//     if provided, or the original input string if no matching variable is found or set.
+func extractEnvVariable(value string) string {


Can you please break everything at 80 characters (while configuring your editor to use 8 spaces for tab characters).

Thanks for letting me know! I've checked the code_formatting_rules.md and configured my Visual Studio code settings.json to reflect the Recommended settings for your editor
rules so that it would be reflected automatically in the next PRs:

"editor.insertSpaces": false, "editor.tabSize": 8, "editor.wordWrap": "wordWrapColumn", "editor.wordWrapColumn": 80, "go.formatTool": "gofmt", "go.useLanguageServer": true, "editor.formatOnSave": true, "go.lintTool": "golangci-lint", "go.lintFlags": [ "--fast" ], "[markdown]": { "files.eol": "\n", "files.insertFinalNewline": true, "editor.wordWrap": "bounded", "editor.wordWrapColumn": 80 }

ProofOfKeags

Great work! 🙏🏼

The main feedback I have is that the regex you put together has a slight inaccuracy to it. Other than that, there are a couple opportunities for simplification.

As @guggero points out, you should consult the style guide to ensure all those guidelines are followed. You can ensure these checks pass locally by running make fmt-check and make lint.

Finally, please add to the latest release notes a short description of this change.

ProofOfKeags · 2024-01-03T22:33:01Z

config.go

+//   - string: The value of the specified environment variable, or the default value
+//     if provided, or the original input string if no matching variable is found or set.
+func extractEnvVariable(value string) string {
+	re := regexp.MustCompile(`^(?:\${([a-zA-Z_][a-zA-Z0-9_]*)?(:-(\S+))?})|(?:\$([a-zA-Z_][a-zA-Z0-9_]*))$`)


When I plug this into a diagram tool, I notice that the start of line and end of line characters are broken up across the branches. I don't believe you intended this. I would suggest making it so that both branches are anchored at the beginning and end of line.

Ah yes, this makes sense, and thanks a lot for mentioning this diagram tool the regular expressions make more sense now :)

Here is after updating the regular expression in the diagram tool so that both branches are anchored at the beginning and end of the line.

Yeah I wasn't going to eyeball that regex and be like "LGTM" 😅

config.go

ProofOfKeags · 2024-01-03T22:36:59Z

config.go

+	matches := re.FindStringSubmatch(value)
+
+	if len(matches) > 0 {


I'd combine these lines to make it such that the matches variable is only valid in the block scope.

config.go

mohamedawnallah · 2024-01-05T17:41:01Z

Thanks for the great feedback @guggero @ProofOfKeags I've addressed your suggestions!

guggero · 2024-01-08T08:47:16Z

Before I'm going to review in detail again, just one comment: It feels weird to me that we would want to support different formats and even fall back values within the parsing code in lnd itself.
It makes the regular expression quite complicated and hard to maintain. What's the use case you see for allowing to specify a fall back value? This would need to be documented really well, otherwise I can already see all the support issues being created because users unfamiliar with the bash like syntax remove the - character after the colon because they think it's part of the fall back value instead of the actual bash syntax.

ProofOfKeags

Seems good. Gonna defer to @guggero as far as "should we do this". But assuming we want to, the change itself seems good to go.

lightninglabs-deploy · 2024-03-05T01:24:52Z

@mohamedawnallah, remember to re-request review from reviewers when ready

ProofOfKeags · 2024-03-05T22:30:03Z

I'm going to unsubscribe from this since I have approved. Please ping me if my input is needed again.

mohamedawnallah · 2024-03-09T22:59:10Z

@guggero I'd like to discuss this issue again anytime you're available. I agree with your thoughts That the changes that this PR proposes add much complexity compared to the value it added. I made the fallback value since the main issue #8295 mentioned that in the demo rpcuser=${BTC_RPCUSER:-btc} also that bitcoind does support this fall-back value and To be honest I don't have any use case in mind in the meantime for allowing to specify a fall-back value. That said I'm very open to suggestions e.g. having one format something like ${ENVIRONMENT_VARIABLE} would solve the issue?

What's the use case you see for allowing to specify a fallback value?

cc: @ProofOfKeags

guggero · 2024-03-11T07:22:30Z

@guggero I'd like to discuss this issue again anytime you're available. I agree with your thoughts That the changes that this PR proposes add much complexity compared to the value it added. I made the fallback value since the main issue #8295 mentioned that in the demo rpcuser=${BTC_RPCUSER:-btc} also that bitcoind does support this fall-back value and To be honest I don't have any use case in mind in the meantime for allowing to specify a fall-back value. That said I'm very open to suggestions e.g. having one format something like ${ENVIRONMENT_VARIABLE} would solve the issue?

Okay, I guess it could be useful to have a fall back value. Although I think that can also lead to hard-to-find errors if you're not expecting that.

So I guess my main ask here is to simplify the regular expression, so it's easier to read and understand.
My suggestion would be to have an expression per supported format, then have a switch statement that checks which one matches. Then the extraction will be easier to understand as well.

mohamedawnallah · 2024-03-11T10:38:48Z

So I guess my main ask here is to simplify the regular expression, so it's easier to read and understand. My suggestion would be to have an expression per supported format, then have a switch statement that checks which one matches. Then the extraction will be easier to understand as well.

This looks great! I'm gonna submit a patch and will let you know. Thanks for sharing!

coderabbitai · 2024-03-14T15:17:07Z

Important

Auto Review Skipped

Auto reviews are limited to the following labels: llm-review. Please add one of these labels to enable auto reviews.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository.

To trigger a single review, invoke the @coderabbitai review command.

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>.
- Generate unit-tests for this file.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai generate unit tests for this file.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai generate interesting stats about this repository and render them as a table.
- @coderabbitai show all the console.log statements in this repository.
- @coderabbitai read src/utils.ts and generate unit tests.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
The JSON schema for the configuration file is available here.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

CodeRabbit Discord Community

Join our Discord Community to get help, request features, and share feedback.

guggero

Looks very good now, thanks for the changes!
Just a couple of nits, then we're good to merge 💯

guggero · 2024-03-14T15:21:08Z

config.go

+		`^\$\{([a-zA-Z_][a-zA-Z0-9_]*):-([\S]+)\}$`,
+	)
+
+	// Match against supported formats


nit: missing full stop at end of sentence.

guggero · 2024-03-14T15:21:36Z

config.go

+	rpcUser := supplyEnvValue(string(userSubmatches[1]))
+	rpcPass := supplyEnvValue(string(passSubmatches[1]))
+
+	return rpcUser, rpcPass,


nit: can fit more on one line now (maybe even all values).

config.go

config_test.go

guggero · 2024-03-14T15:25:37Z

config_test.go

@@ -52,3 +53,67 @@ func TestConfigToFlatMap(t *testing.T) {
 	require.Equal(t, redactedPassword, result["db.etcd.pass"])
 	require.Equal(t, redactedPassword, result["db.postgres.dsn"])
 }
+
+func TestSupplyEnvValue(t *testing.T) {
+	// Mock environment variables for testing


nit: missing full stop.

guggero · 2024-03-14T15:26:34Z

config_test.go

+func TestSupplyEnvValue(t *testing.T) {
+	// Mock environment variables for testing
+	os.Setenv("EXISTING_VAR", "existing_value")
+	os.Setenv("EMPTY_VAR", "")


Should use t.Setenv() instead, so they're cleaned up automatically at the end of the test.

guggero · 2024-03-14T15:26:56Z

config_test.go

+	for _, test := range tests {
+		t.Run(test.description, func(t *testing.T) {
+			result := supplyEnvValue(test.input)
+			if result != test.expected {


nit: should use require.Equal() for new code.

In this commit, we support utilizing the usage of enviroment variables in `lnd.conf` file for `rpcuser` and `rpcpass` fields by adding `supplyEnvValue` function in `config.go` that returns the value of the specified environment variable, the fall-back value if provided, or the original input string if no matching environment variables are found or set.

mohamedawnallah · 2024-03-14T16:04:00Z

Thanks a lot, @guggero for your suggestion. It is now much simpler. I've addressed all of your feedback!

guggero

LGTM 🎉

ProofOfKeags

🫡

chevdor mentioned this pull request Dec 29, 2023

[feature]: Support config via ENV to better protect secrets #8295

Closed

mohamedawnallah force-pushed the lnconfig-support-env-vars-for-rpcuser-rpcpassword branch from 40d78af to 4f42f8b Compare January 1, 2024 21:14

saubyk added the config Parameters/arguments/config file related issues/PRs label Jan 2, 2024

saubyk assigned mohamedawnallah Jan 2, 2024

saubyk added this to the v0.18.0 milestone Jan 2, 2024

saubyk requested a review from ProofOfKeags January 2, 2024 20:11

guggero reviewed Jan 3, 2024

View reviewed changes

ProofOfKeags requested changes Jan 3, 2024

View reviewed changes

mohamedawnallah force-pushed the lnconfig-support-env-vars-for-rpcuser-rpcpassword branch from 75b8252 to 187c4df Compare January 5, 2024 17:21

mohamedawnallah requested review from guggero and ProofOfKeags January 5, 2024 17:38

mohamedawnallah force-pushed the lnconfig-support-env-vars-for-rpcuser-rpcpassword branch 2 times, most recently from 6f8b242 to e489af2 Compare January 5, 2024 18:36

ProofOfKeags approved these changes Jan 8, 2024

View reviewed changes

guggero removed their request for review January 9, 2024 11:45

saubyk removed this from the v0.18.0 milestone Jan 10, 2024

mohamedawnallah force-pushed the lnconfig-support-env-vars-for-rpcuser-rpcpassword branch from e489af2 to ddb70d3 Compare March 14, 2024 15:17

guggero reviewed Mar 14, 2024

View reviewed changes

mohamedawnallah force-pushed the lnconfig-support-env-vars-for-rpcuser-rpcpassword branch from ddb70d3 to fb2a634 Compare March 14, 2024 15:32

mohamedawnallah added 3 commits March 14, 2024 17:53

config_test.go: add test cases for supplyEnvValue

3e5f03e

docs: update the release-notes-0.18.0.md

0add4fc

mohamedawnallah force-pushed the lnconfig-support-env-vars-for-rpcuser-rpcpassword branch from fb2a634 to 0add4fc Compare March 14, 2024 15:56

guggero approved these changes Mar 14, 2024

View reviewed changes

guggero requested a review from ProofOfKeags March 14, 2024 16:20

ProofOfKeags approved these changes Mar 15, 2024

View reviewed changes

guggero merged commit 51ebc20 into lightningnetwork:master Mar 15, 2024
26 of 27 checks passed

mohamedawnallah deleted the lnconfig-support-env-vars-for-rpcuser-rpcpassword branch March 31, 2024 12:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lnconfig: Support utilizing Environment Variables in `lnd.conf` for `rpcuser` and `rpcpass` fields. #8310

lnconfig: Support utilizing Environment Variables in `lnd.conf` for `rpcuser` and `rpcpass` fields. #8310

mohamedawnallah commented Dec 23, 2023 •

edited

guggero Jan 3, 2024

mohamedawnallah Jan 5, 2024 •

edited

ProofOfKeags left a comment

ProofOfKeags Jan 3, 2024

mohamedawnallah Jan 5, 2024 •

edited

ProofOfKeags Jan 5, 2024

ProofOfKeags Jan 3, 2024

mohamedawnallah commented Jan 5, 2024

guggero commented Jan 8, 2024

ProofOfKeags left a comment

lightninglabs-deploy commented Mar 5, 2024

ProofOfKeags commented Mar 5, 2024

mohamedawnallah commented Mar 9, 2024 •

edited

guggero commented Mar 11, 2024

mohamedawnallah commented Mar 11, 2024

coderabbitai bot commented Mar 14, 2024

Auto Review Skipped

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (`.coderabbit.yaml`)

CodeRabbit Discord Community

guggero left a comment

guggero Mar 14, 2024

guggero Mar 14, 2024

guggero Mar 14, 2024

guggero Mar 14, 2024

guggero Mar 14, 2024

mohamedawnallah commented Mar 14, 2024

guggero left a comment

ProofOfKeags left a comment

		matches := re.FindStringSubmatch(value)

		if len(matches) > 0 {

lnconfig: Support utilizing Environment Variables in lnd.conf for rpcuser and rpcpass fields. #8310

lnconfig: Support utilizing Environment Variables in lnd.conf for rpcuser and rpcpass fields. #8310

Conversation

mohamedawnallah commented Dec 23, 2023 • edited

Change Description

Steps to Test

Pull Request Checklist

Testing

Code Style and Documentation

Choose a reason for hiding this comment

mohamedawnallah Jan 5, 2024 • edited

Choose a reason for hiding this comment

ProofOfKeags left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mohamedawnallah Jan 5, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mohamedawnallah commented Jan 5, 2024

guggero commented Jan 8, 2024

ProofOfKeags left a comment

Choose a reason for hiding this comment

lightninglabs-deploy commented Mar 5, 2024

ProofOfKeags commented Mar 5, 2024

mohamedawnallah commented Mar 9, 2024 • edited

guggero commented Mar 11, 2024

mohamedawnallah commented Mar 11, 2024

coderabbitai bot commented Mar 14, 2024

Auto Review Skipped

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (.coderabbit.yaml)

CodeRabbit Discord Community

guggero left a 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

mohamedawnallah commented Mar 14, 2024

guggero left a comment

Choose a reason for hiding this comment

ProofOfKeags left a comment

Choose a reason for hiding this comment

lnconfig: Support utilizing Environment Variables in `lnd.conf` for `rpcuser` and `rpcpass` fields. #8310

lnconfig: Support utilizing Environment Variables in `lnd.conf` for `rpcuser` and `rpcpass` fields. #8310

mohamedawnallah commented Dec 23, 2023 •

edited

mohamedawnallah Jan 5, 2024 •

edited

mohamedawnallah Jan 5, 2024 •

edited

mohamedawnallah commented Mar 9, 2024 •

edited

CodeRabbit Configration File (`.coderabbit.yaml`)