Add support for secret refs via a secret manager by TheSpiritXIII · Pull Request #572 · prometheus/common

TheSpiritXIII · 2024-02-02T14:21:18Z

Part 2 for secret provider support within common. See: https://groups.google.com/g/prometheus-developers/c/WKOej_pnhXg

Part 1 is here: #538

bwplotka

Amazing! Generally LGTM, but lots of minor suggestions.

Disclaimer: @TheSpiritXIII is in my Google team. I tried to get more hands on this review for weeks, but no response. The secret provider feature was generally accepted 1, 2, so allowing myself to move forward after the careful review

config/http_config.go

bwplotka

LGTM, just some nits and one potentially blocking item (did you meant to make TLSConfigOption withSecretManager private?)

config/http_config.go

bwplotka · 2024-05-14T17:37:51Z

config/http_config.go

+type TLSConfigOption func(options *tlsConfigOptions)
+
+// WithSecretManager allows setting the secret manager.
+func withSecretManager(manager SecretManager) TLSConfigOption {


Shouldn't this be:

Suggested change

func withSecretManager(manager SecretManager) TLSConfigOption {

func WithSecretManagerTLSConfigOption(manager SecretManager) TLSConfigOption {

(for dedup and to be exported)?

We can do this with a common object that implements both interfaces. What do you think of the new solution?

Whatever is simpler here

config/http_config.go

bwplotka · 2024-05-14T17:42:49Z

config/http_config.go

+		if !rt.clientSecret.immutable() {
+			rt.mtx.RLock()
+			changed = secret != rt.lastSecret
+			rt.mtx.RUnlock()
+		}
 	}
-	rt.mtx.RLock()
-	changed = secret != rt.secret
-	rt.mtx.RUnlock()

 	if changed || rt.rt == nil {


Would it be harmful if we kill this if (827 line) and do the changed = ... even on rt.rt == nil case? Bit simpler code and then we can remove the || rt.rt == nil part from if 879 (also simpler). WDYT?

Is this what you mean?

Signed-off-by: Daniel Hrabovcak <thespiritxiii@gmail.com>

bwplotka · 2024-05-15T11:59:33Z

config/http_config.go

-		if err != nil {
-			return nil, fmt.Errorf("unable to read oauth2 client secret: %w", err)
+	// Fetch the secret if it's our first run or always if the secret can change.
+	if rt.rt == nil || (rt.clientSecret != nil && !rt.clientSecret.immutable()) {


This not ended up better (or worse) after my suggestions -- might be my misunderstanding, so I ended up prototyping how it might look, proposed in TheSpiritXIII#1

I also fixed a small race with reading rt.rt 🤔I think this race was before too.

No regression and I only fix that & refactor in my PR, so let's merge this one and review my PR if there is a need (

common/config/http_config.go

Line 759 in 58f58f8

if changed || rt.rt == nil {

)

bwplotka

Let's merge! thanks!

bwplotka · 2024-05-15T13:33:07Z

config/http_config.go

-		if err != nil {
-			return nil, fmt.Errorf("unable to read oauth2 client secret: %w", err)
+	// Fetch the secret if it's our first run or always if the secret can change.
+	if rt.rt == nil || (rt.clientSecret != nil && !rt.clientSecret.immutable()) {


No regression and I only fix that & refactor in my PR, so let's merge this one and review my PR if there is a need (

common/config/http_config.go

Line 759 in 58f58f8

if changed || rt.rt == nil {

)

TheSpiritXIII changed the title ~~Use common interface to fetch secrets in HTTP client config~~ Add support for secret refs via a secret manager Feb 2, 2024

TheSpiritXIII mentioned this pull request Feb 2, 2024

Native Kubernetes secrets for authorization GoogleCloudPlatform/prometheus#130

Closed

TheSpiritXIII force-pushed the http-secret-ref branch from 106011c to 88d7263 Compare February 12, 2024 20:05

TheSpiritXIII mentioned this pull request Feb 12, 2024

Use common interface to fetch secrets in HTTP client config #538

Merged

TheSpiritXIII force-pushed the http-secret-ref branch from 88d7263 to 7fd0a65 Compare April 18, 2024 19:54

TheSpiritXIII marked this pull request as ready for review April 18, 2024 20:02

TheSpiritXIII mentioned this pull request Apr 18, 2024

Add secret provider interface under feature flag prometheus/prometheus#13955

Open

TheSpiritXIII force-pushed the http-secret-ref branch from 7fd0a65 to f3006cd Compare April 19, 2024 18:56

TheSpiritXIII force-pushed the http-secret-ref branch from f3006cd to 5033109 Compare May 9, 2024 13:29

bwplotka approved these changes May 9, 2024

View reviewed changes

bwplotka reviewed May 9, 2024

View reviewed changes

config/http_config.go Show resolved Hide resolved

TheSpiritXIII force-pushed the http-secret-ref branch 2 times, most recently from 1cfae07 to 6ff78c6 Compare May 14, 2024 16:23

TheSpiritXIII requested a review from bwplotka May 14, 2024 16:24

TheSpiritXIII force-pushed the http-secret-ref branch from 6ff78c6 to bdd4ba7 Compare May 14, 2024 17:40

bwplotka approved these changes May 14, 2024

View reviewed changes

TheSpiritXIII force-pushed the http-secret-ref branch from bdd4ba7 to 97ce796 Compare May 14, 2024 18:42

TheSpiritXIII requested a review from bwplotka May 14, 2024 18:45

Add support for secret refs

0e50973

Signed-off-by: Daniel Hrabovcak <thespiritxiii@gmail.com>

TheSpiritXIII force-pushed the http-secret-ref branch from 97ce796 to 0e50973 Compare May 14, 2024 18:49

bwplotka reviewed May 15, 2024

View reviewed changes

bwplotka approved these changes May 15, 2024

View reviewed changes

bwplotka merged commit a7407da into prometheus:main May 15, 2024

This was referenced Jun 7, 2024

Expose secret as SecretReader and InlineSecret from config package #650

Merged

Update vendored mimir-prometheus grafana/mimir#8305

Merged

	func withSecretManager(manager SecretManager) TLSConfigOption {
	func WithSecretManagerTLSConfigOption(manager SecretManager) TLSConfigOption {

Conversation

TheSpiritXIII commented Feb 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bwplotka left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bwplotka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bwplotka May 14, 2024

Choose a reason for hiding this comment

Uh oh!

TheSpiritXIII May 14, 2024

Choose a reason for hiding this comment

Uh oh!

bwplotka May 15, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bwplotka May 14, 2024

Choose a reason for hiding this comment

Uh oh!

TheSpiritXIII May 14, 2024

Choose a reason for hiding this comment

Uh oh!

bwplotka May 15, 2024

Choose a reason for hiding this comment

Uh oh!

bwplotka May 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bwplotka left a comment

Choose a reason for hiding this comment

Uh oh!

bwplotka May 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

TheSpiritXIII commented Feb 2, 2024 •

edited

Loading

bwplotka left a comment •

edited

Loading

bwplotka May 15, 2024 •

edited

Loading

bwplotka May 15, 2024 •

edited

Loading