reloader: don't fail on envvar expansion errors #7429

rexagod · 2024-06-12T18:58:34Z

Don't fail the reloader on environment variable expansion errors.

Refer: prometheus-operator/prometheus-operator#6136

I added CHANGELOG entry for this change.
Change is not relevant to the end user.

Changes

Reloader: do not fail on environment variable expansion errors.

Verification

Modified the tests to cover the changes.

saswatamcode · 2024-06-13T06:52:03Z

pkg/reloader/reloader.go

+				return err
+			}
+			r.configApplyErrors.Inc()
+			level.Error(r.logger).Log("msg", "expand environment variables", "err", err)


So by logging out, sidecar doesn't fail, but there would be errors on Prometheus?

Ah, due to unset variables? Referencing the parent issue, there's a need to allow the reloader to not crash on such instances, as is the case for all >1 file modifications (and reloads), and thus align environment expansion errors to that same behavior.

saswatamcode

LGTM. Maybe @MichaHoffmann also wants to take a look?

simonpasquier · 2024-06-13T12:50:13Z

Maybe we can make the behavior configurable? FWIW most of the prometheus config reloader functionality comes from the pkg/reloader but as long as we have an option to create a Reloader instance that doesn't fail on env subst, it is also fine to keep the current behavior as the default.

simonpasquier · 2024-06-13T12:54:23Z

pkg/reloader/reloader.go

