Use temporary credentials to access S3 bucket #18
Conversation
…ibly EC2 as well)
|
Thank you so much for this contribution. As I dive into the code review, based on a quick glance of the PR, there are three items that I would like to include in this change. I can do my best to fill them in or if you (@FlorianSW) can add them it would be much appreciated.
|
|
@dekobon You're welcome to change whatever you think is needed :) Regarding the reference to To the setup with EC2: That should be the same as with any other VM setup. The EC2 instance should only get a role associated to it. How to do that is explained in the AWS docs: https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/iam-roles-for-amazon-ec2.html#launch-instance-with-role I never really worked with EC2 instance (apart from a bit here and there), so I don't really know much about that without doing it once. But it should be straight forward tbh. For the ECS part: I thought about that already and wanted to add the CloudFormation template I used to test the ECS integration. I obviously forgot about that in the PR 🤣 Let me add the template to the original PR text. With it, it is as easy as pushing a built image to an ECR and creating a stack with it. I'm not sure if that should be part of the repo? |
|
Regarding item 1, yeah I was thinking primarily for readability. If it ever needs configuring we can work on that when the time comes. |
Got it, let me change that and add a new commit to the PR 👍
I added the CloudFormation template I used to test the ECS feature (in a zip, as GH does not support to add yml directly...). This will setup all the required things like an ECS service + task and a load balancer (which is maybe not needed if you attach a public IP to the task itself, so there might be room to simplify this stack massively). For the EC2 part, I don't have the possibility right now to check that out, really. I can only imagine what to do, but not actually try it out :/ If others are using EC2 instances, that would be a massive help here (testing and maybe even an example CloudFormation template or something). |
|
I just reviewed the code for the stand-alone systemd example install and it looks like that will need to be updated as well. I'm thinking we will make a separate ticket for it and do it post-merge. |
Fixed that in the last commit, as well as the extraction of the IP addresses 👍 |
test/unit/s3gateway_test.js
Outdated
| return Promise.resolve('A_TOKEN'); | ||
| }, | ||
| }); | ||
| } else if (url === 'http://169.254.169.254/latest/meta-data/iam/security-credentials/s3access') { |
There was a problem hiding this comment.
I'm still debugging why the unit tests are segfaulting. I hope to have an update soon.
However, I have a question about this line. I don't see this exact URL used in s3gateway.js. I see the URL: http://169.254.169.254/latest/meta-data/iam/security-credentials/ without the trailing string s3access. Is this matching the value that you are expecting?
There was a problem hiding this comment.
Ah, good catch! That was a copy-paste from one of my first commits. This line basically only works, if the role attached to the EC2 instance is named "s3access", which is usually not the case. I'll change that 👍
| r.log = function(msg) { | ||
| console.log(msg); | ||
| } | ||
|
|
There was a problem hiding this comment.
Here we will need to add an additional function to the r instance when running as a unit test in order to avoid an exception (which is what is causing the segfault - a patch will be included in the next njs release):
r.return = function(code, body) {
console.log(`RESPONSE RETURN: ${code} ${body}`);
}There was a problem hiding this comment.
I will add the function, but I don't think it is valuable to log the whole response, isn't it? I might simply add an "assertion" that the correct code is returned, ok? :)
There was a problem hiding this comment.
Great! That dealt with one of the two segfaults that were happening. Using an assertion here makes a lot of sense.
| secretAccessKey: process.env['S3_SECRET_KEY'], | ||
| sessionToken: null, | ||
| }; | ||
| } else if (process.env['AWS_CONTAINER_CREDENTIALS_RELATIVE_URI']) { |
There was a problem hiding this comment.
For some reason, this fails to match in the unit tests. I'll work some more on debugging.
There was a problem hiding this comment.
The problem is that process.env cannot be modified at runtime. We will need to refactor to allow for this value to be set by the unit tests.
There was a problem hiding this comment.
Hmm, you sure? I ran the tests once (it is a bit difficult for me to run the tests except in the CI tbh :(), and they worked. During the patches, I changed, however, the env variable back to the one injected by AWS (AWS_CONTAINER_CREDENTIALS_RELATIVE_URI), the unit tests still sets the non-default one (AWS_CONTAINER_CREDENTIALS_ABSOLUTE_URI), I'll change that and then this should work (at least on my newbie understanding, you might still be right 🙈)
There was a problem hiding this comment.
Sadly, I'm not that sure. I did some experiments in njs with trying to add in new values to process.env and found that I couldn't do it. How did you run the unit tests? I'm running them by executing:
/usr/bin/njs -t module -p common/etc/nginx test/unit/s3gateway_test.js
If you run test.sh it will also execute the unit tests in addition to the integration tests.
There was a problem hiding this comment.
I did another simpler test:
process.env['FOO'] = 'bar';
console.log(process.env['FOO']);$ njs process_env.js
bar
In this case it looks like it is successfully modifying the environment variables. I'm going to dive in a bit deeper.
There was a problem hiding this comment.
I used test.sh. Let me try to run them again (in both ways). However, I use a windows environment (together with cygwin) 🤦 which made things a bit harder iirc.
There was a problem hiding this comment.
I just run the tests (after I fixed some more issues with mocking a Response promise 🙈) and they run fine now (the unit tests, in a docker container) :)
| if [ -z "${S3_DEBUG}" ]; then | ||
| S3_DEBUG=false | ||
| fi | ||
| source ../../common/check_env.sh |
There was a problem hiding this comment.
This script will need more work than just checking for the variables. We will need to modify how line 73 writes out environment variables.
There was a problem hiding this comment.
Am I missing something in the diff? I don't see where the parts I touched modifies env variables 🤔
There was a problem hiding this comment.
My apologies, I was trying to say that there is a fair bit more work to do here in addition to the changes already done.
|
I just discovered that in order to use the AWS metadata endpoint from within a container running in EC2, you need to run the following command to allow for additional network hops from your EC2 instance: We will want to add a note about that in the documentation. |
|
A few updates on the current progress:
All in all, we are pretty close. If anyone has tips on deploying with ECS, it would be much appreciated. |
|
Sorry for not being that active lately 🙈
I'll add some docs and push it to this PR (also updating to issue/8).
What is this about? 🤔 |
I have an AWS CloudFormation file that I can adapt and share for purposes of testing. I will post it shortly! |
|
Ok, here is the CloudFormation file and usage instructions. Hopefully this helps with testing. Let me know if you guys run into any issues with this. CloudFormation FileAWSTemplateFormatVersion: '2010-09-09'
Parameters:
NewBucketName:
Default: <ENTER_RANDOM_BUCKET_NAME_HERE>
Description: S3 Bucket Name
Type: String
Subnet1:
Default: <ENTER_YOUR_VALUE_HERE>
Description: ID of the first subnet to be used for resources
Type: String
Subnet2:
Default: <ENTER_YOUR_VALUE_HERE>
Description: ID of the second subnet to be used for resources
Type: String
VpcId:
Default: <ENTER_YOUR_VALUE_HERE>
Description: ID of the VPC to be used for resources
Type: String
ContainerName:
Default: s3gateway
Description: Name of the NGINX Container. No need to change this
Type: String
ResourceNamePrefix:
Default: nginx-s3-gateway
Description: Common prefix used for resource names. No need to change this
Type: String
Outputs:
PublicDNS:
Description: DNS name of load balancer
Value: !GetAtt 'ALB.DNSName'
Resources:
ALB:
Properties:
Name: !Join
- '-'
- - !Ref 'ResourceNamePrefix'
- alb
SecurityGroups:
- !GetAtt 'S3GatewaySG.GroupId'
SubnetMappings:
- SubnetId: !Ref 'Subnet1'
- SubnetId: !Ref 'Subnet2'
Type: application
Type: AWS::ElasticLoadBalancingV2::LoadBalancer
ALBHttpListener:
Properties:
DefaultActions:
- ForwardConfig:
TargetGroups:
- TargetGroupArn: !Ref 'ALBTargetGroup'
Type: forward
LoadBalancerArn: !Ref 'ALB'
Port: 80
Protocol: HTTP
Type: AWS::ElasticLoadBalancingV2::Listener
ALBTargetGroup:
Properties:
Name: !Join
- '-'
- - !Ref 'ResourceNamePrefix'
- log-group
Port: 80
Protocol: HTTP
ProtocolVersion: HTTP1
TargetGroupAttributes:
- Key: deregistration_delay.timeout_seconds
Value: '150'
TargetType: ip
VpcId: !Ref 'VpcId'
Type: AWS::ElasticLoadBalancingV2::TargetGroup
Cluster:
Properties:
ClusterName: !Join
- '-'
- - !Ref 'ResourceNamePrefix'
- cluster
Type: AWS::ECS::Cluster
ECSLogGroup:
Properties:
LogGroupName: !Join
- '-'
- - !Ref 'ResourceNamePrefix'
- logs
RetentionInDays: 14
Type: AWS::Logs::LogGroup
ECSTaskExecutionPolicy:
Properties:
PolicyDocument:
Statement:
- Action:
- logs:CreateLogStream
- logs:CreateLogGroup
- logs:PutLogEvents
Effect: Allow
Resource:
- '*'
Version: '2012-10-17'
PolicyName: !Join
- '-'
- - !Ref 'ResourceNamePrefix'
- ecs-task-execution-policy
Roles:
- !Ref 'ECSTaskExecutionRole'
Type: AWS::IAM::Policy
ECSTaskExecutionRole:
Properties:
AssumeRolePolicyDocument:
Statement:
- Action:
- sts:AssumeRole
Effect: Allow
Principal:
Service:
- ecs-tasks.amazonaws.com
Version: '2012-10-17'
Description: An IAM role to enable ECS agents to perform AWS operations such as creating CloudWatch logs.
RoleName: !Join
- '-'
- - !Ref 'ResourceNamePrefix'
- ecs-task-execution-role
Type: AWS::IAM::Role
ECSTaskPolicy:
Properties:
PolicyDocument:
Statement:
- Action:
- s3:GetObject
- s3:ListBucket
Effect: Allow
Resource:
- !Sub
- arn:aws:s3:::${bucketName}/*
- bucketName: !Ref 'NewBucketName'
- !Sub
- arn:aws:s3:::${bucketName}
- bucketName: !Ref 'NewBucketName'
Version: '2012-10-17'
PolicyName: !Join
- '-'
- - !Ref 'ResourceNamePrefix'
- ecs-task-policy
Roles:
- !Ref 'ECSTaskRole'
Type: AWS::IAM::Policy
ECSTaskRole:
Properties:
AssumeRolePolicyDocument:
Statement:
- Action:
- sts:AssumeRole
Effect: Allow
Principal:
Service:
- ecs-tasks.amazonaws.com
Version: '2012-10-17'
Description: An IAM role to enable ECS containers to perform AWS operations such as accessing S3 buckets.
RoleName: !Join
- '-'
- - !Ref 'ResourceNamePrefix'
- ecs-task-role
Type: AWS::IAM::Role
S3Bucket:
Properties:
BucketName: !Ref 'NewBucketName'
Type: AWS::S3::Bucket
S3GatewaySG:
Properties:
GroupDescription: Security group for NGINX S3 Gateway Infra
GroupName: !Join
- '-'
- - !Ref 'ResourceNamePrefix'
- sg
SecurityGroupEgress:
- CidrIp: '0.0.0.0/0'
Description: Allow all outbound IPv4 traffic
IpProtocol: '-1'
- CidrIpv6: ::/0
Description: Allow all outbound IPv6 traffic
IpProtocol: '-1'
SecurityGroupIngress:
- CidrIp: '0.0.0.0/0'
Description: Allow HTTP Traffic on 80
FromPort: 80
IpProtocol: tcp
ToPort: 80
- CidrIp: '0.0.0.0/0'
Description: Allow HTTPS Traffic on 443
FromPort: 443
IpProtocol: tcp
ToPort: 443
VpcId: !Ref 'VpcId'
Type: AWS::EC2::SecurityGroup
S3GatewayService:
DependsOn: ALBHttpListener
Properties:
Cluster: !Ref 'Cluster'
DesiredCount: 1
LaunchType: FARGATE
LoadBalancers:
- ContainerName: !Ref 'ContainerName'
ContainerPort: 80
TargetGroupArn: !Ref 'ALBTargetGroup'
NetworkConfiguration:
AwsvpcConfiguration:
AssignPublicIp: ENABLED
SecurityGroups:
- !GetAtt 'S3GatewaySG.GroupId'
Subnets:
- !Ref 'Subnet1'
- !Ref 'Subnet2'
ServiceName: !Join
- '-'
- - !Ref 'ResourceNamePrefix'
- service
TaskDefinition: !Ref 'TaskDefinition'
Type: AWS::ECS::Service
TaskDefinition:
DependsOn: S3Bucket
Properties:
ContainerDefinitions:
- Environment:
- Name: ALLOW_DIRECTORY_LIST
Value: 'true'
- Name: AWS_SIGS_VERSION
Value: '4'
- Name: S3_BUCKET_NAME
Value: !Ref 'S3Bucket'
- Name: S3_REGION
Value: !Ref 'AWS::Region'
- Name: S3_SERVER_PORT
Value: '443'
- Name: S3_SERVER_PROTO
Value: https
- Name: S3_SERVER
Value: !Join
- .
- - s3
- !Ref 'AWS::Region'
- amazonaws.com
- Name: S3_STYLE
Value: default
- Name: S3_DEBUG
Value: 'false'
Image: ghcr.io/nginxinc/nginx-s3-gateway/nginx-oss-s3-gateway:latest-njs-oss-20220310
LogConfiguration:
LogDriver: awslogs
Options:
awslogs-group: !Ref 'ECSLogGroup'
awslogs-region: !Ref 'AWS::Region'
awslogs-stream-prefix: ecs
Name: !Ref 'ContainerName'
PortMappings:
- ContainerPort: 80
HostPort: 80
Cpu: '1024'
ExecutionRoleArn: !GetAtt 'ECSTaskExecutionRole.Arn'
Family: !Ref 'ContainerName'
Memory: '2048'
NetworkMode: awsvpc
RequiresCompatibilities:
- FARGATE
TaskRoleArn: !GetAtt 'ECSTaskRole.Arn'
Type: AWS::ECS::TaskDefinitionUsage
|
When an exception happens within a promise (and even if it is caught), it will result in a segmentation fault of njs. It is fixed in the following two commits to njs: We rely on catching an exception in order to not have a race condition here: https://github.com/nginxinc/nginx-s3-gateway/blob/issue/8/common/etc/nginx/include/s3gateway.js#L257 |
|
@ajschmidt8 I added the docs you contributed above to the branch under your name. I hope that is ok. Commit: 20a156e |
Absolutely. No problems here. It's a little unrefined in its current state since I was just re-using some stuff I had already written. I will try to tweak it later this week to spin up a VPC with subnets too so that those variables don't need to be changed per user. |
|
Any updates here? What's left on the to-do list? |
|
@ajschmidt8 - Everything is fully functioning to my knowledge in the issue/8 branch. This branch is publishing a container image to the github container registry, so it can be used as-is right now. I'm waiting for a new njs release before merging into master in order for a bug affecting instance profile support to be resolved. |
|
Hello, I didn't really investigated the issue, I wanted to clarify its state before going crazy :) |
|
Hi @cyuste the gateway configuration should be working in its current state. The only reason that I've held off from merging is that I'm waiting for the latest release of njs to drop for nginx plus. Can you share what environment variables you started the gateway with (please redact sensitive data)? Also, can you share how you pulled in the container image? |
@dekobon, do you know when that will be? Just trying to gauge when this will be merged into the main branch. |
|
I've been told that the next release of njs for NGINX Plus is scheduled for release next week. |
|
Hello @dekobon , This is what I did: Download repo from https://github.com/FlorianSW/nginx-s3-gateway/archive/refs/heads/issue/8.zip Create settings file Run: docker run --env-file ./settings -p8433:80 --name nginx-oss-s3-gateway nginx-oss-s3-gateway:issue-8 The error is the same from the previous comment. |
|
Hi @cyuste , I think I may have found the issue. In the steps you provided above, you are pulling code from @FlorianSW 's branch. I've taken Florian's code and made additional changes. The latest code is in the issue/8 branch of this repository. As such, the download url to use above should be: https://github.com/nginxinc/nginx-s3-gateway/archive/refs/heads/issue/8.zip |
|
I should probably remove my branch there, so there is no confusion 🙈 however, that's not possible until the PR is open, if I'm not mistaken. Let's just keep the fingers crossed that the last blockers will be resolved soon 👍 |
|
Thank you both very much for your support, works perfectly, I would run now some tests to check if it fits our needs. Great work! |
|
This PR is merged in commit: 0df77d6 |
This PR adds two new possible paths from where credentials can be retrieved:
The currently used static credentials from the environment still works as well.
This code will try to automatically select the best approach of getting credentials in the following order
(the first ones are favoured over the last ones):
S3_environment variablesFixes #8
Example template of how to deploy to ECS:
template.yml.zip