feat: support sigv4 signing

[obanby]

• Added Tripper to enable signing requests
• Added Signer to implement the AWS sigv4 signing standard

I based my PR on this comment #161 (comment) and https://docs.aws.amazon.com/AmazonS3/latest/API/sig-v4-header-based-auth.html

Note: I don't have deep knowledge about the sigv4, so please provide as much feedback as possible, and I will be happy to address it :)

Addresses: #93

[CLAassistant]

All committers have signed the CLA.

[mike-sol]

This is great, enables easy export to Amazon Managed Prometheus. 👍

[mike-sol]

Can I buy someone a coffee / beer / etc to get this reviewed and merged?

[olegbespalov]

Hey @mike-sol !

The pull request is on our radar, but the team is busy with the upcoming K6 release. We'll return to it once we finish. Sorry for the delay!

[olegbespalov]

Hi @obanby!

Thanks for the PR, and sorry for the delay in reviewing!

First of all, I'd like to say that, like you, I don't have a deep knowledge of sigv4. That's why I checked the AWS Go SDK to familiarize myself with it and made some of my comments based on the logic that I found implemented there.

Another question is, have you tested these changes if they work?

[obanby]

Hi @obanby!

Thanks for the PR, and sorry for the delay in reviewing!

First of all, I'd like to say that, like you, I don't have a deep knowledge of sigv4. That's why I checked the AWS Go SDK to familiarize myself with it and made some of my comments based on the logic that I found implemented there.

Another question is, have you tested these changes if they work?

Thanks @olegbespalov, yes I have tested with our aws managed prometheus.

[joanlopez]

Hey @obanby, thanks for your contribution! 🙌🏻

I have reviewed this PR as well, and I don't have any other comments beyond what @olegbespalov has already mentioned. So, feel free to try to address them all and I'll be happy to give my 👍🏻 once ready!

[obanby]

Thanks @olegbespalov @joanlopez, I addressed all the comments.

One thing I noticed, but didn't know how to go about it is: erroring out if only partial config is passed in. Say a user sets the the SIGV4_REGION without setting the access key. Any ideas how to fail with an informative message?

[olegbespalov]

Hey @obanby

Thanks for the update; I'll go through it at the beginning of next week!

One thing I noticed, but didn't know how to go about it is: erroring out if only partial config is passed in. Say a user sets the the SIGV4_REGION without setting the access key. Any ideas how to fail with an informative message?

You could try collecting the validation errors in the slice, something like:

errs := make([]string, 0)

if len(strings.TrimSpace(config.Region)) == 0 {
	errs = append(errs, "sigV4 config `Region` must be set")
}

if len(strings.TrimSpace(config.AwsSecretAccessKey)) == 0 {
	errs = append(errs, "sigV4 config `AwsSecretAccessKey` must be set")
}

if len(strings.TrimSpace(config.AwsAccessKeyID)) == 0 {
	errs = append(errs, "sigV4 config `AwsAccessKeyID` must be set")
}

if len(errs) > 0 {
	return nil, errors.New("can't initialize a sigv4 round tripper:" + strings.Join(errs, ", "))
}

[obanby]

Thanks @olegbespalov, but to clarify. The current error handling should have caught that config.AwsSecretAccessKey is not set, and returns the error to the tripper client. However, that error doesn't correctly get propagated to console. So the user end up with a 403 unauthorized, because they didn't see any errors about sigv4. So my question is, how can this error propagate all the way up (preferably also short circuit the test)

[olegbespalov]

Right, then I believe we should add validation somewhere around RemoteConfig e.g. https://github.com/grafana/xk6-output-prometheus-remote/pull/169/files#diff-35f036a9d95de5edce2e31d56e65f748e31a64950ca6af842f241bf8dc01bf7fR126

sigvcfg := &sigv4.Config{
	Region:             conf.SigV4Region.String,
	AwsAccessKeyID:     conf.SigV4AccessKey.String,
	AwsSecretAccessKey: conf.SigV4SecretKey.String,
}

err := sigvcfg.Validate()
if err != nil {
    return nil, err
}

hc.SigV4 = sigvcfg

[obanby]

Sounds good, I will try that.

I will leave it till your review next week and address it then with any other comments you have :)

[olegbespalov]

Hey @obanby !

Just in case, I've reviewed the rest of the changes, and it looks good; maybe the only missing part is the validation that we discussed before to advise customers faster about misconfiguration.

So, thanks for implementing that!

[obanby]

Thanks @olegbespalov was just taking sometime to see how to go about it. My last commit I addressed the partial config issue, that's kinda what I was pointing out, see if you like that and we can go from there :)

[olegbespalov]

Looks good to me! 🚀 Thanks for the contribution! 🙇

Let's wait till @joanlopez will give a final look/feedback!

[obanby]

I addressed some of the issues flagged by the CI lint job.

@olegbespalov @joanlopez please have a look. I hope we can get this in soon :)

[obanby]

@olegbespalov @joanlopez, when will we be able to merge this please?

[olegbespalov]

Hey @obanby, you have my approval, but we still need the review from @joanlopez for merging.

In the meantime, could you please address comments from the linter? Most of them look like auto-fixes, so it's just a matter of running golangci-lint run --fix against the files. You could also validate if that helps by running the make lint command locally.

[obanby]

Thanks all, I have address the changes flagged by make lint.

There are two missing, one is not related to my changes, and the other will require adding another function to breeak down the method, but doesn't seem like the approach to take.

pkg/remotewrite/config.go:289    funlen  Function 'parseEnvs' is too long (84 > 80)
pkg/remotewrite/trend.go:117:21  gosec   G115: integer overflow conversion int -> uint16

[olegbespalov]

@obanby, no worries. We will take care of these linter issues. We'll merge this PR soon. Thanks again for your contribution! 🙇 🎉

[joanlopez]

Thank you @obanby for your contribution! 🙇🏻
Thanks @olegbespalov for driving this to getting it merged! 💟