Skip to content
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

Allow fetching configs from remote sources (s3, http) #33

Merged
merged 6 commits into from
Nov 2, 2018

Conversation

julienduchesne
Copy link
Contributor

No description provided.

@coveralls
Copy link

coveralls commented Oct 30, 2018

Pull Request Test Coverage Report for Build 637

  • 27 of 69 (39.13%) changed or added relevant lines in 2 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage decreased (-0.4%) to 14.948%

Changes Missing Coverage Covered Lines Changed/Added Lines %
main.go 0 1 0.0%
config.go 27 68 39.71%
Files with Coverage Reduction New Missed Lines %
config.go 1 45.76%
Totals Coverage Status
Change from base Build 631: -0.4%
Covered Lines: 129
Relevant Lines: 863

💛 - Coveralls

jocgir
jocgir previously approved these changes Oct 31, 2018
Copy link
Contributor

@jocgir jocgir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is correct. But in an ideal world, I would like something more generic (not AWS SM specific).

Like:

config-location: arn:aws:secretsmanager:region:account-id:
or
config-location: arn:aws:ssm:region:account-id:document/tgf/config
or
config-location: /shared-drive/path/tgf-config/
or even
config-location: https://github.com/company/tgf-config/

combined with:

config-default: tgf-config
config-path: ops-config:infra-config:test-config

@julienduchesne
Copy link
Contributor Author

I like using config-location instead of secrets-manager-id but I think the config location should be entered completely everytime. This allows to fetch configs from more than one source. Example: We want to store our main config in secretsmanager for reliability but we may want to store additional scripts in bitbucket where it is easier to modify but we don't care if the fetch fails.

@jocgir
Copy link
Contributor

jocgir commented Oct 31, 2018

I understand your point, but the idea here to have a location is to be able to also fetch other configurations and be able to override the default config locally without having to replace the whole string.

example, we have:
default initialization:
config-default = "tgf-config"

in parameter store:
config-location = "arn:aws:secretsmanager:region:account-id:"

We then fetch config-location+config-default = arn:aws:secretsmanager:region:account-id:tgf-config

If I want to override the config-default in my test, I simply have to override config-default in a local file:
config-default = mytest

Then the system will try to fetch confguration from arn:aws:secretsmanager:region:account-id:mytest instead.

Or I can also specify:
config-path = "ops-config:infra-config

Then the system will try to to fetch:
arn:aws:secretsmanager:region:account-id:tgf-config
and
arn:aws:secretsmanager:region:account-id:infra-config
and
arn:aws:secretsmanager:region:account-id:ops-config

I voluntary inverted the two last paths to ensure that the first config declared in config-path has precedence.

Also, in the future, we may add support for other config-location such as shared folders, etc. The same mechanism will still work whatever is the chosen config-location

@julienduchesne
Copy link
Contributor Author

julienduchesne commented Oct 31, 2018

Alright, it just seems like a weird config. It works now because the default-config is to be appended to the config location. But what if your configs are bitbucket.org/infrarepo/tgf-config and bitbucket.org/opsrepo/tgf-config. Also, we currently can't use tgf-config in our own account because there seems to be a bug with permissions (probably the same one you got). I renamed to TGFConfig and now it works. Using a two part configuration location, we'll have to set two parameters in SSM.

I'm just giving scenarios where saving ourselves from writing arn:aws:secretsmanager:region:account-id: two or three times will bite us.

@jocgir
Copy link
Contributor

jocgir commented Oct 31, 2018

I would say that if the user specify a full-length config, then we should simply consider it as an absolute path vs a relative path.

The idea here is not having our users to know the account number of the base path if they simply want to add override or extra configuration (path).

Regarding the bug. I had this problem because I did not store back the secret value after changing the KMS key. I simply have to overwrite the value to make it works.

@julienduchesne
Copy link
Contributor Author

julienduchesne commented Oct 31, 2018

It's not the same then. I think my issue stems from

Note

If you specify an ARN, we generally recommend that you specify a complete ARN. You can specify a partial ARN too—for example, if you don’t include the final hyphen and six random characters that Secrets Manager adds at the end of the ARN when you created the secret. A partial ARN match can work as long as it uniquely matches only one secret. However, if your secret has a name that ends in a hyphen followed by six characters (before Secrets Manager adds the hyphen and six characters to the ARN) and you try to use that as a partial ARN, then those characters cause Secrets Manager to assume that you’re specifying a complete ARN. This confusion can cause unexpected results. To avoid this situation, we recommend that you don’t create secret names that end with a hyphen followed by six characters.

Here: https://docs.aws.amazon.com/secretsmanager/latest/apireference/API_GetSecretValue.html

I have a new tgf-config but when I fetch it (without the full ID ex: tgf-config-dazyWX), it doesn't work because it must have been saved with a wrong permission initially.

$ aws secretsmanager get-secret-value --secret-id arn:aws:secretsmanager:us-east-1:account:secret:tgf-config-QDv2vm
{
    "Name": "tgf-config",
    "VersionId": "16206412-4E91-4CFA-A748-70C5F304A5D8",
    "SecretString": "TO_REPLACE",
    "VersionStages": [
        "AWSCURRENT"
    ],
    "CreatedDate": 1540904807.861,
    "ARN": "arn:aws:secretsmanager:us-east-1:account:secret:tgf-config-QDv2vm"
}

$ aws secretsmanager get-secret-value --secret-id arn:aws:secretsmanager:us-east-1:account:secret:tgf-config

An error occurred (AccessDeniedException) when calling the GetSecretValue operation: User: arn:aws:sts::064790157154:assumed-role/user-role-ops-specialist/botocore-session-1540986089 is not authorized to perform: secretsmanager:GetSecretValue on resource: arn:aws:secretsmanager:us-east-1:account:secret:tgf-config-8Gnbry

@julienduchesne julienduchesne changed the title Allow fetching secretsmanager from another account Allow fetching configs from remote sources (s3, http) Nov 1, 2018
@julienduchesne
Copy link
Contributor Author

Ready for review @jocgir

config.go Outdated
configPaths = strings.Split(configPathString, ":")
}

aws_helper.InitAwsSession("")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The remote configuration could be elsewhere than AWS. We should init the AWS Session until it is absolutely required.

@julienduchesne julienduchesne merged commit cea99d2 into master Nov 2, 2018
@julienduchesne julienduchesne deleted the secrets-manager-acount branch November 2, 2018 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants