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: defer config value type assertion to load stage instead of parse #2286

Merged
merged 2 commits into from Sep 21, 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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 9 additions & 0 deletions .changelog/87dcac43e51040c38f8e72fb9b874042.json
@@ -0,0 +1,9 @@
{
"id": "87dcac43-e510-40c3-8f8e-72fb9b874042",
"type": "bugfix",
"description": "Move type assertion of config values out of the parsing stage, which resolves an issue where the contents of a profile would silently be dropped with certain numeric formats.",
"modules": [
"config",
"internal/ini"
]
}
9 changes: 9 additions & 0 deletions .changelog/8e58ee8fce61452eb60126004c893c2d.json
@@ -0,0 +1,9 @@
{
"id": "8e58ee8f-ce61-452e-b601-26004c893c2d",
"type": "bugfix",
"description": "Fixed a bug where merging `max_attempts` or `duration_seconds` fields across shared config files with invalid values would silently default them to 0.",
"modules": [
"config",
"internal/ini"
]
}
68 changes: 28 additions & 40 deletions config/shared_config.go
Expand Up @@ -740,6 +740,8 @@ func mergeSections(dst *ini.Sections, src ini.Sections) error {
defaultsModeKey,
retryModeKey,
caBundleKey,
roleDurationSecondsKey,
retryMaxAttemptsKey,

ssoSessionNameKey,
ssoAccountIDKey,
Expand All @@ -753,16 +755,6 @@ func mergeSections(dst *ini.Sections, src ini.Sections) error {
}
}

intKeys := []string{
roleDurationSecondsKey,
retryMaxAttemptsKey,
}
for i := range intKeys {
if err := mergeIntKey(&srcSection, &dstSection, sectionName, intKeys[i]); err != nil {
return err
}
}

// set srcSection on dst srcSection
*dst = dst.SetSection(sectionName, dstSection)
}
Expand All @@ -789,26 +781,6 @@ func mergeStringKey(srcSection *ini.Section, dstSection *ini.Section, sectionNam
return nil
}

func mergeIntKey(srcSection *ini.Section, dstSection *ini.Section, sectionName, key string) error {
if srcSection.Has(key) {
srcValue := srcSection.Int(key)
v, err := ini.NewIntValue(srcValue)
if err != nil {
return fmt.Errorf("error merging %s, %w", key, err)
}

if dstSection.Has(key) {
dstSection.Logs = append(dstSection.Logs, newMergeKeyLogMessage(sectionName, key,
dstSection.SourceFile[key], srcSection.SourceFile[key]))

}

dstSection.UpdateValue(key, v)
dstSection.UpdateSourceFile(key, srcSection.SourceFile[key])
}
return nil
}

func newMergeKeyLogMessage(sectionName, key, dstSourceFile, srcSourceFile string) string {
return fmt.Sprintf("For profile: %v, overriding %v value, defined in %v "+
"with a %v value found in a duplicate profile defined at file %v. \n",
Expand Down Expand Up @@ -962,9 +934,16 @@ func (c *SharedConfig) setFromIniSection(profile string, section ini.Section) er
updateString(&c.SSOAccountID, section, ssoAccountIDKey)
updateString(&c.SSORoleName, section, ssoRoleNameKey)

// we're retaining a behavioral quirk with this field that existed before
// the removal of literal parsing for #2276:
// - if the key is missing, the config field will not be set
// - if the key is set to a non-numeric, the config field will be set to 0
if section.Has(roleDurationSecondsKey) {
d := time.Duration(section.Int(roleDurationSecondsKey)) * time.Second
c.RoleDurationSeconds = &d
if v, ok := section.Int(roleDurationSecondsKey); ok {
c.RoleDurationSeconds = aws.Duration(time.Duration(v) * time.Second)
} else {
c.RoleDurationSeconds = aws.Duration(time.Duration(0))
}
}

updateString(&c.CredentialProcess, section, credentialProcessKey)
Expand Down Expand Up @@ -1314,12 +1293,13 @@ func updateInt(dst *int, section ini.Section, key string) error {
if !section.Has(key) {
return nil
}
if vt, _ := section.ValueType(key); vt != ini.IntegerType {
return fmt.Errorf("invalid value %s=%s, expect integer",
key, section.String(key))

v, ok := section.Int(key)
if !ok {
return fmt.Errorf("invalid value %s=%s, expect integer", key, section.String(key))
}
*dst = int(section.Int(key))

*dst = int(v)
return nil
}

Expand All @@ -1329,7 +1309,10 @@ func updateBool(dst *bool, section ini.Section, key string) {
if !section.Has(key) {
return
}
*dst = section.Bool(key)

// retains pre-#2276 behavior where non-bool value would resolve to false
v, _ := section.Bool(key)
*dst = v
}

// updateBoolPtr will only update the dst with the value in the section key,
Expand All @@ -1338,8 +1321,11 @@ func updateBoolPtr(dst **bool, section ini.Section, key string) {
if !section.Has(key) {
return
}

// retains pre-#2276 behavior where non-bool value would resolve to false
v, _ := section.Bool(key)
*dst = new(bool)
**dst = section.Bool(key)
**dst = v
}

// updateEndpointDiscoveryType will only update the dst with the value in the section, if
Expand Down Expand Up @@ -1371,7 +1357,8 @@ func updateUseDualStackEndpoint(dst *aws.DualStackEndpointState, section ini.Sec
return
}

if section.Bool(key) {
// retains pre-#2276 behavior where non-bool value would resolve to false
if v, _ := section.Bool(key); v {
*dst = aws.DualStackEndpointStateEnabled
} else {
*dst = aws.DualStackEndpointStateDisabled
Expand All @@ -1387,7 +1374,8 @@ func updateUseFIPSEndpoint(dst *aws.FIPSEndpointState, section ini.Section, key
return
}

if section.Bool(key) {
// retains pre-#2276 behavior where non-bool value would resolve to false
if v, _ := section.Bool(key); v {
*dst = aws.FIPSEndpointStateEnabled
} else {
*dst = aws.FIPSEndpointStateDisabled
Expand Down
4 changes: 1 addition & 3 deletions internal/ini/ini_lexer_test.go
Expand Up @@ -8,8 +8,6 @@ import (
)

func TestTokenize(t *testing.T) {
numberToken := newToken(TokenLit, []rune("123"), IntegerType)
numberToken.base = 10
cases := []struct {
r io.Reader
expectedTokens []Token
Expand All @@ -22,7 +20,7 @@ func TestTokenize(t *testing.T) {
newToken(TokenWS, []rune(" "), NoneType),
newToken(TokenOp, []rune("="), NoneType),
newToken(TokenWS, []rune(" "), NoneType),
numberToken,
newToken(TokenLit, []rune("123"), StringType),
},
},
{
Expand Down