-
Notifications
You must be signed in to change notification settings - Fork 30
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added Secret Manager support #208
Conversation
"github.com/aws/aws-sdk-go/service/secretsmanager" | ||
"github.com/aws/aws-sdk-go/aws" | ||
"github.com/aws/aws-sdk-go/aws/session" | ||
"encoding/base64" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there any sort of linting for import order? other projects separate out stdlib / elastic / 3rd party
@@ -62,6 +66,39 @@ func getIntFromEnv(name string) (int, error) { | |||
return value, nil | |||
} | |||
|
|||
func getSecret(secretName string) (string, error) { | |||
region := os.Getenv("AWS_REGION") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might want to check region != "" and return a descriptive error
secretString = *result.SecretString | ||
} else { | ||
decodedBinarySecretBytes := make([]byte, base64.StdEncoding.DecodedLen(len(result.SecretBinary))) | ||
len, err := base64.StdEncoding.Decode(decodedBinarySecretBytes, result.SecretBinary) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
len
is a builtin and, while the compiler might let you use it, it'd probably be better to choose a different variable name
if err != nil { | ||
return "", err | ||
} | ||
secretString = string(decodedBinarySecretBytes[:len]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
according to the docs, [Encoding.Decode] writes at most DecodedLen(len(src)) bytes to dst
, so the [:len]
shouldn't be necessary -- it should only differ if there's an error and there's a short write, in which case you'll return the error anyway.
result, err := getSecret(apmServerApiKeySMSecretId) | ||
if err != nil { | ||
Log.Fatalf("Failed loading APM Server ApiKey from Secrets Manager: %v", err) | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the else
can be dropped, and just executed regularly (the Fatalf
will abort the program if there's an error)
result, err := getSecret(apmServerSecretTokenSMSecretId) | ||
if err != nil { | ||
Log.Fatalf("Failed loading APM Server Secret Token from Secrets Manager: %v", err) | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above
@elastic/apm-server could you please help pushing this over the finish line; I suggest for this PR rather than only reviewing, commit small code improvements directly to free up @AlexanderWert. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, aside from the small test correction.
I was also able to get the PR to work end-to-end by following the repro steps.
if config.LogLevel != zapcore.InfoLevel { | ||
t.Log("Log level not set correctly") | ||
t.Fail() | ||
} | ||
} | ||
|
||
func TestGetSecretCalled(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that the Unit test, when run sequentially together with the whole test suite, fails due to previously set values of the SECRET_TOKEN
and the API_KEY
.
func TestGetSecretCalled(t *testing.T) { | |
func TestGetSecretCalled(t *testing.T) { | |
os.Clearenv() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the test actually to confirm that there's a preference between the managed and unmanaged secrets, since that wasn't being tested
Closes #105
Closes #206
Description
Added two new configuration options:
ELASTIC_APM_SECRETS_MANAGER_SECRET_TOKEN_ID
ELASTIC_APM_SECRETS_MANAGER_API_KEY_ID
If a secret manager id is provided through the above config options, the extension retrieves the corresponding secret from the AWS Secret manager. The secret must be in the same AWS region.
If both, a plain text secret is provided (i.e.
ELASTIC_APM_SECRET_TOKEN
) and theELASTIC_APM_SECRETS_MANAGER_SECRET_TOKEN_ID
, then the value from the Secret Manager takes precedence.Retrieving the secret from the Secret Manager happens once at the cold start of the extension.
Steps for end to end testing:
Step 1: Create a secret in the AWS Secret Manager.
Plain Text
aws/secretsmanager
(No additional key permissions needed for this)Step 2: Create and setup the Lambda function
ELASTIC_APM_SECRET_TOKEN
orELASTIC_APM_API_KEY
specify theELASTIC_APM_SECRETS_MANAGER_SECRET_TOKEN_ID
orELASTIC_APM_SECRETS_MANAGER_API_KEY_ID
variables. Use thesecret-name
as the value.Step 3: Add permissions
Configuration
>Permissions
>Execution Role
click on the execution role to edit the permissionsStep 4: Invoke the lambda function and validate data in Elastic APM UI