feat(remote/aws): support AWS Secrets Manager as remote component #718

hainenber · 2024-04-30T12:18:14Z

PR Description

Which issue(s) this PR fixes

Closes #689

Notes to the Reviewer

Should I poll the Secrets Manager API continuously to fetch latest secret. Pricing per API calls for Secrets Manager is much expensive compared to AWS SSM Parameter Store and AWS S3 API calls so I'm hesitant to implement poller. Would like to hear everyone thoughts about it.
I expose the inner component.Component in componenttest in order to verify the component's health status. Is this A-OK?

PR Checklist

CHANGELOG.md updated
Documentation added
Tests added

mattdurham · 2024-04-30T12:57:37Z

I think it would be nice if it could poll but set the default to not poll. Most of our pulling mechanisms support it so it would be odd if this one did not have that capability.

hainenber · 2024-04-30T14:18:27Z

Thanks Matt! I'll implement the poller and fix whatever makes the tests failed.

Maybe in the long run, we can have a reusable implementation. But that's for another day, I suppose :D

hainenber · 2024-05-01T15:38:54Z

internal/component/remote/s3/s3.go

@@ -106,11 +100,11 @@ func (s *Component) Run(ctx context.Context) error {
 func (s *Component) Update(args component.Arguments) error {
 	newArgs := args.(Arguments)

-	s3cfg, err := generateS3Config(newArgs)
+	awsCfg, err := aws_common_config.GenerateAWSConfig(newArgs.Options.Client)
 	if err != nil {
 		return nil


@rfratto pinging 'cause you authored this in the past. Do you know why this returns nil in the present of non-nil error? 👀

I think remote.s3 is originally from @mattdurham; do you remember if this is intentional? It does look a little like a typo to silently ignore the error here.

Ooof, I cant think of a good reason why I did that.

docs/sources/reference/components/remote.aws.secrets_manager.md

clayton-cornell · 2024-05-13T16:42:38Z

Docs look ok.

…tions used in remote AWS components Signed-off-by: hainenber <[email protected]>

Signed-off-by: hainenber <[email protected]>

…n test in Docker-present env Signed-off-by: hainenber <[email protected]>

Signed-off-by: hainenber <[email protected]>

…ial poller is configured Signed-off-by: hainenber <[email protected]>

Co-authored-by: Clayton Cornell <[email protected]>

Signed-off-by: hainenber <[email protected]>

clayton-cornell

Approving for docs. Still needs a code review fom @grafana/grafana-alloy-maintainers

Signed-off-by: hainenber <[email protected]>

rfratto · 2024-07-10T12:49:39Z

I think it would be nice if it could poll but set the default to not poll. Most of our pulling mechanisms support it so it would be odd if this one did not have that capability.

@mattdurham Can you help me understand setting the default to not poll for updates? Don't all of our components default to polling in the background?

rfratto

Nice! Few questions (for both you and @mattdurham) but this is coming along nicely :)

rfratto · 2024-07-10T12:45:48Z

internal/component/remote/s3/types.go

-	UsePathStyle  bool              `alloy:"use_path_style,attr,optional"`
-	Region        string            `alloy:"region,attr,optional"`
-	SigningRegion string            `alloy:"signing_region,attr,optional"`
+	aws_common_config.Client


Can we add a test for decoding this? I don't think alloy/syntax supports anonymous fields like this, and they have to be named like

type Client struct { CommonConfig aws_common_config.Client `alloy:",squash"` UsePathStyle bool `alloy:"use_path_style,attr,optional"` }

rfratto · 2024-07-10T12:48:09Z

internal/component/common/config/aws/aws.go

+			// The S3 compatible system used for testing with does not require signing region, so it's fine to be blank
+			// but when using a proxy to real S3 it needs to be injected.
+			return aws.Endpoint{
+				PartitionID:   "aws",


This wasn't set before in the old code; does this break anything for remote.s3?

rfratto · 2024-07-10T12:48:17Z

internal/component/common/config/aws/aws.go

+	if o.Region != "" {
+		configOptions = append(configOptions, aws_config.WithRegion(o.Region))
+	}


This wasn't set before in the old code; does this break anything for remote.s3?

rfratto · 2024-07-10T12:52:41Z

internal/component/remote/aws/secrets_manager/secrets_manager.go

+
+// DefaultArguments holds default settings for Arguments.
+var DefaultArguments = Arguments{
+	SecretVersion: "AWSCURRENT",
+}
+
+// SetToDefault implements syntax.Defaulter.
+func (a *Arguments) SetToDefault() {
+	*a = DefaultArguments
+}


We've started to move away from the pattern of having default arguments as a variable because in some cases (like a default map or slice) memory gets accidentally shared and the defaults get overridden.

Suggested change

// DefaultArguments holds default settings for Arguments.

var DefaultArguments = Arguments{

SecretVersion: "AWSCURRENT",

}

// SetToDefault implements syntax.Defaulter.

func (a *Arguments) SetToDefault() {

*a = DefaultArguments

}

// SetToDefault implements syntax.Defaulter.

func (a *Arguments) SetToDefault() {

*a = Arguments{

SecretVersion: "AWSCURRENT",

}

}

rfratto · 2024-07-10T12:55:14Z

internal/component/remote/aws/secrets_manager/secrets_manager.go

+	if s.pollFrequency > 0 {
+		go s.handleSecretUpdate(ctx)
+		go s.watcher.run(ctx)
+	}


There's a bug here: If the poll frequency is set to non-zero after the component was already constructed, then it will never poll.

rfratto · 2024-07-10T12:57:19Z

internal/component/remote/aws/secrets_manager/watcher.go

+	}
+}
+
+func (w *watcher) getSecretAsynchronously(ctx context.Context) {


nit: technically this method gets the secret synchronously. A more fitting name might be getAndSendSecret, as the method gets the secret and then writes the result to a channel.

rfratto · 2024-07-10T12:57:30Z

internal/component/remote/aws/secrets_manager/watcher.go

+	w.mut.Lock()
+	defer w.mut.Unlock()
+
+	res := w.getSecret(context.Background())


Is there a reason we're not propagating the context here?

mattdurham · 2024-07-10T13:22:30Z

My thought on the default to not polling is related to this comment Pricing per API calls for Secrets Manager is much expensive compared to AWS SSM Parameter Store and AWS S3 API. if it is expensive default to no but allow it to be poll. Also fine with the other direction, default to a low poll rate.

rfratto · 2024-07-10T14:00:23Z

Comparing the pricing of AWS Secrets Manager to S3 does show that secrets manager is more expensive, but perhaps not significantly:

AWS Secrets manager: $0.05 per 10,000 API calls
S3: $0.004 per 10,000 GET calls

It will take 200,000 polls of secrets for it to cost $1 USD. If we set the polling interval to hourly, and you had 100 Alloy instances all running the same component, that would still only be 73,000 polls per month ($0.36).

It seems like the cost is reasonable enough if we used a 1h polling interval by default. If you're running Alloy in an environment where you need AWS secrets manager (likely Enterprise-like), running Alloy itself probably costs orders of magnitude more money than the secrets manager bill would be.

WDYT @mattdurham?

hainenber requested a review from clayton-cornell as a code owner April 30, 2024 12:18

hainenber changed the title ~~Support aws secret manager as remote component~~ Support AWS Secrets Manager as remote component Apr 30, 2024

hainenber changed the title ~~Support AWS Secrets Manager as remote component~~ feat(remote/aws): support AWS Secrets Manager as remote component May 1, 2024

hainenber commented May 1, 2024

View reviewed changes

clayton-cornell reviewed May 2, 2024

View reviewed changes

clayton-cornell added the type/docs Docs Squad label across all Grafana Labs repos label May 2, 2024

rfratto added the backport-to-agent:no PR should NOT be backported to the agent repo. label May 3, 2024

clayton-cornell requested a review from a team May 13, 2024 16:42

hainenber force-pushed the support-aws-secret-manager-as-remote-component branch from 68ec808 to e453b57 Compare May 14, 2024 15:02

hainenber and others added 10 commits May 16, 2024 22:05

feat(comp/common/cfg/aws): separate package for common typse and func…

0613cec

…tions used in remote AWS components Signed-off-by: hainenber <[email protected]>

feat(comp/remote/s3): use util func and type for AWS client config

4043010

Signed-off-by: hainenber <[email protected]>

feat(comp/test): expose internal test component to check for its health

cbdb0e3

Signed-off-by: hainenber <[email protected]>

feat(comp/remote/aws/secrets_manager): add remote.aws.secrets_manager

7824d1d

Signed-off-by: hainenber <[email protected]>

fix(comp/remote/aws/secret_manager): correct go:build directive to ru…

5ccfc92

…n test in Docker-present env Signed-off-by: hainenber <[email protected]>

feat(remote/aws/secrets_manager): implement poller + update doc

631a2a2

Signed-off-by: hainenber <[email protected]>

fix(remote/aws/secrets_manager): only reset poller's ticker when init…

4a9dc80

…ial poller is configured Signed-off-by: hainenber <[email protected]>

Apply suggestions from Clayton's code review

21f1b2e

Co-authored-by: Clayton Cornell <[email protected]>

chore: clean up go.mod

1e2dcfc

Signed-off-by: hainenber <[email protected]>

chore: use renamed internal/runtime for component test

20a34d0

Signed-off-by: hainenber <[email protected]>

hainenber force-pushed the support-aws-secret-manager-as-remote-component branch from e453b57 to 20a34d0 Compare May 16, 2024 15:07

clayton-cornell approved these changes Jun 3, 2024

View reviewed changes

rfratto assigned mattdurham Jun 14, 2024

hainenber and others added 2 commits July 9, 2024 20:42

Merge branch 'main' into support-aws-secret-manager-as-remote-component

a417e68

chore(build): reconcile go.sum

48fd080

Signed-off-by: hainenber <[email protected]>

rfratto reviewed Jul 10, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(remote/aws): support AWS Secrets Manager as remote component #718

feat(remote/aws): support AWS Secrets Manager as remote component #718

hainenber commented Apr 30, 2024 •

edited

Loading

mattdurham commented Apr 30, 2024

hainenber commented Apr 30, 2024

hainenber May 1, 2024

rfratto Jul 10, 2024

mattdurham Jul 10, 2024

clayton-cornell commented May 13, 2024

clayton-cornell left a comment

rfratto commented Jul 10, 2024

rfratto left a comment

rfratto Jul 10, 2024

rfratto Jul 10, 2024

rfratto Jul 10, 2024

rfratto Jul 10, 2024

rfratto Jul 10, 2024

rfratto Jul 10, 2024

rfratto Jul 10, 2024

mattdurham commented Jul 10, 2024

rfratto commented Jul 10, 2024

feat(remote/aws): support AWS Secrets Manager as remote component #718

Are you sure you want to change the base?

feat(remote/aws): support AWS Secrets Manager as remote component #718

Conversation

hainenber commented Apr 30, 2024 • edited Loading

PR Description

Which issue(s) this PR fixes

Notes to the Reviewer

PR Checklist

mattdurham commented Apr 30, 2024

hainenber commented Apr 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clayton-cornell commented May 13, 2024

clayton-cornell left a comment

Choose a reason for hiding this comment

rfratto commented Jul 10, 2024

rfratto 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattdurham commented Jul 10, 2024

rfratto commented Jul 10, 2024

hainenber commented Apr 30, 2024 •

edited

Loading