From 1b57c785cd74afdad0493a6af87a36c924faf2c9 Mon Sep 17 00:00:00 2001 From: Abhishek Choudhary Date: Tue, 26 May 2026 12:23:30 +0800 Subject: [PATCH] fix(config): reject bare and null forms of unsupported sections Closes #33. The previous `cfg.
!= nil` checks only rejected explicitly-empty sections (`upstreams: []`). YAML like `upstreams:` (bare key, no value) and `upstreams: null` unmarshalled to a nil slice and slipped past, even though the user had clearly declared an unsupported section. - Add `markPresentUnsupportedSections` in configutil: a two-pass parse over the raw input (`map[string]json.RawMessage` for JSON, `map[string]yaml.Node` for YAML) that flips the four unsupported section fields from nil to a non-nil empty slice when the key is present, regardless of its value shape. - Route `config validate` through `configutil.ReadConfigFile` so it inherits the same key-presence detection that diff/sync already use. Drops validate's now-duplicate file-reading code. - Extend `TestConfigValidate_EmptyUnsupportedSections` with 12 subtests covering empty-list, bare, and null shapes for all four sections, plus 2 JSON-null variants. All pass. --- pkg/cmd/config/configutil/configutil.go | 59 +++++++++++++++++++-- pkg/cmd/config/validate/validate.go | 34 ++----------- pkg/cmd/config/validate/validate_test.go | 65 +++++++++++++++++++++--- 3 files changed, 118 insertions(+), 40 deletions(-) diff --git a/pkg/cmd/config/configutil/configutil.go b/pkg/cmd/config/configutil/configutil.go index fcb3a5b..ed54000 100644 --- a/pkg/cmd/config/configutil/configutil.go +++ b/pkg/cmd/config/configutil/configutil.go @@ -100,9 +100,61 @@ func ReadConfigFile(file string) (api.ConfigFile, error) { } } + if err := markPresentUnsupportedSections(trimmed, &cfg); err != nil { + return api.ConfigFile{}, err + } + return cfg, nil } +// markPresentUnsupportedSections does a key-presence pass over the raw input +// and flips unsupported-section slice fields from nil to an empty (but non-nil) +// slice when the key was declared. This lets the downstream `!= nil` checks in +// ValidateSupportedSections / ValidateConfigFile reject bare (`upstreams:`) and +// null (`upstreams: null`) forms, not just explicit `upstreams: []`. +func markPresentUnsupportedSections(trimmed []byte, cfg *api.ConfigFile) error { + if len(trimmed) == 0 { + return nil + } + + keys := map[string]bool{} + switch trimmed[0] { + case '[': + // Top-level JSON/YAML array can't carry section keys. + return nil + case '{': + var raw map[string]json.RawMessage + if err := json.Unmarshal(trimmed, &raw); err != nil { + return fmt.Errorf("failed to parse JSON file: %w", err) + } + for k := range raw { + keys[k] = true + } + default: + var raw map[string]yaml.Node + if err := yaml.Unmarshal(trimmed, &raw); err != nil { + return fmt.Errorf("failed to parse YAML file: %w", err) + } + for k := range raw { + keys[k] = true + } + } + + if keys["upstreams"] && cfg.Upstreams == nil { + cfg.Upstreams = []api.Upstream{} + } + if keys["consumer_groups"] && cfg.ConsumerGroups == nil { + cfg.ConsumerGroups = []api.ConsumerGroup{} + } + if keys["plugin_configs"] && cfg.PluginConfigs == nil { + cfg.PluginConfigs = []interface{}{} + } + if keys["service_templates"] && cfg.ServiceTemplates == nil { + cfg.ServiceTemplates = []interface{}{} + } + return nil +} + // FetchRemoteConfig fetches all runtime resources from API7 EE // for the given gateway group and assembles them into a ConfigFile. func FetchRemoteConfig(client *api.Client, gatewayGroup string) (*api.ConfigFile, error) { @@ -227,10 +279,9 @@ func ComputeDiff(local, remote api.ConfigFile) (*DiffResult, error) { func ValidateSupportedSections(cfg api.ConfigFile) error { var unsupported []string - // Reject explicitly-empty unsupported sections (e.g. `upstreams: []`) - // in addition to non-empty ones. Bare `upstreams:` or `upstreams: null` - // unmarshal as nil and slip past this check; that's a separate hardening - // (would require yaml.Node / two-pass parsing) tracked elsewhere. + // Bare (`upstreams:`) and null (`upstreams: null`) forms are normalized to + // non-nil empty slices in ReadConfigFile via markPresentUnsupportedSections, + // so this typed nil-check rejects all three shapes ([], bare, null). if cfg.Upstreams != nil { unsupported = append(unsupported, "upstreams") } diff --git a/pkg/cmd/config/validate/validate.go b/pkg/cmd/config/validate/validate.go index 421107c..550b7b6 100644 --- a/pkg/cmd/config/validate/validate.go +++ b/pkg/cmd/config/validate/validate.go @@ -1,20 +1,17 @@ package validate import ( - "bytes" - "encoding/json" "fmt" "net/http" - "os" "regexp" "strings" "github.com/spf13/cobra" - "gopkg.in/yaml.v3" "github.com/api7/a7/internal/config" "github.com/api7/a7/pkg/api" cmd "github.com/api7/a7/pkg/cmd" + "github.com/api7/a7/pkg/cmd/config/configutil" "github.com/api7/a7/pkg/cmdutil" "github.com/api7/a7/pkg/iostreams" ) @@ -54,23 +51,11 @@ func NewCmdValidate(f *cmd.Factory) *cobra.Command { } func validateRun(opts *Options) error { - data, err := readFile(opts.File) + cfg, err := configutil.ReadConfigFile(opts.File) if err != nil { return err } - var cfg api.ConfigFile - trimmed := bytes.TrimSpace(data) - if len(trimmed) > 0 && (trimmed[0] == '{' || trimmed[0] == '[') { - if err := json.Unmarshal(trimmed, &cfg); err != nil { - return fmt.Errorf("failed to parse JSON file: %w", err) - } - } else { - if err := yaml.Unmarshal(trimmed, &cfg); err != nil { - return fmt.Errorf("failed to parse YAML file: %w", err) - } - } - errs := ValidateConfigFile(cfg) if len(errs) > 0 { return fmt.Errorf("config validation failed:\n- %s", strings.Join(errs, "\n- ")) @@ -80,14 +65,6 @@ func validateRun(opts *Options) error { return nil } -func readFile(path string) ([]byte, error) { - data, err := os.ReadFile(path) - if err != nil { - return nil, fmt.Errorf("failed to read file: %w", err) - } - return data, nil -} - func ValidateConfigFile(cfg api.ConfigFile) []string { var errs []string @@ -97,10 +74,9 @@ func ValidateConfigFile(cfg api.ConfigFile) []string { errs = append(errs, "version must be \"1\"") } - // Reject explicitly-empty unsupported sections (e.g. `upstreams: []`) - // in addition to non-empty ones. Bare `upstreams:` or `upstreams: null` - // unmarshal as nil and slip past this check; that's a separate hardening - // (would require yaml.Node / two-pass parsing) tracked elsewhere. + // Bare (`upstreams:`) and null (`upstreams: null`) forms are normalized to + // non-nil empty slices in configutil.ReadConfigFile, so this typed nil-check + // rejects all three shapes ([], bare, null). if cfg.Upstreams != nil { errs = append(errs, "upstreams are not supported as top-level API7 EE resources; define upstream inline on services instead") } diff --git a/pkg/cmd/config/validate/validate_test.go b/pkg/cmd/config/validate/validate_test.go index 5ae55e7..38b55c6 100644 --- a/pkg/cmd/config/validate/validate_test.go +++ b/pkg/cmd/config/validate/validate_test.go @@ -288,9 +288,10 @@ func TestConfigValidate_MissingFileFlag(t *testing.T) { } // TestConfigValidate_EmptyUnsupportedSections asserts that declaring an -// unsupported top-level section (upstreams, consumer_groups, service_templates) -// is rejected even when the section is explicitly empty. Presence alone is -// enough — the user is asserting an unsupported resource type. +// unsupported top-level section (upstreams, consumer_groups, plugin_configs, +// service_templates) is rejected for every "empty" shape the user might write: +// explicit `[]`, bare (`upstreams:` with no value), and `null`. Presence of +// the key alone is enough — the user is asserting an unsupported resource type. func TestConfigValidate_EmptyUnsupportedSections(t *testing.T) { cases := []struct { name string @@ -298,25 +299,75 @@ func TestConfigValidate_EmptyUnsupportedSections(t *testing.T) { wantErr string }{ { - name: "upstreams", + name: "upstreams_empty_list", body: "version: \"1\"\nupstreams: []\n", wantErr: "upstreams are not supported", }, { - name: "consumer_groups", + name: "upstreams_bare", + body: "version: \"1\"\nupstreams:\n", + wantErr: "upstreams are not supported", + }, + { + name: "upstreams_null", + body: "version: \"1\"\nupstreams: null\n", + wantErr: "upstreams are not supported", + }, + { + name: "consumer_groups_empty_list", body: "version: \"1\"\nconsumer_groups: []\n", wantErr: "consumer_groups are not supported", }, { - name: "plugin_configs", + name: "consumer_groups_bare", + body: "version: \"1\"\nconsumer_groups:\n", + wantErr: "consumer_groups are not supported", + }, + { + name: "consumer_groups_null", + body: "version: \"1\"\nconsumer_groups: null\n", + wantErr: "consumer_groups are not supported", + }, + { + name: "plugin_configs_empty_list", body: "version: \"1\"\nplugin_configs: []\n", wantErr: "plugin_configs are not supported", }, { - name: "service_templates", + name: "plugin_configs_bare", + body: "version: \"1\"\nplugin_configs:\n", + wantErr: "plugin_configs are not supported", + }, + { + name: "plugin_configs_null", + body: "version: \"1\"\nplugin_configs: null\n", + wantErr: "plugin_configs are not supported", + }, + { + name: "service_templates_empty_list", body: "version: \"1\"\nservice_templates: []\n", wantErr: "service_templates are not supported", }, + { + name: "service_templates_bare", + body: "version: \"1\"\nservice_templates:\n", + wantErr: "service_templates are not supported", + }, + { + name: "service_templates_null", + body: "version: \"1\"\nservice_templates: null\n", + wantErr: "service_templates are not supported", + }, + { + name: "json_upstreams_null", + body: `{"version": "1", "upstreams": null}`, + wantErr: "upstreams are not supported", + }, + { + name: "json_consumer_groups_null", + body: `{"version": "1", "consumer_groups": null}`, + wantErr: "consumer_groups are not supported", + }, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) {