From 59e81a728eea9198002a226ae4e43e921dba26e8 Mon Sep 17 00:00:00 2001 From: cskh Date: Tue, 26 Jul 2022 16:54:53 -0400 Subject: [PATCH] chore: removed unused method AddService (#13905) - This AddService is not used anywhere. AddServiceWithChecks is place of AddService - Test code is updated --- agent/acl_test.go | 16 ++--- agent/agent_endpoint_test.go | 18 ++--- agent/local/state.go | 13 +--- agent/local/state_test.go | 70 +++++++++---------- .../catalog/config_source_test.go | 2 +- agent/proxycfg-sources/local/sync_test.go | 8 +-- agent/user_event_test.go | 4 +- 7 files changed, 62 insertions(+), 69 deletions(-) diff --git a/agent/acl_test.go b/agent/acl_test.go index 2e8664c9f2..79cc5f7b70 100644 --- a/agent/acl_test.go +++ b/agent/acl_test.go @@ -274,10 +274,10 @@ func TestACL_vetServiceRegister(t *testing.T) { // Try to register over a service without write privs to the existing // service. - a.State.AddService(&structs.NodeService{ + a.State.AddServiceWithChecks(&structs.NodeService{ ID: "my-service", Service: "other", - }, "") + }, nil, "") err = a.vetServiceRegister(serviceRWSecret, &structs.NodeService{ ID: "my-service", Service: "service", @@ -304,10 +304,10 @@ func TestACL_vetServiceUpdateWithAuthorizer(t *testing.T) { require.Contains(t, err.Error(), "Unknown service") // Update with write privs. - a.State.AddService(&structs.NodeService{ + a.State.AddServiceWithChecks(&structs.NodeService{ ID: "my-service", Service: "service", - }, "") + }, nil, "") err = vetServiceUpdate(serviceRWSecret, structs.NewServiceID("my-service", nil)) require.NoError(t, err) @@ -361,10 +361,10 @@ func TestACL_vetCheckRegisterWithAuthorizer(t *testing.T) { // Try to register over a service check without write privs to the // existing service. - a.State.AddService(&structs.NodeService{ + a.State.AddServiceWithChecks(&structs.NodeService{ ID: "my-service", Service: "service", - }, "") + }, nil, "") a.State.AddCheck(&structs.HealthCheck{ CheckID: types.CheckID("my-check"), ServiceID: "my-service", @@ -410,10 +410,10 @@ func TestACL_vetCheckUpdateWithAuthorizer(t *testing.T) { require.Contains(t, err.Error(), "Unknown check") // Update service check with write privs. - a.State.AddService(&structs.NodeService{ + a.State.AddServiceWithChecks(&structs.NodeService{ ID: "my-service", Service: "service", - }, "") + }, nil, "") a.State.AddCheck(&structs.HealthCheck{ CheckID: types.CheckID("my-service-check"), ServiceID: "my-service", diff --git a/agent/agent_endpoint_test.go b/agent/agent_endpoint_test.go index 7bde623872..270cc7dc13 100644 --- a/agent/agent_endpoint_test.go +++ b/agent/agent_endpoint_test.go @@ -93,7 +93,7 @@ func TestAgent_Services(t *testing.T) { }, Port: 5000, } - require.NoError(t, a.State.AddService(srv1, "")) + require.NoError(t, a.State.AddServiceWithChecks(srv1, nil, "")) req, _ := http.NewRequest("GET", "/v1/agent/services", nil) resp := httptest.NewRecorder() @@ -128,7 +128,7 @@ func TestAgent_ServicesFiltered(t *testing.T) { }, Port: 5000, } - require.NoError(t, a.State.AddService(srv1, "")) + require.NoError(t, a.State.AddServiceWithChecks(srv1, nil, "")) // Add another service srv2 := &structs.NodeService{ @@ -140,7 +140,7 @@ func TestAgent_ServicesFiltered(t *testing.T) { }, Port: 1234, } - require.NoError(t, a.State.AddService(srv2, "")) + require.NoError(t, a.State.AddServiceWithChecks(srv2, nil, "")) req, _ := http.NewRequest("GET", "/v1/agent/services?filter="+url.QueryEscape("foo in Meta"), nil) resp := httptest.NewRecorder() @@ -188,7 +188,7 @@ func TestAgent_Services_ExternalConnectProxy(t *testing.T) { Upstreams: structs.TestUpstreams(t), }, } - a.State.AddService(srv1, "") + a.State.AddServiceWithChecks(srv1, nil, "") req, _ := http.NewRequest("GET", "/v1/agent/services", nil) resp := httptest.NewRecorder() @@ -232,7 +232,7 @@ func TestAgent_Services_Sidecar(t *testing.T) { }, }, } - a.State.AddService(srv1, "") + a.State.AddServiceWithChecks(srv1, nil, "") req, _ := http.NewRequest("GET", "/v1/agent/services", nil) resp := httptest.NewRecorder() @@ -281,7 +281,7 @@ func TestAgent_Services_MeshGateway(t *testing.T) { }, }, } - a.State.AddService(srv1, "") + a.State.AddServiceWithChecks(srv1, nil, "") req, _ := http.NewRequest("GET", "/v1/agent/services", nil) resp := httptest.NewRecorder() @@ -325,7 +325,7 @@ func TestAgent_Services_TerminatingGateway(t *testing.T) { }, }, } - require.NoError(t, a.State.AddService(srv1, "")) + require.NoError(t, a.State.AddServiceWithChecks(srv1, nil, "")) req, _ := http.NewRequest("GET", "/v1/agent/services", nil) resp := httptest.NewRecorder() @@ -370,7 +370,7 @@ func TestAgent_Services_ACLFilter(t *testing.T) { }, } for _, s := range services { - a.State.AddService(s, "") + a.State.AddServiceWithChecks(s, nil, "") } t.Run("no token", func(t *testing.T) { @@ -7994,7 +7994,7 @@ func TestAgent_Services_ExposeConfig(t *testing.T) { }, }, } - a.State.AddService(srv1, "") + a.State.AddServiceWithChecks(srv1, nil, "") req, _ := http.NewRequest("GET", "/v1/agent/services", nil) resp := httptest.NewRecorder() diff --git a/agent/local/state.go b/agent/local/state.go index 74641a0683..7909982dba 100644 --- a/agent/local/state.go +++ b/agent/local/state.go @@ -256,15 +256,6 @@ func (l *State) aclTokenForServiceSync(id structs.ServiceID, fallback func() str return fallback() } -// AddService is used to add a service entry to the local state. -// This entry is persistent and the agent will make a best effort to -// ensure it is registered -func (l *State) AddService(service *structs.NodeService, token string) error { - l.Lock() - defer l.Unlock() - return l.addServiceLocked(service, token) -} - func (l *State) addServiceLocked(service *structs.NodeService, token string) error { if service == nil { return fmt.Errorf("no service") @@ -293,7 +284,9 @@ func (l *State) addServiceLocked(service *structs.NodeService, token string) err return nil } -// AddServiceWithChecks adds a service and its check tp the local state atomically +// AddServiceWithChecks adds a service entry and its checks to the local state atomically +// This entry is persistent and the agent will make a best effort to +// ensure it is registered func (l *State) AddServiceWithChecks(service *structs.NodeService, checks []*structs.HealthCheck, token string) error { l.Lock() defer l.Unlock() diff --git a/agent/local/state_test.go b/agent/local/state_test.go index 686c86a935..7aa539ea0b 100644 --- a/agent/local/state_test.go +++ b/agent/local/state_test.go @@ -64,7 +64,7 @@ func TestAgentAntiEntropy_Services(t *testing.T) { EnterpriseMeta: *structs.DefaultEnterpriseMetaInDefaultPartition(), } assert.False(t, a.State.ServiceExists(structs.ServiceID{ID: srv1.ID})) - a.State.AddService(srv1, "") + a.State.AddServiceWithChecks(srv1, nil, "") assert.True(t, a.State.ServiceExists(structs.ServiceID{ID: srv1.ID})) args.Service = srv1 if err := a.RPC("Catalog.Register", args, &out); err != nil { @@ -83,7 +83,7 @@ func TestAgentAntiEntropy_Services(t *testing.T) { }, EnterpriseMeta: *structs.DefaultEnterpriseMetaInDefaultPartition(), } - a.State.AddService(srv2, "") + a.State.AddServiceWithChecks(srv2, nil, "") srv2_mod := new(structs.NodeService) *srv2_mod = *srv2 @@ -105,7 +105,7 @@ func TestAgentAntiEntropy_Services(t *testing.T) { }, EnterpriseMeta: *structs.DefaultEnterpriseMetaInDefaultPartition(), } - a.State.AddService(srv3, "") + a.State.AddServiceWithChecks(srv3, nil, "") // Exists remote (delete) srv4 := &structs.NodeService{ @@ -137,7 +137,7 @@ func TestAgentAntiEntropy_Services(t *testing.T) { }, EnterpriseMeta: *structs.DefaultEnterpriseMetaInDefaultPartition(), } - a.State.AddService(srv5, "") + a.State.AddServiceWithChecks(srv5, nil, "") srv5_mod := new(structs.NodeService) *srv5_mod = *srv5 @@ -290,7 +290,7 @@ func TestAgentAntiEntropy_Services_ConnectProxy(t *testing.T) { }, EnterpriseMeta: *structs.DefaultEnterpriseMetaInDefaultPartition(), } - a.State.AddService(srv1, "") + a.State.AddServiceWithChecks(srv1, nil, "") require.NoError(t, a.RPC("Catalog.Register", &structs.RegisterRequest{ Datacenter: "dc1", Node: a.Config.NodeName, @@ -311,7 +311,7 @@ func TestAgentAntiEntropy_Services_ConnectProxy(t *testing.T) { }, EnterpriseMeta: *structs.DefaultEnterpriseMetaInDefaultPartition(), } - a.State.AddService(srv2, "") + a.State.AddServiceWithChecks(srv2, nil, "") srv2_mod := clone(srv2) srv2_mod.Port = 9000 @@ -335,7 +335,7 @@ func TestAgentAntiEntropy_Services_ConnectProxy(t *testing.T) { }, EnterpriseMeta: *structs.DefaultEnterpriseMetaInDefaultPartition(), } - a.State.AddService(srv3, "") + a.State.AddServiceWithChecks(srv3, nil, "") // Exists remote (delete) srv4 := &structs.NodeService{ @@ -496,7 +496,7 @@ func TestAgent_ServiceWatchCh(t *testing.T) { Tags: []string{"tag1"}, Port: 6100, } - require.NoError(t, a.State.AddService(srv1, "")) + require.NoError(t, a.State.AddServiceWithChecks(srv1, nil, "")) verifyState := func(ss *local.ServiceState) { require.NotNil(t, ss) @@ -518,7 +518,7 @@ func TestAgent_ServiceWatchCh(t *testing.T) { go func() { srv2 := srv1 srv2.Port = 6200 - require.NoError(t, a.State.AddService(srv2, "")) + require.NoError(t, a.State.AddServiceWithChecks(srv2, nil, "")) }() // We should observe WatchCh close @@ -595,7 +595,7 @@ func TestAgentAntiEntropy_EnableTagOverride(t *testing.T) { }, EnterpriseMeta: *structs.DefaultEnterpriseMetaInDefaultPartition(), } - a.State.AddService(srv1, "") + a.State.AddServiceWithChecks(srv1, nil, "") // register a local service with tag override disabled srv2 := &structs.NodeService{ @@ -610,7 +610,7 @@ func TestAgentAntiEntropy_EnableTagOverride(t *testing.T) { }, EnterpriseMeta: *structs.DefaultEnterpriseMetaInDefaultPartition(), } - a.State.AddService(srv2, "") + a.State.AddServiceWithChecks(srv2, nil, "") // make sure they are both in the catalog if err := a.State.SyncChanges(); err != nil { @@ -722,7 +722,7 @@ func TestAgentAntiEntropy_Services_WithChecks(t *testing.T) { Tags: []string{"primary"}, Port: 5000, } - a.State.AddService(srv, "") + a.State.AddServiceWithChecks(srv, nil, "") chk := &structs.HealthCheck{ Node: a.Config.NodeName, @@ -772,7 +772,7 @@ func TestAgentAntiEntropy_Services_WithChecks(t *testing.T) { Tags: []string{"primary"}, Port: 5000, } - a.State.AddService(srv, "") + a.State.AddServiceWithChecks(srv, nil, "") chk1 := &structs.HealthCheck{ Node: a.Config.NodeName, @@ -873,7 +873,7 @@ func TestAgentAntiEntropy_Services_ACLDeny(t *testing.T) { }, EnterpriseMeta: *structs.DefaultEnterpriseMetaInDefaultPartition(), } - a.State.AddService(srv1, token) + a.State.AddServiceWithChecks(srv1, nil, token) // Create service (allowed) srv2 := &structs.NodeService{ @@ -887,7 +887,7 @@ func TestAgentAntiEntropy_Services_ACLDeny(t *testing.T) { }, EnterpriseMeta: *structs.DefaultEnterpriseMetaInDefaultPartition(), } - a.State.AddService(srv2, token) + a.State.AddServiceWithChecks(srv2, nil, token) if err := a.State.SyncFull(); err != nil { t.Fatalf("err: %v", err) @@ -1332,7 +1332,7 @@ func TestAgentAntiEntropy_Checks_ACLDeny(t *testing.T) { }, EnterpriseMeta: *structs.DefaultEnterpriseMetaInDefaultPartition(), } - a.State.AddService(srv1, "root") + a.State.AddServiceWithChecks(srv1, nil, "root") srv2 := &structs.NodeService{ ID: "api", Service: "api", @@ -1344,7 +1344,7 @@ func TestAgentAntiEntropy_Checks_ACLDeny(t *testing.T) { }, EnterpriseMeta: *structs.DefaultEnterpriseMetaInDefaultPartition(), } - a.State.AddService(srv2, "root") + a.State.AddServiceWithChecks(srv2, nil, "root") if err := a.State.SyncFull(); err != nil { t.Fatalf("err: %v", err) @@ -1861,14 +1861,14 @@ func TestState_ServiceTokens(t *testing.T) { }) t.Run("empty string when there is no token", func(t *testing.T) { - err := l.AddService(&structs.NodeService{ID: "redis"}, "") + err := l.AddServiceWithChecks(&structs.NodeService{ID: "redis"}, nil, "") require.NoError(t, err) require.Equal(t, "", l.ServiceToken(id)) }) t.Run("returns configured token", func(t *testing.T) { - err := l.AddService(&structs.NodeService{ID: "redis"}, "abc123") + err := l.AddServiceWithChecks(&structs.NodeService{ID: "redis"}, nil, "abc123") require.NoError(t, err) require.Equal(t, "abc123", l.ServiceToken(id)) @@ -1931,7 +1931,7 @@ func TestAgent_CheckCriticalTime(t *testing.T) { l.TriggerSyncChanges = func() {} svc := &structs.NodeService{ID: "redis", Service: "redis", Port: 8000} - l.AddService(svc, "") + l.AddServiceWithChecks(svc, nil, "") // Add a passing check and make sure it's not critical. checkID := types.CheckID("redis:1") @@ -2017,8 +2017,8 @@ func TestAgent_AliasCheck(t *testing.T) { l.TriggerSyncChanges = func() {} // Add checks - require.NoError(t, l.AddService(&structs.NodeService{Service: "s1"}, "")) - require.NoError(t, l.AddService(&structs.NodeService{Service: "s2"}, "")) + require.NoError(t, l.AddServiceWithChecks(&structs.NodeService{Service: "s1"}, nil, "")) + require.NoError(t, l.AddServiceWithChecks(&structs.NodeService{Service: "s2"}, nil, "")) require.NoError(t, l.AddCheck(&structs.HealthCheck{CheckID: types.CheckID("c1"), ServiceID: "s1"}, "")) require.NoError(t, l.AddCheck(&structs.HealthCheck{CheckID: types.CheckID("c2"), ServiceID: "s2"}, "")) @@ -2071,7 +2071,7 @@ func TestAgent_AliasCheck_ServiceNotification(t *testing.T) { require.NoError(t, l.AddAliasCheck(structs.NewCheckID(types.CheckID("a1"), nil), structs.NewServiceID("s1", nil), notifyCh)) // Add aliased service, s1, and verify we get notified - require.NoError(t, l.AddService(&structs.NodeService{Service: "s1"}, "")) + require.NoError(t, l.AddServiceWithChecks(&structs.NodeService{Service: "s1"}, nil, "")) select { case <-notifyCh: default: @@ -2079,7 +2079,7 @@ func TestAgent_AliasCheck_ServiceNotification(t *testing.T) { } // Re-adding same service should not lead to a notification - require.NoError(t, l.AddService(&structs.NodeService{Service: "s1"}, "")) + require.NoError(t, l.AddServiceWithChecks(&structs.NodeService{Service: "s1"}, nil, "")) select { case <-notifyCh: t.Fatal("notify received") @@ -2087,7 +2087,7 @@ func TestAgent_AliasCheck_ServiceNotification(t *testing.T) { } // Add different service and verify we do not get notified - require.NoError(t, l.AddService(&structs.NodeService{Service: "s2"}, "")) + require.NoError(t, l.AddServiceWithChecks(&structs.NodeService{Service: "s2"}, nil, "")) select { case <-notifyCh: t.Fatal("notify received") @@ -2189,10 +2189,10 @@ func TestState_RemoveServiceErrorMessages(t *testing.T) { state.TriggerSyncChanges = func() {} // Add 1 service - err := state.AddService(&structs.NodeService{ + err := state.AddServiceWithChecks(&structs.NodeService{ ID: "web-id", Service: "web-name", - }, "") + }, nil, "") require.NoError(t, err) // Attempt to remove service that doesn't exist @@ -2230,9 +2230,9 @@ func TestState_Notify(t *testing.T) { drainCh(notifyCh) // Add a service - err := state.AddService(&structs.NodeService{ + err := state.AddServiceWithChecks(&structs.NodeService{ Service: "web", - }, "fake-token-web") + }, nil, "fake-token-web") require.NoError(t, err) // Should have a notification @@ -2240,10 +2240,10 @@ func TestState_Notify(t *testing.T) { drainCh(notifyCh) // Re-Add same service - err = state.AddService(&structs.NodeService{ + err = state.AddServiceWithChecks(&structs.NodeService{ Service: "web", Port: 4444, - }, "fake-token-web") + }, nil, "fake-token-web") require.NoError(t, err) // Should have a notification @@ -2261,9 +2261,9 @@ func TestState_Notify(t *testing.T) { state.StopNotify(notifyCh) // Add a service - err = state.AddService(&structs.NodeService{ + err = state.AddServiceWithChecks(&structs.NodeService{ Service: "web", - }, "fake-token-web") + }, nil, "fake-token-web") require.NoError(t, err) // Should NOT have a notification @@ -2293,7 +2293,7 @@ func TestAliasNotifications_local(t *testing.T) { Address: "127.0.0.10", Port: 8080, } - a.State.AddService(srv, "") + a.State.AddServiceWithChecks(srv, nil, "") scID := "socat-sidecar-proxy" sc := &structs.NodeService{ @@ -2303,7 +2303,7 @@ func TestAliasNotifications_local(t *testing.T) { Address: "127.0.0.10", Port: 9090, } - a.State.AddService(sc, "") + a.State.AddServiceWithChecks(sc, nil, "") tcpID := types.CheckID("service:socat-tcp") chk0 := &structs.HealthCheck{ diff --git a/agent/proxycfg-sources/catalog/config_source_test.go b/agent/proxycfg-sources/catalog/config_source_test.go index dffb0c2e57..4df59a7d32 100644 --- a/agent/proxycfg-sources/catalog/config_source_test.go +++ b/agent/proxycfg-sources/catalog/config_source_test.go @@ -116,7 +116,7 @@ func TestConfigSource_LocallyManagedService(t *testing.T) { token := "token" localState := testLocalState(t) - localState.AddService(&structs.NodeService{ID: serviceID.ID}, "") + localState.AddServiceWithChecks(&structs.NodeService{ID: serviceID.ID}, nil, "") localWatcher := NewMockWatcher(t) localWatcher.On("Watch", serviceID, nodeName, token). diff --git a/agent/proxycfg-sources/local/sync_test.go b/agent/proxycfg-sources/local/sync_test.go index b73c0e3b3e..62b4e8db76 100644 --- a/agent/proxycfg-sources/local/sync_test.go +++ b/agent/proxycfg-sources/local/sync_test.go @@ -29,10 +29,10 @@ func TestSync(t *testing.T) { state := local.NewState(local.Config{}, hclog.NewNullLogger(), tokens) state.TriggerSyncChanges = func() {} - state.AddService(&structs.NodeService{ + state.AddServiceWithChecks(&structs.NodeService{ ID: serviceID, Kind: structs.ServiceKindConnectProxy, - }, serviceToken) + }, nil, serviceToken) cfgMgr := NewMockConfigManager(t) @@ -96,10 +96,10 @@ func TestSync(t *testing.T) { Return([]proxycfg.ProxyID{}). Maybe() - state.AddService(&structs.NodeService{ + state.AddServiceWithChecks(&structs.NodeService{ ID: serviceID, Kind: structs.ServiceKindConnectProxy, - }, "") + }, nil, "") select { case reg := <-registerCh: diff --git a/agent/user_event_test.go b/agent/user_event_test.go index 3f391ba2f4..8cae94dde2 100644 --- a/agent/user_event_test.go +++ b/agent/user_event_test.go @@ -64,7 +64,7 @@ func TestShouldProcessUserEvent(t *testing.T) { Tags: []string{"test", "foo", "bar", "primary"}, Port: 5000, } - a.State.AddService(srv1, "") + a.State.AddServiceWithChecks(srv1, nil, "") p := &UserEvent{} if !a.shouldProcessUserEvent(p) { @@ -172,7 +172,7 @@ func TestFireReceiveEvent(t *testing.T) { Tags: []string{"test", "foo", "bar", "primary"}, Port: 5000, } - a.State.AddService(srv1, "") + a.State.AddServiceWithChecks(srv1, nil, "") p1 := &UserEvent{Name: "deploy", ServiceFilter: "web"} err := a.UserEvent("dc1", "root", p1)