Skip to content

Comments

Changed creation of AWS Session object#438

Merged
rusenask merged 2 commits intokeel-hq:masterfrom
spaghettifunk:aws_iam_role
Sep 4, 2019
Merged

Changed creation of AWS Session object#438
rusenask merged 2 commits intokeel-hq:masterfrom
spaghettifunk:aws_iam_role

Conversation

@spaghettifunk
Copy link
Contributor

@rusenask I see some potential issues in the design of the https://github.com/keel-hq/keel/tree/master/extension/credentialshelper/aws. The major issue is in the fact that the method GetCredentials(...) has a dependency on the aws.Session object. By instantiating this object inside this method, I cannot test it locally because I cannot set up the region and endpoint (for accessing a local service such as moto or localstack). I tried few ways but it's either very ugly or I break the CredentialsHelper interface.

What I am not happy with, is the fact that I cannot make a proper unit-test without either make it very ugly or breaking the interface. Using a bit more Dependency Injection for this type of issue would benefit the code base I believe.

I'm 100% sure that you (or whoever) had very good reasons to design it that way but I would be happy to contribute in re-thinking a little bit the design of the interface 😄

Anyway, I tested this version in our production cluster. I post here also the policy we have to use to make it work properly

{
    "Version": "2012-10-17",
    "Statement": [
        {
            "Effect": "Allow",
            "Action": [
                "ecr:*",
                "ec2:*"
            ],
            "Resource": "*"
        }
    ]
}

For some reasons, without the ec2:* it doesn't work at all. There is something in the ECR aws-sdk-go (I haven't investigate due to lack of time) that has a dependency on EC2 policy. I tried many different policies and this one is the only one that works.

It solves #425

@rusenask
Copy link
Collaborator

rusenask commented Sep 3, 2019

Hi, I agree that it's not a nice interface to test against. Maybe https://github.com/keel-hq/keel/blob/master/extension/credentialshelper/aws/aws.go#L31 could be moved into https://github.com/keel-hq/keel/blob/master/cmd/keel/main.go package where it's assembled and registered?

There was no good reason to do it either :) I am not using aws personally so can't do much testing either on it.

@spaghettifunk
Copy link
Contributor Author

I give it a go, otherwise do you think we can merge it as it is? If not, I will try to re-define the way credentials are passed and see if I can manage to have a better local tests 😄

@rusenask rusenask merged commit 2e60104 into keel-hq:master Sep 4, 2019
@rusenask
Copy link
Collaborator

rusenask commented Sep 4, 2019

Could you please also create a PR for https://github.com/keel-hq/keel.sh/tree/master/docs#polling-with-aws-ecr with some help on how to configure AWS (probably that policy JSON)

@jcardoso-bv jcardoso-bv mentioned this pull request Sep 10, 2019
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.

2 participants