[extension/sigv4authextension] Initial implementation #8263

erichsueh3 · 2022-03-05T00:02:06Z

Description:
This is the initial implementation of the Sigv4 Authenticator Extension. See #7533 for more information.

Link to tracking Issue:
#7533

Testing:
Unit tests have been added.

Documentation:
Included in the PR is a README and a design document.

anuraaga · 2022-03-07T02:46:20Z

@erichsueh3 Can you squash the commits?

…nsion

extension/sigv4authextension/README.md

extension/sigv4authextension/config_test.go

extension/sigv4authextension/extension_test.go

anuraaga · 2022-03-07T02:54:34Z

extension/sigv4authextension/config_test.go

+		expectedErr error
+	}{
+		{
+			"bad_credentials",


IIUC the test case has a bad role name so it would be better to explicitly call out bad_role. But it is probably best to avoid making requests to AWS from unit tests - I'm not sure if this actually does (the role is completely misformatted so maybe handled by the SDK) so can you check? If it does, we should probably just skip having the role test.

Either way, make sure there is a test with a valid config but missing_credentials (no role, no env vars)

Looks like having a role means that a request to aws is in fact made, as it returns an error operation error STS: AssumeRole, failed to sign request: failed to retrieve credentials: no EC2 IMDS role found, operation error ec2imds: GetMetadata, http response error StatusCode: 403, request to EC2 IMDS failed; I think that we should remove the role test. As for a test with a valid config but missing_credentials with no role/environment variables, I've added in that test and it works as expected. However, it also seems to be making a request to aws (no EC2 IMDS role found, operation error ec2imds: GetMetadata, http response error StatusCode: 403, request to EC2 IMDS failed), so not sure if this is the right way to go about this test (this also applies to the missing creds test in TestGetCredsFromConfig()).

Also, how much value is there in having a test with a valid config but with missing credentials, when trying to test for errors from loading from the config? I think we could remove this whole TestLoadConfigError(), as Validate() will only error as a result of getCredsProviderFromConfig() now after making region/service optional.

If we have a test when creating the extension then it's not as important in the config, we'd want it in one or the other I think (ideally without the IMDS request if we can try my other suggestion #8263 (comment)).

I see, I think for now we can just keep this test in here then?

anuraaga · 2022-03-07T03:15:17Z

extension/sigv4authextension/extension.go

+		provider := stscreds.NewAssumeRoleProvider(stsSvc, cfg.RoleArn, func(o *stscreds.AssumeRoleOptions) {
+			o.RoleSessionName = "otel-" + strconv.FormatInt(time.Now().Unix(), 10)
+		})
+		creds, err := provider.Retrieve(context.Background())


We do want to call Retrieve once in this function to validate credentials eagerly, but what we want to return is provider or awscfg.Credentials, not the result of Retrieve. The result of retrieve is a never-again-refreshed credential, we want to call Retrieve every time we sign.

You'll also want to call NewCredentialsCache for this sts codepath

https://aws.github.io/aws-sdk-go-v2/docs/configuring-sdk/#specify-credentials-programmatically

All credential providers passed to or returned by LoadDefaultConfig are wrapped in a CredentialsCache automatically. This enables caching and concurrency safe credential access. If you explicitly configure a provider on aws.Config directly you must explicitly wrap the provider with this type using NewCredentialsCache.

Made the change to getCredsProviderFromConfig(), returning an *aws.CredentialsProvider instead; we still call Retrieve once for validation, and we also wrap it in NewCredentialsCache when we explicity configure a provider when we have a RoleARN

extension/sigv4authextension/extension.go

anuraaga · 2022-03-07T03:22:20Z

extension/sigv4authextension/extension_test.go

+
+func TestGetCredsFromConfig(t *testing.T) {
+	awsCreds := fetchMockCredentials()
+	t.Setenv("AWS_ACCESS_KEY_ID", awsCreds.AccessKeyID)


Think we shouldn't need env vars here since can set creds on cfg

Also let's add tests for missing creds

I've added back in environment variables into the test for a success case, since according to this guide, environment variables have the highest precedence for credentials (besides CLI options). Without it, we would look at the ~/.aws files, and then at container and instance profile credentials. If we don't set environment variables here, then it will eventually error when it reaches the end and tries to use instance profile credentials. I've added in a test for missing creds, and it errors here at the last step of precedence for credentials. It also makes a request to aws, which we want to avoid, so not sure if having this test is ok or not, or if there's another way to go about this test.

Thanks for looking into this - I guess it is hitting IMDS to try to fetch a cred for an EC2 machine. Since that IP will almost always be unresolvable it's a bit better than hitting the STS endpoint which is always available so maybe we can live with it. Could you try if disabling IMDS helps? Can follow up in another PR though

AWS_EC2_METADATA_DISABLED
https://github.com/aws/aws-sdk-go-v2/blob/c214cb61990441aa165e216a3f7e845c50d21939/config/load_options.go#L169

I tried disabling IMDS by adding awsconfig.WithEC2IMDSClientEnableState(imds.ClientDisabled), as a LoadOption to the LoadDefaultConfig() method call, but it doesn't change anything and we still get the same error of no EC2 IMDS role found, operation error ec2imds: GetMetadata, http response error StatusCode: 403, request to EC2 IMDS failed; I think its better to follow up with this in another PR as well

extension/sigv4authextension/factory_test.go

exporter/awsprometheusremotewriteexporter/README.md

CHANGELOG.md

exporter/awsprometheusremotewriteexporter/README.md

exporter/awsprometheusremotewriteexporter/factory.go

extension/sigv4authextension/signingroundtripper.go

extension/sigv4authextension/signingroundtripper_test.go

…oundTrip()

…subsequent changes

…g, missing cred test for getcredsproviderfromconfig, add to testroundtripper

erichsueh3 · 2022-03-10T23:40:06Z

Hi @jpkrohling, could you take a look at this PR as well? Would like some more eyes on it if possible

jpkrohling · 2022-03-11T18:02:05Z

Sure, I'll add this to my queue.

extension/sigv4authextension/config_test.go

jpkrohling · 2022-03-11T18:37:29Z

extension/sigv4authextension/design.md

+Next, we introduce `extension.go`, which contains the bulk of the implementation of the Sigv4 authenticator. This includes implementing the `ClientAuthenticator` interface and its necessary methods.
+
+
+`Sigv4Auth` is a struct that implements the `ClientAuthenticator` interface. It must implement two methods, `RoundTripper()` and `PerRPCCredentials()`. Additionally, it must also implement a `Start()` and a `Shutdown()` method. For our Sigv4 authenticator, both of these methods won’t do anything, so we instead embed `componenthelper.StartFunc` and `componenthelper.ShutdownFunc` to get that default behavior.


We are very close to merging open-telemetry/opentelemetry-collector#4837, which would make this a bit simpler. Keep an eye on it :-)

Hi @jpkrohling, would this just mean replacing componenthelper.StartFunc with component.StartFunc (and same for Stop)? Want to make sure I'm not misunderstanding anything before I make changes

No, you would just need to call the "NewClientAuthenticator(WithClientRoundTripper(yourfunc))" and you'd get a complete implementation without having to worry about the start/stop/per-RPC implementations.

Ok, understood; this is something that we'd need to wait until the next release to make the changes as well correct?

extension/sigv4authextension/design.md

jpkrohling · 2022-03-11T19:19:30Z

versions.yaml

@@ -80,6 +80,7 @@ module-sets:
      - github.com/open-telemetry/opentelemetry-collector-contrib/extension/observer/k8sobserver
      - github.com/open-telemetry/opentelemetry-collector-contrib/extension/oidcauthextension
      - github.com/open-telemetry/opentelemetry-collector-contrib/extension/pprofextension
+      - github.com/open-telemetry/opentelemetry-collector-contrib/extension/sigv4authextension


Don't forget to add this component to the -releases repo when ready.

anuraaga · 2022-03-12T01:55:46Z

extension/sigv4authextension/README.md

+    * [List of AWS regions](https://docs.aws.amazon.com/AmazonRDS/latest/UserGuide/Concepts.RegionsAndAvailabilityZones.html)
+* `service`: **Optional**. The AWS service for AWS Sigv4
+    * Note that an attempt will be made to obtain a valid service from the endpoint of the service you are exporting to
+* `role_session_name`: **Optional**. The name of a role session. If not provided, one will be constructed with a semi-random identifier.


Move this up to be right after role_arn

Also wondering if it should be

assume_role: arn: session_name:

since session_name doesn't make sense without arn

Yeah, pairing the ARN and session name like that sounds good to me.

Made the changes and now include an AssumeRole struct with an ARN and a SessionName

anuraaga · 2022-03-12T02:05:20Z

extension/sigv4authextension/extension.go

+
+		identifier := cfg.RoleSessionName
+		if identifier == "" {
+			b := make([]byte, 5)


Since we found the default SDK behavior let's be consistent with that, or remove the random logic, but we should avoid this divergent logic

Removed the random logic and use of the otel prefix, so now it will just default to the SDK behavior using nanosecond time.

…lector-contrib into sigv4auth

…second time

Aneurysm9

Minor note about service/region detection warning. Fix quickly if you can, but I think we can handle it separately if needed as well.

extension/sigv4authextension/signingroundtripper.go

erichsueh3 requested a review from a team March 5, 2022 00:02

erichsueh3 requested review from anuraaga, Aneurysm9 and alolita as code owners March 5, 2022 00:02

github-actions bot assigned dmitryax Mar 5, 2022

squash commit: initial implementation of the sigv4 authenticator exte…

b45d19c

…nsion

erichsueh3 force-pushed the sigv4auth branch from 3aba7c4 to b45d19c Compare March 8, 2022 00:50

anuraaga reviewed Mar 8, 2022

View reviewed changes

Aneurysm9 reviewed Mar 9, 2022

View reviewed changes

erichsueh3 added 6 commits March 9, 2022 01:22

README, CHANGELOG, deprecation notice changes

317b1c5

initial changes based on feedback

1892ce4

check config values in inferServiceAndRegion and simplified call in R…

3cf05da

…oundTrip()

change getCredsFromConfig() to getCredsProviderFromConfig() and made …

a6a92ba

…subsequent changes

missing cred test without role for config, remove role test for confi…

283e791

…g, missing cred test for getcredsproviderfromconfig, add to testroundtripper

added RoleSessionName config field, made documentation changes

69d9a10

jpkrohling approved these changes Mar 11, 2022

View reviewed changes

erichsueh3 added 2 commits March 11, 2022 16:02

design doc updates

717d6ba

fix merge conflict and add sigv4 ext to unreleased section of CHANGELOG

53d132a

anuraaga reviewed Mar 12, 2022

View reviewed changes

erichsueh3 added 3 commits March 14, 2022 13:11

Merge branch 'main' of https://github.com/open-o11y/opentelemetry-col…

eb14d35

…lector-contrib into sigv4auth

added AssumeRole config option struct + doc changes

27b3f9b

removed random identifier to be use SDK default identifier using nano…

d8ef292

…second time

anuraaga approved these changes Mar 15, 2022

View reviewed changes

Aneurysm9 approved these changes Mar 15, 2022

View reviewed changes

extension/sigv4authextension/signingroundtripper.go Outdated Show resolved Hide resolved

erichsueh3 added 2 commits March 15, 2022 13:53

fix service/region detection warning

b7328c2

go fmt

8f147e3

Aneurysm9 added the ready to merge Code review completed; ready to merge by maintainers label Mar 15, 2022

jpkrohling changed the title ~~New Component: Sigv4 Authenticator Extension~~ [extension/sigv4authextension] Initial implementation Mar 16, 2022

jpkrohling merged commit 88d0e28 into open-telemetry:main Mar 16, 2022

erichsueh3 deleted the sigv4auth branch March 16, 2022 19:09

erichsueh3 mentioned this pull request Mar 17, 2022

Add [extension/sigv4authextension] to ADOT Collector aws-observability/aws-otel-collector#1065

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[extension/sigv4authextension] Initial implementation #8263

[extension/sigv4authextension] Initial implementation #8263

erichsueh3 commented Mar 5, 2022

anuraaga commented Mar 7, 2022

anuraaga Mar 7, 2022

erichsueh3 Mar 10, 2022

erichsueh3 Mar 10, 2022

anuraaga Mar 11, 2022

erichsueh3 Mar 11, 2022

anuraaga Mar 7, 2022

erichsueh3 Mar 9, 2022

anuraaga Mar 7, 2022

anuraaga Mar 7, 2022

erichsueh3 Mar 10, 2022

anuraaga Mar 11, 2022

erichsueh3 Mar 11, 2022

erichsueh3 commented Mar 10, 2022

jpkrohling commented Mar 11, 2022

jpkrohling Mar 11, 2022

erichsueh3 Mar 12, 2022 •

edited

Loading

jpkrohling Mar 14, 2022

erichsueh3 Mar 14, 2022

jpkrohling Mar 11, 2022

anuraaga Mar 12, 2022

anuraaga Mar 12, 2022

Aneurysm9 Mar 14, 2022

erichsueh3 Mar 15, 2022

anuraaga Mar 12, 2022

erichsueh3 Mar 15, 2022

Aneurysm9 left a comment

		Next, we introduce `extension.go`, which contains the bulk of the implementation of the Sigv4 authenticator. This includes implementing the `ClientAuthenticator` interface and its necessary methods.


		`Sigv4Auth` is a struct that implements the `ClientAuthenticator` interface. It must implement two methods, `RoundTripper()` and `PerRPCCredentials()`. Additionally, it must also implement a `Start()` and a `Shutdown()` method. For our Sigv4 authenticator, both of these methods won’t do anything, so we instead embed `componenthelper.StartFunc` and `componenthelper.ShutdownFunc` to get that default behavior.

[extension/sigv4authextension] Initial implementation #8263

[extension/sigv4authextension] Initial implementation #8263

Conversation

erichsueh3 commented Mar 5, 2022

anuraaga commented Mar 7, 2022

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

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

erichsueh3 commented Mar 10, 2022

jpkrohling commented Mar 11, 2022

Choose a reason for hiding this comment

erichsueh3 Mar 12, 2022 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Aneurysm9 left a comment

Choose a reason for hiding this comment

erichsueh3 Mar 12, 2022 •

edited

Loading