Skip to content

Use custom viper key delimiter to support dots in configuration. #142

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.

Sign up for GitHub

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

Closed
hprabh wants to merge 3 commits into from
Closed

Use custom viper key delimiter to support dots in configuration. #142

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
41a1914
Update
hprabh Apr 14, 2026
37c4af0
Update
hprabh Apr 14, 2026
f0a89b3
Update
hprabh Apr 14, 2026
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
8 changes: 5 additions & 3 deletions pkg/config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,10 @@ func LoadConfig(configPath string) (*Config, error) {
return nil, fmt.Errorf("config file path is required")
}

// Set up viper
v := viper.New()
// Use a custom key delimiter to avoid conflicts with dots in map keys.
// Kubernetes labels (e.g. "kubernetes.io/nodeReady") contain dots that
// viper's default "." delimiter would misinterpret as nested keys.
v := viper.NewWithOptions(viper.KeyDelimiter("::"))
Copy link

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

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

The delimiter string ("::") is duplicated and implicitly couples the KeyDelimiter configuration with every keypath usage. To reduce the chance of future mismatches (e.g., a later IsSet("azure.managedIdentity") reappearing), define a single constant for the delimiter and/or key paths (e.g., const viperKeyDelimiter = "::" and build "azure"+viperKeyDelimiter+"managedIdentity"), so any future changes remain consistent.

Copilot uses AI. Check for mistakes.
v.SetConfigType("json")
v.AutomaticEnv()
v.SetEnvPrefix(envPrefix)
Expand All @@ -69,7 +71,7 @@ func LoadConfig(configPath string) (*Config, error) {
// Track if managedIdentity was explicitly set in config
// This is necessary because viper unmarshals empty JSON objects {} as nil pointers
// Using viper.IsSet() correctly detects if the key was present in the config file
config.isMIExplicitlySet = v.IsSet("azure.managedIdentity")
config.isMIExplicitlySet = v.IsSet("azure::managedIdentity")
Copy link

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

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

The delimiter string ("::") is duplicated and implicitly couples the KeyDelimiter configuration with every keypath usage. To reduce the chance of future mismatches (e.g., a later IsSet("azure.managedIdentity") reappearing), define a single constant for the delimiter and/or key paths (e.g., const viperKeyDelimiter = "::" and build "azure"+viperKeyDelimiter+"managedIdentity"), so any future changes remain consistent.

Copilot uses AI. Check for mistakes.

// Set defaults for any missing values
config.SetDefaults()
Expand Down
140 changes: 140 additions & 0 deletions pkg/config/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package config
import (
"os"
"path/filepath"
"sort"
"strings"
"testing"
)
Expand Down Expand Up @@ -1069,6 +1070,145 @@ func TestAuthenticationMethodValidation(t *testing.T) {
}
}

