Use custom viper key delimiter to support dots in configuration.#142
Use custom viper key delimiter to support dots in configuration.#142hprabh wants to merge 3 commits intoAzure:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR updates configuration loading to support Kubernetes label keys containing dots by changing Viper’s key path delimiter, and adds tests to verify dotted label keys deserialize correctly.
Changes:
- Configure Viper to use
::as the key delimiter instead of.to prevent dotted label keys being treated as nested paths. - Update the
managedIdentitypresence check to use the new delimiter. - Add table-driven tests covering dotted, non-dotted, and mixed label keys.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| pkg/config/config.go | Switches Viper key delimiter to :: and updates a key-path IsSet check accordingly. |
| pkg/config/config_test.go | Adds tests ensuring dotted Kubernetes label keys are preserved during config load. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| }`, | ||
| expectedLabels: map[string]string{ | ||
| "kubernetes.io/nodeready": "true", |
There was a problem hiding this comment.
The expected key "kubernetes.io/nodeready" does not match the input JSON label key "kubernetes.io/nodeReady" (capital R). Unless the production code intentionally normalizes label keys to lowercase (and does so consistently for all keys), this test will fail or encode inconsistent expectations. Align the expected key casing with the actual behavior (either expect "kubernetes.io/nodeReady" as-is, or apply the same normalization to all expected keys).
| "kubernetes.io/nodeready": "true", | |
| "kubernetes.io/nodeReady": "true", |
| // 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) | ||
| } | ||
| } | ||
|
|
||
| // Verify no unexpected labels | ||
| for key := range config.Node.Labels { | ||
| if _, ok := tt.expectedLabels[key]; !ok { | ||
| t.Errorf("unexpected label %q = %q", key, config.Node.Labels[key]) | ||
| } | ||
| } |
There was a problem hiding this comment.
The “no unexpected labels” assertion makes this test brittle to unrelated changes (e.g., adding new default labels in SetDefaults()), causing failures even when dotted-label handling remains correct. Consider scoping this test to only validate dotted-key preservation (e.g., assert expected keys/values are present, and optionally only enforce exact equality for node.labels when the test case explicitly includes all defaults).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func TestLoadConfigWithDottedLabels(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| tests := []struct { | ||
| name string | ||
| configJSON string | ||
| expectedLabels map[string]string | ||
| }{ |
There was a problem hiding this comment.
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).
| 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) |
There was a problem hiding this comment.
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).
| // 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("::")) |
There was a problem hiding this comment.
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.
| // 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") |
There was a problem hiding this comment.
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.
Kubernetes labels allow dots in key names for labels (e.g. kubernetes.io/nodeReady), but viper's default "." delimiter interprets them as nested keys, causing incorrect deserialization. Switch to "::" as the key delimiter and update the managedIdentity IsSet call accordingly.