@@ -701,7 +708,7 @@ func expandEnv(b []byte) (r []byte, err error) {

 		v, ok := os.LookupEnv(string(n))
 		if !ok {
-			err = errors.Errorf("found reference to unset environment variable %q", n)
+			err = errors.Wrapf(expandEnvError, "%s", n)


IIUC the first failed expansion will stop the processing of the config. I'd rather see this as best-effort (e.g. do all possible replacements and record any error).

rexagod · 2024-06-13T20:25:25Z

CI failures don't seem to be stemming from the patch.

simonpasquier · 2024-06-14T07:32:47Z

CHANGELOG.md

@@ -25,6 +25,7 @@ We use *breaking :warning:* to mark changes that are not backward compatible (re

 ### Changed

+- [#7429](https://github.com/thanos-io/thanos/pull/7429): Reloader: introduce `suppressEnvironmentVariablesExpansionErrors` to allow suppressing errors when expanding environment variables in the configuration file. When set, this will ensure that the reloader won't crash when an unset environment variable is encountered. Note that all unset environment variables are left as is, whereas all set environment variables are expanded as usual.


Suggested change

- [#7429](https://github.com/thanos-io/thanos/pull/7429): Reloader: introduce `suppressEnvironmentVariablesExpansionErrors` to allow suppressing errors when expanding environment variables in the configuration file. When set, this will ensure that the reloader won't crash when an unset environment variable is encountered. Note that all unset environment variables are left as is, whereas all set environment variables are expanded as usual.

- [#7429](https://github.com/thanos-io/thanos/pull/7429): Reloader: introduce `suppressEnvironmentVariablesExpansionErrors` to allow suppressing errors when expanding environment variables in the configuration file. When set, this will ensure that the reloader won't consider the operation to fail when an unset environment variable is encountered. Note that all unset environment variables are left as is, whereas all set environment variables are expanded as usual.

simonpasquier · 2024-06-14T07:33:46Z

pkg/reloader/reloader.go

+	cfgFile                                     string
+	cfgOutputFile                               string
+	cfgDirs                                     []CfgDirOption
+	suppressEnvironmentVariablesExpansionErrors bool


(suggestion)

Suggested change

suppressEnvironmentVariablesExpansionErrors bool

discardEnvVarExpansionErrors bool

simonpasquier · 2024-06-14T07:35:10Z

pkg/reloader/reloader.go

+	cfgFile                                     string
+	cfgOutputFile                               string
+	cfgDirs                                     []CfgDirOption
+	suppressEnvironmentVariablesExpansionErrors bool


It needs to be exposed in Options too?

simonpasquier · 2024-06-14T07:37:19Z

pkg/reloader/reloader.go

@@ -692,16 +693,24 @@ func RuntimeInfoURLFromBase(u *url.URL) *url.URL {

 var envRe = regexp.MustCompile(`\$\(([a-zA-Z_0-9]+)\)`)

-func expandEnv(b []byte) (r []byte, err error) {
+func expandEnv(logger log.Logger, b []byte, suppressEnvironmentVariablesExpansionErrors bool, configApplyErrorsPtr *prometheus.Counter) (r []byte, err error) {


rather than passing the additional parameters, why not make it a *Reloader function?

simonpasquier · 2024-06-14T07:38:54Z

pkg/reloader/reloader.go

 		n = n[2 : len(n)-1]

 		v, ok := os.LookupEnv(string(n))
 		if !ok {
-			err = errors.Errorf("found reference to unset environment variable %q", n)
+			(*configApplyErrorsPtr).Inc()


I'd rather see a new metric to track this: either an expansion errors counter or a gauge (the latter probably makes more sense as a counter needs constant increases to trigger an alert).

simonpasquier · 2024-06-14T07:39:06Z

pkg/reloader/reloader.go

+			(*configApplyErrorsPtr).Inc()
+			errStr := errors.Errorf("found reference to unset environment variable %q", n)
+			if suppressEnvironmentVariablesExpansionErrors {
+				level.Debug(logger).Log("msg", "expand environment variable", "err", errStr)


I'd expect a Warn

rexagod · 2024-06-14T11:46:47Z

Test failure seems to indicate that the filesystem changes dropped significantly since the last commit, I don't see any changes in the reloader that may visibly warrant that though. I'll take another look.

Also, the documentation failure seems unrelated (I believe some URLs were moved)?

simonpasquier · 2024-06-14T12:23:48Z

IIUC the failure on the docs job workflow is a glitch with https://outshift.cisco.com/blog/multi-cluster-monitoring

rexagod · 2024-06-14T13:13:48Z

It seems it wasn't moved, but failed to render due to a recent change.

Couldn't find target container #hubspotForm-7f3330bd-39c5-4358-b56d-44b90ae62e8e for HubSpot Form 7f3330bd-39c5-4358-b56d-44b90ae62e8e. Not rendering form onto the page

My guess is they tried to integrate HubSpot which didn't go as planned, and crashed the DOM.

pkg/reloader/reloader.go

simonpasquier · 2024-06-18T08:52:41Z

pkg/reloader/reloader.go

 		n = n[2 : len(n)-1]

 		v, ok := os.LookupEnv(string(n))
 		if !ok {
-			err = errors.Errorf("found reference to unset environment variable %q", n)
+			r.configEnvVarExpansionErrors.Inc()


you'd need to save the number of errors in a local variable and set the gauge before exiting expandEnv()

Ah, right! It makes sense to do this on a per-config basis, so as to not take into account increments from a previous config expansion. I'll make the change.

simonpasquier · 2024-06-18T08:54:56Z

pkg/reloader/reloader.go

+				level.Warn(r.logger).Log("msg", "expand environment variable", "err", errStr)
+				return m
+			}
+			level.Error(r.logger).Log("msg", "expand environment variable", "err", errStr)


hmm the error should be logged by the caller in this case?

simonpasquier

To test similar changes in the past, I opened a PR against prometheus-operator/prometheus-operator replacing the upstream version by my fork. This way I made sure that it worked as expected (e.g. no need for back-and-forth changes between the 2 repos).

pkg/reloader/reloader.go

simonpasquier · 2024-06-18T12:35:54Z

pkg/reloader/reloader.go

@@ -348,8 +360,8 @@ func (r *Reloader) Watch(ctx context.Context) error {
 	}
 }

-func normalize(logger log.Logger, inputFile, outputFile string) error {
-	b, err := os.ReadFile(inputFile)
+func (r *Reloader) normalize(input, output string) error {


(nit) I preferred inputFile and outputFile as it makes it obvious that we talk about files and not data.

Ah, an overlook on my part. Reverted.

rexagod · 2024-06-18T13:58:12Z

@simonpasquier Makes sense, I remember doing a similar thing in node-exporter a while back (I pointed it to a local directory for convenience (faster changes since the other PR was also open and I wanted to test and develop simultaneously), but it's better to use the fork's URL in this case, as we've reached a point where this PR looks good for now (and pointing to a local directory will fail the CI).

I'll raise a PR downstream and hold off on merging this before that's been verified.

Refer: thanos-io/thanos#7429 Fixes: prometheus-operator#6136 Signed-off-by: Pranshu Srivastava <[email protected]>

docs/storage.md

simonpasquier · 2024-06-25T12:34:29Z

CHANGELOG.md

@@ -25,6 +25,7 @@ We use *breaking :warning:* to mark changes that are not backward compatible (re

 ### Changed

+- [#7429](https://github.com/thanos-io/thanos/pull/7429): Reloader: introduce `suppressEnvironmentVariablesExpansionErrors` to allow suppressing errors when expanding environment variables in the configuration file. When set, this will ensure that the reloader won't consider the operation to fail when an unset environment variable is encountered. Note that all unset environment variables are left as is, whereas all set environment variables are expanded as usual.


the name doesn't match the code. Also I'm not sure if the change should be listed in the changelog since it's not user-visible.
cc @saswatamcode

Fixed. Also moved this to the Added section since we are adding to a, IIUC, user-visible API (reloader.Options)?

pkg/reloader/reloader.go

rexagod · 2024-06-25T14:17:37Z

.mdox.validate.yaml

-  # 500 when requested my mdox in GH actions.
-  - regex: 'outshift\.cisco\.com'
+  - regex: 'outshift\.cisco\.com\/blog\/multi-cluster-monitoring'


The domain works just fine, only /blog/multi-cluster-monitoring has a problem.

Allow suppressing environment variables expansion errors when unset, and thus keep the reloader from crashing. Instead leave them as is. Signed-off-by: Pranshu Srivastava <[email protected]>

simonpasquier

lgtm

saswatamcode

Thanks! LGTM!

Refer: thanos-io/thanos#7429 Fixes: prometheus-operator#6136 Signed-off-by: Pranshu Srivastava <[email protected]>

Allow suppressing environment variables expansion errors when unset, and thus keep the reloader from crashing. Instead leave them as is. Signed-off-by: Pranshu Srivastava <[email protected]> Signed-off-by: Tidhar Klein Orbach <[email protected]>

Allow suppressing environment variables expansion errors when unset, and thus keep the reloader from crashing. Instead leave them as is. Signed-off-by: Pranshu Srivastava <[email protected]>

pull-request-size bot added the size/M label Jun 12, 2024

rexagod force-pushed the po-6136 branch 2 times, most recently from d8b54d9 to 8a16f7b Compare June 12, 2024 19:11

saswatamcode reviewed Jun 13, 2024

View reviewed changes

saswatamcode previously approved these changes Jun 13, 2024

View reviewed changes

simonpasquier reviewed Jun 13, 2024

View reviewed changes

rexagod dismissed saswatamcode’s stale review via 0cfb277 June 13, 2024 18:13

rexagod force-pushed the po-6136 branch 3 times, most recently from 9d05302 to 53b0a29 Compare June 13, 2024 18:51

rexagod requested a review from simonpasquier June 13, 2024 20:25

simonpasquier reviewed Jun 14, 2024

View reviewed changes

pull-request-size bot added size/L and removed size/M labels Jun 14, 2024

rexagod force-pushed the po-6136 branch 3 times, most recently from 80de75d to 2d9509d Compare June 14, 2024 08:54

rexagod force-pushed the po-6136 branch 2 times, most recently from a28d14e to 5bcbc33 Compare June 14, 2024 18:56

rexagod requested a review from simonpasquier June 18, 2024 07:36

simonpasquier reviewed Jun 18, 2024

View reviewed changes

rexagod force-pushed the po-6136 branch 2 times, most recently from f3a17f7 to edf9a3f Compare June 18, 2024 11:39

simonpasquier reviewed Jun 18, 2024

View reviewed changes

rexagod force-pushed the po-6136 branch from 4184c86 to 2c88397 Compare June 18, 2024 13:59

rexagod added a commit to rexagod/prometheus-operator that referenced this pull request Jun 23, 2024

reloader: don't fail on envvar expansion errors

b07e2ae

Refer: thanos-io/thanos#7429 Fixes: prometheus-operator#6136 Signed-off-by: Pranshu Srivastava <[email protected]>

rexagod mentioned this pull request Jun 23, 2024

reloader: don't fail on envvar expansion errors prometheus-operator/prometheus-operator#6705

Merged

5 tasks

simonpasquier reviewed Jun 25, 2024

View reviewed changes

rexagod force-pushed the po-6136 branch 4 times, most recently from 509d346 to e2d158d Compare June 25, 2024 14:16

rexagod commented Jun 25, 2024

View reviewed changes

rexagod force-pushed the po-6136 branch from e2d158d to dc5f7c4 Compare June 25, 2024 14:18

reloader: allow suppressing envvar errors

b8edf25

Allow suppressing environment variables expansion errors when unset, and thus keep the reloader from crashing. Instead leave them as is. Signed-off-by: Pranshu Srivastava <[email protected]>

rexagod force-pushed the po-6136 branch from dc5f7c4 to b8edf25 Compare June 25, 2024 14:19

simonpasquier reviewed Jun 26, 2024

View reviewed changes

rexagod requested a review from saswatamcode July 2, 2024 08:08

saswatamcode approved these changes Jul 2, 2024

View reviewed changes

saswatamcode merged commit fcc88c0 into thanos-io:main Jul 2, 2024
20 checks passed

rexagod added a commit to rexagod/prometheus-operator that referenced this pull request Jul 2, 2024

reloader: don't fail on envvar expansion errors

1c1758b

Refer: thanos-io/thanos#7429 Fixes: prometheus-operator#6136 Signed-off-by: Pranshu Srivastava <[email protected]>

rexagod added a commit to rexagod/prometheus-operator that referenced this pull request Jul 2, 2024

reloader: don't fail on envvar expansion errors

fabe51a

Refer: thanos-io/thanos#7429 Fixes: prometheus-operator#6136 Signed-off-by: Pranshu Srivastava <[email protected]>

rexagod added a commit to rexagod/prometheus-operator that referenced this pull request Jul 2, 2024

reloader: don't fail on envvar expansion errors

0002aac

Refer: thanos-io/thanos#7429 Fixes: prometheus-operator#6136 Signed-off-by: Pranshu Srivastava <[email protected]>

dongjiang1989 pushed a commit to kubeservice-stack/prometheus-operator that referenced this pull request Jul 4, 2024

reloader: don't fail on envvar expansion errors

ea8637a

Refer: thanos-io/thanos#7429 Fixes: prometheus-operator#6136 Signed-off-by: Pranshu Srivastava <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

reloader: don't fail on envvar expansion errors #7429

reloader: don't fail on envvar expansion errors #7429

rexagod commented Jun 12, 2024

saswatamcode Jun 13, 2024

rexagod Jun 13, 2024

saswatamcode left a comment

simonpasquier commented Jun 13, 2024

simonpasquier Jun 13, 2024

rexagod commented Jun 13, 2024

simonpasquier Jun 14, 2024

simonpasquier Jun 14, 2024

simonpasquier Jun 14, 2024

simonpasquier Jun 14, 2024

simonpasquier Jun 14, 2024

simonpasquier Jun 14, 2024

rexagod commented Jun 14, 2024 •

edited

Loading

simonpasquier commented Jun 14, 2024

rexagod commented Jun 14, 2024

simonpasquier Jun 18, 2024

rexagod Jun 18, 2024 •

edited

Loading

simonpasquier Jun 18, 2024

simonpasquier left a comment

simonpasquier Jun 18, 2024

rexagod Jun 18, 2024

rexagod commented Jun 18, 2024 •

edited

Loading

simonpasquier Jun 25, 2024

rexagod Jun 25, 2024

rexagod Jun 25, 2024 •

edited

Loading

simonpasquier left a comment

saswatamcode left a comment

		@@ -25,6 +25,7 @@ We use breaking :warning: to mark changes that are not backward compatible (re

		### Changed

		- [#7429](https://github.com/thanos-io/thanos/pull/7429): Reloader: introduce `suppressEnvironmentVariablesExpansionErrors` to allow suppressing errors when expanding environment variables in the configuration file. When set, this will ensure that the reloader won't crash when an unset environment variable is encountered. Note that all unset environment variables are left as is, whereas all set environment variables are expanded as usual.

	suppressEnvironmentVariablesExpansionErrors bool
	discardEnvVarExpansionErrors bool

reloader: don't fail on envvar expansion errors #7429

reloader: don't fail on envvar expansion errors #7429

Conversation

rexagod commented Jun 12, 2024

Changes

Verification

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saswatamcode left a comment

Choose a reason for hiding this comment

simonpasquier commented Jun 13, 2024

Choose a reason for hiding this comment

rexagod commented Jun 13, 2024

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

rexagod commented Jun 14, 2024 • edited Loading

simonpasquier commented Jun 14, 2024

rexagod commented Jun 14, 2024

Choose a reason for hiding this comment

rexagod Jun 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonpasquier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rexagod commented Jun 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rexagod Jun 25, 2024 • edited Loading

Choose a reason for hiding this comment

simonpasquier left a comment

Choose a reason for hiding this comment

saswatamcode left a comment

Choose a reason for hiding this comment

rexagod commented Jun 14, 2024 •

edited

Loading

rexagod Jun 18, 2024 •

edited

Loading

rexagod commented Jun 18, 2024 •

edited

Loading

rexagod Jun 25, 2024 •

edited

Loading