fix: defer config value type assertion to load stage instead of parse (#2286)

Author: lucix-aws

.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"
+    ]
+}

.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"
+    ]
+}

config/shared_config.go

@@ -740,6 +740,8 @@ func mergeSections(dst *ini.Sections, src ini.Sections) error {
 			defaultsModeKey,
 			retryModeKey,
 			caBundleKey,
+			roleDurationSecondsKey,
+			retryMaxAttemptsKey,
 
 			ssoSessionNameKey,
 			ssoAccountIDKey,
@@ -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)
 	}
@@ -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",
@@ -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)
@@ -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
 }
 
@@ -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,
@@ -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
@@ -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
@@ -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

internal/ini/ini_lexer_test.go

@@ -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
@@ -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),
 			},
 		},
 		{