nginx/nginx-s3-gateway

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

Closed this issue · 6 comments

orensharoni commented

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

  • Version of the repo - master
  • S3 backend implementation you are using -AWS
  • Deploying stand-alone using the script
  • NGINX OSS
  • Authentication method-IAM

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

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. #193

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

orensharoni commented

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

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

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

HighOnMikey commented

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

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.
  • 👍1