From 3bbd16fd392001849e79c176f21b545b725164c3 Mon Sep 17 00:00:00 2001 From: freddygv Date: Fri, 27 Sep 2019 17:38:27 -0600 Subject: [PATCH 1/6] Store check type in catalog --- agent/agent.go | 3 ++ agent/agent_endpoint.go | 3 ++ agent/agent_endpoint_test.go | 10 ++++++ agent/health_endpoint_test.go | 57 +++++++++++++++++++++++++++++++++++ agent/structs/check_type.go | 21 +++++++++++++ agent/structs/structs.go | 1 + api/agent.go | 1 + api/agent_test.go | 15 +++++++++ api/health.go | 1 + api/health_test.go | 14 +++++++-- 10 files changed, 124 insertions(+), 2 deletions(-) diff --git a/agent/agent.go b/agent/agent.go index f2e5c318e85f..f2e971ea3535 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -2268,6 +2268,7 @@ func (a *Agent) addServiceInternal(req *addServiceRequest) error { ServiceID: service.ID, ServiceName: service.Service, ServiceTags: service.Tags, + Type: chkType.Type(), } if chkType.Status != "" { check.Status = chkType.Status @@ -3613,6 +3614,7 @@ func (a *Agent) EnableServiceMaintenance(serviceID, reason, token string) error ServiceID: service.ID, ServiceName: service.Service, Status: api.HealthCritical, + Type: "maintenance", } a.AddCheck(check, nil, true, token, ConfigSourceLocal) a.logger.Printf("[INFO] agent: Service %q entered maintenance mode", serviceID) @@ -3659,6 +3661,7 @@ func (a *Agent) EnableNodeMaintenance(reason, token string) { Name: "Node Maintenance Mode", Notes: reason, Status: api.HealthCritical, + Type: "maintenance", } a.AddCheck(check, nil, true, token, ConfigSourceLocal) a.logger.Printf("[INFO] agent: Node entered maintenance mode") diff --git a/agent/agent_endpoint.go b/agent/agent_endpoint.go index 82fff12cd6e0..2bb6b9868351 100644 --- a/agent/agent_endpoint.go +++ b/agent/agent_endpoint.go @@ -487,6 +487,9 @@ func (s *HTTPServer) AgentRegisterCheck(resp http.ResponseWriter, req *http.Requ return nil, nil } + // Store the type of check based on the definition + health.Type = chkType.Type() + if health.ServiceID != "" { // fixup the service name so that vetCheckRegister requires the right ACLs service := s.agent.State.Service(health.ServiceID) diff --git a/agent/agent_endpoint_test.go b/agent/agent_endpoint_test.go index 1de613085e38..99f7dbcf89cb 100644 --- a/agent/agent_endpoint_test.go +++ b/agent/agent_endpoint_test.go @@ -2429,6 +2429,11 @@ func testAgent_RegisterService(t *testing.T, extraHCL string) { if len(checks) != 3 { t.Fatalf("bad: %v", checks) } + for _, c := range checks { + if c.Type != "ttl" { + t.Fatalf("expected ttl check type, got %s", c.Type) + } + } if len(a.checkTTLs) != 3 { t.Fatalf("missing test check ttls: %v", a.checkTTLs) @@ -4066,6 +4071,11 @@ func TestAgent_RegisterCheck_Service(t *testing.T) { if result["memcache_check2"].ServiceID != "memcache" { t.Fatalf("bad: %#v", result["memcached_check2"]) } + + // Make sure the new check has the right type + if result["memcache_check2"].Type != "ttl" { + t.Fatalf("expected TTL type, got %s", result["memcache_check2"].Type) + } } func TestAgent_Monitor(t *testing.T) { diff --git a/agent/health_endpoint_test.go b/agent/health_endpoint_test.go index 8304976e8332..a134ff126ae0 100644 --- a/agent/health_endpoint_test.go +++ b/agent/health_endpoint_test.go @@ -336,6 +336,7 @@ func TestHealthServiceChecks(t *testing.T) { Node: a.Config.NodeName, Name: "consul check", ServiceID: "consul", + Type: "grpc", }, } @@ -357,6 +358,9 @@ func TestHealthServiceChecks(t *testing.T) { if len(nodes) != 1 { t.Fatalf("bad: %v", obj) } + if nodes[0].Type != "grpc" { + t.Fatalf("expected grpc check type, got %s", nodes[0].Type) + } } func TestHealthServiceChecks_NodeMetaFilter(t *testing.T) { @@ -970,6 +974,59 @@ func TestHealthServiceNodes_PassingFilter(t *testing.T) { }) } +func TestHealthServiceNodes_CheckType(t *testing.T) { + t.Parallel() + a := NewTestAgent(t, t.Name(), "") + defer a.Shutdown() + testrpc.WaitForLeader(t, a.RPC, "dc1") + + req, _ := http.NewRequest("GET", "/v1/health/service/consul?dc=dc1", nil) + resp := httptest.NewRecorder() + obj, err := a.srv.HealthServiceNodes(resp, req) + require.NoError(t, err) + assertIndex(t, resp) + + // Should be 1 health check for consul + nodes := obj.(structs.CheckServiceNodes) + if len(nodes) != 1 { + t.Fatalf("expected 1 node, got %d", len(nodes)) + } + + args := &structs.RegisterRequest{ + Datacenter: "dc1", + Node: a.Config.NodeName, + Address: "127.0.0.1", + NodeMeta: map[string]string{"somekey": "somevalue"}, + Check: &structs.HealthCheck{ + Node: a.Config.NodeName, + Name: "consul check", + ServiceID: "consul", + Type: "grpc", + }, + } + + var out struct{} + require.NoError(t, a.RPC("Catalog.Register", args, &out)) + + req, _ = http.NewRequest("GET", "/v1/health/service/consul?dc=dc1", nil) + resp = httptest.NewRecorder() + obj, err = a.srv.HealthServiceNodes(resp, req) + require.NoError(t, err) + + assertIndex(t, resp) + + // Should be a non-nil empty list for checks + nodes = obj.(structs.CheckServiceNodes) + require.Len(t, nodes, 1) + require.Len(t, nodes[0].Checks, 2) + + for _, check := range nodes[0].Checks { + if check.Name == "consul check" && check.Type != "grpc" { + t.Fatalf("exptected grpc check type, got %s", check.Type) + } + } +} + func TestHealthServiceNodes_WanTranslation(t *testing.T) { t.Parallel() a1 := NewTestAgent(t, t.Name(), ` diff --git a/agent/structs/check_type.go b/agent/structs/check_type.go index a2282bdd11d7..5bfd1c3a6702 100644 --- a/agent/structs/check_type.go +++ b/agent/structs/check_type.go @@ -124,3 +124,24 @@ func (c *CheckType) IsDocker() bool { func (c *CheckType) IsGRPC() bool { return c.GRPC != "" && c.Interval > 0 } + +func (c *CheckType) Type() string { + switch { + case c.IsGRPC(): + return "grpc" + case c.IsHTTP(): + return "http" + case c.IsTTL(): + return "ttl" + case c.IsTCP(): + return "tcp" + case c.IsAlias(): + return "alias" + case c.IsDocker(): + return "docker" + case c.IsScript(): + return "script" + default: + return "" + } +} diff --git a/agent/structs/structs.go b/agent/structs/structs.go index 6aa61534da42..c5b3cbce23c7 100644 --- a/agent/structs/structs.go +++ b/agent/structs/structs.go @@ -1139,6 +1139,7 @@ type HealthCheck struct { ServiceID string // optional associated service ServiceName string // optional service name ServiceTags []string // optional service tags + Type string // Check type: http/ttl/tcp/etc Definition HealthCheckDefinition `bexpr:"-"` diff --git a/api/agent.go b/api/agent.go index 7a5c4272096a..daa06215a45d 100644 --- a/api/agent.go +++ b/api/agent.go @@ -52,6 +52,7 @@ type AgentCheck struct { Output string ServiceID string ServiceName string + Type string Definition HealthCheckDefinition } diff --git a/api/agent_test.go b/api/agent_test.go index 3b8e61f4a7b0..d0735c65b093 100644 --- a/api/agent_test.go +++ b/api/agent_test.go @@ -777,6 +777,9 @@ func TestAPI_AgentChecks(t *testing.T) { if chk.Status != HealthCritical { t.Fatalf("check not critical: %v", chk) } + if chk.Type != "ttl" { + t.Fatalf("expected type ttl, got %s", chk.Type) + } if err := agent.CheckDeregister("foo"); err != nil { t.Fatalf("err: %v", err) @@ -951,6 +954,9 @@ func TestAPI_AgentChecks_serviceBound(t *testing.T) { if check.ServiceID != "redis" { t.Fatalf("missing service association for check: %v", check) } + if check.Type != "ttl" { + t.Fatalf("expected type ttl, got %s", check.Type) + } } func TestAPI_AgentChecks_Docker(t *testing.T) { @@ -997,6 +1003,9 @@ func TestAPI_AgentChecks_Docker(t *testing.T) { if check.ServiceID != "redis" { t.Fatalf("missing service association for check: %v", check) } + if check.Type != "docker" { + t.Fatalf("expected type ttl, got %s", check.Type) + } } func TestAPI_AgentJoin(t *testing.T) { @@ -1145,6 +1154,9 @@ func TestAPI_ServiceMaintenance(t *testing.T) { if strings.Contains(check.CheckID, "maintenance") { t.Fatalf("should have removed health check") } + if check.Type != "maintenance" { + t.Fatalf("expected type 'maintenance', got %s", check.Type) + } } } @@ -1193,6 +1205,9 @@ func TestAPI_NodeMaintenance(t *testing.T) { if strings.Contains(check.CheckID, "maintenance") { t.Fatalf("should have removed health check") } + if check.Type != "maintenance" { + t.Fatalf("expected type 'maintenance', got %s", check.Type) + } } } diff --git a/api/health.go b/api/health.go index 9faf6b665aa8..5908882867a0 100644 --- a/api/health.go +++ b/api/health.go @@ -36,6 +36,7 @@ type HealthCheck struct { ServiceID string ServiceName string ServiceTags []string + Type string Definition HealthCheckDefinition diff --git a/api/health_test.go b/api/health_test.go index c953ed3636a8..40cfb1b3ed34 100644 --- a/api/health_test.go +++ b/api/health_test.go @@ -223,6 +223,7 @@ func TestAPI_HealthChecks(t *testing.T) { ServiceID: "foo", ServiceName: "foo", ServiceTags: []string{"bar"}, + Type: "ttl", }, } @@ -272,8 +273,11 @@ func TestAPI_HealthChecks_NodeMetaFilter(t *testing.T) { if meta.LastIndex == 0 { r.Fatalf("bad: %v", meta) } - if len(checks) == 0 { - r.Fatalf("Bad: %v", checks) + if len(checks) != 1 { + r.Fatalf("expected 1 check, got %d", len(checks)) + } + if checks[0].Type != "ttl" { + r.Fatalf("expected type ttl, got %s", checks[0].Type) } }) } @@ -356,6 +360,12 @@ func TestAPI_HealthService_SingleTag(t *testing.T) { require.NotEqual(r, meta.LastIndex, 0) require.Len(r, services, 1) require.Equal(r, services[0].Service.ID, "foo1") + + for _, check := range services[0].Checks { + if check.CheckID == "service:foo1" && check.Type != "ttl" { + r.Fatalf("expected type ttl, got %s", check.Type) + } + } }) } func TestAPI_HealthService_MultipleTags(t *testing.T) { From 8a0db6ce0c02891c18d264486292dd935694807a Mon Sep 17 00:00:00 2001 From: freddygv Date: Fri, 27 Sep 2019 17:48:33 -0600 Subject: [PATCH 2/6] Fix testAgent_AddService --- agent/agent_test.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/agent/agent_test.go b/agent/agent_test.go index 13916fb4eeb6..00eb9ea455e1 100644 --- a/agent/agent_test.go +++ b/agent/agent_test.go @@ -393,6 +393,7 @@ func testAgent_AddService(t *testing.T, extraHCL string) { ServiceID: "svcid1", ServiceName: "svcname1", ServiceTags: []string{"tag1"}, + Type: "ttl", }, }, }, @@ -438,6 +439,7 @@ func testAgent_AddService(t *testing.T, extraHCL string) { ServiceID: "svcid2", ServiceName: "svcname2", ServiceTags: []string{"tag2"}, + Type: "ttl", }, "check-noname": &structs.HealthCheck{ Node: "node1", @@ -447,6 +449,7 @@ func testAgent_AddService(t *testing.T, extraHCL string) { ServiceID: "svcid2", ServiceName: "svcname2", ServiceTags: []string{"tag2"}, + Type: "ttl", }, "service:svcid2:3": &structs.HealthCheck{ Node: "node1", @@ -456,6 +459,7 @@ func testAgent_AddService(t *testing.T, extraHCL string) { ServiceID: "svcid2", ServiceName: "svcname2", ServiceTags: []string{"tag2"}, + Type: "ttl", }, "service:svcid2:4": &structs.HealthCheck{ Node: "node1", @@ -465,6 +469,7 @@ func testAgent_AddService(t *testing.T, extraHCL string) { ServiceID: "svcid2", ServiceName: "svcname2", ServiceTags: []string{"tag2"}, + Type: "ttl", }, }, }, From c3e91c1df12775a55e6ea9e157b3331a15bd8d19 Mon Sep 17 00:00:00 2001 From: freddygv Date: Sun, 29 Sep 2019 18:59:42 -0600 Subject: [PATCH 3/6] More test fixes --- agent/agent_test.go | 19 +++++++++++++++++-- agent/structs/structs_filtering_test.go | 5 +++++ 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/agent/agent_test.go b/agent/agent_test.go index 00eb9ea455e1..1f7e2626ba4c 100644 --- a/agent/agent_test.go +++ b/agent/agent_test.go @@ -833,8 +833,23 @@ func testAgent_RemoveServiceRemovesAllChecks(t *testing.T, extraHCL string) { svc := &structs.NodeService{ID: "redis", Service: "redis", Port: 8000} chk1 := &structs.CheckType{CheckID: "chk1", Name: "chk1", TTL: time.Minute} chk2 := &structs.CheckType{CheckID: "chk2", Name: "chk2", TTL: 2 * time.Minute} - hchk1 := &structs.HealthCheck{Node: "node1", CheckID: "chk1", Name: "chk1", Status: "critical", ServiceID: "redis", ServiceName: "redis"} - hchk2 := &structs.HealthCheck{Node: "node1", CheckID: "chk2", Name: "chk2", Status: "critical", ServiceID: "redis", ServiceName: "redis"} + hchk1 := &structs.HealthCheck{ + Node: "node1", + CheckID: "chk1", + Name: "chk1", + Status: "critical", + ServiceID: "redis", + ServiceName: "redis", + Type: "ttl", + } + hchk2 := &structs.HealthCheck{Node: "node1", + CheckID: "chk2", + Name: "chk2", + Status: "critical", + ServiceID: "redis", + ServiceName: "redis", + Type: "ttl", + } // register service with chk1 if err := a.AddService(svc, []*structs.CheckType{chk1}, false, "", ConfigSourceLocal); err != nil { diff --git a/agent/structs/structs_filtering_test.go b/agent/structs/structs_filtering_test.go index ca20fad2b491..c4473524b7a1 100644 --- a/agent/structs/structs_filtering_test.go +++ b/agent/structs/structs_filtering_test.go @@ -462,6 +462,11 @@ var expectedFieldConfigHealthCheck bexpr.FieldConfigurations = bexpr.FieldConfig SupportedOperations: []bexpr.MatchOperator{bexpr.MatchIsEmpty, bexpr.MatchIsNotEmpty, bexpr.MatchIn, bexpr.MatchNotIn}, StructFieldName: "ServiceTags", }, + "Type": &bexpr.FieldConfiguration{ + CoerceFn: bexpr.CoerceString, + SupportedOperations: []bexpr.MatchOperator{bexpr.MatchEqual, bexpr.MatchNotEqual, bexpr.MatchIn, bexpr.MatchNotIn, bexpr.MatchMatches, bexpr.MatchNotMatches}, + StructFieldName: "Type", + }, } var expectedFieldConfigCheckServiceNode bexpr.FieldConfigurations = bexpr.FieldConfigurations{ From 277a8b7efb035e3c227b9b0a274b8e1873adc1a0 Mon Sep 17 00:00:00 2001 From: freddygv Date: Wed, 2 Oct 2019 13:03:22 -0600 Subject: [PATCH 4/6] Populate check.Type in Catalog.Register --- agent/catalog_endpoint_test.go | 53 ++++++++++++++++++++++++++++++++ agent/consul/catalog_endpoint.go | 7 +++++ agent/structs/structs.go | 26 ++++++++++++++++ 3 files changed, 86 insertions(+) diff --git a/agent/catalog_endpoint_test.go b/agent/catalog_endpoint_test.go index 98a7d0875c3f..afefcb309176 100644 --- a/agent/catalog_endpoint_test.go +++ b/agent/catalog_endpoint_test.go @@ -520,6 +520,59 @@ func TestCatalogServices_NodeMetaFilter(t *testing.T) { } } +func TestCatalogRegister_checkRegistration(t *testing.T) { + t.Parallel() + a := NewTestAgent(t, t.Name(), "") + defer a.Shutdown() + + // Register node with a service and check + check := structs.HealthCheck{ + Node: "foo", + CheckID: "foo-check", + Name: "foo check", + ServiceID: "api", + Definition: structs.HealthCheckDefinition{ + TCP: "localhost:8888", + Interval: 5 * time.Second, + }, + } + + args := &structs.RegisterRequest{ + Datacenter: "dc1", + Node: "foo", + Address: "127.0.0.1", + Service: &structs.NodeService{ + Service: "api", + }, + Check: &check, + } + + var out struct{} + if err := a.RPC("Catalog.Register", args, &out); err != nil { + t.Fatalf("err: %v", err) + } + + retry.Run(t, func(r *retry.R) { + req, _ := http.NewRequest("GET", "/v1/health/checks/api", nil) + resp := httptest.NewRecorder() + obj, err := a.srv.HealthServiceChecks(resp, req) + if err != nil { + r.Fatalf("err: %v", err) + } + + checks := obj.(structs.HealthChecks) + if len(checks) != 1 { + r.Fatalf("expected 1 check, got: %d", len(checks)) + } + if checks[0].CheckID != check.CheckID { + r.Fatalf("expected check id %s, got %s", check.Type, checks[0].Type) + } + if checks[0].Type != "tcp" { + r.Fatalf("expected check type tcp, got %s", checks[0].Type) + } + }) +} + func TestCatalogServiceNodes(t *testing.T) { t.Parallel() a := NewTestAgent(t, t.Name(), "") diff --git a/agent/consul/catalog_endpoint.go b/agent/consul/catalog_endpoint.go index c2d43263ba81..0828d0f28e36 100644 --- a/agent/consul/catalog_endpoint.go +++ b/agent/consul/catalog_endpoint.go @@ -125,6 +125,13 @@ func (c *Catalog) Register(args *structs.RegisterRequest, reply *struct{}) error check.Node = args.Node } checkPreApply(check) + + // Populate check type for cases when this is a direct reg with /catalog/register + // Typically this is populated when a check is registered with an agent + if check.Type == "" { + chkType := check.CheckType() + check.Type = chkType.Type() + } } // Check the complete register request against the given ACL policy. diff --git a/agent/structs/structs.go b/agent/structs/structs.go index c5b3cbce23c7..b303158cbeed 100644 --- a/agent/structs/structs.go +++ b/agent/structs/structs.go @@ -1255,6 +1255,32 @@ func (c *HealthCheck) Clone() *HealthCheck { return clone } +func (c *HealthCheck) CheckType() *CheckType { + return &CheckType{ + CheckID: c.CheckID, + Name: c.Name, + Status: c.Status, + Notes: c.Notes, + + ScriptArgs: c.Definition.ScriptArgs, + AliasNode: c.Definition.AliasNode, + AliasService: c.Definition.AliasService, + HTTP: c.Definition.HTTP, + GRPC: c.Definition.GRPC, + GRPCUseTLS: c.Definition.GRPCUseTLS, + Header: c.Definition.Header, + Method: c.Definition.Method, + TCP: c.Definition.TCP, + Interval: c.Definition.Interval, + DockerContainerID: c.Definition.DockerContainerID, + Shell: c.Definition.Shell, + TLSSkipVerify: c.Definition.TLSSkipVerify, + Timeout: c.Definition.Timeout, + TTL: c.Definition.TTL, + DeregisterCriticalServiceAfter: c.Definition.DeregisterCriticalServiceAfter, + } +} + // HealthChecks is a collection of HealthCheck structs. type HealthChecks []*HealthCheck From ec7a57f666fb8a90ed0aaf16779b453d124771b2 Mon Sep 17 00:00:00 2001 From: freddygv Date: Wed, 2 Oct 2019 15:59:50 -0600 Subject: [PATCH 5/6] Fix txn test --- api/txn_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/api/txn_test.go b/api/txn_test.go index b940546d1971..0d62b5460f14 100644 --- a/api/txn_test.go +++ b/api/txn_test.go @@ -194,6 +194,7 @@ func TestAPI_ClientTxn(t *testing.T) { DeregisterCriticalServiceAfter: ReadableDuration(20 * time.Second), DeregisterCriticalServiceAfterDuration: 20 * time.Second, }, + Type: "tcp", CreateIndex: ret.Results[4].Check.CreateIndex, ModifyIndex: ret.Results[4].Check.CreateIndex, }, @@ -212,6 +213,7 @@ func TestAPI_ClientTxn(t *testing.T) { DeregisterCriticalServiceAfter: ReadableDuration(160 * time.Second), DeregisterCriticalServiceAfterDuration: 160 * time.Second, }, + Type: "tcp", CreateIndex: ret.Results[4].Check.CreateIndex, ModifyIndex: ret.Results[4].Check.CreateIndex, }, From 496b44ba3c8683c94d7fe68b4f788d5edc0f880a Mon Sep 17 00:00:00 2001 From: freddygv Date: Wed, 9 Oct 2019 13:32:30 +0200 Subject: [PATCH 6/6] Update catalog comment --- agent/consul/catalog_endpoint.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/agent/consul/catalog_endpoint.go b/agent/consul/catalog_endpoint.go index 0828d0f28e36..35c2ea5b8b6f 100644 --- a/agent/consul/catalog_endpoint.go +++ b/agent/consul/catalog_endpoint.go @@ -126,8 +126,8 @@ func (c *Catalog) Register(args *structs.RegisterRequest, reply *struct{}) error } checkPreApply(check) - // Populate check type for cases when this is a direct reg with /catalog/register - // Typically this is populated when a check is registered with an agent + // Populate check type for cases when a check is registered in the catalog directly + // and not via anti-entropy if check.Type == "" { chkType := check.CheckType() check.Type = chkType.Type()