Added support for new command line parameter --disable-imds-v1 to disable IMDSv1 for Elastic BeanStalk environments. #323
Conversation
ashishdhingra
added
feature-request
|
|
||
|
|
||
| // For new environments, disable IMDSv1 by default (unless explicitly enabled in additional options). | ||
| createRequest.OptionSettings.Add(new ConfigurationOptionSetting() |
There was a problem hiding this comment.
Why does this need to be both the here and in AddAdditionalOptions? Seems redundant and you had to add code in AddAdditionalOptions to handle the fact that it was potentially added by the parent caller.
There was a problem hiding this comment.
I misunderstood the purpose here which was to force all new deployments to have disable to true. We don't want to do that because that could break users and we don't have a UI element in Visual Studio for users to toggle this.
@ashishdhingra is going to change the logic to remove this section and in the AddAdditionalOptions only set the disable if the user explicitly set the field. The VS toolkit can decide if they want to add a checkbox users can see and default the value to true.
There was a problem hiding this comment.
Removed the default disabling of IMDSv1 since it could break existing customers using legacy Visual Studio tooling. Also simplified code in AddAdditionalOptions to not check if existing setting is already added.
| @@ -92,6 +93,8 @@ internal void ParseCommandArguments(CommandOptions values) | |||
| this.LoadBalancerType = tuple.Item2.StringValue; | |||
| if ((tuple = values.FindCommandOption(EBDefinedCommandOptions.ARGUMENT_ENABLE_STICKY_SESSIONS.Switch)) != null) | |||
| this.EnableStickySessions = tuple.Item2.BoolValue; | |||
| if ((tuple = values.FindCommandOption(EBDefinedCommandOptions.ARGUMENT_DISABLE_IMDS_V1.Switch)) != null) | |||
There was a problem hiding this comment.
The PersistSettings method also needs to be updated for the new switch.
https://github.com/aws/aws-extensions-for-dotnet-cli/blob/master/src/Amazon.ElasticBeanstalk.Tools/Commands/CommandProperties.cs#L103
There was a problem hiding this comment.
Done in latest commit
ashishdhingra
force-pushed
the
user/ashdhin/eb-disableimdsv1-issue290
branch
from
821a787 to
3147d66
Compare
ashishdhingra
changed the title
ashishdhingra
changed the title
…able IMDSv1 for Elastic BeanStalk environments.
ashishdhingra
force-pushed
the
user/ashdhin/eb-disableimdsv1-issue290
branch
from
3147d66 to
a801761
Compare
@ashovlin Added details on how this was tested. Please review. |
Issue #, if available: #290 (internal issue: DOTNET-7165)
Description of changes:
Adds feature to disable
IMDSv1by default for new Elastic BeanStalk environments. Also adds support for new command line parameter--disable-imds-v1useful for updating existing environments.Refer https://docs.aws.amazon.com/elasticbeanstalk/latest/dg/environments-cfg-ec2-imds.html#environments-cfg-ec2-imds.namespace for details on how to disable IMDSv1.
EDIT: (Per review comment from @normj) We should not disable IMDSv1 by default for new environments since it could break existing users using legacy Visual Studio tooling. The AWS Toolkit for Visual Studio doesn't have checkbox for the new flag and hence users might not know if IMDSv1 is being disabled by default.
Testing:
The changes were tested in personal AWS account (via Visual Studio debugging and observing state in AWS EB console) for below scenarios (executed AWS .NET Extensions CLI command):
--disable-imds-v1set totrue.--disable-imds-v1set totrue. This executed update EB environment scenario and disabled IMDSv1.After this PR is released we also need to create PR to push CLI extensions changes to AWS Toolkit for Visual Studio repository and thereafter their team could decide to add GUI checkbox for the new setting.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.