Skip to content
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

fix gmsa,fsx capability bug #3540

Merged
merged 1 commit into from
Feb 13, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion agent/app/agent_capability.go
Original file line number Diff line number Diff line change
Expand Up @@ -312,7 +312,7 @@ func (agent *ecsAgent) appendDockerDependentCapabilities(capabilities []*ecs.Att
}

func (agent *ecsAgent) appendGMSACapabilities(capabilities []*ecs.Attribute) []*ecs.Attribute {
if agent.cfg.GMSACapable {
if agent.cfg.GMSACapable.Enabled() {
return appendNameOnlyAttribute(capabilities, attributePrefix+capabilityGMSA)
}

Expand Down
2 changes: 1 addition & 1 deletion agent/app/agent_capability_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1432,7 +1432,7 @@ func TestAppendGMSACapabilities(t *testing.T) {

agent := &ecsAgent{
cfg: &config.Config{
GMSACapable: true,
GMSACapable: config.BooleanDefaultFalse{Value: config.ExplicitlyEnabled},
},
}

Expand Down
2 changes: 1 addition & 1 deletion agent/app/agent_capability_windows.go
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ func (agent *ecsAgent) appendIPv6Capability(capabilities []*ecs.Attribute) []*ec
}

func (agent *ecsAgent) appendFSxWindowsFileServerCapabilities(capabilities []*ecs.Attribute) []*ecs.Attribute {
if agent.cfg.FSxWindowsFileServerCapable {
if agent.cfg.FSxWindowsFileServerCapable.Enabled() {
return appendNameOnlyAttribute(capabilities, attributePrefix+capabilityFSxWindowsFileServer)
}

Expand Down
6 changes: 3 additions & 3 deletions agent/app/agent_capability_windows_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -236,7 +236,7 @@ func TestAppendGMSACapabilitiesFalse(t *testing.T) {

agent := &ecsAgent{
cfg: &config.Config{
GMSACapable: false,
GMSACapable: config.BooleanDefaultFalse{Value: config.ExplicitlyDisabled},
},
}

Expand All @@ -258,7 +258,7 @@ func TestAppendFSxWindowsFileServerCapabilities(t *testing.T) {

agent := &ecsAgent{
cfg: &config.Config{
FSxWindowsFileServerCapable: true,
FSxWindowsFileServerCapable: config.BooleanDefaultFalse{Value: config.ExplicitlyEnabled},
},
}

Expand All @@ -280,7 +280,7 @@ func TestAppendFSxWindowsFileServerCapabilitiesFalse(t *testing.T) {

agent := &ecsAgent{
cfg: &config.Config{
FSxWindowsFileServerCapable: false,
FSxWindowsFileServerCapable: config.BooleanDefaultFalse{Value: config.ExplicitlyDisabled},
},
}

Expand Down
2 changes: 1 addition & 1 deletion agent/config/config_unix.go
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ func DefaultConfig() Config {
NvidiaRuntime: DefaultNvidiaRuntime,
CgroupCPUPeriod: defaultCgroupCPUPeriod,
GMSACapable: parseGMSACapability(),
FSxWindowsFileServerCapable: false,
FSxWindowsFileServerCapable: BooleanDefaultFalse{Value: ExplicitlyDisabled},
RuntimeStatsLogFile: defaultRuntimeStatsLogFile,
EnableRuntimeStats: BooleanDefaultFalse{Value: NotSet},
ShouldExcludeIPv6PortBinding: BooleanDefaultTrue{Value: ExplicitlyEnabled},
Expand Down
4 changes: 2 additions & 2 deletions agent/config/config_windows.go
Original file line number Diff line number Diff line change
Expand Up @@ -142,8 +142,8 @@ func DefaultConfig() Config {
SharedVolumeMatchFullConfig: BooleanDefaultFalse{Value: ExplicitlyDisabled}, //only requiring shared volumes to match on name, which is default docker behavior
PollMetrics: BooleanDefaultFalse{Value: NotSet},
PollingMetricsWaitDuration: DefaultPollingMetricsWaitDuration,
GMSACapable: true,
FSxWindowsFileServerCapable: true,
GMSACapable: BooleanDefaultFalse{Value: ExplicitlyDisabled},
FSxWindowsFileServerCapable: BooleanDefaultFalse{Value: ExplicitlyDisabled},
Comment on lines -145 to +146
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found the following initialization code for FSxWindowsFileServerCapable, can we confirm that this is still applicable?

func parseFSxWindowsFileServerCapability() bool {

Copy link
Contributor Author

@arun-annamalai arun-annamalai Feb 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

PauseContainerImageName: DefaultPauseContainerImageName,
PauseContainerTag: DefaultPauseContainerTag,
CNIPluginsPath: filepath.Join(ecsBinaryDir, defaultCNIPluginDirName),
Expand Down
17 changes: 9 additions & 8 deletions agent/config/parse_linux.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,14 +25,14 @@ import (
"github.com/cihub/seelog"
)

func parseGMSACapability() bool {
func parseGMSACapability() BooleanDefaultFalse {
envStatus := utils.ParseBool(os.Getenv("ECS_GMSA_SUPPORTED"), true)
if envStatus {
// Check if domain join check override is present
skipDomainJoinCheck := utils.ParseBool(os.Getenv(envSkipDomainJoinCheck), false)
if skipDomainJoinCheck {
seelog.Infof("Skipping domain join validation based on environment override")
return true
return BooleanDefaultFalse{Value: ExplicitlyEnabled}
}

// check if the credentials fetcher socket is created and exists
Expand All @@ -42,15 +42,15 @@ func parseGMSACapability() bool {
if err != nil {
if os.IsNotExist(err) {
seelog.Errorf("CREDENTIALS_FETCHER_HOST_DIR not found, err: %v", err)
return false
return BooleanDefaultFalse{Value: ExplicitlyDisabled}
}
}

//skip domain join check if the domainless gMSA is supported by setting the env variable
domainlessGMSAUser := os.Getenv("CREDENTIALS_FETCHER_SECRET_NAME_FOR_DOMAINLESS_GMSA")
if domainlessGMSAUser != "" && len(domainlessGMSAUser) > 0 {
seelog.Info("domainless gMSA support is enabled")
return true
return BooleanDefaultFalse{Value: ExplicitlyEnabled}
}

// returns true if the container instance is domain joined
Expand All @@ -59,16 +59,17 @@ func parseGMSACapability() bool {

if !isDomainJoined {
seelog.Error("gMSA on linux requires domain joined instance. Did not find expected env var ECS_DOMAIN_JOINED_LINUX_INSTANCE=true")
return BooleanDefaultFalse{Value: ExplicitlyDisabled}
}
return isDomainJoined
return BooleanDefaultFalse{Value: ExplicitlyEnabled}
}
}
seelog.Debug("env variables to support gMSA are not set")
return false
return BooleanDefaultFalse{Value: ExplicitlyDisabled}
}

func parseFSxWindowsFileServerCapability() bool {
return false
func parseFSxWindowsFileServerCapability() BooleanDefaultFalse {
return BooleanDefaultFalse{Value: ExplicitlyDisabled}
}

var IsWindows2016 = func() (bool, error) {
Expand Down
8 changes: 4 additions & 4 deletions agent/config/parse_linux_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,25 +27,25 @@ func TestParseGMSACapabilitySupported(t *testing.T) {
t.Setenv("ECS_DOMAIN_JOINED_LINUX_INSTANCE", "True")
t.Setenv("CREDENTIALS_FETCHER_HOST_DIR", "/var/run")

assert.True(t, parseGMSACapability())
assert.True(t, parseGMSACapability().Enabled())
}

func TestParseGMSACapabilityNonDomainJoined(t *testing.T) {
t.Setenv("ECS_GMSA_SUPPORTED", "True")
t.Setenv("ECS_DOMAIN_JOINED_LINUX_INSTANCE", "False")

assert.False(t, parseGMSACapability())
assert.False(t, parseGMSACapability().Enabled())
}

func TestParseGMSACapabilityUnsupported(t *testing.T) {
t.Setenv("ECS_GMSA_SUPPORTED", "False")

assert.False(t, parseGMSACapability())
assert.False(t, parseGMSACapability().Enabled())
}

func TestSkipDomainJoinCheckParseGMSACapability(t *testing.T) {
t.Setenv("ECS_GMSA_SUPPORTED", "True")
t.Setenv("ZZZ_SKIP_DOMAIN_JOIN_CHECK_NOT_SUPPORTED_IN_PRODUCTION", "True")

assert.True(t, parseGMSACapability())
assert.True(t, parseGMSACapability().Enabled())
}
8 changes: 4 additions & 4 deletions agent/config/parse_unsupported.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,12 @@ import (
"strings"
)

func parseGMSACapability() bool {
return false
func parseGMSACapability() BooleanDefaultFalse {
return BooleanDefaultFalse{Value: ExplicitlyDisabled}
}

func parseFSxWindowsFileServerCapability() bool {
return false
func parseFSxWindowsFileServerCapability() BooleanDefaultFalse {
return BooleanDefaultFalse{Value: ExplicitlyDisabled}
}

var IsWindows2016 = func() (bool, error) {
Expand Down
31 changes: 31 additions & 0 deletions agent/config/parse_unsupported_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
//go:build !linux && !windows
// +build !linux,!windows

// Copyright Amazon.com Inc. or its affiliates. All Rights Reserved.
//
// Licensed under the Apache License, Version 2.0 (the "License"). You may
// not use this file except in compliance with the License. A copy of the
// License is located at
//
// httpaws.amazon.com/apache2.0/
//
// or in the "license" file accompanying this file. This file is distributed
// on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
// express or implied. See the License for the specific language governing
// permissions and limitations under the License.

package config

import (
"testing"

"github.com/stretchr/testify/assert"
)

func TestParseGMSACapabilitySupported(t *testing.T) {
assert.False(t, parseGMSACapability().Enabled())
}

func TestParseFSxWindowsFileServerCapability(t *testing.T) {
assert.False(t, parseGMSACapability().Enabled())
}
21 changes: 11 additions & 10 deletions agent/config/parse_windows.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,41 +35,42 @@ const (
)

// parseGMSACapability is used to determine if gMSA support can be enabled
func parseGMSACapability() bool {
func parseGMSACapability() BooleanDefaultFalse {
envStatus := utils.ParseBool(os.Getenv("ECS_GMSA_SUPPORTED"), true)
return checkDomainJoinWithEnvOverride(envStatus)
}

// parseFSxWindowsFileServerCapability is used to determine if fsxWindowsFileServer support can be enabled
func parseFSxWindowsFileServerCapability() bool {
func parseFSxWindowsFileServerCapability() BooleanDefaultFalse {
// fsxwindowsfileserver is not supported on Windows 2016 and non-domain-joined container instances
status, err := IsWindows2016()
if err != nil || status == true {
return false
return BooleanDefaultFalse{Value: ExplicitlyDisabled}
}

envStatus := utils.ParseBool(os.Getenv("ECS_FSX_WINDOWS_FILE_SERVER_SUPPORTED"), true)
return checkDomainJoinWithEnvOverride(envStatus)
}

func checkDomainJoinWithEnvOverride(envStatus bool) bool {
func checkDomainJoinWithEnvOverride(envStatus bool) BooleanDefaultFalse {
if envStatus {
// Check if domain join check override is present
skipDomainJoinCheck := utils.ParseBool(os.Getenv(envSkipDomainJoinCheck), false)
if skipDomainJoinCheck {
seelog.Debug("Skipping domain join validation based on environment override")
return true
return BooleanDefaultFalse{Value: ExplicitlyEnabled}
}
// check if container instance is domain joined.
// If container instance is not domain joined, explicitly disable feature configuration.
status, err := isDomainJoined()
if err == nil && status == true {
return true
if err != nil {
seelog.Errorf("Unable to determine valid domain join with err: %v", err)
}
if status == true {
return BooleanDefaultFalse{Value: ExplicitlyEnabled}
}
seelog.Errorf("Unable to determine valid domain join: %v", err)
}

return false
return BooleanDefaultFalse{Value: ExplicitlyDisabled}
}

// isDomainJoined is used to validate if container instance is part of a valid active directory.
Expand Down
4 changes: 2 additions & 2 deletions agent/config/parse_windows_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ func TestParseGMSACapability(t *testing.T) {
os.Setenv("ECS_GMSA_SUPPORTED", "False")
defer os.Unsetenv("ECS_GMSA_SUPPORTED")

assert.False(t, parseGMSACapability())
assert.False(t, parseGMSACapability().Enabled())
}

func TestParseBooleanEnvVar(t *testing.T) {
Expand All @@ -49,5 +49,5 @@ func TestParseFSxWindowsFileServerCapability(t *testing.T) {
os.Setenv("ECS_FSX_WINDOWS_FILE_SERVER_SUPPORTED", "False")
defer os.Unsetenv("ECS_FSX_WINDOWS_FILE_SERVER_SUPPORTED")

assert.False(t, parseFSxWindowsFileServerCapability())
assert.False(t, parseFSxWindowsFileServerCapability().Enabled())
}
4 changes: 2 additions & 2 deletions agent/config/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -329,14 +329,14 @@ type Config struct {

// GMSACapable is the config option to indicate if gMSA is supported.
// It should be enabled by default only if the container instance is part of a valid active directory domain.
GMSACapable bool
GMSACapable BooleanDefaultFalse

// VolumePluginCapabilities specifies the capabilities of the ecs volume plugin.
VolumePluginCapabilities []string

// FSxWindowsFileServerCapable is the config option to indicate if fsxWindowsFileServer is supported.
// It should be enabled by default only if the container instance is part of a valid active directory domain.
FSxWindowsFileServerCapable bool
FSxWindowsFileServerCapable BooleanDefaultFalse

// External specifies whether agent is running on external compute capacity (i.e. outside of aws).
External BooleanDefaultFalse
Expand Down
2 changes: 1 addition & 1 deletion agent/engine/engine_sudo_linux_integ_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -767,7 +767,7 @@ func TestGMSATaskFile(t *testing.T) {
cfg := defaultTestConfigIntegTest()
cfg.TaskCPUMemLimit.Value = config.ExplicitlyDisabled
cfg.TaskCleanupWaitDuration = 3 * time.Second
cfg.GMSACapable = true
cfg.GMSACapable = config.BooleanDefaultFalse{Value: config.ExplicitlyEnabled}
cfg.AWSRegion = "us-west-2"

taskEngine, done, _ := setupGMSALinux(cfg, nil, t)
Expand Down
2 changes: 1 addition & 1 deletion agent/engine/engine_windows_integ_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -455,7 +455,7 @@ func defaultTestConfigIntegTestGMSA() *config.Config {
cfg.TaskCPUMemLimit.Value = config.ExplicitlyDisabled
cfg.ImagePullBehavior = config.ImagePullPreferCachedBehavior

cfg.GMSACapable = true
cfg.GMSACapable = config.BooleanDefaultFalse{Value: config.ExplicitlyEnabled}
cfg.AWSRegion = "us-west-2"

return cfg
Expand Down