fix gmsa,fsx capability bug #3540
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
arun-annamalai
force-pushed
the
dev
branch
10 times, most recently
from
9e6e33f
to
94fe265
Compare
|
Reran the failed tests one more time. |
rawahars
approved these changes
Jan 24, 2023
YashdalfTheGray
approved these changes
Jan 24, 2023
fierlion
requested changes
Feb 2, 2023
Comment on lines
-145
to
+146
| GMSACapable: true, | ||
| FSxWindowsFileServerCapable: true, | ||
| GMSACapable: BooleanDefaultFalse{Value: ExplicitlyDisabled}, | ||
| FSxWindowsFileServerCapable: BooleanDefaultFalse{Value: ExplicitlyDisabled}, |
There was a problem hiding this comment.
Here we are changing the default FSsWindowsCapability from true to false, but I don't see where it's being enabled. This is a behavior change, (as FSx Windows has been available for 2 years). Can you point out where this is being enabled?
There was a problem hiding this comment.
I found the following initialization code for FSxWindowsFileServerCapable, can we confirm that this is still applicable?
There was a problem hiding this comment.
yes the area that Yash has linked is the exact area where we determine whether to advertise the FXs and gMSA capabilities. We should not default advertise gMSA and FSx because not all machines are domain joined.
yinyic
approved these changes
Feb 7, 2023
fierlion
approved these changes
Feb 13, 2023
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This pull request fixes how the Agent determines whether it is gMSA and fsx capable. Currently when either of these capabilities are set to false, the
config.mergeDefaultConfig(errs)line will set these back to true on Windows since the lhs is the boolean false and the default config (rhs) contains the boolean true.Implementation details
This fix is done by changing the native
booleanto now useBooleanDefaultFalseTesting
This change was tested both by using unit tests and a custom ecs agent installed on a windows ec2 instance which was connected to a cluster. The logs were viewed to see what capabilities were advertised upon agent startup.
New tests cover the changes:
yes
Description for the changelog
Bug - Fixed a bug that incorrectly advertised the gMSA and fsx capability
Licensing
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.