From 1c8eca2bfd22d80d3844a36352645d89b0575c9b Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Mon, 30 Nov 2020 12:53:46 -0500 Subject: [PATCH 1/5] agent: rename AddService->AddServiceFromSource In preparation for extracting a single AddService func that accepts a request struct. --- agent/agent.go | 12 +++-- agent/agent_endpoint.go | 19 ++++---- agent/agent_endpoint_test.go | 47 +++++++++---------- agent/agent_test.go | 86 +++++++++++++++++------------------ agent/service_checks_test.go | 7 +-- agent/service_manager_test.go | 19 ++++---- agent/sidecar_service_test.go | 5 +- command/maint/maint_test.go | 9 ++-- 8 files changed, 106 insertions(+), 98 deletions(-) diff --git a/agent/agent.go b/agent/agent.go index 21e7b98a1..6496493d0 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -1890,10 +1890,10 @@ func (a *Agent) readPersistedServiceConfigs() (map[structs.ServiceID]*structs.Se return out, nil } -// AddServiceAndReplaceChecks is used to add a service entry and its check. Any check for this service missing from chkTypes will be deleted. +// AddService is used to add a service entry and its check. Any check for this service missing from chkTypes will be deleted. // This entry is persistent and the agent will make a best effort to // ensure it is registered -func (a *Agent) AddServiceAndReplaceChecks(service *structs.NodeService, chkTypes []*structs.CheckType, persist bool, token string, source configSource) error { +func (a *Agent) AddService(service *structs.NodeService, chkTypes []*structs.CheckType, persist bool, token string, source configSource) error { a.stateLock.Lock() defer a.stateLock.Unlock() return a.addServiceLocked(&addServiceRequest{ @@ -1910,10 +1910,12 @@ func (a *Agent) AddServiceAndReplaceChecks(service *structs.NodeService, chkType }) } -// AddService is used to add a service entry. +// AddServiceFromSource is used to add a service entry. // This entry is persistent and the agent will make a best effort to -// ensure it is registered -func (a *Agent) AddService(service *structs.NodeService, chkTypes []*structs.CheckType, persist bool, token string, source configSource) error { +// ensure it is registered. +// TODO: move to _test.go +// Deprecated: use AddService +func (a *Agent) AddServiceFromSource(service *structs.NodeService, chkTypes []*structs.CheckType, persist bool, token string, source configSource) error { a.stateLock.Lock() defer a.stateLock.Unlock() return a.addServiceLocked(&addServiceRequest{ diff --git a/agent/agent_endpoint.go b/agent/agent_endpoint.go index 3c2735c92..a00e17620 100644 --- a/agent/agent_endpoint.go +++ b/agent/agent_endpoint.go @@ -10,6 +10,12 @@ import ( "github.com/hashicorp/go-memdb" "github.com/mitchellh/hashstructure" + "github.com/hashicorp/go-bexpr" + "github.com/hashicorp/serf/coordinate" + "github.com/hashicorp/serf/serf" + "github.com/prometheus/client_golang/prometheus" + "github.com/prometheus/client_golang/prometheus/promhttp" + "github.com/hashicorp/consul/acl" cachetype "github.com/hashicorp/consul/agent/cache-types" "github.com/hashicorp/consul/agent/debug" @@ -22,11 +28,6 @@ import ( "github.com/hashicorp/consul/logging" "github.com/hashicorp/consul/logging/monitor" "github.com/hashicorp/consul/types" - "github.com/hashicorp/go-bexpr" - "github.com/hashicorp/serf/coordinate" - "github.com/hashicorp/serf/serf" - "github.com/prometheus/client_golang/prometheus" - "github.com/prometheus/client_golang/prometheus/promhttp" ) type Self struct { @@ -992,22 +993,22 @@ func (s *HTTPHandlers) AgentRegisterService(resp http.ResponseWriter, req *http. } if replaceExistingChecks { - if err := s.agent.AddServiceAndReplaceChecks(ns, chkTypes, true, token, ConfigSourceRemote); err != nil { + if err := s.agent.AddService(ns, chkTypes, true, token, ConfigSourceRemote); err != nil { return nil, err } } else { - if err := s.agent.AddService(ns, chkTypes, true, token, ConfigSourceRemote); err != nil { + if err := s.agent.AddServiceFromSource(ns, chkTypes, true, token, ConfigSourceRemote); err != nil { return nil, err } } // Add sidecar. if sidecar != nil { if replaceExistingChecks { - if err := s.agent.AddServiceAndReplaceChecks(sidecar, sidecarChecks, true, sidecarToken, ConfigSourceRemote); err != nil { + if err := s.agent.AddService(sidecar, sidecarChecks, true, sidecarToken, ConfigSourceRemote); err != nil { return nil, err } } else { - if err := s.agent.AddService(sidecar, sidecarChecks, true, sidecarToken, ConfigSourceRemote); err != nil { + if err := s.agent.AddServiceFromSource(sidecar, sidecarChecks, true, sidecarToken, ConfigSourceRemote); err != nil { return nil, err } } diff --git a/agent/agent_endpoint_test.go b/agent/agent_endpoint_test.go index 73dc267b1..a19b84940 100644 --- a/agent/agent_endpoint_test.go +++ b/agent/agent_endpoint_test.go @@ -18,6 +18,12 @@ import ( "testing" "time" + "github.com/hashicorp/go-uuid" + "github.com/hashicorp/serf/serf" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "golang.org/x/time/rate" + "github.com/hashicorp/consul/acl" "github.com/hashicorp/consul/agent/config" "github.com/hashicorp/consul/agent/connect" @@ -34,11 +40,6 @@ import ( "github.com/hashicorp/consul/sdk/testutil/retry" "github.com/hashicorp/consul/testrpc" "github.com/hashicorp/consul/types" - "github.com/hashicorp/go-uuid" - "github.com/hashicorp/serf/serf" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" - "golang.org/x/time/rate" ) func makeReadOnlyAgentACL(t *testing.T, srv *HTTPHandlers) string { @@ -735,21 +736,21 @@ func TestAgent_HealthServiceByID(t *testing.T) { ID: "mysql", Service: "mysql", } - if err := a.AddService(service, nil, false, "", ConfigSourceLocal); err != nil { + if err := a.AddServiceFromSource(service, nil, false, "", ConfigSourceLocal); err != nil { t.Fatalf("err: %v", err) } service = &structs.NodeService{ ID: "mysql2", Service: "mysql2", } - if err := a.AddService(service, nil, false, "", ConfigSourceLocal); err != nil { + if err := a.AddServiceFromSource(service, nil, false, "", ConfigSourceLocal); err != nil { t.Fatalf("err: %v", err) } service = &structs.NodeService{ ID: "mysql3", Service: "mysql3", } - if err := a.AddService(service, nil, false, "", ConfigSourceLocal); err != nil { + if err := a.AddServiceFromSource(service, nil, false, "", ConfigSourceLocal); err != nil { t.Fatalf("err: %v", err) } @@ -933,42 +934,42 @@ func TestAgent_HealthServiceByName(t *testing.T) { ID: "mysql1", Service: "mysql-pool-r", } - if err := a.AddService(service, nil, false, "", ConfigSourceLocal); err != nil { + if err := a.AddServiceFromSource(service, nil, false, "", ConfigSourceLocal); err != nil { t.Fatalf("err: %v", err) } service = &structs.NodeService{ ID: "mysql2", Service: "mysql-pool-r", } - if err := a.AddService(service, nil, false, "", ConfigSourceLocal); err != nil { + if err := a.AddServiceFromSource(service, nil, false, "", ConfigSourceLocal); err != nil { t.Fatalf("err: %v", err) } service = &structs.NodeService{ ID: "mysql3", Service: "mysql-pool-rw", } - if err := a.AddService(service, nil, false, "", ConfigSourceLocal); err != nil { + if err := a.AddServiceFromSource(service, nil, false, "", ConfigSourceLocal); err != nil { t.Fatalf("err: %v", err) } service = &structs.NodeService{ ID: "mysql4", Service: "mysql-pool-rw", } - if err := a.AddService(service, nil, false, "", ConfigSourceLocal); err != nil { + if err := a.AddServiceFromSource(service, nil, false, "", ConfigSourceLocal); err != nil { t.Fatalf("err: %v", err) } service = &structs.NodeService{ ID: "httpd1", Service: "httpd", } - if err := a.AddService(service, nil, false, "", ConfigSourceLocal); err != nil { + if err := a.AddServiceFromSource(service, nil, false, "", ConfigSourceLocal); err != nil { t.Fatalf("err: %v", err) } service = &structs.NodeService{ ID: "httpd2", Service: "httpd", } - if err := a.AddService(service, nil, false, "", ConfigSourceLocal); err != nil { + if err := a.AddServiceFromSource(service, nil, false, "", ConfigSourceLocal); err != nil { t.Fatalf("err: %v", err) } @@ -1180,13 +1181,13 @@ func TestAgent_HealthServicesACLEnforcement(t *testing.T) { ID: "mysql1", Service: "mysql", } - require.NoError(t, a.AddService(service, nil, false, "", ConfigSourceLocal)) + require.NoError(t, a.AddServiceFromSource(service, nil, false, "", ConfigSourceLocal)) service = &structs.NodeService{ ID: "foo1", Service: "foo", } - require.NoError(t, a.AddService(service, nil, false, "", ConfigSourceLocal)) + require.NoError(t, a.AddServiceFromSource(service, nil, false, "", ConfigSourceLocal)) // no token t.Run("no-token-health-by-id", func(t *testing.T) { @@ -4014,10 +4015,10 @@ func testAgent_RegisterServiceDeregisterService_Sidecar(t *testing.T, extraHCL s testrpc.WaitForLeader(t, a.RPC, "dc1") if tt.preRegister != nil { - require.NoError(a.AddService(tt.preRegister, nil, false, "", ConfigSourceLocal)) + require.NoError(a.AddServiceFromSource(tt.preRegister, nil, false, "", ConfigSourceLocal)) } if tt.preRegister2 != nil { - require.NoError(a.AddService(tt.preRegister2, nil, false, "", ConfigSourceLocal)) + require.NoError(a.AddServiceFromSource(tt.preRegister2, nil, false, "", ConfigSourceLocal)) } // Create an ACL token with require policy @@ -4319,7 +4320,7 @@ func TestAgent_DeregisterService(t *testing.T) { ID: "test", Service: "test", } - if err := a.AddService(service, nil, false, "", ConfigSourceLocal); err != nil { + if err := a.AddServiceFromSource(service, nil, false, "", ConfigSourceLocal); err != nil { t.Fatalf("err: %v", err) } @@ -4351,7 +4352,7 @@ func TestAgent_DeregisterService_ACLDeny(t *testing.T) { ID: "test", Service: "test", } - if err := a.AddService(service, nil, false, "", ConfigSourceLocal); err != nil { + if err := a.AddServiceFromSource(service, nil, false, "", ConfigSourceLocal); err != nil { t.Fatalf("err: %v", err) } @@ -4429,7 +4430,7 @@ func TestAgent_ServiceMaintenance_Enable(t *testing.T) { ID: "test", Service: "test", } - if err := a.AddService(service, nil, false, "", ConfigSourceLocal); err != nil { + if err := a.AddServiceFromSource(service, nil, false, "", ConfigSourceLocal); err != nil { t.Fatalf("err: %v", err) } @@ -4476,7 +4477,7 @@ func TestAgent_ServiceMaintenance_Disable(t *testing.T) { ID: "test", Service: "test", } - if err := a.AddService(service, nil, false, "", ConfigSourceLocal); err != nil { + if err := a.AddServiceFromSource(service, nil, false, "", ConfigSourceLocal); err != nil { t.Fatalf("err: %v", err) } @@ -4517,7 +4518,7 @@ func TestAgent_ServiceMaintenance_ACLDeny(t *testing.T) { ID: "test", Service: "test", } - if err := a.AddService(service, nil, false, "", ConfigSourceLocal); err != nil { + if err := a.AddServiceFromSource(service, nil, false, "", ConfigSourceLocal); err != nil { t.Fatalf("err: %v", err) } diff --git a/agent/agent_test.go b/agent/agent_test.go index 2a6c42df3..71e943694 100644 --- a/agent/agent_test.go +++ b/agent/agent_test.go @@ -529,7 +529,7 @@ func testAgent_AddService(t *testing.T, extraHCL string) { t.Run(tt.desc, func(t *testing.T) { // check the service registration t.Run(tt.srv.ID, func(t *testing.T) { - err := a.AddService(tt.srv, tt.chkTypes, false, "", ConfigSourceLocal) + err := a.AddServiceFromSource(tt.srv, tt.chkTypes, false, "", ConfigSourceLocal) if err != nil { t.Fatalf("err: %v", err) } @@ -638,7 +638,7 @@ func testAgent_AddServices_AliasUpdateCheckNotReverted(t *testing.T, extraHCL st chkTypes, err := service.CheckTypes() require.NoError(t, err) - require.NoError(t, a.AddService(ns, chkTypes, false, service.Token, ConfigSourceLocal)) + require.NoError(t, a.AddServiceFromSource(ns, chkTypes, false, service.Token, ConfigSourceLocal)) } retry.Run(t, func(r *retry.R) { @@ -665,7 +665,7 @@ func test_createAlias(t *testing.T, agent *TestAgent, chk *structs.CheckType, ex if chk.CheckID == "" { chk.CheckID = types.CheckID(fmt.Sprintf("check-%d", serviceNum)) } - err := agent.AddService(srv, []*structs.CheckType{chk}, false, "", ConfigSourceLocal) + err := agent.AddServiceFromSource(srv, []*structs.CheckType{chk}, false, "", ConfigSourceLocal) assert.NoError(t, err) return func(r *retry.R) { t.Helper() @@ -712,7 +712,7 @@ func TestAgent_CheckAliasRPC(t *testing.T) { // We ensure to not block and update Agent's index srv.Tags = []string{fmt.Sprintf("tag-%s", time.Now())} assert.NoError(t, a.waitForUp()) - err := a.AddService(srv, []*structs.CheckType{}, false, "", ConfigSourceLocal) + err := a.AddServiceFromSource(srv, []*structs.CheckType{}, false, "", ConfigSourceLocal) assert.NoError(t, err) } shutdownAgent := func() { @@ -727,7 +727,7 @@ func TestAgent_CheckAliasRPC(t *testing.T) { testrpc.WaitForTestAgent(t, a.RPC, "dc1") assert.NoError(t, a.waitForUp()) - err := a.AddService(srv, []*structs.CheckType{}, false, "", ConfigSourceLocal) + err := a.AddServiceFromSource(srv, []*structs.CheckType{}, false, "", ConfigSourceLocal) assert.NoError(t, err) retry.Run(t, func(r *retry.R) { @@ -832,12 +832,12 @@ func testAgent_AddServiceNoExec(t *testing.T, extraHCL string) { Interval: 15 * time.Second, } - err := a.AddService(srv, []*structs.CheckType{chk}, false, "", ConfigSourceLocal) + err := a.AddServiceFromSource(srv, []*structs.CheckType{chk}, false, "", ConfigSourceLocal) if err == nil || !strings.Contains(err.Error(), "Scripts are disabled on this agent") { t.Fatalf("err: %v", err) } - err = a.AddService(srv, []*structs.CheckType{chk}, false, "", ConfigSourceRemote) + err = a.AddServiceFromSource(srv, []*structs.CheckType{chk}, false, "", ConfigSourceRemote) if err == nil || !strings.Contains(err.Error(), "Scripts are disabled on this agent") { t.Fatalf("err: %v", err) } @@ -879,7 +879,7 @@ func testAgent_AddServiceNoRemoteExec(t *testing.T, extraHCL string) { Interval: 15 * time.Second, } - err := a.AddService(srv, []*structs.CheckType{chk}, false, "", ConfigSourceRemote) + err := a.AddServiceFromSource(srv, []*structs.CheckType{chk}, false, "", ConfigSourceRemote) if err == nil || !strings.Contains(err.Error(), "Scripts are disabled on this agent") { t.Fatalf("err: %v", err) } @@ -932,7 +932,7 @@ func TestCacheRateLimit(t *testing.T) { Address: fmt.Sprintf("10.0.1.%d", i%255), } - err := a.AddService(srv, []*structs.CheckType{}, false, "", ConfigSourceRemote) + err := a.AddServiceFromSource(srv, []*structs.CheckType{}, false, "", ConfigSourceRemote) require.Nil(t, err) } @@ -1007,7 +1007,7 @@ func TestAddServiceIPv4TaggedDefault(t *testing.T) { Address: "10.0.1.2", } - err := a.AddService(srv, []*structs.CheckType{}, false, "", ConfigSourceRemote) + err := a.AddServiceFromSource(srv, []*structs.CheckType{}, false, "", ConfigSourceRemote) require.Nil(t, err) ns := a.State.Service(structs.NewServiceID("my_service_id", nil)) @@ -1040,7 +1040,7 @@ func TestAddServiceIPv6TaggedDefault(t *testing.T) { Address: "::5", } - err := a.AddService(srv, []*structs.CheckType{}, false, "", ConfigSourceRemote) + err := a.AddServiceFromSource(srv, []*structs.CheckType{}, false, "", ConfigSourceRemote) require.Nil(t, err) ns := a.State.Service(structs.NewServiceID("my_service_id", nil)) @@ -1079,7 +1079,7 @@ func TestAddServiceIPv4TaggedSet(t *testing.T) { }, } - err := a.AddService(srv, []*structs.CheckType{}, false, "", ConfigSourceRemote) + err := a.AddServiceFromSource(srv, []*structs.CheckType{}, false, "", ConfigSourceRemote) require.Nil(t, err) ns := a.State.Service(structs.NewServiceID("my_service_id", nil)) @@ -1118,7 +1118,7 @@ func TestAddServiceIPv6TaggedSet(t *testing.T) { }, } - err := a.AddService(srv, []*structs.CheckType{}, false, "", ConfigSourceRemote) + err := a.AddServiceFromSource(srv, []*structs.CheckType{}, false, "", ConfigSourceRemote) require.Nil(t, err) ns := a.State.Service(structs.NewServiceID("my_service_id", nil)) @@ -1173,7 +1173,7 @@ func testAgent_RemoveService(t *testing.T, extraHCL string) { } chkTypes := []*structs.CheckType{{TTL: time.Minute}} - if err := a.AddService(srv, chkTypes, false, "", ConfigSourceLocal); err != nil { + if err := a.AddServiceFromSource(srv, chkTypes, false, "", ConfigSourceLocal); err != nil { t.Fatalf("err: %v", err) } @@ -1208,7 +1208,7 @@ func testAgent_RemoveService(t *testing.T, extraHCL string) { {TTL: time.Minute}, {TTL: 30 * time.Second}, } - if err := a.AddService(srv, chkTypes, false, "", ConfigSourceLocal); err != nil { + if err := a.AddServiceFromSource(srv, chkTypes, false, "", ConfigSourceLocal); err != nil { t.Fatalf("err: %v", err) } @@ -1222,7 +1222,7 @@ func testAgent_RemoveService(t *testing.T, extraHCL string) { {TTL: time.Minute}, {TTL: 30 * time.Second}, } - if err := a.AddService(srv, chkTypes, false, "", ConfigSourceLocal); err != nil { + if err := a.AddServiceFromSource(srv, chkTypes, false, "", ConfigSourceLocal); err != nil { t.Fatalf("err: %v", err) } @@ -1294,7 +1294,7 @@ func testAgent_RemoveServiceRemovesAllChecks(t *testing.T, extraHCL string) { } // register service with chk1 - if err := a.AddService(svc, []*structs.CheckType{chk1}, false, "", ConfigSourceLocal); err != nil { + if err := a.AddServiceFromSource(svc, []*structs.CheckType{chk1}, false, "", ConfigSourceLocal); err != nil { t.Fatal("Failed to register service", err) } @@ -1302,7 +1302,7 @@ func testAgent_RemoveServiceRemovesAllChecks(t *testing.T, extraHCL string) { requireCheckExists(t, a, "chk1") // update the service with chk2 - if err := a.AddService(svc, []*structs.CheckType{chk2}, false, "", ConfigSourceLocal); err != nil { + if err := a.AddServiceFromSource(svc, []*structs.CheckType{chk2}, false, "", ConfigSourceLocal); err != nil { t.Fatal("Failed to update service", err) } @@ -1359,7 +1359,7 @@ func verifyIndexChurn(t *testing.T, tags []string) { Tags: tags, Weights: weights, } - if err := a.AddService(svc, nil, true, "", ConfigSourceLocal); err != nil { + if err := a.AddServiceFromSource(svc, nil, true, "", ConfigSourceLocal); err != nil { t.Fatalf("err: %v", err) } @@ -1767,7 +1767,7 @@ func TestAgent_RestoreServiceWithAliasCheck(t *testing.T) { registerServicesAndChecks := func(t *testing.T, a *TestAgent) { // add one persistent service with a simple check - require.NoError(t, a.AddService( + require.NoError(t, a.AddServiceFromSource( &structs.NodeService{ ID: "ping", Service: "ping", @@ -1786,7 +1786,7 @@ func TestAgent_RestoreServiceWithAliasCheck(t *testing.T) { // add one persistent sidecar service with an alias check in the manner // of how sidecar_service would add it - require.NoError(t, a.AddService( + require.NoError(t, a.AddServiceFromSource( &structs.NodeService{ ID: "ping-sidecar-proxy", Service: "ping-sidecar-proxy", @@ -2276,7 +2276,7 @@ func testAgent_PersistService(t *testing.T, extraHCL string) { file := filepath.Join(a.Config.DataDir, servicesDir, stringHash(svc.ID)) // Check is not persisted unless requested - if err := a.AddService(svc, nil, false, "", ConfigSourceLocal); err != nil { + if err := a.AddServiceFromSource(svc, nil, false, "", ConfigSourceLocal); err != nil { t.Fatalf("err: %v", err) } if _, err := os.Stat(file); err == nil { @@ -2284,7 +2284,7 @@ func testAgent_PersistService(t *testing.T, extraHCL string) { } // Persists to file if requested - if err := a.AddService(svc, nil, true, "mytoken", ConfigSourceLocal); err != nil { + if err := a.AddServiceFromSource(svc, nil, true, "mytoken", ConfigSourceLocal); err != nil { t.Fatalf("err: %v", err) } if _, err := os.Stat(file); err != nil { @@ -2308,7 +2308,7 @@ func testAgent_PersistService(t *testing.T, extraHCL string) { // Updates service definition on disk svc.Port = 8001 - if err := a.AddService(svc, nil, true, "mytoken", ConfigSourceLocal); err != nil { + if err := a.AddServiceFromSource(svc, nil, true, "mytoken", ConfigSourceLocal); err != nil { t.Fatalf("err: %v", err) } expected, err = json.Marshal(persistedService{ @@ -2431,7 +2431,7 @@ func testAgent_PurgeService(t *testing.T, extraHCL string) { } file := filepath.Join(a.Config.DataDir, servicesDir, stringHash(svc.ID)) - if err := a.AddService(svc, nil, true, "", ConfigSourceLocal); err != nil { + if err := a.AddServiceFromSource(svc, nil, true, "", ConfigSourceLocal); err != nil { t.Fatalf("err: %v", err) } // Exists @@ -2448,7 +2448,7 @@ func testAgent_PurgeService(t *testing.T, extraHCL string) { } // Re-add the service - if err := a.AddService(svc, nil, true, "", ConfigSourceLocal); err != nil { + if err := a.AddServiceFromSource(svc, nil, true, "", ConfigSourceLocal); err != nil { t.Fatalf("err: %v", err) } @@ -2494,7 +2494,7 @@ func testAgent_PurgeServiceOnDuplicate(t *testing.T, extraHCL string) { } // First persist the service - require.NoError(t, a.AddService(svc1, nil, true, "", ConfigSourceLocal)) + require.NoError(t, a.AddServiceFromSource(svc1, nil, true, "", ConfigSourceLocal)) a.Shutdown() // Try bringing the agent back up with the service already @@ -2742,9 +2742,9 @@ func TestAgent_DeregisterPersistedSidecarAfterRestart(t *testing.T) { require.NoError(t, err) // First persist the check - err = a.AddService(srv, nil, true, "", ConfigSourceLocal) + err = a.AddServiceFromSource(srv, nil, true, "", ConfigSourceLocal) require.NoError(t, err) - err = a.AddService(connectSrv, nil, true, "", ConfigSourceLocal) + err = a.AddServiceFromSource(connectSrv, nil, true, "", ConfigSourceLocal) require.NoError(t, err) // check both services were registered @@ -2814,7 +2814,7 @@ func TestAgent_unloadChecks(t *testing.T) { Tags: []string{"foo"}, Port: 8000, } - if err := a.AddService(svc, nil, false, "", ConfigSourceLocal); err != nil { + if err := a.AddServiceFromSource(svc, nil, false, "", ConfigSourceLocal); err != nil { t.Fatalf("err: %v", err) } @@ -3093,7 +3093,7 @@ func testAgent_unloadServices(t *testing.T, extraHCL string) { } // Register the service - if err := a.AddService(svc, nil, false, "", ConfigSourceLocal); err != nil { + if err := a.AddServiceFromSource(svc, nil, false, "", ConfigSourceLocal); err != nil { t.Fatalf("err: %v", err) } @@ -3125,7 +3125,7 @@ func TestAgent_Service_MaintenanceMode(t *testing.T) { } // Register the service - if err := a.AddService(svc, nil, false, "", ConfigSourceLocal); err != nil { + if err := a.AddServiceFromSource(svc, nil, false, "", ConfigSourceLocal); err != nil { t.Fatalf("err: %v", err) } @@ -3206,7 +3206,7 @@ func TestAgent_Service_Reap(t *testing.T) { } // Register the service. - if err := a.AddService(svc, chkTypes, false, "", ConfigSourceLocal); err != nil { + if err := a.AddServiceFromSource(svc, chkTypes, false, "", ConfigSourceLocal); err != nil { t.Fatalf("err: %v", err) } @@ -3263,7 +3263,7 @@ func TestAgent_Service_NoReap(t *testing.T) { } // Register the service. - if err := a.AddService(svc, chkTypes, false, "", ConfigSourceLocal); err != nil { + if err := a.AddServiceFromSource(svc, chkTypes, false, "", ConfigSourceLocal); err != nil { t.Fatalf("err: %v", err) } @@ -3310,7 +3310,7 @@ func testAgent_AddService_restoresSnapshot(t *testing.T, extraHCL string) { Tags: []string{"foo"}, Port: 8000, } - require.NoError(t, a.AddService(svc, nil, false, "", ConfigSourceLocal)) + require.NoError(t, a.AddServiceFromSource(svc, nil, false, "", ConfigSourceLocal)) // Register a check check1 := &structs.HealthCheck{ @@ -3325,7 +3325,7 @@ func testAgent_AddService_restoresSnapshot(t *testing.T, extraHCL string) { // Re-registering the service preserves the state of the check chkTypes := []*structs.CheckType{{TTL: 30 * time.Second}} - require.NoError(t, a.AddService(svc, chkTypes, false, "", ConfigSourceLocal)) + require.NoError(t, a.AddServiceFromSource(svc, chkTypes, false, "", ConfigSourceLocal)) check := requireCheckExists(t, a, "service:redis") require.Equal(t, api.HealthPassing, check.Status) } @@ -3346,7 +3346,7 @@ func TestAgent_AddCheck_restoresSnapshot(t *testing.T) { Tags: []string{"foo"}, Port: 8000, } - if err := a.AddService(svc, nil, false, "", ConfigSourceLocal); err != nil { + if err := a.AddServiceFromSource(svc, nil, false, "", ConfigSourceLocal); err != nil { t.Fatalf("err: %v", err) } @@ -3431,7 +3431,7 @@ func TestAgent_checkStateSnapshot(t *testing.T) { Tags: []string{"foo"}, Port: 8000, } - if err := a.AddService(svc, nil, false, "", ConfigSourceLocal); err != nil { + if err := a.AddServiceFromSource(svc, nil, false, "", ConfigSourceLocal); err != nil { t.Fatalf("err: %v", err) } @@ -4250,7 +4250,7 @@ func TestAgent_RerouteExistingHTTPChecks(t *testing.T) { TLSSkipVerify: true, }, } - if err := a.AddService(svc, chks, false, "", ConfigSourceLocal); err != nil { + if err := a.AddServiceFromSource(svc, chks, false, "", ConfigSourceLocal); err != nil { t.Fatalf("failed to add svc: %v", err) } @@ -4273,7 +4273,7 @@ func TestAgent_RerouteExistingHTTPChecks(t *testing.T) { }, }, } - if err := a.AddService(proxy, nil, false, "", ConfigSourceLocal); err != nil { + if err := a.AddServiceFromSource(proxy, nil, false, "", ConfigSourceLocal); err != nil { t.Fatalf("failed to add svc: %v", err) } @@ -4326,7 +4326,7 @@ func TestAgent_RerouteExistingHTTPChecks(t *testing.T) { }, }, } - if err := a.AddService(proxy, nil, false, "", ConfigSourceLocal); err != nil { + if err := a.AddServiceFromSource(proxy, nil, false, "", ConfigSourceLocal); err != nil { t.Fatalf("failed to add svc: %v", err) } @@ -4369,7 +4369,7 @@ func TestAgent_RerouteNewHTTPChecks(t *testing.T) { Address: "localhost", Port: 8080, } - if err := a.AddService(svc, nil, false, "", ConfigSourceLocal); err != nil { + if err := a.AddServiceFromSource(svc, nil, false, "", ConfigSourceLocal); err != nil { t.Fatalf("failed to add svc: %v", err) } @@ -4391,7 +4391,7 @@ func TestAgent_RerouteNewHTTPChecks(t *testing.T) { }, }, } - if err := a.AddService(proxy, nil, false, "", ConfigSourceLocal); err != nil { + if err := a.AddServiceFromSource(proxy, nil, false, "", ConfigSourceLocal); err != nil { t.Fatalf("failed to add svc: %v", err) } diff --git a/agent/service_checks_test.go b/agent/service_checks_test.go index 652ab2436..5b82aca28 100644 --- a/agent/service_checks_test.go +++ b/agent/service_checks_test.go @@ -5,12 +5,13 @@ import ( "testing" "time" + "github.com/stretchr/testify/require" + "github.com/hashicorp/consul/agent/cache" cachetype "github.com/hashicorp/consul/agent/cache-types" "github.com/hashicorp/consul/agent/checks" "github.com/hashicorp/consul/agent/structs" "github.com/hashicorp/consul/testrpc" - "github.com/stretchr/testify/require" ) // Integration test for ServiceHTTPBasedChecks cache-type @@ -62,7 +63,7 @@ func TestAgent_ServiceHTTPChecksNotification(t *testing.T) { }, } // Adding TTL type should lead to a timeout, since only HTTP-based checks are watched - if err := a.AddService(&service, chkTypes[2:], false, "", ConfigSourceLocal); err != nil { + if err := a.AddServiceFromSource(&service, chkTypes[2:], false, "", ConfigSourceLocal); err != nil { t.Fatalf("failed to add service: %v", err) } @@ -74,7 +75,7 @@ func TestAgent_ServiceHTTPChecksNotification(t *testing.T) { } // Adding service with HTTP checks should lead notification for them - if err := a.AddService(&service, chkTypes[0:2], false, "", ConfigSourceLocal); err != nil { + if err := a.AddServiceFromSource(&service, chkTypes[0:2], false, "", ConfigSourceLocal); err != nil { t.Fatalf("failed to add service: %v", err) } diff --git a/agent/service_manager_test.go b/agent/service_manager_test.go index 4d5264fc6..073ac577d 100644 --- a/agent/service_manager_test.go +++ b/agent/service_manager_test.go @@ -8,10 +8,11 @@ import ( "path/filepath" "testing" + "github.com/stretchr/testify/require" + "github.com/hashicorp/consul/agent/structs" "github.com/hashicorp/consul/sdk/testutil/retry" "github.com/hashicorp/consul/testrpc" - "github.com/stretchr/testify/require" ) func TestServiceManager_RegisterService(t *testing.T) { @@ -47,7 +48,7 @@ func TestServiceManager_RegisterService(t *testing.T) { Port: 8000, EnterpriseMeta: *structs.DefaultEnterpriseMeta(), } - require.NoError(a.AddService(svc, nil, false, "", ConfigSourceLocal)) + require.NoError(a.AddServiceFromSource(svc, nil, false, "", ConfigSourceLocal)) // Verify both the service and sidecar. redisService := a.State.Service(structs.NewServiceID("redis", nil)) @@ -118,7 +119,7 @@ func TestServiceManager_RegisterSidecar(t *testing.T) { }, EnterpriseMeta: *structs.DefaultEnterpriseMeta(), } - require.NoError(a.AddService(svc, nil, false, "", ConfigSourceLocal)) + require.NoError(a.AddServiceFromSource(svc, nil, false, "", ConfigSourceLocal)) // Verify sidecar got global config loaded sidecarService := a.State.Service(structs.NewServiceID("web-sidecar-proxy", nil)) @@ -191,7 +192,7 @@ func TestServiceManager_RegisterMeshGateway(t *testing.T) { EnterpriseMeta: *structs.DefaultEnterpriseMeta(), } - require.NoError(a.AddService(svc, nil, false, "", ConfigSourceLocal)) + require.NoError(a.AddServiceFromSource(svc, nil, false, "", ConfigSourceLocal)) // Verify gateway got global config loaded gateway := a.State.Service(structs.NewServiceID("mesh-gateway", nil)) @@ -251,7 +252,7 @@ func TestServiceManager_RegisterTerminatingGateway(t *testing.T) { EnterpriseMeta: *structs.DefaultEnterpriseMeta(), } - require.NoError(a.AddService(svc, nil, false, "", ConfigSourceLocal)) + require.NoError(a.AddServiceFromSource(svc, nil, false, "", ConfigSourceLocal)) // Verify gateway got global config loaded gateway := a.State.Service(structs.NewServiceID("terminating-gateway", nil)) @@ -386,12 +387,12 @@ func TestServiceManager_PersistService_API(t *testing.T) { configFile := filepath.Join(a.Config.DataDir, serviceConfigDir, svcID.StringHash()) // Service is not persisted unless requested, but we always persist service configs. - require.NoError(a.AddService(svc, nil, false, "", ConfigSourceRemote)) + require.NoError(a.AddServiceFromSource(svc, nil, false, "", ConfigSourceRemote)) requireFileIsAbsent(t, svcFile) requireFileIsPresent(t, configFile) // Persists to file if requested - require.NoError(a.AddService(svc, nil, true, "mytoken", ConfigSourceRemote)) + require.NoError(a.AddServiceFromSource(svc, nil, true, "mytoken", ConfigSourceRemote)) requireFileIsPresent(t, svcFile) requireFileIsPresent(t, configFile) @@ -432,7 +433,7 @@ func TestServiceManager_PersistService_API(t *testing.T) { // Updates service definition on disk svc.Proxy.LocalServicePort = 8001 - require.NoError(a.AddService(svc, nil, true, "mytoken", ConfigSourceRemote)) + require.NoError(a.AddServiceFromSource(svc, nil, true, "mytoken", ConfigSourceRemote)) requireFileIsPresent(t, svcFile) requireFileIsPresent(t, configFile) @@ -720,7 +721,7 @@ func TestServiceManager_Disabled(t *testing.T) { }, EnterpriseMeta: *structs.DefaultEnterpriseMeta(), } - require.NoError(a.AddService(svc, nil, false, "", ConfigSourceLocal)) + require.NoError(a.AddServiceFromSource(svc, nil, false, "", ConfigSourceLocal)) // Verify sidecar got global config loaded sidecarService := a.State.Service(structs.NewServiceID("web-sidecar-proxy", nil)) diff --git a/agent/sidecar_service_test.go b/agent/sidecar_service_test.go index 1e5dc0633..d287c169a 100644 --- a/agent/sidecar_service_test.go +++ b/agent/sidecar_service_test.go @@ -5,8 +5,9 @@ import ( "testing" "time" - "github.com/hashicorp/consul/agent/structs" "github.com/stretchr/testify/require" + + "github.com/hashicorp/consul/agent/structs" ) func TestAgent_sidecarServiceFromNodeService(t *testing.T) { @@ -333,7 +334,7 @@ func TestAgent_sidecarServiceFromNodeService(t *testing.T) { defer a.Shutdown() if tt.preRegister != nil { - err := a.AddService(tt.preRegister.NodeService(), nil, false, "", ConfigSourceLocal) + err := a.AddServiceFromSource(tt.preRegister.NodeService(), nil, false, "", ConfigSourceLocal) require.NoError(err) } diff --git a/command/maint/maint_test.go b/command/maint/maint_test.go index b418ac8ea..78e481339 100644 --- a/command/maint/maint_test.go +++ b/command/maint/maint_test.go @@ -4,9 +4,10 @@ import ( "strings" "testing" + "github.com/mitchellh/cli" + "github.com/hashicorp/consul/agent" "github.com/hashicorp/consul/agent/structs" - "github.com/mitchellh/cli" ) func TestMaintCommand_noTabs(t *testing.T) { @@ -53,7 +54,7 @@ func TestMaintCommand_NoArgs(t *testing.T) { ID: "test", Service: "test", } - if err := a.AddService(service, nil, false, "", agent.ConfigSourceLocal); err != nil { + if err := a.AddServiceFromSource(service, nil, false, "", agent.ConfigSourceLocal); err != nil { t.Fatalf("err: %v", err) } if err := a.EnableServiceMaintenance(structs.NewServiceID("test", nil), "broken 1", ""); err != nil { @@ -161,7 +162,7 @@ func TestMaintCommand_EnableServiceMaintenance(t *testing.T) { ID: "test", Service: "test", } - if err := a.AddService(service, nil, false, "", agent.ConfigSourceLocal); err != nil { + if err := a.AddServiceFromSource(service, nil, false, "", agent.ConfigSourceLocal); err != nil { t.Fatalf("err: %v", err) } @@ -199,7 +200,7 @@ func TestMaintCommand_DisableServiceMaintenance(t *testing.T) { ID: "test", Service: "test", } - if err := a.AddService(service, nil, false, "", agent.ConfigSourceLocal); err != nil { + if err := a.AddServiceFromSource(service, nil, false, "", agent.ConfigSourceLocal); err != nil { t.Fatalf("err: %v", err) } From de1a80b368918997862046ff9af0c59792aabd51 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Mon, 30 Nov 2020 13:14:15 -0500 Subject: [PATCH 2/5] agent: use a single method for Agent.AddService --- agent/agent.go | 22 ++++++++-------------- agent/agent_endpoint.go | 38 +++++++++++++++++++++----------------- agent/sidecar_service.go | 5 +++-- 3 files changed, 32 insertions(+), 33 deletions(-) diff --git a/agent/agent.go b/agent/agent.go index 6496493d0..93958e6d6 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -1893,21 +1893,15 @@ func (a *Agent) readPersistedServiceConfigs() (map[structs.ServiceID]*structs.Se // AddService is used to add a service entry and its check. Any check for this service missing from chkTypes will be deleted. // This entry is persistent and the agent will make a best effort to // ensure it is registered -func (a *Agent) AddService(service *structs.NodeService, chkTypes []*structs.CheckType, persist bool, token string, source configSource) error { +func (a *Agent) AddService(req addServiceRequest) error { + // service *structs.NodeService, chkTypes []*structs.CheckType, persist bool, token string, source configSource + req.waitForCentralConfig = true + req.persistServiceConfig = true a.stateLock.Lock() defer a.stateLock.Unlock() - return a.addServiceLocked(&addServiceRequest{ - service: service, - chkTypes: chkTypes, - previousDefaults: nil, - waitForCentralConfig: true, - persist: persist, - persistServiceConfig: true, - token: token, - replaceExistingChecks: true, - source: source, - snap: a.snapshotCheckState(), - }) + + req.snap = a.State.Checks(structs.WildcardEnterpriseMeta()) + return a.addServiceLocked(&req) } // AddServiceFromSource is used to add a service entry. @@ -2374,7 +2368,7 @@ func (a *Agent) removeServiceLocked(serviceID structs.ServiceID, persist bool) e } func (a *Agent) removeServiceSidecars(serviceID structs.ServiceID, persist bool) error { - sidecarSID := structs.NewServiceID(a.sidecarServiceID(serviceID.ID), &serviceID.EnterpriseMeta) + sidecarSID := structs.NewServiceID(sidecarServiceID(serviceID.ID), &serviceID.EnterpriseMeta) if sidecar := a.State.Service(sidecarSID); sidecar != nil { // Double check that it's not just an ID collision and we actually added // this from a sidecar. diff --git a/agent/agent_endpoint.go b/agent/agent_endpoint.go index a00e17620..780933d84 100644 --- a/agent/agent_endpoint.go +++ b/agent/agent_endpoint.go @@ -992,25 +992,29 @@ func (s *HTTPHandlers) AgentRegisterService(resp http.ResponseWriter, req *http. replaceExistingChecks = true } - if replaceExistingChecks { - if err := s.agent.AddService(ns, chkTypes, true, token, ConfigSourceRemote); err != nil { - return nil, err - } - } else { - if err := s.agent.AddServiceFromSource(ns, chkTypes, true, token, ConfigSourceRemote); err != nil { - return nil, err - } + addReq := addServiceRequest{ + service: ns, + chkTypes: chkTypes, + persist: true, + token: token, + source: ConfigSourceRemote, + replaceExistingChecks: replaceExistingChecks, } - // Add sidecar. + if err := s.agent.AddService(addReq); err != nil { + return nil, err + } + if sidecar != nil { - if replaceExistingChecks { - if err := s.agent.AddService(sidecar, sidecarChecks, true, sidecarToken, ConfigSourceRemote); err != nil { - return nil, err - } - } else { - if err := s.agent.AddServiceFromSource(sidecar, sidecarChecks, true, sidecarToken, ConfigSourceRemote); err != nil { - return nil, err - } + addReq := addServiceRequest{ + service: sidecar, + chkTypes: sidecarChecks, + persist: true, + token: sidecarToken, + source: ConfigSourceRemote, + replaceExistingChecks: replaceExistingChecks, + } + if err := s.agent.AddService(addReq); err != nil { + return nil, err } } s.syncChanges() diff --git a/agent/sidecar_service.go b/agent/sidecar_service.go index 327b0e9f8..bd78adb15 100644 --- a/agent/sidecar_service.go +++ b/agent/sidecar_service.go @@ -9,7 +9,7 @@ import ( "github.com/hashicorp/consul/agent/structs" ) -func (a *Agent) sidecarServiceID(serviceID string) string { +func sidecarServiceID(serviceID string) string { return serviceID + "-sidecar-proxy" } @@ -29,6 +29,7 @@ func (a *Agent) sidecarServiceID(serviceID string) string { // The third return argument is the effective Token to use for the sidecar // registration. This will be the same as the token parameter passed unless the // SidecarService definition contains a distinct one. +// TODO: return addServiceRequest func (a *Agent) sidecarServiceFromNodeService(ns *structs.NodeService, token string) (*structs.NodeService, []*structs.CheckType, string, error) { if ns.Connect.SidecarService == nil { return nil, nil, "", nil @@ -39,7 +40,7 @@ func (a *Agent) sidecarServiceFromNodeService(ns *structs.NodeService, token str // Override the ID which must always be consistent for a given outer service // ID. We rely on this for lifecycle management of the nested definition. - sidecar.ID = a.sidecarServiceID(ns.ID) + sidecar.ID = sidecarServiceID(ns.ID) // for now at least these must be identical sidecar.EnterpriseMeta = ns.EnterpriseMeta From a0b11b4c20790027351a8ea79da885ca99721287 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Mon, 30 Nov 2020 13:26:58 -0500 Subject: [PATCH 3/5] agent: move deprecated AddServiceFromSource to a test file The method is only used in tests, and only exists for legacy calls. There was one other package which used this method in tests. Export the AddServiceRequest and a couple of its fields so the new function can be used in those tests. --- agent/agent.go | 66 ++++++++-------------- agent/agent_endpoint.go | 12 ++-- agent/agent_endpoint_test.go | 36 ++++++------ agent/agent_test.go | 100 +++++++++++++++++++--------------- agent/service_checks_test.go | 4 +- agent/service_manager.go | 33 +++++------ agent/service_manager_test.go | 16 +++--- agent/sidecar_service.go | 2 +- agent/sidecar_service_test.go | 2 +- command/maint/maint_test.go | 33 +++++++---- 10 files changed, 153 insertions(+), 151 deletions(-) diff --git a/agent/agent.go b/agent/agent.go index 93958e6d6..ebe6d92a0 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -1893,7 +1893,7 @@ func (a *Agent) readPersistedServiceConfigs() (map[structs.ServiceID]*structs.Se // AddService is used to add a service entry and its check. Any check for this service missing from chkTypes will be deleted. // This entry is persistent and the agent will make a best effort to // ensure it is registered -func (a *Agent) AddService(req addServiceRequest) error { +func (a *Agent) AddService(req AddServiceRequest) error { // service *structs.NodeService, chkTypes []*structs.CheckType, persist bool, token string, source configSource req.waitForCentralConfig = true req.persistServiceConfig = true @@ -1904,36 +1904,14 @@ func (a *Agent) AddService(req addServiceRequest) error { return a.addServiceLocked(&req) } -// AddServiceFromSource is used to add a service entry. -// This entry is persistent and the agent will make a best effort to -// ensure it is registered. -// TODO: move to _test.go -// Deprecated: use AddService -func (a *Agent) AddServiceFromSource(service *structs.NodeService, chkTypes []*structs.CheckType, persist bool, token string, source configSource) error { - a.stateLock.Lock() - defer a.stateLock.Unlock() - return a.addServiceLocked(&addServiceRequest{ - service: service, - chkTypes: chkTypes, - previousDefaults: nil, - waitForCentralConfig: true, - persist: persist, - persistServiceConfig: true, - token: token, - replaceExistingChecks: false, - source: source, - snap: a.snapshotCheckState(), - }) -} - // addServiceLocked adds a service entry to the service manager if enabled, or directly // to the local state if it is not. This function assumes the state lock is already held. -func (a *Agent) addServiceLocked(req *addServiceRequest) error { +func (a *Agent) addServiceLocked(req *AddServiceRequest) error { req.fixupForAddServiceLocked() - req.service.EnterpriseMeta.Normalize() + req.Service.EnterpriseMeta.Normalize() - if err := a.validateService(req.service, req.chkTypes); err != nil { + if err := a.validateService(req.Service, req.chkTypes); err != nil { return err } @@ -1949,7 +1927,7 @@ func (a *Agent) addServiceLocked(req *addServiceRequest) error { return a.addServiceInternal(req) } -// addServiceRequest is the union of arguments for calling both +// AddServiceRequest is the union of arguments for calling both // addServiceLocked and addServiceInternal. The overlap was significant enough // to warrant merging them and indicating which fields are meant to be set only // in one of the two contexts. @@ -1959,8 +1937,8 @@ func (a *Agent) addServiceLocked(req *addServiceRequest) error { // // The ServiceManager.AddService signature is largely just a passthrough for // addServiceLocked and should be treated as such. -type addServiceRequest struct { - service *structs.NodeService +type AddServiceRequest struct { + Service *structs.NodeService chkTypes []*structs.CheckType previousDefaults *structs.ServiceConfigResponse // just for: addServiceLocked waitForCentralConfig bool // just for: addServiceLocked @@ -1970,25 +1948,25 @@ type addServiceRequest struct { persistServiceConfig bool token string replaceExistingChecks bool - source configSource + Source configSource snap map[structs.CheckID]*structs.HealthCheck } -func (r *addServiceRequest) fixupForAddServiceLocked() { +func (r *AddServiceRequest) fixupForAddServiceLocked() { r.persistService = nil r.persistDefaults = nil } -func (r *addServiceRequest) fixupForAddServiceInternal() { +func (r *AddServiceRequest) fixupForAddServiceInternal() { r.previousDefaults = nil r.waitForCentralConfig = false } // addServiceInternal adds the given service and checks to the local state. -func (a *Agent) addServiceInternal(req *addServiceRequest) error { +func (a *Agent) addServiceInternal(req *AddServiceRequest) error { req.fixupForAddServiceInternal() var ( - service = req.service + service = req.Service chkTypes = req.chkTypes persistService = req.persistService persistDefaults = req.persistDefaults @@ -1996,7 +1974,7 @@ func (a *Agent) addServiceInternal(req *addServiceRequest) error { persistServiceConfig = req.persistServiceConfig token = req.token replaceExistingChecks = req.replaceExistingChecks - source = req.source + source = req.Source snap = req.snap ) @@ -3122,8 +3100,8 @@ func (a *Agent) loadServices(conf *config.RuntimeConfig, snap map[structs.CheckI ns.Connect.SidecarService = nil sid := ns.CompoundServiceID() - err = a.addServiceLocked(&addServiceRequest{ - service: ns, + err = a.addServiceLocked(&AddServiceRequest{ + Service: ns, chkTypes: chkTypes, previousDefaults: persistedServiceConfigs[sid], waitForCentralConfig: false, // exclusively use cached values @@ -3131,7 +3109,7 @@ func (a *Agent) loadServices(conf *config.RuntimeConfig, snap map[structs.CheckI persistServiceConfig: false, // don't rewrite the file with the same data we just read token: service.Token, replaceExistingChecks: false, // do default behavior - source: ConfigSourceLocal, + Source: ConfigSourceLocal, snap: snap, }) if err != nil { @@ -3141,8 +3119,8 @@ func (a *Agent) loadServices(conf *config.RuntimeConfig, snap map[structs.CheckI // If there is a sidecar service, register that too. if sidecar != nil { sidecarServiceID := sidecar.CompoundServiceID() - err = a.addServiceLocked(&addServiceRequest{ - service: sidecar, + err = a.addServiceLocked(&AddServiceRequest{ + Service: sidecar, chkTypes: sidecarChecks, previousDefaults: persistedServiceConfigs[sidecarServiceID], waitForCentralConfig: false, // exclusively use cached values @@ -3150,7 +3128,7 @@ func (a *Agent) loadServices(conf *config.RuntimeConfig, snap map[structs.CheckI persistServiceConfig: false, // don't rewrite the file with the same data we just read token: sidecarToken, replaceExistingChecks: false, // do default behavior - source: ConfigSourceLocal, + Source: ConfigSourceLocal, snap: snap, }) if err != nil { @@ -3238,8 +3216,8 @@ func (a *Agent) loadServices(conf *config.RuntimeConfig, snap map[structs.CheckI "service", serviceID.String(), "file", file, ) - err = a.addServiceLocked(&addServiceRequest{ - service: p.Service, + err = a.addServiceLocked(&AddServiceRequest{ + Service: p.Service, chkTypes: nil, previousDefaults: persistedServiceConfigs[serviceID], waitForCentralConfig: false, // exclusively use cached values @@ -3247,7 +3225,7 @@ func (a *Agent) loadServices(conf *config.RuntimeConfig, snap map[structs.CheckI persistServiceConfig: false, // don't rewrite the file with the same data we just read token: p.Token, replaceExistingChecks: false, // do default behavior - source: source, + Source: source, snap: snap, }) if err != nil { diff --git a/agent/agent_endpoint.go b/agent/agent_endpoint.go index 780933d84..10fe70c6d 100644 --- a/agent/agent_endpoint.go +++ b/agent/agent_endpoint.go @@ -992,12 +992,12 @@ func (s *HTTPHandlers) AgentRegisterService(resp http.ResponseWriter, req *http. replaceExistingChecks = true } - addReq := addServiceRequest{ - service: ns, + addReq := AddServiceRequest{ + Service: ns, chkTypes: chkTypes, persist: true, token: token, - source: ConfigSourceRemote, + Source: ConfigSourceRemote, replaceExistingChecks: replaceExistingChecks, } if err := s.agent.AddService(addReq); err != nil { @@ -1005,12 +1005,12 @@ func (s *HTTPHandlers) AgentRegisterService(resp http.ResponseWriter, req *http. } if sidecar != nil { - addReq := addServiceRequest{ - service: sidecar, + addReq := AddServiceRequest{ + Service: sidecar, chkTypes: sidecarChecks, persist: true, token: sidecarToken, - source: ConfigSourceRemote, + Source: ConfigSourceRemote, replaceExistingChecks: replaceExistingChecks, } if err := s.agent.AddService(addReq); err != nil { diff --git a/agent/agent_endpoint_test.go b/agent/agent_endpoint_test.go index a19b84940..1949ed82a 100644 --- a/agent/agent_endpoint_test.go +++ b/agent/agent_endpoint_test.go @@ -736,21 +736,21 @@ func TestAgent_HealthServiceByID(t *testing.T) { ID: "mysql", Service: "mysql", } - if err := a.AddServiceFromSource(service, nil, false, "", ConfigSourceLocal); err != nil { + if err := a.addServiceFromSource(service, nil, false, "", ConfigSourceLocal); err != nil { t.Fatalf("err: %v", err) } service = &structs.NodeService{ ID: "mysql2", Service: "mysql2", } - if err := a.AddServiceFromSource(service, nil, false, "", ConfigSourceLocal); err != nil { + if err := a.addServiceFromSource(service, nil, false, "", ConfigSourceLocal); err != nil { t.Fatalf("err: %v", err) } service = &structs.NodeService{ ID: "mysql3", Service: "mysql3", } - if err := a.AddServiceFromSource(service, nil, false, "", ConfigSourceLocal); err != nil { + if err := a.addServiceFromSource(service, nil, false, "", ConfigSourceLocal); err != nil { t.Fatalf("err: %v", err) } @@ -934,42 +934,42 @@ func TestAgent_HealthServiceByName(t *testing.T) { ID: "mysql1", Service: "mysql-pool-r", } - if err := a.AddServiceFromSource(service, nil, false, "", ConfigSourceLocal); err != nil { + if err := a.addServiceFromSource(service, nil, false, "", ConfigSourceLocal); err != nil { t.Fatalf("err: %v", err) } service = &structs.NodeService{ ID: "mysql2", Service: "mysql-pool-r", } - if err := a.AddServiceFromSource(service, nil, false, "", ConfigSourceLocal); err != nil { + if err := a.addServiceFromSource(service, nil, false, "", ConfigSourceLocal); err != nil { t.Fatalf("err: %v", err) } service = &structs.NodeService{ ID: "mysql3", Service: "mysql-pool-rw", } - if err := a.AddServiceFromSource(service, nil, false, "", ConfigSourceLocal); err != nil { + if err := a.addServiceFromSource(service, nil, false, "", ConfigSourceLocal); err != nil { t.Fatalf("err: %v", err) } service = &structs.NodeService{ ID: "mysql4", Service: "mysql-pool-rw", } - if err := a.AddServiceFromSource(service, nil, false, "", ConfigSourceLocal); err != nil { + if err := a.addServiceFromSource(service, nil, false, "", ConfigSourceLocal); err != nil { t.Fatalf("err: %v", err) } service = &structs.NodeService{ ID: "httpd1", Service: "httpd", } - if err := a.AddServiceFromSource(service, nil, false, "", ConfigSourceLocal); err != nil { + if err := a.addServiceFromSource(service, nil, false, "", ConfigSourceLocal); err != nil { t.Fatalf("err: %v", err) } service = &structs.NodeService{ ID: "httpd2", Service: "httpd", } - if err := a.AddServiceFromSource(service, nil, false, "", ConfigSourceLocal); err != nil { + if err := a.addServiceFromSource(service, nil, false, "", ConfigSourceLocal); err != nil { t.Fatalf("err: %v", err) } @@ -1181,13 +1181,13 @@ func TestAgent_HealthServicesACLEnforcement(t *testing.T) { ID: "mysql1", Service: "mysql", } - require.NoError(t, a.AddServiceFromSource(service, nil, false, "", ConfigSourceLocal)) + require.NoError(t, a.addServiceFromSource(service, nil, false, "", ConfigSourceLocal)) service = &structs.NodeService{ ID: "foo1", Service: "foo", } - require.NoError(t, a.AddServiceFromSource(service, nil, false, "", ConfigSourceLocal)) + require.NoError(t, a.addServiceFromSource(service, nil, false, "", ConfigSourceLocal)) // no token t.Run("no-token-health-by-id", func(t *testing.T) { @@ -4015,10 +4015,10 @@ func testAgent_RegisterServiceDeregisterService_Sidecar(t *testing.T, extraHCL s testrpc.WaitForLeader(t, a.RPC, "dc1") if tt.preRegister != nil { - require.NoError(a.AddServiceFromSource(tt.preRegister, nil, false, "", ConfigSourceLocal)) + require.NoError(a.addServiceFromSource(tt.preRegister, nil, false, "", ConfigSourceLocal)) } if tt.preRegister2 != nil { - require.NoError(a.AddServiceFromSource(tt.preRegister2, nil, false, "", ConfigSourceLocal)) + require.NoError(a.addServiceFromSource(tt.preRegister2, nil, false, "", ConfigSourceLocal)) } // Create an ACL token with require policy @@ -4320,7 +4320,7 @@ func TestAgent_DeregisterService(t *testing.T) { ID: "test", Service: "test", } - if err := a.AddServiceFromSource(service, nil, false, "", ConfigSourceLocal); err != nil { + if err := a.addServiceFromSource(service, nil, false, "", ConfigSourceLocal); err != nil { t.Fatalf("err: %v", err) } @@ -4352,7 +4352,7 @@ func TestAgent_DeregisterService_ACLDeny(t *testing.T) { ID: "test", Service: "test", } - if err := a.AddServiceFromSource(service, nil, false, "", ConfigSourceLocal); err != nil { + if err := a.addServiceFromSource(service, nil, false, "", ConfigSourceLocal); err != nil { t.Fatalf("err: %v", err) } @@ -4430,7 +4430,7 @@ func TestAgent_ServiceMaintenance_Enable(t *testing.T) { ID: "test", Service: "test", } - if err := a.AddServiceFromSource(service, nil, false, "", ConfigSourceLocal); err != nil { + if err := a.addServiceFromSource(service, nil, false, "", ConfigSourceLocal); err != nil { t.Fatalf("err: %v", err) } @@ -4477,7 +4477,7 @@ func TestAgent_ServiceMaintenance_Disable(t *testing.T) { ID: "test", Service: "test", } - if err := a.AddServiceFromSource(service, nil, false, "", ConfigSourceLocal); err != nil { + if err := a.addServiceFromSource(service, nil, false, "", ConfigSourceLocal); err != nil { t.Fatalf("err: %v", err) } @@ -4518,7 +4518,7 @@ func TestAgent_ServiceMaintenance_ACLDeny(t *testing.T) { ID: "test", Service: "test", } - if err := a.AddServiceFromSource(service, nil, false, "", ConfigSourceLocal); err != nil { + if err := a.addServiceFromSource(service, nil, false, "", ConfigSourceLocal); err != nil { t.Fatalf("err: %v", err) } diff --git a/agent/agent_test.go b/agent/agent_test.go index 71e943694..4fb395277 100644 --- a/agent/agent_test.go +++ b/agent/agent_test.go @@ -529,7 +529,7 @@ func testAgent_AddService(t *testing.T, extraHCL string) { t.Run(tt.desc, func(t *testing.T) { // check the service registration t.Run(tt.srv.ID, func(t *testing.T) { - err := a.AddServiceFromSource(tt.srv, tt.chkTypes, false, "", ConfigSourceLocal) + err := a.addServiceFromSource(tt.srv, tt.chkTypes, false, "", ConfigSourceLocal) if err != nil { t.Fatalf("err: %v", err) } @@ -572,6 +572,20 @@ func testAgent_AddService(t *testing.T, extraHCL string) { } } +// addServiceFromSource is a test helper that exists to maintain an old function +// signature that was used in many tests. +// Deprecated: use AddService +func (a *Agent) addServiceFromSource(service *structs.NodeService, chkTypes []*structs.CheckType, persist bool, token string, source configSource) error { + return a.AddService(AddServiceRequest{ + Service: service, + chkTypes: chkTypes, + persist: persist, + token: token, + replaceExistingChecks: false, + Source: source, + }) +} + func TestAgent_AddServices_AliasUpdateCheckNotReverted(t *testing.T) { if testing.Short() { t.Skip("too slow for testing.Short") @@ -638,7 +652,7 @@ func testAgent_AddServices_AliasUpdateCheckNotReverted(t *testing.T, extraHCL st chkTypes, err := service.CheckTypes() require.NoError(t, err) - require.NoError(t, a.AddServiceFromSource(ns, chkTypes, false, service.Token, ConfigSourceLocal)) + require.NoError(t, a.addServiceFromSource(ns, chkTypes, false, service.Token, ConfigSourceLocal)) } retry.Run(t, func(r *retry.R) { @@ -665,7 +679,7 @@ func test_createAlias(t *testing.T, agent *TestAgent, chk *structs.CheckType, ex if chk.CheckID == "" { chk.CheckID = types.CheckID(fmt.Sprintf("check-%d", serviceNum)) } - err := agent.AddServiceFromSource(srv, []*structs.CheckType{chk}, false, "", ConfigSourceLocal) + err := agent.addServiceFromSource(srv, []*structs.CheckType{chk}, false, "", ConfigSourceLocal) assert.NoError(t, err) return func(r *retry.R) { t.Helper() @@ -712,7 +726,7 @@ func TestAgent_CheckAliasRPC(t *testing.T) { // We ensure to not block and update Agent's index srv.Tags = []string{fmt.Sprintf("tag-%s", time.Now())} assert.NoError(t, a.waitForUp()) - err := a.AddServiceFromSource(srv, []*structs.CheckType{}, false, "", ConfigSourceLocal) + err := a.addServiceFromSource(srv, []*structs.CheckType{}, false, "", ConfigSourceLocal) assert.NoError(t, err) } shutdownAgent := func() { @@ -727,7 +741,7 @@ func TestAgent_CheckAliasRPC(t *testing.T) { testrpc.WaitForTestAgent(t, a.RPC, "dc1") assert.NoError(t, a.waitForUp()) - err := a.AddServiceFromSource(srv, []*structs.CheckType{}, false, "", ConfigSourceLocal) + err := a.addServiceFromSource(srv, []*structs.CheckType{}, false, "", ConfigSourceLocal) assert.NoError(t, err) retry.Run(t, func(r *retry.R) { @@ -832,12 +846,12 @@ func testAgent_AddServiceNoExec(t *testing.T, extraHCL string) { Interval: 15 * time.Second, } - err := a.AddServiceFromSource(srv, []*structs.CheckType{chk}, false, "", ConfigSourceLocal) + err := a.addServiceFromSource(srv, []*structs.CheckType{chk}, false, "", ConfigSourceLocal) if err == nil || !strings.Contains(err.Error(), "Scripts are disabled on this agent") { t.Fatalf("err: %v", err) } - err = a.AddServiceFromSource(srv, []*structs.CheckType{chk}, false, "", ConfigSourceRemote) + err = a.addServiceFromSource(srv, []*structs.CheckType{chk}, false, "", ConfigSourceRemote) if err == nil || !strings.Contains(err.Error(), "Scripts are disabled on this agent") { t.Fatalf("err: %v", err) } @@ -879,7 +893,7 @@ func testAgent_AddServiceNoRemoteExec(t *testing.T, extraHCL string) { Interval: 15 * time.Second, } - err := a.AddServiceFromSource(srv, []*structs.CheckType{chk}, false, "", ConfigSourceRemote) + err := a.addServiceFromSource(srv, []*structs.CheckType{chk}, false, "", ConfigSourceRemote) if err == nil || !strings.Contains(err.Error(), "Scripts are disabled on this agent") { t.Fatalf("err: %v", err) } @@ -932,7 +946,7 @@ func TestCacheRateLimit(t *testing.T) { Address: fmt.Sprintf("10.0.1.%d", i%255), } - err := a.AddServiceFromSource(srv, []*structs.CheckType{}, false, "", ConfigSourceRemote) + err := a.addServiceFromSource(srv, []*structs.CheckType{}, false, "", ConfigSourceRemote) require.Nil(t, err) } @@ -1007,7 +1021,7 @@ func TestAddServiceIPv4TaggedDefault(t *testing.T) { Address: "10.0.1.2", } - err := a.AddServiceFromSource(srv, []*structs.CheckType{}, false, "", ConfigSourceRemote) + err := a.addServiceFromSource(srv, []*structs.CheckType{}, false, "", ConfigSourceRemote) require.Nil(t, err) ns := a.State.Service(structs.NewServiceID("my_service_id", nil)) @@ -1040,7 +1054,7 @@ func TestAddServiceIPv6TaggedDefault(t *testing.T) { Address: "::5", } - err := a.AddServiceFromSource(srv, []*structs.CheckType{}, false, "", ConfigSourceRemote) + err := a.addServiceFromSource(srv, []*structs.CheckType{}, false, "", ConfigSourceRemote) require.Nil(t, err) ns := a.State.Service(structs.NewServiceID("my_service_id", nil)) @@ -1079,7 +1093,7 @@ func TestAddServiceIPv4TaggedSet(t *testing.T) { }, } - err := a.AddServiceFromSource(srv, []*structs.CheckType{}, false, "", ConfigSourceRemote) + err := a.addServiceFromSource(srv, []*structs.CheckType{}, false, "", ConfigSourceRemote) require.Nil(t, err) ns := a.State.Service(structs.NewServiceID("my_service_id", nil)) @@ -1118,7 +1132,7 @@ func TestAddServiceIPv6TaggedSet(t *testing.T) { }, } - err := a.AddServiceFromSource(srv, []*structs.CheckType{}, false, "", ConfigSourceRemote) + err := a.addServiceFromSource(srv, []*structs.CheckType{}, false, "", ConfigSourceRemote) require.Nil(t, err) ns := a.State.Service(structs.NewServiceID("my_service_id", nil)) @@ -1173,7 +1187,7 @@ func testAgent_RemoveService(t *testing.T, extraHCL string) { } chkTypes := []*structs.CheckType{{TTL: time.Minute}} - if err := a.AddServiceFromSource(srv, chkTypes, false, "", ConfigSourceLocal); err != nil { + if err := a.addServiceFromSource(srv, chkTypes, false, "", ConfigSourceLocal); err != nil { t.Fatalf("err: %v", err) } @@ -1208,7 +1222,7 @@ func testAgent_RemoveService(t *testing.T, extraHCL string) { {TTL: time.Minute}, {TTL: 30 * time.Second}, } - if err := a.AddServiceFromSource(srv, chkTypes, false, "", ConfigSourceLocal); err != nil { + if err := a.addServiceFromSource(srv, chkTypes, false, "", ConfigSourceLocal); err != nil { t.Fatalf("err: %v", err) } @@ -1222,7 +1236,7 @@ func testAgent_RemoveService(t *testing.T, extraHCL string) { {TTL: time.Minute}, {TTL: 30 * time.Second}, } - if err := a.AddServiceFromSource(srv, chkTypes, false, "", ConfigSourceLocal); err != nil { + if err := a.addServiceFromSource(srv, chkTypes, false, "", ConfigSourceLocal); err != nil { t.Fatalf("err: %v", err) } @@ -1294,7 +1308,7 @@ func testAgent_RemoveServiceRemovesAllChecks(t *testing.T, extraHCL string) { } // register service with chk1 - if err := a.AddServiceFromSource(svc, []*structs.CheckType{chk1}, false, "", ConfigSourceLocal); err != nil { + if err := a.addServiceFromSource(svc, []*structs.CheckType{chk1}, false, "", ConfigSourceLocal); err != nil { t.Fatal("Failed to register service", err) } @@ -1302,7 +1316,7 @@ func testAgent_RemoveServiceRemovesAllChecks(t *testing.T, extraHCL string) { requireCheckExists(t, a, "chk1") // update the service with chk2 - if err := a.AddServiceFromSource(svc, []*structs.CheckType{chk2}, false, "", ConfigSourceLocal); err != nil { + if err := a.addServiceFromSource(svc, []*structs.CheckType{chk2}, false, "", ConfigSourceLocal); err != nil { t.Fatal("Failed to update service", err) } @@ -1359,7 +1373,7 @@ func verifyIndexChurn(t *testing.T, tags []string) { Tags: tags, Weights: weights, } - if err := a.AddServiceFromSource(svc, nil, true, "", ConfigSourceLocal); err != nil { + if err := a.addServiceFromSource(svc, nil, true, "", ConfigSourceLocal); err != nil { t.Fatalf("err: %v", err) } @@ -1767,7 +1781,7 @@ func TestAgent_RestoreServiceWithAliasCheck(t *testing.T) { registerServicesAndChecks := func(t *testing.T, a *TestAgent) { // add one persistent service with a simple check - require.NoError(t, a.AddServiceFromSource( + require.NoError(t, a.addServiceFromSource( &structs.NodeService{ ID: "ping", Service: "ping", @@ -1786,7 +1800,7 @@ func TestAgent_RestoreServiceWithAliasCheck(t *testing.T) { // add one persistent sidecar service with an alias check in the manner // of how sidecar_service would add it - require.NoError(t, a.AddServiceFromSource( + require.NoError(t, a.addServiceFromSource( &structs.NodeService{ ID: "ping-sidecar-proxy", Service: "ping-sidecar-proxy", @@ -2276,7 +2290,7 @@ func testAgent_PersistService(t *testing.T, extraHCL string) { file := filepath.Join(a.Config.DataDir, servicesDir, stringHash(svc.ID)) // Check is not persisted unless requested - if err := a.AddServiceFromSource(svc, nil, false, "", ConfigSourceLocal); err != nil { + if err := a.addServiceFromSource(svc, nil, false, "", ConfigSourceLocal); err != nil { t.Fatalf("err: %v", err) } if _, err := os.Stat(file); err == nil { @@ -2284,7 +2298,7 @@ func testAgent_PersistService(t *testing.T, extraHCL string) { } // Persists to file if requested - if err := a.AddServiceFromSource(svc, nil, true, "mytoken", ConfigSourceLocal); err != nil { + if err := a.addServiceFromSource(svc, nil, true, "mytoken", ConfigSourceLocal); err != nil { t.Fatalf("err: %v", err) } if _, err := os.Stat(file); err != nil { @@ -2308,7 +2322,7 @@ func testAgent_PersistService(t *testing.T, extraHCL string) { // Updates service definition on disk svc.Port = 8001 - if err := a.AddServiceFromSource(svc, nil, true, "mytoken", ConfigSourceLocal); err != nil { + if err := a.addServiceFromSource(svc, nil, true, "mytoken", ConfigSourceLocal); err != nil { t.Fatalf("err: %v", err) } expected, err = json.Marshal(persistedService{ @@ -2431,7 +2445,7 @@ func testAgent_PurgeService(t *testing.T, extraHCL string) { } file := filepath.Join(a.Config.DataDir, servicesDir, stringHash(svc.ID)) - if err := a.AddServiceFromSource(svc, nil, true, "", ConfigSourceLocal); err != nil { + if err := a.addServiceFromSource(svc, nil, true, "", ConfigSourceLocal); err != nil { t.Fatalf("err: %v", err) } // Exists @@ -2448,7 +2462,7 @@ func testAgent_PurgeService(t *testing.T, extraHCL string) { } // Re-add the service - if err := a.AddServiceFromSource(svc, nil, true, "", ConfigSourceLocal); err != nil { + if err := a.addServiceFromSource(svc, nil, true, "", ConfigSourceLocal); err != nil { t.Fatalf("err: %v", err) } @@ -2494,7 +2508,7 @@ func testAgent_PurgeServiceOnDuplicate(t *testing.T, extraHCL string) { } // First persist the service - require.NoError(t, a.AddServiceFromSource(svc1, nil, true, "", ConfigSourceLocal)) + require.NoError(t, a.addServiceFromSource(svc1, nil, true, "", ConfigSourceLocal)) a.Shutdown() // Try bringing the agent back up with the service already @@ -2742,9 +2756,9 @@ func TestAgent_DeregisterPersistedSidecarAfterRestart(t *testing.T) { require.NoError(t, err) // First persist the check - err = a.AddServiceFromSource(srv, nil, true, "", ConfigSourceLocal) + err = a.addServiceFromSource(srv, nil, true, "", ConfigSourceLocal) require.NoError(t, err) - err = a.AddServiceFromSource(connectSrv, nil, true, "", ConfigSourceLocal) + err = a.addServiceFromSource(connectSrv, nil, true, "", ConfigSourceLocal) require.NoError(t, err) // check both services were registered @@ -2814,7 +2828,7 @@ func TestAgent_unloadChecks(t *testing.T) { Tags: []string{"foo"}, Port: 8000, } - if err := a.AddServiceFromSource(svc, nil, false, "", ConfigSourceLocal); err != nil { + if err := a.addServiceFromSource(svc, nil, false, "", ConfigSourceLocal); err != nil { t.Fatalf("err: %v", err) } @@ -3093,7 +3107,7 @@ func testAgent_unloadServices(t *testing.T, extraHCL string) { } // Register the service - if err := a.AddServiceFromSource(svc, nil, false, "", ConfigSourceLocal); err != nil { + if err := a.addServiceFromSource(svc, nil, false, "", ConfigSourceLocal); err != nil { t.Fatalf("err: %v", err) } @@ -3125,7 +3139,7 @@ func TestAgent_Service_MaintenanceMode(t *testing.T) { } // Register the service - if err := a.AddServiceFromSource(svc, nil, false, "", ConfigSourceLocal); err != nil { + if err := a.addServiceFromSource(svc, nil, false, "", ConfigSourceLocal); err != nil { t.Fatalf("err: %v", err) } @@ -3206,7 +3220,7 @@ func TestAgent_Service_Reap(t *testing.T) { } // Register the service. - if err := a.AddServiceFromSource(svc, chkTypes, false, "", ConfigSourceLocal); err != nil { + if err := a.addServiceFromSource(svc, chkTypes, false, "", ConfigSourceLocal); err != nil { t.Fatalf("err: %v", err) } @@ -3263,7 +3277,7 @@ func TestAgent_Service_NoReap(t *testing.T) { } // Register the service. - if err := a.AddServiceFromSource(svc, chkTypes, false, "", ConfigSourceLocal); err != nil { + if err := a.addServiceFromSource(svc, chkTypes, false, "", ConfigSourceLocal); err != nil { t.Fatalf("err: %v", err) } @@ -3310,7 +3324,7 @@ func testAgent_AddService_restoresSnapshot(t *testing.T, extraHCL string) { Tags: []string{"foo"}, Port: 8000, } - require.NoError(t, a.AddServiceFromSource(svc, nil, false, "", ConfigSourceLocal)) + require.NoError(t, a.addServiceFromSource(svc, nil, false, "", ConfigSourceLocal)) // Register a check check1 := &structs.HealthCheck{ @@ -3325,7 +3339,7 @@ func testAgent_AddService_restoresSnapshot(t *testing.T, extraHCL string) { // Re-registering the service preserves the state of the check chkTypes := []*structs.CheckType{{TTL: 30 * time.Second}} - require.NoError(t, a.AddServiceFromSource(svc, chkTypes, false, "", ConfigSourceLocal)) + require.NoError(t, a.addServiceFromSource(svc, chkTypes, false, "", ConfigSourceLocal)) check := requireCheckExists(t, a, "service:redis") require.Equal(t, api.HealthPassing, check.Status) } @@ -3346,7 +3360,7 @@ func TestAgent_AddCheck_restoresSnapshot(t *testing.T) { Tags: []string{"foo"}, Port: 8000, } - if err := a.AddServiceFromSource(svc, nil, false, "", ConfigSourceLocal); err != nil { + if err := a.addServiceFromSource(svc, nil, false, "", ConfigSourceLocal); err != nil { t.Fatalf("err: %v", err) } @@ -3431,7 +3445,7 @@ func TestAgent_checkStateSnapshot(t *testing.T) { Tags: []string{"foo"}, Port: 8000, } - if err := a.AddServiceFromSource(svc, nil, false, "", ConfigSourceLocal); err != nil { + if err := a.addServiceFromSource(svc, nil, false, "", ConfigSourceLocal); err != nil { t.Fatalf("err: %v", err) } @@ -4250,7 +4264,7 @@ func TestAgent_RerouteExistingHTTPChecks(t *testing.T) { TLSSkipVerify: true, }, } - if err := a.AddServiceFromSource(svc, chks, false, "", ConfigSourceLocal); err != nil { + if err := a.addServiceFromSource(svc, chks, false, "", ConfigSourceLocal); err != nil { t.Fatalf("failed to add svc: %v", err) } @@ -4273,7 +4287,7 @@ func TestAgent_RerouteExistingHTTPChecks(t *testing.T) { }, }, } - if err := a.AddServiceFromSource(proxy, nil, false, "", ConfigSourceLocal); err != nil { + if err := a.addServiceFromSource(proxy, nil, false, "", ConfigSourceLocal); err != nil { t.Fatalf("failed to add svc: %v", err) } @@ -4326,7 +4340,7 @@ func TestAgent_RerouteExistingHTTPChecks(t *testing.T) { }, }, } - if err := a.AddServiceFromSource(proxy, nil, false, "", ConfigSourceLocal); err != nil { + if err := a.addServiceFromSource(proxy, nil, false, "", ConfigSourceLocal); err != nil { t.Fatalf("failed to add svc: %v", err) } @@ -4369,7 +4383,7 @@ func TestAgent_RerouteNewHTTPChecks(t *testing.T) { Address: "localhost", Port: 8080, } - if err := a.AddServiceFromSource(svc, nil, false, "", ConfigSourceLocal); err != nil { + if err := a.addServiceFromSource(svc, nil, false, "", ConfigSourceLocal); err != nil { t.Fatalf("failed to add svc: %v", err) } @@ -4391,7 +4405,7 @@ func TestAgent_RerouteNewHTTPChecks(t *testing.T) { }, }, } - if err := a.AddServiceFromSource(proxy, nil, false, "", ConfigSourceLocal); err != nil { + if err := a.addServiceFromSource(proxy, nil, false, "", ConfigSourceLocal); err != nil { t.Fatalf("failed to add svc: %v", err) } diff --git a/agent/service_checks_test.go b/agent/service_checks_test.go index 5b82aca28..19c6c60be 100644 --- a/agent/service_checks_test.go +++ b/agent/service_checks_test.go @@ -63,7 +63,7 @@ func TestAgent_ServiceHTTPChecksNotification(t *testing.T) { }, } // Adding TTL type should lead to a timeout, since only HTTP-based checks are watched - if err := a.AddServiceFromSource(&service, chkTypes[2:], false, "", ConfigSourceLocal); err != nil { + if err := a.addServiceFromSource(&service, chkTypes[2:], false, "", ConfigSourceLocal); err != nil { t.Fatalf("failed to add service: %v", err) } @@ -75,7 +75,7 @@ func TestAgent_ServiceHTTPChecksNotification(t *testing.T) { } // Adding service with HTTP checks should lead notification for them - if err := a.AddServiceFromSource(&service, chkTypes[0:2], false, "", ConfigSourceLocal); err != nil { + if err := a.addServiceFromSource(&service, chkTypes[0:2], false, "", ConfigSourceLocal); err != nil { t.Fatalf("failed to add service: %v", err) } diff --git a/agent/service_manager.go b/agent/service_manager.go index d6874adee..725b93df1 100644 --- a/agent/service_manager.go +++ b/agent/service_manager.go @@ -4,12 +4,13 @@ import ( "fmt" "sync" - "github.com/hashicorp/consul/agent/cache" - cachetype "github.com/hashicorp/consul/agent/cache-types" - "github.com/hashicorp/consul/agent/structs" "github.com/imdario/mergo" "github.com/mitchellh/copystructure" "golang.org/x/net/context" + + "github.com/hashicorp/consul/agent/cache" + cachetype "github.com/hashicorp/consul/agent/cache-types" + "github.com/hashicorp/consul/agent/structs" ) // The ServiceManager is a layer for service registration in between the agent @@ -83,7 +84,7 @@ func (s *ServiceManager) Start() { } // runOnce will process a single registration request -func (s *ServiceManager) registerOnce(args *addServiceRequest) error { +func (s *ServiceManager) registerOnce(args *AddServiceRequest) error { s.agent.stateLock.Lock() defer s.agent.stateLock.Unlock() @@ -120,14 +121,14 @@ func (s *ServiceManager) registerOnce(args *addServiceRequest) error { // merged with the global defaults before registration. // // NOTE: the caller must hold the Agent.stateLock! -func (s *ServiceManager) AddService(req *addServiceRequest) error { +func (s *ServiceManager) AddService(req *AddServiceRequest) error { req.fixupForAddServiceLocked() - req.service.EnterpriseMeta.Normalize() + req.Service.EnterpriseMeta.Normalize() // For now only proxies have anything that can be configured // centrally. So bypass the whole manager for regular services. - if !req.service.IsSidecarProxy() && !req.service.IsGateway() { + if !req.Service.IsSidecarProxy() && !req.Service.IsGateway() { // previousDefaults are ignored here because they are only relevant for central config. req.persistService = nil req.persistDefaults = nil @@ -136,7 +137,7 @@ func (s *ServiceManager) AddService(req *addServiceRequest) error { } var ( - service = req.service + service = req.Service chkTypes = req.chkTypes previousDefaults = req.previousDefaults waitForCentralConfig = req.waitForCentralConfig @@ -144,7 +145,7 @@ func (s *ServiceManager) AddService(req *addServiceRequest) error { persistServiceConfig = req.persistServiceConfig token = req.token replaceExistingChecks = req.replaceExistingChecks - source = req.source + source = req.Source ) reg := &serviceRegistration{ @@ -267,8 +268,8 @@ func (w *serviceConfigWatch) RegisterAndStart( // The first time we do this interactively, we need to know if it // failed for validation reasons which we only get back from the // initial underlying add service call. - err = w.agent.addServiceInternal(&addServiceRequest{ - service: merged, + err = w.agent.addServiceInternal(&AddServiceRequest{ + Service: merged, chkTypes: w.registration.chkTypes, persistService: w.registration.service, persistDefaults: serviceDefaults, @@ -276,7 +277,7 @@ func (w *serviceConfigWatch) RegisterAndStart( persistServiceConfig: persistServiceConfig, token: w.registration.token, replaceExistingChecks: w.registration.replaceExistingChecks, - source: w.registration.source, + Source: w.registration.source, snap: w.agent.snapshotCheckState(), }) if err != nil { @@ -408,8 +409,8 @@ func (w *serviceConfigWatch) handleUpdate(ctx context.Context, event cache.Updat } registerReq := &asyncRegisterRequest{ - Args: &addServiceRequest{ - service: merged, + Args: &AddServiceRequest{ + Service: merged, chkTypes: w.registration.chkTypes, persistService: w.registration.service, persistDefaults: serviceDefaults, @@ -417,7 +418,7 @@ func (w *serviceConfigWatch) handleUpdate(ctx context.Context, event cache.Updat persistServiceConfig: true, token: w.registration.token, replaceExistingChecks: w.registration.replaceExistingChecks, - source: w.registration.source, + Source: w.registration.source, }, Reply: make(chan error, 1), } @@ -441,7 +442,7 @@ func (w *serviceConfigWatch) handleUpdate(ctx context.Context, event cache.Updat } type asyncRegisterRequest struct { - Args *addServiceRequest + Args *AddServiceRequest Reply chan error } diff --git a/agent/service_manager_test.go b/agent/service_manager_test.go index 073ac577d..ea1d6c4f1 100644 --- a/agent/service_manager_test.go +++ b/agent/service_manager_test.go @@ -48,7 +48,7 @@ func TestServiceManager_RegisterService(t *testing.T) { Port: 8000, EnterpriseMeta: *structs.DefaultEnterpriseMeta(), } - require.NoError(a.AddServiceFromSource(svc, nil, false, "", ConfigSourceLocal)) + require.NoError(a.addServiceFromSource(svc, nil, false, "", ConfigSourceLocal)) // Verify both the service and sidecar. redisService := a.State.Service(structs.NewServiceID("redis", nil)) @@ -119,7 +119,7 @@ func TestServiceManager_RegisterSidecar(t *testing.T) { }, EnterpriseMeta: *structs.DefaultEnterpriseMeta(), } - require.NoError(a.AddServiceFromSource(svc, nil, false, "", ConfigSourceLocal)) + require.NoError(a.addServiceFromSource(svc, nil, false, "", ConfigSourceLocal)) // Verify sidecar got global config loaded sidecarService := a.State.Service(structs.NewServiceID("web-sidecar-proxy", nil)) @@ -192,7 +192,7 @@ func TestServiceManager_RegisterMeshGateway(t *testing.T) { EnterpriseMeta: *structs.DefaultEnterpriseMeta(), } - require.NoError(a.AddServiceFromSource(svc, nil, false, "", ConfigSourceLocal)) + require.NoError(a.addServiceFromSource(svc, nil, false, "", ConfigSourceLocal)) // Verify gateway got global config loaded gateway := a.State.Service(structs.NewServiceID("mesh-gateway", nil)) @@ -252,7 +252,7 @@ func TestServiceManager_RegisterTerminatingGateway(t *testing.T) { EnterpriseMeta: *structs.DefaultEnterpriseMeta(), } - require.NoError(a.AddServiceFromSource(svc, nil, false, "", ConfigSourceLocal)) + require.NoError(a.addServiceFromSource(svc, nil, false, "", ConfigSourceLocal)) // Verify gateway got global config loaded gateway := a.State.Service(structs.NewServiceID("terminating-gateway", nil)) @@ -387,12 +387,12 @@ func TestServiceManager_PersistService_API(t *testing.T) { configFile := filepath.Join(a.Config.DataDir, serviceConfigDir, svcID.StringHash()) // Service is not persisted unless requested, but we always persist service configs. - require.NoError(a.AddServiceFromSource(svc, nil, false, "", ConfigSourceRemote)) + require.NoError(a.addServiceFromSource(svc, nil, false, "", ConfigSourceRemote)) requireFileIsAbsent(t, svcFile) requireFileIsPresent(t, configFile) // Persists to file if requested - require.NoError(a.AddServiceFromSource(svc, nil, true, "mytoken", ConfigSourceRemote)) + require.NoError(a.addServiceFromSource(svc, nil, true, "mytoken", ConfigSourceRemote)) requireFileIsPresent(t, svcFile) requireFileIsPresent(t, configFile) @@ -433,7 +433,7 @@ func TestServiceManager_PersistService_API(t *testing.T) { // Updates service definition on disk svc.Proxy.LocalServicePort = 8001 - require.NoError(a.AddServiceFromSource(svc, nil, true, "mytoken", ConfigSourceRemote)) + require.NoError(a.addServiceFromSource(svc, nil, true, "mytoken", ConfigSourceRemote)) requireFileIsPresent(t, svcFile) requireFileIsPresent(t, configFile) @@ -721,7 +721,7 @@ func TestServiceManager_Disabled(t *testing.T) { }, EnterpriseMeta: *structs.DefaultEnterpriseMeta(), } - require.NoError(a.AddServiceFromSource(svc, nil, false, "", ConfigSourceLocal)) + require.NoError(a.addServiceFromSource(svc, nil, false, "", ConfigSourceLocal)) // Verify sidecar got global config loaded sidecarService := a.State.Service(structs.NewServiceID("web-sidecar-proxy", nil)) diff --git a/agent/sidecar_service.go b/agent/sidecar_service.go index bd78adb15..ac3e02c66 100644 --- a/agent/sidecar_service.go +++ b/agent/sidecar_service.go @@ -29,7 +29,7 @@ func sidecarServiceID(serviceID string) string { // The third return argument is the effective Token to use for the sidecar // registration. This will be the same as the token parameter passed unless the // SidecarService definition contains a distinct one. -// TODO: return addServiceRequest +// TODO: return AddServiceRequest func (a *Agent) sidecarServiceFromNodeService(ns *structs.NodeService, token string) (*structs.NodeService, []*structs.CheckType, string, error) { if ns.Connect.SidecarService == nil { return nil, nil, "", nil diff --git a/agent/sidecar_service_test.go b/agent/sidecar_service_test.go index d287c169a..0840d8044 100644 --- a/agent/sidecar_service_test.go +++ b/agent/sidecar_service_test.go @@ -334,7 +334,7 @@ func TestAgent_sidecarServiceFromNodeService(t *testing.T) { defer a.Shutdown() if tt.preRegister != nil { - err := a.AddServiceFromSource(tt.preRegister.NodeService(), nil, false, "", ConfigSourceLocal) + err := a.addServiceFromSource(tt.preRegister.NodeService(), nil, false, "", ConfigSourceLocal) require.NoError(err) } diff --git a/command/maint/maint_test.go b/command/maint/maint_test.go index 78e481339..18ee9eb40 100644 --- a/command/maint/maint_test.go +++ b/command/maint/maint_test.go @@ -50,11 +50,14 @@ func TestMaintCommand_NoArgs(t *testing.T) { defer a.Shutdown() // Register the service and put it into maintenance mode - service := &structs.NodeService{ - ID: "test", - Service: "test", + addReq := agent.AddServiceRequest{ + Service: &structs.NodeService{ + ID: "test", + Service: "test", + }, + Source: agent.ConfigSourceLocal, } - if err := a.AddServiceFromSource(service, nil, false, "", agent.ConfigSourceLocal); err != nil { + if err := a.AddService(addReq); err != nil { t.Fatalf("err: %v", err) } if err := a.EnableServiceMaintenance(structs.NewServiceID("test", nil), "broken 1", ""); err != nil { @@ -158,11 +161,14 @@ func TestMaintCommand_EnableServiceMaintenance(t *testing.T) { defer a.Shutdown() // Register the service - service := &structs.NodeService{ - ID: "test", - Service: "test", + addReq := agent.AddServiceRequest{ + Service: &structs.NodeService{ + ID: "test", + Service: "test", + }, + Source: agent.ConfigSourceLocal, } - if err := a.AddServiceFromSource(service, nil, false, "", agent.ConfigSourceLocal); err != nil { + if err := a.AddService(addReq); err != nil { t.Fatalf("err: %v", err) } @@ -196,11 +202,14 @@ func TestMaintCommand_DisableServiceMaintenance(t *testing.T) { defer a.Shutdown() // Register the service - service := &structs.NodeService{ - ID: "test", - Service: "test", + addReq := agent.AddServiceRequest{ + Service: &structs.NodeService{ + ID: "test", + Service: "test", + }, + Source: agent.ConfigSourceLocal, } - if err := a.AddServiceFromSource(service, nil, false, "", agent.ConfigSourceLocal); err != nil { + if err := a.AddService(addReq); err != nil { t.Fatalf("err: %v", err) } From 493e987a88bb1fb83a8803b046d8ff260550e6b6 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Mon, 30 Nov 2020 13:46:14 -0500 Subject: [PATCH 4/5] agent: addServiceIternalRequest Move fields that are only relevant for addServiceInternal onto a new struct and embed AddServiceRequest. --- agent/agent.go | 35 ++++++++---------------- agent/service_manager.go | 59 ++++++++++++++++++++-------------------- 2 files changed, 40 insertions(+), 54 deletions(-) diff --git a/agent/agent.go b/agent/agent.go index ebe6d92a0..38a02792f 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -1901,14 +1901,12 @@ func (a *Agent) AddService(req AddServiceRequest) error { defer a.stateLock.Unlock() req.snap = a.State.Checks(structs.WildcardEnterpriseMeta()) - return a.addServiceLocked(&req) + return a.addServiceLocked(req) } // addServiceLocked adds a service entry to the service manager if enabled, or directly // to the local state if it is not. This function assumes the state lock is already held. -func (a *Agent) addServiceLocked(req *AddServiceRequest) error { - req.fixupForAddServiceLocked() - +func (a *Agent) addServiceLocked(req AddServiceRequest) error { req.Service.EnterpriseMeta.Normalize() if err := a.validateService(req.Service, req.chkTypes); err != nil { @@ -1919,12 +1917,8 @@ func (a *Agent) addServiceLocked(req *AddServiceRequest) error { return a.serviceManager.AddService(req) } - // previousDefaults are ignored here because they are only relevant for central config. - req.persistService = nil - req.persistDefaults = nil req.persistServiceConfig = false - - return a.addServiceInternal(req) + return a.addServiceInternal(addServiceInternalRequest{AddServiceRequest: req}) } // AddServiceRequest is the union of arguments for calling both @@ -1942,8 +1936,6 @@ type AddServiceRequest struct { chkTypes []*structs.CheckType previousDefaults *structs.ServiceConfigResponse // just for: addServiceLocked waitForCentralConfig bool // just for: addServiceLocked - persistService *structs.NodeService // just for: addServiceInternal - persistDefaults *structs.ServiceConfigResponse // just for: addServiceInternal persist bool persistServiceConfig bool token string @@ -1952,19 +1944,14 @@ type AddServiceRequest struct { snap map[structs.CheckID]*structs.HealthCheck } -func (r *AddServiceRequest) fixupForAddServiceLocked() { - r.persistService = nil - r.persistDefaults = nil -} - -func (r *AddServiceRequest) fixupForAddServiceInternal() { - r.previousDefaults = nil - r.waitForCentralConfig = false +type addServiceInternalRequest struct { + AddServiceRequest + persistService *structs.NodeService + persistDefaults *structs.ServiceConfigResponse } // addServiceInternal adds the given service and checks to the local state. -func (a *Agent) addServiceInternal(req *AddServiceRequest) error { - req.fixupForAddServiceInternal() +func (a *Agent) addServiceInternal(req addServiceInternalRequest) error { var ( service = req.Service chkTypes = req.chkTypes @@ -3100,7 +3087,7 @@ func (a *Agent) loadServices(conf *config.RuntimeConfig, snap map[structs.CheckI ns.Connect.SidecarService = nil sid := ns.CompoundServiceID() - err = a.addServiceLocked(&AddServiceRequest{ + err = a.addServiceLocked(AddServiceRequest{ Service: ns, chkTypes: chkTypes, previousDefaults: persistedServiceConfigs[sid], @@ -3119,7 +3106,7 @@ func (a *Agent) loadServices(conf *config.RuntimeConfig, snap map[structs.CheckI // If there is a sidecar service, register that too. if sidecar != nil { sidecarServiceID := sidecar.CompoundServiceID() - err = a.addServiceLocked(&AddServiceRequest{ + err = a.addServiceLocked(AddServiceRequest{ Service: sidecar, chkTypes: sidecarChecks, previousDefaults: persistedServiceConfigs[sidecarServiceID], @@ -3216,7 +3203,7 @@ func (a *Agent) loadServices(conf *config.RuntimeConfig, snap map[structs.CheckI "service", serviceID.String(), "file", file, ) - err = a.addServiceLocked(&AddServiceRequest{ + err = a.addServiceLocked(AddServiceRequest{ Service: p.Service, chkTypes: nil, previousDefaults: persistedServiceConfigs[serviceID], diff --git a/agent/service_manager.go b/agent/service_manager.go index 725b93df1..5fcaa3334 100644 --- a/agent/service_manager.go +++ b/agent/service_manager.go @@ -84,7 +84,7 @@ func (s *ServiceManager) Start() { } // runOnce will process a single registration request -func (s *ServiceManager) registerOnce(args *AddServiceRequest) error { +func (s *ServiceManager) registerOnce(args addServiceInternalRequest) error { s.agent.stateLock.Lock() defer s.agent.stateLock.Unlock() @@ -121,19 +121,14 @@ func (s *ServiceManager) registerOnce(args *AddServiceRequest) error { // merged with the global defaults before registration. // // NOTE: the caller must hold the Agent.stateLock! -func (s *ServiceManager) AddService(req *AddServiceRequest) error { - req.fixupForAddServiceLocked() - +func (s *ServiceManager) AddService(req AddServiceRequest) error { req.Service.EnterpriseMeta.Normalize() // For now only proxies have anything that can be configured // centrally. So bypass the whole manager for regular services. if !req.Service.IsSidecarProxy() && !req.Service.IsGateway() { - // previousDefaults are ignored here because they are only relevant for central config. - req.persistService = nil - req.persistDefaults = nil req.persistServiceConfig = false - return s.agent.addServiceInternal(req) + return s.agent.addServiceInternal(addServiceInternalRequest{AddServiceRequest: req}) } var ( @@ -268,17 +263,19 @@ func (w *serviceConfigWatch) RegisterAndStart( // The first time we do this interactively, we need to know if it // failed for validation reasons which we only get back from the // initial underlying add service call. - err = w.agent.addServiceInternal(&AddServiceRequest{ - Service: merged, - chkTypes: w.registration.chkTypes, - persistService: w.registration.service, - persistDefaults: serviceDefaults, - persist: w.registration.persist, - persistServiceConfig: persistServiceConfig, - token: w.registration.token, - replaceExistingChecks: w.registration.replaceExistingChecks, - Source: w.registration.source, - snap: w.agent.snapshotCheckState(), + err = w.agent.addServiceInternal(addServiceInternalRequest{ + AddServiceRequest: AddServiceRequest{ + Service: merged, + chkTypes: w.registration.chkTypes, + persist: w.registration.persist, + persistServiceConfig: persistServiceConfig, + token: w.registration.token, + replaceExistingChecks: w.registration.replaceExistingChecks, + Source: w.registration.source, + snap: w.agent.snapshotCheckState(), + }, + persistService: w.registration.service, + persistDefaults: serviceDefaults, }) if err != nil { return fmt.Errorf("error updating service registration: %v", err) @@ -409,16 +406,18 @@ func (w *serviceConfigWatch) handleUpdate(ctx context.Context, event cache.Updat } registerReq := &asyncRegisterRequest{ - Args: &AddServiceRequest{ - Service: merged, - chkTypes: w.registration.chkTypes, - persistService: w.registration.service, - persistDefaults: serviceDefaults, - persist: w.registration.persist, - persistServiceConfig: true, - token: w.registration.token, - replaceExistingChecks: w.registration.replaceExistingChecks, - Source: w.registration.source, + Args: addServiceInternalRequest{ + AddServiceRequest: AddServiceRequest{ + Service: merged, + chkTypes: w.registration.chkTypes, + persist: w.registration.persist, + persistServiceConfig: true, + token: w.registration.token, + replaceExistingChecks: w.registration.replaceExistingChecks, + Source: w.registration.source, + }, + persistService: w.registration.service, + persistDefaults: serviceDefaults, }, Reply: make(chan error, 1), } @@ -442,7 +441,7 @@ func (w *serviceConfigWatch) handleUpdate(ctx context.Context, event cache.Updat } type asyncRegisterRequest struct { - Args *AddServiceRequest + Args addServiceInternalRequest Reply chan error } From 1ce7cdd892fad2fae2edb3b13f81a6831f97159a Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Mon, 30 Nov 2020 14:08:26 -0500 Subject: [PATCH 5/5] agent: use fields directly, not temp variables The temprorary variables make it much harder to trace where and how struct fields are used. If a field is only used a small number of times than refer to the field directly. --- agent/agent.go | 42 +++++++++++++++------------------------- agent/service_manager.go | 37 +++++++++++++---------------------- 2 files changed, 29 insertions(+), 50 deletions(-) diff --git a/agent/agent.go b/agent/agent.go index 38a02792f..6d688b804 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -1894,7 +1894,6 @@ func (a *Agent) readPersistedServiceConfigs() (map[structs.ServiceID]*structs.Se // This entry is persistent and the agent will make a best effort to // ensure it is registered func (a *Agent) AddService(req AddServiceRequest) error { - // service *structs.NodeService, chkTypes []*structs.CheckType, persist bool, token string, source configSource req.waitForCentralConfig = true req.persistServiceConfig = true a.stateLock.Lock() @@ -1952,18 +1951,7 @@ type addServiceInternalRequest struct { // addServiceInternal adds the given service and checks to the local state. func (a *Agent) addServiceInternal(req addServiceInternalRequest) error { - var ( - service = req.Service - chkTypes = req.chkTypes - persistService = req.persistService - persistDefaults = req.persistDefaults - persist = req.persist - persistServiceConfig = req.persistServiceConfig - token = req.token - replaceExistingChecks = req.replaceExistingChecks - source = req.Source - snap = req.snap - ) + service := req.Service // Pause the service syncs during modification a.PauseSync() @@ -1999,11 +1987,11 @@ func (a *Agent) addServiceInternal(req addServiceInternalRequest) error { } // Create an associated health check - for i, chkType := range chkTypes { + for i, chkType := range req.chkTypes { checkID := string(chkType.CheckID) if checkID == "" { checkID = fmt.Sprintf("service:%s", service.ID) - if len(chkTypes) > 1 { + if len(req.chkTypes) > 1 { checkID += fmt.Sprintf(":%d", i+1) } } @@ -2032,7 +2020,7 @@ func (a *Agent) addServiceInternal(req addServiceInternalRequest) error { } // Restore the fields from the snapshot. - prev, ok := snap[cid] + prev, ok := req.snap[cid] if ok { check.Output = prev.Output check.Status = prev.Status @@ -2059,20 +2047,22 @@ func (a *Agent) addServiceInternal(req addServiceInternalRequest) error { } } - err := a.State.AddServiceWithChecks(service, checks, token) + err := a.State.AddServiceWithChecks(service, checks, req.token) if err != nil { a.cleanupRegistration(cleanupServices, cleanupChecks) return err } + source := req.Source + persist := req.persist for i := range checks { - if err := a.addCheck(checks[i], chkTypes[i], service, token, source); err != nil { + if err := a.addCheck(checks[i], req.chkTypes[i], service, req.token, source); err != nil { a.cleanupRegistration(cleanupServices, cleanupChecks) return err } if persist && a.config.DataDir != "" { - if err := a.persistCheck(checks[i], chkTypes[i], source); err != nil { + if err := a.persistCheck(checks[i], req.chkTypes[i], source); err != nil { a.cleanupRegistration(cleanupServices, cleanupChecks) return err @@ -2095,10 +2085,10 @@ func (a *Agent) addServiceInternal(req addServiceInternalRequest) error { a.resetExposedChecks(psid) } - if persistServiceConfig && a.config.DataDir != "" { + if req.persistServiceConfig && a.config.DataDir != "" { var err error - if persistDefaults != nil { - err = a.persistServiceConfig(service.CompoundServiceID(), persistDefaults) + if req.persistDefaults != nil { + err = a.persistServiceConfig(service.CompoundServiceID(), req.persistDefaults) } else { err = a.purgeServiceConfig(service.CompoundServiceID()) } @@ -2111,17 +2101,17 @@ func (a *Agent) addServiceInternal(req addServiceInternalRequest) error { // Persist the service to a file if persist && a.config.DataDir != "" { - if persistService == nil { - persistService = service + if req.persistService == nil { + req.persistService = service } - if err := a.persistService(persistService, source); err != nil { + if err := a.persistService(req.persistService, source); err != nil { a.cleanupRegistration(cleanupServices, cleanupChecks) return err } } - if replaceExistingChecks { + if req.replaceExistingChecks { for checkID, keep := range existingChecks { if !keep { a.removeCheckLocked(checkID, persist) diff --git a/agent/service_manager.go b/agent/service_manager.go index 5fcaa3334..5badb3a26 100644 --- a/agent/service_manager.go +++ b/agent/service_manager.go @@ -131,31 +131,20 @@ func (s *ServiceManager) AddService(req AddServiceRequest) error { return s.agent.addServiceInternal(addServiceInternalRequest{AddServiceRequest: req}) } - var ( - service = req.Service - chkTypes = req.chkTypes - previousDefaults = req.previousDefaults - waitForCentralConfig = req.waitForCentralConfig - persist = req.persist - persistServiceConfig = req.persistServiceConfig - token = req.token - replaceExistingChecks = req.replaceExistingChecks - source = req.Source - ) - + // TODO: replace serviceRegistration with AddServiceRequest reg := &serviceRegistration{ - service: service, - chkTypes: chkTypes, - persist: persist, - token: token, - replaceExistingChecks: replaceExistingChecks, - source: source, + service: req.Service, + chkTypes: req.chkTypes, + persist: req.persist, + token: req.token, + replaceExistingChecks: req.replaceExistingChecks, + source: req.Source, } s.servicesLock.Lock() defer s.servicesLock.Unlock() - sid := service.CompoundServiceID() + sid := req.Service.CompoundServiceID() // If a service watch already exists, shut it down and replace it. oldWatch, updating := s.services[sid] @@ -174,9 +163,9 @@ func (s *ServiceManager) AddService(req AddServiceRequest) error { err := watch.RegisterAndStart( s.ctx, - previousDefaults, - waitForCentralConfig, - persistServiceConfig, + req.previousDefaults, + req.waitForCentralConfig, + req.persistServiceConfig, &s.running, ) if err != nil { @@ -186,9 +175,9 @@ func (s *ServiceManager) AddService(req AddServiceRequest) error { s.services[sid] = watch if updating { - s.agent.logger.Debug("updated local registration for service", "service", service.ID) + s.agent.logger.Debug("updated local registration for service", "service", req.Service.ID) } else { - s.agent.logger.Debug("added local registration for service", "service", service.ID) + s.agent.logger.Debug("added local registration for service", "service", req.Service.ID) } return nil