Snowflake-Labs / terraform-snowflake-api-integration-with-geff-aws

Terraform module to create resources across the Snowflake and AWS providers and establish proper relationships within those resources.
https://registry.terraform.io/modules/Snowflake-Labs/api-integration-with-geff-aws/snowflake/latest
Apache License 2.0
23 stars 9 forks source link

Modify lambda to use ecr image #41

Closed sfc-gh-pkommini closed 2 years ago

sfc-gh-pkommini commented 3 years ago

Modify lambda to use ecr image

sfc-gh-afedorov commented 3 years ago

@sfc-gh-spanda could you take a look to make sure the code seems all correct and similar style as our internal repos? Let's try to keep design choices consistent if there are any.

This looks all good to me w.r.t. code, but let's review the high level goal here and what it adds for users of this repo. I see the value in deploying this on DockerHub but this seems to be pointing to an ECR image that is built separately. It seems that if we want to follow the threat model we use with SnowAlert we should point this repo to DockerHub, or if we want external folks to self-host the containers in ECR, we should provide a way for them to build the ECR container.

sfc-gh-spanda commented 3 years ago

@sfc-gh-spanda could you take a look to make sure the code seems all correct and similar style as our internal repos? Let's try to keep design choices consistent if there are any.

This looks all good to me w.r.t. code, but let's review the high level goal here and what it adds for users of this repo. I see the value in deploying this on DockerHub but this seems to be pointing to an ECR image that is built separately. It seems that if we want to follow the threat model we use with SnowAlert we should point this repo to DockerHub, or if we want external folks to self-host the containers in ECR, we should provide a way for them to build the ECR container.

@sfc-gh-afedorov : In that case, we may also include the dockerfile to build the geff lambda image and put it on the instructions.

sfc-gh-afedorov commented 3 years ago

I think the Dockerfile and a shell script are in the GEFF repo. Are we thinking we should have that repo publish to DockerHub, pypi, etc, and then other repos can depend on that one? That can make sense to me, but then we should make it clear in these docs that this is so.

sfc-gh-spanda commented 3 years ago

I think the Dockerfile and a shell script are in the GEFF repo. Are we thinking we should have that repo publish to DockerHub, pypi, etc, and then other repos can depend on that one? That can make sense to me, but then we should make it clear in these docs that this is so.

We can do that as well, point the geff repo to publish to dockerhub and update the instructions here to depend on the dockerhub image, that can be a choice and since the geff repo already has the link to this repo for the setup, self hosting the image can be an option too.

sfc-gh-pkommini commented 2 years ago

@sfc-gh-afedorov @sfc-gh-spanda To clarify, ECR push is not optional. Lambda relies on the ECR image_uri to run the container.

The dockerfile that specifies how to build the image and depends on the directory structure of the geff repo to copy stuff into the image and hence dockerfile must reside in the GEFF repo. Dockerfiles aren't generally stored in the terraform module. Building the docker image falls in the CI/CD side of the geff python code. The terraform module is more of the infrastructure side of things and I don't think it makes sense to mix the two.

CI/CD is meant to build the image, We can additionally have the same CI/CD push to dockerhub and have instructions in this repo's README.md on how users can pull from dockerhub and push to ECR. As of now I have ecr.sh in the GEFF repo which provides instructions for building the image from code and pushing to ECR. I can put that ecr.sh here in this repo but I can't move the Dockerfile here.

Let me know if that helps.