-
Notifications
You must be signed in to change notification settings - Fork 30
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for .NET 9 and support for disabling IMDS v1 #878
Conversation
@@ -134,8 +134,8 @@ private void SwitchToSelfContainedBuildIfNeeded(Recommendation recommendation) | |||
return; | |||
|
|||
// Elastic Beanstalk doesn't currently have .NET 7 preinstalled. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: forgot to add .NET9 to the comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
@@ -238,6 +242,18 @@ private void ConfigureBeanstalkEnvironment(Configuration settings, string beanst | |||
} | |||
}; | |||
|
|||
if (newDeployment || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if newDeployment
is true, then the if
statement is true without checking the 2nd condition.
If newDeployment
is false, the 2nd condition will also be false since you are using &&
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The second condition was supposed to be !newDeployment
for if doing a redeployment then skip if disable IMDSv1 is set to default. I had done some renaming refactoring where the variable used to be called performRedeployment
and switched the naming to be newDeployment
thinking that looks cleaner but forgot to update the inverse logic here.
I have made the changes and ran through my series of deployment tests with old and new AWS account to confirm correct behavior.
@@ -238,6 +242,18 @@ private void ConfigureBeanstalkEnvironment(Configuration settings, string beanst | |||
} | |||
}; | |||
|
|||
if (newDeployment || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
Could you also add a |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #878 +/- ##
=======================================
Coverage 62.39% 62.39%
=======================================
Files 279 279
Lines 10905 10908 +3
Branches 1515 1515
=======================================
+ Hits 6804 6806 +2
- Misses 3564 3565 +1
Partials 537 537 ☔ View full report in Codecov by Sentry. |
…k recipes The support for disabling IMDS v1 provides a mechanism for transitions EC2 Launch Configuration to Launch Templates
bd614ef
to
7bf06e5
Compare
Issue #, if available:
#877
Description of changes:
EC2 made a change that new accounts created after Oct 1 2024. This means today if an account created after Oct 1st tries to deploy to Beanstalk using our tooling it will fail because it can't create a LaunchConfiguration.
To force Beanstalk you need to set one of the following 4 settings.
true
(For reference the Beantalk Developer Guide: https://docs.aws.amazon.com/elasticbeanstalk/latest/dg/environments-cfg-autoscaling-launch-templates.html)
None of these setting are currently settable in the tooling. Ideally we should add support for now I added
DisableIMDSv1
. The other settings require more settings to be set when enabled and there isn't an obvious default.The
DisableIMDSv1
is a boolean property but we have to be careful not to automatically set this totrue
on existing environments when users do a redeployment. That could break environments. The compromise I did was modeled the property as 3 state value: Enabled, Disabled and Default.The
Default
value is the the default for the new setting in the recipe. UsingDefault
means in the CDK project to setDisableIMDSv1
for new deployments but for redeployments leave the setting alone. That way existing users when they upgrade to the new version of the tooling no change will be made to their environments when they redeploy. This required adding metadata being passed into the CDK project whether it was being run for a new deployment or not.I decided to make the changes to add .NET 9 support while I was waiting for test deployments to finish. The change was just to add the .NET 9 target in the recipe rules files and update the docker manifest.
Testing
I have run all of the unit and integ tests. Before making any changes I created a new AWS account and reproduced the failure situation. After making the changes I was able to do deployments and redeployments with both my old account and new account. I also ran through .NET 9 deployments.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.