From e6afcc9e6bceea2ea4bd062f089fd67ce2e5635c Mon Sep 17 00:00:00 2001 From: Riddhi Shah Date: Wed, 5 Oct 2022 15:16:46 -0700 Subject: [PATCH 1/5] Service http checks data source for agentless proxies Adds another datasource for proxycfg.HTTPChecks, for use on server agents. Typically these checks are performed by local client agents and there is no equivalent of this in agentless. Hence, it is mostly a no-op on servers but in the case where the service is present within the local state, it delegates to the cache data source. --- agent/agent.go | 1 + agent/proxycfg-glue/glue.go | 13 +-- agent/proxycfg-glue/service_http_checks.go | 39 ++++++++ .../proxycfg-glue/service_http_checks_test.go | 95 +++++++++++++++++++ 4 files changed, 139 insertions(+), 9 deletions(-) create mode 100644 agent/proxycfg-glue/service_http_checks.go create mode 100644 agent/proxycfg-glue/service_http_checks_test.go diff --git a/agent/agent.go b/agent/agent.go index 3de22c6ae5e..dd7ce5f0810 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -4399,6 +4399,7 @@ func (a *Agent) proxyDataSources() proxycfg.DataSources { sources.FederationStateListMeshGateways = proxycfgglue.ServerFederationStateListMeshGateways(deps) sources.GatewayServices = proxycfgglue.ServerGatewayServices(deps) sources.Health = proxycfgglue.ServerHealth(deps, proxycfgglue.ClientHealth(a.rpcClientHealth)) + sources.HTTPChecks = proxycfgglue.ServerHTTPChecks(deps, proxycfgglue.CacheHTTPChecks(a.cache), a.State) sources.Intentions = proxycfgglue.ServerIntentions(deps) sources.IntentionUpstreams = proxycfgglue.ServerIntentionUpstreams(deps) sources.IntentionUpstreamsDestination = proxycfgglue.ServerIntentionUpstreamsDestination(deps) diff --git a/agent/proxycfg-glue/glue.go b/agent/proxycfg-glue/glue.go index 862c09e817a..039988d8ce9 100644 --- a/agent/proxycfg-glue/glue.go +++ b/agent/proxycfg-glue/glue.go @@ -55,6 +55,8 @@ type Store interface { // // Note: there isn't a server-local equivalent of this data source because // "agentless" proxies obtain certificates via SDS served by consul-dataplane. +// If SDS is not supported on consul-dataplane, data is sourced from the server agent cache +// even for "agentless" proxies. func CacheCARoots(c *cache.Cache) proxycfg.CARoots { return &cacheProxyDataSource[*structs.DCSpecificRequest]{c, cachetype.ConnectCARootName} } @@ -74,20 +76,13 @@ func CacheServiceGateways(c *cache.Cache) proxycfg.GatewayServices { return &cacheProxyDataSource[*structs.ServiceSpecificRequest]{c, cachetype.ServiceGatewaysName} } -// CacheHTTPChecks satisifies the proxycfg.HTTPChecks interface by sourcing -// data from the agent cache. -// -// Note: there isn't a server-local equivalent of this data source because only -// services registered to the local agent can be health checked by it. -func CacheHTTPChecks(c *cache.Cache) proxycfg.HTTPChecks { - return &cacheProxyDataSource[*cachetype.ServiceHTTPChecksRequest]{c, cachetype.ServiceHTTPChecksName} -} - // CacheLeafCertificate satisifies the proxycfg.LeafCertificate interface by // sourcing data from the agent cache. // // Note: there isn't a server-local equivalent of this data source because // "agentless" proxies obtain certificates via SDS served by consul-dataplane. +// If SDS is not supported on consul-dataplane, data is sourced from the server agent cache +// even for "agentless" proxies. func CacheLeafCertificate(c *cache.Cache) proxycfg.LeafCertificate { return &cacheProxyDataSource[*cachetype.ConnectCALeafRequest]{c, cachetype.ConnectCALeafName} } diff --git a/agent/proxycfg-glue/service_http_checks.go b/agent/proxycfg-glue/service_http_checks.go new file mode 100644 index 00000000000..5b208c9cc24 --- /dev/null +++ b/agent/proxycfg-glue/service_http_checks.go @@ -0,0 +1,39 @@ +package proxycfgglue + +import ( + "context" + + "github.com/hashicorp/consul/agent/cache" + cachetype "github.com/hashicorp/consul/agent/cache-types" + "github.com/hashicorp/consul/agent/local" + "github.com/hashicorp/consul/agent/proxycfg" + "github.com/hashicorp/consul/agent/structs" +) + +// CacheHTTPChecks satisifies the proxycfg.HTTPChecks interface by sourcing +// data from the agent cache. +func CacheHTTPChecks(c *cache.Cache) proxycfg.HTTPChecks { + return &cacheProxyDataSource[*cachetype.ServiceHTTPChecksRequest]{c, cachetype.ServiceHTTPChecksName} +} + +// ServerHTTPChecks satisifies the proxycfg.HTTPChecks interface. +// It sources data from the server agent cache if the service exists in the local state, else +// it is a no-op since the checks can only be performed by local agents. +func ServerHTTPChecks(deps ServerDataSourceDeps, cacheSource proxycfg.HTTPChecks, localState *local.State) proxycfg.HTTPChecks { + return serverHTTPChecks{deps, cacheSource, localState} +} + +type serverHTTPChecks struct { + deps ServerDataSourceDeps + cacheSource proxycfg.HTTPChecks + localState *local.State +} + +func (c serverHTTPChecks) Notify(ctx context.Context, req *cachetype.ServiceHTTPChecksRequest, correlationID string, ch chan<- proxycfg.UpdateEvent) error { + // if the service exists in the server agent local state, delegate to the cache data source + if c.localState.ServiceExists(structs.ServiceID{ID: req.ServiceID, EnterpriseMeta: req.EnterpriseMeta}) { + return c.cacheSource.Notify(ctx, req, correlationID, ch) + } + c.deps.Logger.Debug("http-checks: no-op") + return nil +} diff --git a/agent/proxycfg-glue/service_http_checks_test.go b/agent/proxycfg-glue/service_http_checks_test.go new file mode 100644 index 00000000000..1f4cd9e02e9 --- /dev/null +++ b/agent/proxycfg-glue/service_http_checks_test.go @@ -0,0 +1,95 @@ +package proxycfgglue + +import ( + "context" + "errors" + "testing" + + "github.com/hashicorp/go-hclog" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + + cachetype "github.com/hashicorp/consul/agent/cache-types" + "github.com/hashicorp/consul/agent/local" + "github.com/hashicorp/consul/agent/proxycfg" + "github.com/hashicorp/consul/agent/structs" + "github.com/hashicorp/consul/agent/token" +) + +func TestServerHTTPChecks(t *testing.T) { + var ( + ctx = context.Background() + svcID = "web-sidecar-proxy-1" + req = &cachetype.ServiceHTTPChecksRequest{ServiceID: svcID} + correlationID = "correlation-id" + ch = make(chan<- proxycfg.UpdateEvent) + cacheResult = errors.New("KABOOM") + ) + + type testCase struct { + name string + serviceInLocalState bool + expectedResult error + } + + run := func(t *testing.T, tc testCase) { + serviceID := structs.NewServiceID(svcID, nil) + localState := testLocalState(t) + mockCacheSource := newMockServiceHTTPChecks(t) + if tc.serviceInLocalState { + require.NoError(t, localState.AddServiceWithChecks(&structs.NodeService{ID: serviceID.ID}, nil, "")) + mockCacheSource.On("Notify", ctx, req, correlationID, ch).Return(cacheResult) + } else { + mockCacheSource.AssertNotCalled(t, "Notify") + } + + dataSource := ServerHTTPChecks(ServerDataSourceDeps{Logger: hclog.NewNullLogger()}, mockCacheSource, localState) + err := dataSource.Notify(ctx, req, correlationID, ch) + require.Equal(t, tc.expectedResult, err) + } + + testcases := []testCase{ + { + name: "delegate to cache source if service in local state", + serviceInLocalState: true, + expectedResult: cacheResult, + }, + { + name: "no-op if service not in local state", + serviceInLocalState: false, + expectedResult: nil, + }, + } + + for _, tc := range testcases { + t.Run(tc.name, func(t *testing.T) { + run(t, tc) + }) + } + +} + +func newMockServiceHTTPChecks(t *testing.T) *mockServiceHTTPChecks { + mock := &mockServiceHTTPChecks{} + mock.Mock.Test(t) + + t.Cleanup(func() { mock.AssertExpectations(t) }) + + return mock +} + +type mockServiceHTTPChecks struct { + mock.Mock +} + +func (m *mockServiceHTTPChecks) Notify(ctx context.Context, req *cachetype.ServiceHTTPChecksRequest, correlationID string, ch chan<- proxycfg.UpdateEvent) error { + return m.Called(ctx, req, correlationID, ch).Error(0) +} + +func testLocalState(t *testing.T) *local.State { + t.Helper() + + l := local.NewState(local.Config{}, hclog.NewNullLogger(), &token.Store{}) + l.TriggerSyncChanges = func() {} + return l +} From a8062c79632e9cf3a75b0fc0c74fc2a60cfc852c Mon Sep 17 00:00:00 2001 From: Riddhi Shah Date: Fri, 7 Oct 2022 14:33:00 -0700 Subject: [PATCH 2/5] Add changelog --- .changelog/14924.txt | 3 +++ agent/proxycfg-glue/service_http_checks.go | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) create mode 100644 .changelog/14924.txt diff --git a/.changelog/14924.txt b/.changelog/14924.txt new file mode 100644 index 00000000000..4e7fa5f85ba --- /dev/null +++ b/.changelog/14924.txt @@ -0,0 +1,3 @@ +```release-note:bug +server: fixes the error trying to source proxy configuration for http checks, in case of proxies using consul-dataplane. +``` \ No newline at end of file diff --git a/agent/proxycfg-glue/service_http_checks.go b/agent/proxycfg-glue/service_http_checks.go index 5b208c9cc24..8df0b572d35 100644 --- a/agent/proxycfg-glue/service_http_checks.go +++ b/agent/proxycfg-glue/service_http_checks.go @@ -34,6 +34,6 @@ func (c serverHTTPChecks) Notify(ctx context.Context, req *cachetype.ServiceHTTP if c.localState.ServiceExists(structs.ServiceID{ID: req.ServiceID, EnterpriseMeta: req.EnterpriseMeta}) { return c.cacheSource.Notify(ctx, req, correlationID, ch) } - c.deps.Logger.Debug("http-checks: no-op") + c.deps.Logger.Debug("service-http-checks: no-op") return nil } From 69df3fe47aa6c042db69b2cd67663e5c22d62a95 Mon Sep 17 00:00:00 2001 From: Riddhi Shah Date: Mon, 10 Oct 2022 11:36:07 -0700 Subject: [PATCH 3/5] Address feedback --- agent/agent.go | 2 +- agent/cache-types/service_checks.go | 1 + agent/proxycfg-glue/service_http_checks.go | 12 ++++++---- .../proxycfg-glue/service_http_checks_test.go | 23 ++++++++++++++----- agent/proxycfg/connect_proxy.go | 1 + 5 files changed, 27 insertions(+), 12 deletions(-) diff --git a/agent/agent.go b/agent/agent.go index dd7ce5f0810..5cd3696f7cb 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -4399,7 +4399,7 @@ func (a *Agent) proxyDataSources() proxycfg.DataSources { sources.FederationStateListMeshGateways = proxycfgglue.ServerFederationStateListMeshGateways(deps) sources.GatewayServices = proxycfgglue.ServerGatewayServices(deps) sources.Health = proxycfgglue.ServerHealth(deps, proxycfgglue.ClientHealth(a.rpcClientHealth)) - sources.HTTPChecks = proxycfgglue.ServerHTTPChecks(deps, proxycfgglue.CacheHTTPChecks(a.cache), a.State) + sources.HTTPChecks = proxycfgglue.ServerHTTPChecks(deps, a.config.NodeName, proxycfgglue.CacheHTTPChecks(a.cache), a.State) sources.Intentions = proxycfgglue.ServerIntentions(deps) sources.IntentionUpstreams = proxycfgglue.ServerIntentionUpstreams(deps) sources.IntentionUpstreamsDestination = proxycfgglue.ServerIntentionUpstreamsDestination(deps) diff --git a/agent/cache-types/service_checks.go b/agent/cache-types/service_checks.go index a42cb3a8ef5..3688db7e2ba 100644 --- a/agent/cache-types/service_checks.go +++ b/agent/cache-types/service_checks.go @@ -103,6 +103,7 @@ func (c *ServiceHTTPChecks) Fetch(opts cache.FetchOptions, req cache.Request) (c // directly to any Consul servers. type ServiceHTTPChecksRequest struct { ServiceID string + NodeName string MinQueryIndex uint64 MaxQueryTime time.Duration acl.EnterpriseMeta diff --git a/agent/proxycfg-glue/service_http_checks.go b/agent/proxycfg-glue/service_http_checks.go index 8df0b572d35..da62c516749 100644 --- a/agent/proxycfg-glue/service_http_checks.go +++ b/agent/proxycfg-glue/service_http_checks.go @@ -19,21 +19,23 @@ func CacheHTTPChecks(c *cache.Cache) proxycfg.HTTPChecks { // ServerHTTPChecks satisifies the proxycfg.HTTPChecks interface. // It sources data from the server agent cache if the service exists in the local state, else // it is a no-op since the checks can only be performed by local agents. -func ServerHTTPChecks(deps ServerDataSourceDeps, cacheSource proxycfg.HTTPChecks, localState *local.State) proxycfg.HTTPChecks { - return serverHTTPChecks{deps, cacheSource, localState} +func ServerHTTPChecks(deps ServerDataSourceDeps, nodeName string, cacheSource proxycfg.HTTPChecks, localState *local.State) proxycfg.HTTPChecks { + return serverHTTPChecks{deps, nodeName, cacheSource, localState} } type serverHTTPChecks struct { deps ServerDataSourceDeps + nodeName string cacheSource proxycfg.HTTPChecks localState *local.State } func (c serverHTTPChecks) Notify(ctx context.Context, req *cachetype.ServiceHTTPChecksRequest, correlationID string, ch chan<- proxycfg.UpdateEvent) error { - // if the service exists in the server agent local state, delegate to the cache data source - if c.localState.ServiceExists(structs.ServiceID{ID: req.ServiceID, EnterpriseMeta: req.EnterpriseMeta}) { + // if the service exists in the current server agent local state, delegate to the cache data source + if req.NodeName == c.nodeName && c.localState.ServiceExists(structs.ServiceID{ID: req.ServiceID, EnterpriseMeta: req.EnterpriseMeta}) { return c.cacheSource.Notify(ctx, req, correlationID, ch) } - c.deps.Logger.Debug("service-http-checks: no-op") + l := c.deps.Logger.With("service_id", req.ServiceID) + l.Debug("service-http-checks: no-op") return nil } diff --git a/agent/proxycfg-glue/service_http_checks_test.go b/agent/proxycfg-glue/service_http_checks_test.go index 1f4cd9e02e9..fc1e68124c7 100644 --- a/agent/proxycfg-glue/service_http_checks_test.go +++ b/agent/proxycfg-glue/service_http_checks_test.go @@ -20,15 +20,16 @@ func TestServerHTTPChecks(t *testing.T) { var ( ctx = context.Background() svcID = "web-sidecar-proxy-1" - req = &cachetype.ServiceHTTPChecksRequest{ServiceID: svcID} correlationID = "correlation-id" ch = make(chan<- proxycfg.UpdateEvent) cacheResult = errors.New("KABOOM") + nodeName = "server-1" ) type testCase struct { name string serviceInLocalState bool + req *cachetype.ServiceHTTPChecksRequest expectedResult error } @@ -38,25 +39,35 @@ func TestServerHTTPChecks(t *testing.T) { mockCacheSource := newMockServiceHTTPChecks(t) if tc.serviceInLocalState { require.NoError(t, localState.AddServiceWithChecks(&structs.NodeService{ID: serviceID.ID}, nil, "")) - mockCacheSource.On("Notify", ctx, req, correlationID, ch).Return(cacheResult) + } + if tc.req.NodeName == nodeName && tc.serviceInLocalState { + mockCacheSource.On("Notify", ctx, tc.req, correlationID, ch).Return(cacheResult) } else { mockCacheSource.AssertNotCalled(t, "Notify") } - dataSource := ServerHTTPChecks(ServerDataSourceDeps{Logger: hclog.NewNullLogger()}, mockCacheSource, localState) - err := dataSource.Notify(ctx, req, correlationID, ch) + dataSource := ServerHTTPChecks(ServerDataSourceDeps{Logger: hclog.NewNullLogger()}, nodeName, mockCacheSource, localState) + err := dataSource.Notify(ctx, tc.req, correlationID, ch) require.Equal(t, tc.expectedResult, err) } testcases := []testCase{ { - name: "delegate to cache source if service in local state", + name: "delegate to cache source if service in local state of the server node", serviceInLocalState: true, + req: &cachetype.ServiceHTTPChecksRequest{ServiceID: svcID, NodeName: nodeName}, expectedResult: cacheResult, }, { - name: "no-op if service not in local state", + name: "no-op if service not in local state of server node", serviceInLocalState: false, + req: &cachetype.ServiceHTTPChecksRequest{ServiceID: svcID, NodeName: nodeName}, + expectedResult: nil, + }, + { + name: "no-op if service with same ID in local state but belongs to different node", + serviceInLocalState: true, + req: &cachetype.ServiceHTTPChecksRequest{ServiceID: svcID, NodeName: "server-2"}, expectedResult: nil, }, } diff --git a/agent/proxycfg/connect_proxy.go b/agent/proxycfg/connect_proxy.go index 69764843d6a..1ac1cff2cfe 100644 --- a/agent/proxycfg/connect_proxy.go +++ b/agent/proxycfg/connect_proxy.go @@ -96,6 +96,7 @@ func (s *handlerConnectProxy) initialize(ctx context.Context) (ConfigSnapshot, e // Watch for service check updates err = s.dataSources.HTTPChecks.Notify(ctx, &cachetype.ServiceHTTPChecksRequest{ ServiceID: s.proxyCfg.DestinationServiceID, + NodeName: s.source.Node, EnterpriseMeta: s.proxyID.EnterpriseMeta, }, svcChecksWatchIDPrefix+structs.ServiceIDString(s.proxyCfg.DestinationServiceID, &s.proxyID.EnterpriseMeta), s.ch) if err != nil { From 653206413d16320fc80069a12c887d986f37922e Mon Sep 17 00:00:00 2001 From: Riddhi Shah Date: Mon, 10 Oct 2022 16:12:12 -0700 Subject: [PATCH 4/5] Include nodename in req for test --- agent/service_checks_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/agent/service_checks_test.go b/agent/service_checks_test.go index 19c6c60be97..1e987221926 100644 --- a/agent/service_checks_test.go +++ b/agent/service_checks_test.go @@ -40,6 +40,7 @@ func TestAgent_ServiceHTTPChecksNotification(t *testing.T) { // Watch for service check updates err := a.cache.Notify(ctx, cachetype.ServiceHTTPChecksName, &cachetype.ServiceHTTPChecksRequest{ ServiceID: service.ID, + NodeName: a.Config.NodeName, }, "service-checks:"+service.ID, ch) if err != nil { t.Fatalf("failed to set cache notification: %v", err) From ac79b85949273e137022fe13121922ebc3b48559 Mon Sep 17 00:00:00 2001 From: Riddhi Shah Date: Tue, 11 Oct 2022 09:25:13 -0700 Subject: [PATCH 5/5] Pass node name in proxy mgr config --- agent/agent.go | 1 + 1 file changed, 1 insertion(+) diff --git a/agent/agent.go b/agent/agent.go index 5cd3696f7cb..f59b2497c4d 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -671,6 +671,7 @@ func (a *Agent) Start(ctx context.Context) error { Source: &structs.QuerySource{ Datacenter: a.config.Datacenter, Segment: a.config.SegmentName, + Node: a.config.NodeName, NodePartition: a.config.PartitionOrEmpty(), }, DNSConfig: proxycfg.DNSConfig{