Skip to content

Commit

Permalink
Fix: service LocallyRegisteredAsSidecar property is not persisted
Browse files Browse the repository at this point in the history
When a service is deregistered, we check whever matching services were
registered as sidecar along with it and deregister them as well.
To determine if a service is indeed a sidecar we check the
structs.ServiceNode.LocallyRegisteredAsSidecar property. However, to
avoid interal API leakage, it is excluded from JSON serialization,
meaning it is not persisted to disk either.
When the agent is restarted, this property lost and sidecars are no
longer deregistered along with their parent service.
To fix this, we now specifically save this property in the persisted
service file.
  • Loading branch information
ShimmerGlass committed Oct 13, 2020
1 parent 52451cf commit 1c8369b
Show file tree
Hide file tree
Showing 3 changed files with 85 additions and 3 deletions.
3 changes: 3 additions & 0 deletions .changelog/8924.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:bug
connect: fix connect sidecars registered via the API not being automatically deregistered with their parent service after an agent restart by persisting the LocallyRegisteredAsSidecar property.
```
16 changes: 13 additions & 3 deletions agent/agent.go
Original file line number Diff line number Diff line change
Expand Up @@ -1701,6 +1701,11 @@ type persistedService struct {
Token string
Service *structs.NodeService
Source string
// whether this service was registered as a sidecar, see structs.NodeService
// we store this field here because it is excluded from json serialization
// to exclude it from API output, but we need it to properly deregister
// persisted sidecars.
LocallyRegisteredAsSidecar bool `json:",omitempty"`
}

// persistService saves a service definition to a JSON file in the data dir
Expand All @@ -1709,9 +1714,10 @@ func (a *Agent) persistService(service *structs.NodeService, source configSource
svcPath := filepath.Join(a.config.DataDir, servicesDir, svcID.StringHash())

wrapped := persistedService{
Token: a.State.ServiceToken(service.CompoundServiceID()),
Service: service,
Source: source.String(),
Token: a.State.ServiceToken(service.CompoundServiceID()),
Service: service,
Source: source.String(),
LocallyRegisteredAsSidecar: service.LocallyRegisteredAsSidecar,
}
encoded, err := json.Marshal(wrapped)
if err != nil {
Expand Down Expand Up @@ -3160,6 +3166,10 @@ func (a *Agent) loadServices(conf *config.RuntimeConfig, snap map[structs.CheckI
continue
}
}

// Restore LocallyRegisteredAsSidecar, see persistedService.LocallyRegisteredAsSidecar
p.Service.LocallyRegisteredAsSidecar = p.LocallyRegisteredAsSidecar

serviceID := p.Service.CompoundServiceID()

source, ok := ConfigSourceFromName(p.Source)
Expand Down
69 changes: 69 additions & 0 deletions agent/agent_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2442,6 +2442,75 @@ func TestAgent_PurgeCheckOnDuplicate(t *testing.T) {
require.Equal(t, expected, result)
}

func TestAgent_DeregisterPersistedSidecarAfterRestart(t *testing.T) {
t.Parallel()
nodeID := NodeID()
a := StartTestAgent(t, TestAgent{
HCL: `
node_id = "` + nodeID + `"
node_name = "Node ` + nodeID + `"
server = false
bootstrap = false
enable_central_service_config = false
`})
defer a.Shutdown()

srv := &structs.NodeService{
ID: "svc",
Service: "svc",
Weights: &structs.Weights{
Passing: 2,
Warning: 1,
},
Tags: []string{"tag2"},
Port: 8200,
EnterpriseMeta: *structs.DefaultEnterpriseMeta(),

Connect: structs.ServiceConnect{
SidecarService: &structs.ServiceDefinition{},
},
}

connectSrv, _, _, err := a.sidecarServiceFromNodeService(srv, "")
require.NoError(t, err)

// First persist the check
err = a.AddService(srv, nil, true, "", ConfigSourceLocal)
require.NoError(t, err)
err = a.AddService(connectSrv, nil, true, "", ConfigSourceLocal)
require.NoError(t, err)

// check both services were registered
require.NotNil(t, a.State.Service(srv.CompoundServiceID()))
require.NotNil(t, a.State.Service(connectSrv.CompoundServiceID()))

a.Shutdown()

// Start again with the check registered in config
a2 := StartTestAgent(t, TestAgent{
Name: "Agent2",
DataDir: a.DataDir,
HCL: `
node_id = "` + nodeID + `"
node_name = "Node ` + nodeID + `"
server = false
bootstrap = false
enable_central_service_config = false
`})
defer a2.Shutdown()

// check both services were restored
require.NotNil(t, a2.State.Service(srv.CompoundServiceID()))
require.NotNil(t, a2.State.Service(connectSrv.CompoundServiceID()))

err = a2.RemoveService(srv.CompoundServiceID())
require.NoError(t, err)

// check both services were deregistered
require.Nil(t, a2.State.Service(srv.CompoundServiceID()))
require.Nil(t, a2.State.Service(connectSrv.CompoundServiceID()))
}

func TestAgent_loadChecks_token(t *testing.T) {
t.Parallel()
a := NewTestAgent(t, `
Expand Down

0 comments on commit 1c8369b

Please sign in to comment.