nginxinc / nginx-s3-gateway

NGINX S3 Caching Gateway
Apache License 2.0
496 stars 126 forks source link

using standalone_ubuntu_oss_install.sh on ec2 fail the EC@ instance check #206

Closed orensharoni closed 7 months ago

orensharoni commented 7 months ago

The bug when running the standalone_ubuntu_oss_install.sh on an ec2 instance the check fail and requires a AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY despite running on an ec2 with the proper Role

To Reproduce Steps to reproduce the behavior:

  1. clone to an ec2 instance
  2. run standalone_ubuntu_oss_install.sh
  3. See error

Expected behavior the script needs to pass the ec2 test.

Your environment

Additional context I think it is since the check curl --output /dev/null --silent --head --fail --connect-timeout 2 "http://169.254.169.254" refers to IMDSv1 and it needs to change to IMDSv2, i used the following: TOKEN=curl --silent --fail --connect-timeout 2 -X PUT "http://169.254.169.254/latest/api/token" -H "X-aws-ec2-metadata-token-ttl-seconds: 21600"\ && curl --output /dev/null --silent --head --fail --connect-timeout 2 -H "X-aws-ec2-metadata-token: $TOKEN" -v http://169.254.169.254/latest/meta-data/

4141done commented 7 months ago

Thank you for your report @orensharoni , I'll take a look this week. We did have a PR come in that addressed this in the container startup script so it's likely we just need to port that change over to the script you're using. https://github.com/nginxinc/nginx-s3-gateway/pull/193

Feel free to open a PR if you get to it before I do 🐳

orensharoni commented 7 months ago

hi, will love to help , i have the fix working locally I can't push a branch to create the PR - no contributor privlige

4141done commented 7 months ago

hi, will love to help , i have the fix working locally I can't push a branch to create the PR - no contributor privlige

That would be great! You'll need to first fork this repository, create a branch on your local fork, then follow this guide to open a pull request: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/creating-a-pull-request-from-a-fork

Question for you - It seems that most new ec2 instances have support for both IMDSv2 and IMDSv1. Would it make sense to leave the elif branch for IMDSv1 as a fallback if someone has an old instance? For example:

if [ ! -z ${AWS_CONTAINER_CREDENTIALS_RELATIVE_URI+x} ]; then
  echo "Running inside an ECS task, using container credentials"
  uses_iam_creds=1
elif TOKEN=$(curl -X PUT --silent --fail --connect-timeout 2 --max-time 2 "http://169.254.169.254/latest/api/token" -H "X-aws-ec2-metadata-token-ttl-seconds: 21600") && \
  curl -H "X-aws-ec2-metadata-token: $TOKEN" --output /dev/null --silent --head --fail --connect-timeout 2 --max-time 5 "http://169.254.169.254"; then 
  echo "Running inside an EC2 instance, using IMDSv2 for credentials"
  uses_iam_creds=1
elif curl --output /dev/null --silent --head --fail --connect-timeout 2 "http://169.254.169.254"; then
  echo "Running inside an EC2 instance, using IMDSv1 for credentials"
  uses_iam_creds=1
else
  required+=("AWS_ACCESS_KEY_ID" "AWS_SECRET_ACCESS_KEY")
  uses_iam_creds=0
fi
orensharoni commented 7 months ago

It makes sense to have both I'll do that, specifically, as it doesn't affect the software itself

HighOnMikey commented 7 months ago

FYI, Just checking for a successful status at http://169.254.169.254 will break installations running on GCP, OpenStack, and other providers that use 169.254.169.254 for metadata.

e:

As we're talking about the standalone install file, I just noticed lines 315 and 320 both need to be changed to /etc/nginx/nginx.conf.

4141done commented 7 months ago

FYI, Just checking for a successful status at http://169.254.169.254 will break installations running on GCP, OpenStack, and other providers that use 169.254.169.254 for metadata.

e:

As we're talking about the standalone install file, I just noticed lines 315 and 320 both need to be changed to /etc/nginx/nginx.conf.

Thank you for for bringing this up, @HighOnMikey

  1. In regard to support for non AWS platforms - my understanding is that this change will not cause support for non-AWS platforms to regress since we'll fall back to the check that's currently being done. I think your point is valuable but I'd suggest we file a separate issue to handle support for other compatible platforms with this script (if we decided to do it).
  2. Thank you for catching this - my read is the same as yours - we're adding NGINX config syntax to a Systemd config file. We can likely correct this as we're testing this change.