Replace Viper with encoding/json for config loading#143
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
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 replaces Viper-based config loading with encoding/json to preserve JSON map keys exactly (notably dotted Kubernetes label keys) and removes implicit env-var override behavior.
Changes:
- Replace Viper config parsing with
os.ReadFile+json.Unmarshal. - Adjust managed identity “explicitly set” detection to rely on JSON pointer presence.
- Add regression tests ensuring node label keys with dots/slashes are preserved.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/config/config.go | Removes Viper and loads config JSON directly with encoding/json; updates MI presence tracking. |
| pkg/config/config_test.go | Adds tests to ensure node label keys (including dotted segments) survive config load unchanged. |
| pkg/config/structs.go | Removes Viper-specific comment from MI configuration helper. |
| go.mod | Drops Viper dependency and associated indirect deps. |
| go.sum | Removes checksums for Viper and transitive dependencies. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| configJSON := fmt.Sprintf(baseAzureJSON, tt.labelsJSON) |
There was a problem hiding this comment.
t.Parallel() inside a range loop subtest captures the loop variable tt. On Go versions prior to 1.22 this can cause all parallel subtests to read the final tt value (flaky/incorrect assertions). Rebind tt := tt (or index into the slice) before t.Run to make the tests correct across Go versions.
| data, err := os.ReadFile(filepath.Clean(configPath)) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to read config file at %s: %w", configPath, err) | ||
| } | ||
|
|
||
| // Unmarshal config | ||
| config := &Config{} | ||
| if err := v.Unmarshal(config); err != nil { | ||
| if err := json.Unmarshal(data, config); err != nil { | ||
| return nil, fmt.Errorf("error unmarshaling config: %w", err) | ||
| } |
There was a problem hiding this comment.
json.Unmarshal silently ignores unknown fields, which can allow config typos to go unnoticed (potentially leading to confusing runtime behavior). Consider decoding via json.Decoder with DisallowUnknownFields() and returning an error that includes the configPath to make misconfigurations easier to diagnose.
| tests := []struct { | ||
| name string | ||
| labelsJSON string | ||
| expectedLabels map[string]string // only the user-supplied labels; defaults are checked separately |
There was a problem hiding this comment.
The inline comment says “defaults are checked separately”, but this test function doesn’t appear to verify defaults (and there’s no pointer here to where that happens). Either add an explicit assertion for expected default labels in this test, or adjust/remove the comment to avoid misleading future maintainers.
| expectedLabels map[string]string // only the user-supplied labels; defaults are checked separately | |
| expectedLabels map[string]string // labels expected to be preserved from the config file |
Replace Viper with
encoding/jsonfor config loading. encoding/json has no concept of a key-path delimiter and preserves map keys exactly as written. As a side effect: Viper's implicit AKS_NODE_CONTROLLER_* env-var override feature is removed. It was untested, undocumented, and its key-mapping behavior (lowercasing all field names) was fragile. No existing code or scripts relied on it.