aws / amazon-ecs-init

ecs-init is now part of the amazon-ecs-agent repo https://github.com/aws/amazon-ecs-agent/tree/master/ecs-init
https://github.com/aws/amazon-ecs-agent
Apache License 2.0
200 stars 117 forks source link

Add support for running in non-EC2 environment #383

Closed fenxiong closed 3 years ago

fenxiong commented 3 years ago

Summary

Add support for running in external (non-EC2) environment. This includes changes to:

  1. Detect whether we are running in external env by checking env "ECS_EXTERNAL". When packing the ecs init rpm, ecs init will load an ecs.config file as an env file that sets this to true;
  2. When detecting that we are running in external env, add the following behavior changes:
    • Disable ec2 metadata call;
    • Add host credentials bind mount;
    • Disable task networking.

This pull request only make change to code but not packaging, just to allow the code to work with non-EC2 environment. Packaging changes will be added separately.

Implementation details

Testing

Added unit test. Manually tested by building the rpm and run it in a centos:7 vagrant image, verified agent is started correctly.

New tests cover the changes: yes

Description for the changelog

N/A

Licensing

This contribution is under the terms of the Apache 2.0 License:

fenxiong commented 3 years ago

Can we make sure that we are not adding the capabilities not needed for external? This will help limit the permissions/access agent has on customer instances

yes. updated to skip adding sys/net admin capabilities for external

fenxiong commented 3 years ago

I think this works but overall I don't love that the RunningInExternal checks are scattered all over the place. Do you think there could be an easy way of customizing all that needs to be customized when RunningInExternal()=true?

IMO, merging them allow us to see all the external related customization easier, but keeping them separate allow each individual component to work correctly by itself (for example, if we want to merge the check in getContainerConfig and getHostConfig by post processing the return values of both in a single function, that makes them incorrect by themselves and depend on another function). so i would prefer keeping them separate unless there are too many. if you know any better way let me know though.

angelcar commented 3 years ago

I think this works but overall I don't love that the RunningInExternal checks are scattered all over the place. Do you think there could be an easy way of customizing all that needs to be customized when RunningInExternal()=true?

IMO, merging them allow us to see all the external related customization easier, but keeping them separate allow each individual component to work correctly by itself (for example, if we want to merge the check in getContainerConfig and getHostConfig by post processing the return values of both in a single function, that makes them incorrect by themselves and depend on another function). so i would prefer keeping them separate unless there are too many. if you know any better way let me know though.

I see, yeah, I was thininh we could create options in an idiomatic golang way, but I guess you're right, if there are more checks in the future that it becomes hard to see what is external and what is not, we can refactor then