func TestLoadConfigWithDottedLabels(t *testing.T) {
t.Parallel()

tests := []struct {
name string
configJSON string
expectedLabels map[string]string
}{
Comment on lines +1073 to +1080
Copy link

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

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

LoadConfig sets a process-wide singleton (configInstance) under a mutex (per the LoadConfig implementation), so running this test and its subtests in parallel can cause cross-test interference/flakiness if other tests also call LoadConfig and rely on the singleton. Consider removing t.Parallel() at either the top-level test or the subtest level (mandatory if any other config tests depend on global config state), or refactor tests to avoid the singleton side effect (e.g., a non-singleton loading path for tests).

Copilot uses AI. Check for mistakes.
{
name: "labels with dotted keys are preserved",
configJSON: `{
"azure": {
"subscriptionId": "12345678-1234-1234-1234-123456789012",
"tenantId": "12345678-1234-1234-1234-123456789012",
"cloud": "AzurePublicCloud",
"bootstrapToken": { "token": "abcdef.0123456789abcdef" },
"targetCluster": {
"resourceId": "/subscriptions/12345678-1234-1234-1234-123456789012/resourceGroups/test-rg/providers/Microsoft.ContainerService/managedClusters/test-cluster",
"location": "eastus"
}
},
"node": {
"labels": {
"kubernetes.io/nodeReady": "true",
"node.kubernetes.io/instance-type": "Standard_D2s_v3"
},
"kubelet": {
"serverURL": "https://test-cluster.hcp.eastus.azmk8s.io:443",
"caCertData": "LS0tLS1CRUdJTi1DRVJUSUZJQ0FURS0tLS0t"
}
}
}`,
expectedLabels: map[string]string{
"kubernetes.io/nodeready": "true",
"node.kubernetes.io/instance-type": "Standard_D2s_v3",
"kubernetes.azure.com/managed": "false", // default label
},
},
{
name: "labels without dots still work",
configJSON: `{
"azure": {
"subscriptionId": "12345678-1234-1234-1234-123456789012",
"tenantId": "12345678-1234-1234-1234-123456789012",
"cloud": "AzurePublicCloud",
"bootstrapToken": { "token": "abcdef.0123456789abcdef" },
"targetCluster": {
"resourceId": "/subscriptions/12345678-1234-1234-1234-123456789012/resourceGroups/test-rg/providers/Microsoft.ContainerService/managedClusters/test-cluster",
"location": "eastus"
}
},
"node": {
"labels": {
"env": "production",
"team": "platform"
},
"kubelet": {
"serverURL": "https://test-cluster.hcp.eastus.azmk8s.io:443",
"caCertData": "LS0tLS1CRUdJTi1DRVJUSUZJQ0FURS0tLS0t"
}
}
}`,
expectedLabels: map[string]string{
"env": "production",
"team": "platform",
"kubernetes.azure.com/managed": "false",
},
},
{
name: "mixed dotted and simple labels",
configJSON: `{
"azure": {
"subscriptionId": "12345678-1234-1234-1234-123456789012",
"tenantId": "12345678-1234-1234-1234-123456789012",
"cloud": "AzurePublicCloud",
"bootstrapToken": { "token": "abcdef.0123456789abcdef" },
"targetCluster": {
"resourceId": "/subscriptions/12345678-1234-1234-1234-123456789012/resourceGroups/test-rg/providers/Microsoft.ContainerService/managedClusters/test-cluster",
"location": "eastus"
}
},
"node": {
"labels": {
"env": "staging",
"topology.kubernetes.io/zone": "eastus-1",
"disktype": "ssd"
},
"kubelet": {
"serverURL": "https://test-cluster.hcp.eastus.azmk8s.io:443",
"caCertData": "LS0tLS1CRUdJTi1DRVJUSUZJQ0FURS0tLS0t"
}
}
}`,
expectedLabels: map[string]string{
"env": "staging",
"topology.kubernetes.io/zone": "eastus-1",
"disktype": "ssd",
"kubernetes.azure.com/managed": "false",
},
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
t.Parallel()

configFile := filepath.Join(t.TempDir(), "config.json")
if err := os.WriteFile(configFile, []byte(tt.configJSON), 0o600); err != nil {
t.Fatalf("Failed to write test config file: %v", err)
}

config, err := LoadConfig(configFile)
Comment on lines +1175 to +1184
Copy link

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

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

LoadConfig sets a process-wide singleton (configInstance) under a mutex (per the LoadConfig implementation), so running this test and its subtests in parallel can cause cross-test interference/flakiness if other tests also call LoadConfig and rely on the singleton. Consider removing t.Parallel() at either the top-level test or the subtest level (mandatory if any other config tests depend on global config state), or refactor tests to avoid the singleton side effect (e.g., a non-singleton loading path for tests).

Copilot uses AI. Check for mistakes.
if err != nil {
t.Fatalf("LoadConfig() unexpected error: %v", err)
}

// Verify all expected labels are present with correct values
for key, expectedVal := range tt.expectedLabels {
gotVal, ok := config.Node.Labels[key]
if !ok {
t.Errorf("expected label %q not found in config.Node.Labels (got keys: %v)", key, labelKeys(config.Node.Labels))
} else if gotVal != expectedVal {
t.Errorf("label %q = %q, want %q", key, gotVal, expectedVal)
}
}
})
}
}

// labelKeys returns the keys of a map for diagnostic output.
func labelKeys(m map[string]string) []string {
keys := make([]string, 0, len(m))
for k := range m {
keys = append(keys, k)
}
sort.Strings(keys)
return keys
}
Comment thread
hprabh marked this conversation as resolved.

func TestIsBootstrapTokenConfigured(t *testing.T) {
tests := []struct {
name string
Expand Down
Loading