From cc2c8fb92b6eb7a0ce318d269ff5c4d2bcd5f712 Mon Sep 17 00:00:00 2001 From: Poonam Jadhav Date: Mon, 26 Aug 2024 11:10:57 -0400 Subject: [PATCH] NET-5912/service-defaults protocol validation (#21593) * fix: add validation for protocol field on service-defaults config entry * test: update test cases with correct protocol --- agent/config/builder.go | 4 +++- agent/config_endpoint_test.go | 4 ++-- agent/consul/config_endpoint.go | 4 +++- agent/consul/config_replication_test.go | 6 +++--- agent/structs/config_entry.go | 9 ++++++-- agent/structs/config_entry_test.go | 10 ++++++++- api/config_entry_test.go | 6 +++--- command/config/write/config_write_test.go | 25 +++++++++++++++++++++-- 8 files changed, 53 insertions(+), 15 deletions(-) diff --git a/agent/config/builder.go b/agent/config/builder.go index 64e9120fde..5c2eba5c55 100644 --- a/agent/config/builder.go +++ b/agent/config/builder.go @@ -22,12 +22,13 @@ import ( "time" "github.com/armon/go-metrics/prometheus" + "golang.org/x/time/rate" + "github.com/hashicorp/go-bexpr" "github.com/hashicorp/go-hclog" "github.com/hashicorp/go-multierror" "github.com/hashicorp/go-sockaddr/template" "github.com/hashicorp/memberlist" - "golang.org/x/time/rate" "github.com/hashicorp/consul/agent/cache" "github.com/hashicorp/consul/agent/checks" @@ -774,6 +775,7 @@ func (b *builder) build() (rt RuntimeConfig, err error) { if err != nil { return RuntimeConfig{}, fmt.Errorf("config_entries.bootstrap[%d]: %s", i, err) } + // Ensure Normalize is called before Validate for accurate validation if err := entry.Normalize(); err != nil { return RuntimeConfig{}, fmt.Errorf("config_entries.bootstrap[%d]: %s", i, err) } diff --git a/agent/config_endpoint_test.go b/agent/config_endpoint_test.go index 8697b55e5b..ec00b172d1 100644 --- a/agent/config_endpoint_test.go +++ b/agent/config_endpoint_test.go @@ -612,7 +612,7 @@ func TestConfig_Apply_CAS(t *testing.T) { { "Kind": "service-defaults", "Name": "foo", - "Protocol": "udp" + "Protocol": "http" } `)) req, _ = http.NewRequest("PUT", "/v1/config?cas=0", body) @@ -628,7 +628,7 @@ func TestConfig_Apply_CAS(t *testing.T) { { "Kind": "service-defaults", "Name": "foo", - "Protocol": "udp" + "Protocol": "http" } `)) req, _ = http.NewRequest("PUT", fmt.Sprintf("/v1/config?cas=%d", entry.GetRaftIndex().ModifyIndex), body) diff --git a/agent/consul/config_endpoint.go b/agent/consul/config_endpoint.go index a78859c350..96906dac68 100644 --- a/agent/consul/config_endpoint.go +++ b/agent/consul/config_endpoint.go @@ -10,10 +10,11 @@ import ( metrics "github.com/armon/go-metrics" "github.com/armon/go-metrics/prometheus" + hashstructure_v2 "github.com/mitchellh/hashstructure/v2" + "github.com/hashicorp/go-bexpr" "github.com/hashicorp/go-hclog" memdb "github.com/hashicorp/go-memdb" - hashstructure_v2 "github.com/mitchellh/hashstructure/v2" "github.com/hashicorp/consul/acl" "github.com/hashicorp/consul/agent/configentry" @@ -85,6 +86,7 @@ func (c *ConfigEntry) Apply(args *structs.ConfigEntryRequest, reply *bool) error } // Normalize and validate the incoming config entry as if it came from a user. + // Ensure Normalize is called before Validate for accurate validation if err := args.Entry.Normalize(); err != nil { return err } diff --git a/agent/consul/config_replication_test.go b/agent/consul/config_replication_test.go index e2c4fbee8d..3117e046a4 100644 --- a/agent/consul/config_replication_test.go +++ b/agent/consul/config_replication_test.go @@ -6,11 +6,11 @@ package consul import ( "context" "fmt" - "github.com/oklog/ulid/v2" - "github.com/stretchr/testify/assert" "os" "testing" + "github.com/oklog/ulid/v2" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/hashicorp/consul/agent/structs" @@ -129,7 +129,7 @@ func TestReplication_ConfigEntries(t *testing.T) { Entry: &structs.ServiceConfigEntry{ Kind: structs.ServiceDefaults, Name: fmt.Sprintf("svc-%d", i), - Protocol: "udp", + Protocol: "tcp", }, } diff --git a/agent/structs/config_entry.go b/agent/structs/config_entry.go index 5d419e0832..32b4e0c89d 100644 --- a/agent/structs/config_entry.go +++ b/agent/structs/config_entry.go @@ -12,12 +12,11 @@ import ( "time" "github.com/miekg/dns" - - "github.com/hashicorp/go-multierror" "github.com/mitchellh/hashstructure" "github.com/mitchellh/mapstructure" "github.com/hashicorp/consul-net-rpc/go-msgpack/codec" + "github.com/hashicorp/go-multierror" "github.com/hashicorp/consul/acl" "github.com/hashicorp/consul/agent/cache" @@ -269,6 +268,12 @@ func (e *ServiceConfigEntry) Validate() error { validationErr = multierror.Append(validationErr, fmt.Errorf("invalid value for balance_inbound_connections: %v", e.BalanceInboundConnections)) } + switch e.Protocol { + case "", "http", "http2", "grpc", "tcp": + default: + validationErr = multierror.Append(validationErr, fmt.Errorf("invalid value for protocol: %v", e.Protocol)) + } + // External endpoints are invalid with an existing service's upstream configuration if e.UpstreamConfig != nil && e.Destination != nil { validationErr = multierror.Append(validationErr, errors.New("UpstreamConfig and Destination are mutually exclusive for service defaults")) diff --git a/agent/structs/config_entry_test.go b/agent/structs/config_entry_test.go index e57e2c4041..4c092acc46 100644 --- a/agent/structs/config_entry_test.go +++ b/agent/structs/config_entry_test.go @@ -10,12 +10,12 @@ import ( "time" "github.com/google/go-cmp/cmp" - "github.com/hashicorp/hcl" "github.com/mitchellh/copystructure" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/hashicorp/consul-net-rpc/go-msgpack/codec" + "github.com/hashicorp/hcl" "github.com/hashicorp/consul/acl" "github.com/hashicorp/consul/agent/cache" @@ -3225,6 +3225,14 @@ func TestServiceConfigEntry(t *testing.T) { }, validateErr: `Invalid MutualTLSMode "invalid-mtls-mode". Must be one of "", "strict", or "permissive".`, }, + "validate: invalid Protocol in service-defaults": { + entry: &ServiceConfigEntry{ + Kind: ServiceDefaults, + Name: "web", + Protocol: "blah", + }, + validateErr: `invalid value for protocol: blah`, + }, } testConfigEntryNormalizeAndValidate(t, cases) } diff --git a/api/config_entry_test.go b/api/config_entry_test.go index 2461c387b3..d6fe8373c9 100644 --- a/api/config_entry_test.go +++ b/api/config_entry_test.go @@ -104,7 +104,7 @@ func TestAPI_ConfigEntries(t *testing.T) { service := &ServiceConfigEntry{ Kind: ServiceDefaults, Name: "foo", - Protocol: "udp", + Protocol: "http", MutualTLSMode: MutualTLSModeStrict, Meta: map[string]string{ "foo": "bar", @@ -124,7 +124,7 @@ func TestAPI_ConfigEntries(t *testing.T) { service2 := &ServiceConfigEntry{ Kind: ServiceDefaults, Name: "bar", - Protocol: "tcp", + Protocol: "http", Destination: dest, } @@ -176,7 +176,7 @@ func TestAPI_ConfigEntries(t *testing.T) { require.True(t, written) // update no cas - service.Protocol = "http" + service.Protocol = "tcp" _, wm, err = config_entries.Set(service, nil) require.NoError(t, err) diff --git a/command/config/write/config_write_test.go b/command/config/write/config_write_test.go index 15e7a47465..ae782c0826 100644 --- a/command/config/write/config_write_test.go +++ b/command/config/write/config_write_test.go @@ -43,7 +43,7 @@ func TestConfigWrite(t *testing.T) { _, err := f.WriteString(` Kind = "service-defaults" Name = "web" - Protocol = "udp" + Protocol = "tcp" `) require.NoError(t, err) @@ -65,7 +65,7 @@ func TestConfigWrite(t *testing.T) { require.True(t, ok) require.Equal(t, api.ServiceDefaults, svc.Kind) require.Equal(t, "web", svc.Name) - require.Equal(t, "udp", svc.Protocol) + require.Equal(t, "tcp", svc.Protocol) }) t.Run("Stdin", func(t *testing.T) { @@ -170,6 +170,27 @@ kind = "proxy-defaults" `Config entry written: proxy-defaults/global`) require.Equal(t, 0, code) }) + + // Test that protocol field is first normalized and then validated + // before writing the config entry + t.Run("service defaults config entry mixed case in protocol field", func(t *testing.T) { + stdin := new(bytes.Buffer) + stdin.WriteString(` + Kind = "service-defaults" + Name = "web" + Protocol = "TcP" +`) + + ui := cli.NewMockUi() + c := New(ui) + c.testStdin = stdin + + code := c.Run([]string{"-http-addr=" + a.HTTPAddr(), "-"}) + require.Empty(t, ui.ErrorWriter.String()) + require.Contains(t, ui.OutputWriter.String(), + `Config entry written: service-defaults/web`) + require.Equal(t, 0, code) + }) } func TestConfigWrite_Warning(t *testing.T) {