From 019c3fe2151138107fcb554ec2ecae4d28899842 Mon Sep 17 00:00:00 2001 From: Pablo Baeyens Date: Wed, 1 Dec 2021 12:56:13 +0100 Subject: [PATCH 1/6] [receiver/prometheusreceiver] Set host.name to 127.0.0.1 for 0.0.0.0 instances --- .../prometheusreceiver/internal/prom_to_otlp.go | 13 +++++++++++++ .../internal/prom_to_otlp_test.go | 7 +++++++ receiver/prometheusreceiver/internal/transaction.go | 1 + 3 files changed, 21 insertions(+) diff --git a/receiver/prometheusreceiver/internal/prom_to_otlp.go b/receiver/prometheusreceiver/internal/prom_to_otlp.go index 6cd9dfa8de7a..17df1c520f4d 100644 --- a/receiver/prometheusreceiver/internal/prom_to_otlp.go +++ b/receiver/prometheusreceiver/internal/prom_to_otlp.go @@ -21,11 +21,24 @@ import ( conventions "go.opentelemetry.io/collector/model/semconv/v1.5.0" ) +const ( + localHostIPv4 = "127.0.0.1" + anyIPIPv4 = "0.0.0.0" +) + +func sanitizeHost(host string) string { + if host == anyIPIPv4 { + return localHostIPv4 + } + return host +} + func createNodeAndResourcePdata(job, instance, scheme string) pdata.Resource { host, port, err := net.SplitHostPort(instance) if err != nil { host = instance } + host = sanitizeHost(host) resource := pdata.NewResource() attrs := resource.Attributes() attrs.UpsertString(conventions.AttributeServiceName, job) diff --git a/receiver/prometheusreceiver/internal/prom_to_otlp_test.go b/receiver/prometheusreceiver/internal/prom_to_otlp_test.go index 4038cde1302c..43c6640dc4f6 100644 --- a/receiver/prometheusreceiver/internal/prom_to_otlp_test.go +++ b/receiver/prometheusreceiver/internal/prom_to_otlp_test.go @@ -108,6 +108,13 @@ func TestCreateNodeAndResourcePromToOTLP(t *testing.T) { "job", "", "", "http", "", }), }, + { + name: "0.0.0.0 address", + job: "job", instance: "0.0.0.0:8888", scheme: "http", + want: makeResourceWithJobInstanceScheme(&jobInstanceDefinition{ + "job", "0.0.0.0:8888", "127.0.0.1", "http", "8888", + }), + }, } for _, tt := range tests { diff --git a/receiver/prometheusreceiver/internal/transaction.go b/receiver/prometheusreceiver/internal/transaction.go index 7d74457a0bb6..c2cfeee8640f 100644 --- a/receiver/prometheusreceiver/internal/transaction.go +++ b/receiver/prometheusreceiver/internal/transaction.go @@ -242,6 +242,7 @@ func createNodeAndResource(job, instance, scheme string) (*commonpb.Node, *resou if err != nil { host = instance } + host = sanitizeHost(host) node := &commonpb.Node{ ServiceInfo: &commonpb.ServiceInfo{Name: job}, Identifier: &commonpb.ProcessIdentifier{ From ecb320eb8cd776c74fe9bd0b543b37f44d2ebad0 Mon Sep 17 00:00:00 2001 From: Pablo Baeyens Date: Mon, 6 Dec 2021 12:44:21 +0100 Subject: [PATCH 2/6] [receiver/prometheusreceiver] Do not include localhost-like or unspecified `host.name`s --- .../internal/prom_to_otlp.go | 28 ++++++++++++------- .../internal/prom_to_otlp_test.go | 20 +++++++------ .../internal/transaction.go | 6 +++- 3 files changed, 34 insertions(+), 20 deletions(-) diff --git a/receiver/prometheusreceiver/internal/prom_to_otlp.go b/receiver/prometheusreceiver/internal/prom_to_otlp.go index 17df1c520f4d..a83758154753 100644 --- a/receiver/prometheusreceiver/internal/prom_to_otlp.go +++ b/receiver/prometheusreceiver/internal/prom_to_otlp.go @@ -21,16 +21,23 @@ import ( conventions "go.opentelemetry.io/collector/model/semconv/v1.5.0" ) -const ( - localHostIPv4 = "127.0.0.1" - anyIPIPv4 = "0.0.0.0" -) +// isAllowedHost returns if a host can be used as a value for the 'host.name' key. +// localhost-like hosts and unspecified (0.0.0.0) hosts are not allowed. +func isAllowedHost(host string) bool { + ip := net.ParseIP(host) + if ip != nil { + // An IP is allowed if + // - it's not local (e.g. belongs to 127.0.0.0/8 or ::1/128) and + // - it's not unspecficied (e.g. the 0.0.0.0 address). + return !ip.IsLoopback() && !ip.IsUnspecified() + } -func sanitizeHost(host string) string { - if host == anyIPIPv4 { - return localHostIPv4 + if host == "localhost" { + return false } - return host + + // not an IP, not 'localhost', assume it is allowed. + return true } func createNodeAndResourcePdata(job, instance, scheme string) pdata.Resource { @@ -38,11 +45,12 @@ func createNodeAndResourcePdata(job, instance, scheme string) pdata.Resource { if err != nil { host = instance } - host = sanitizeHost(host) resource := pdata.NewResource() attrs := resource.Attributes() attrs.UpsertString(conventions.AttributeServiceName, job) - attrs.UpsertString(conventions.AttributeHostName, host) + if isAllowedHost(host) { + attrs.UpsertString(conventions.AttributeHostName, host) + } attrs.UpsertString(jobAttr, job) attrs.UpsertString(instanceAttr, instance) attrs.UpsertString(portAttr, port) diff --git a/receiver/prometheusreceiver/internal/prom_to_otlp_test.go b/receiver/prometheusreceiver/internal/prom_to_otlp_test.go index 43c6640dc4f6..01b14dcb48b4 100644 --- a/receiver/prometheusreceiver/internal/prom_to_otlp_test.go +++ b/receiver/prometheusreceiver/internal/prom_to_otlp_test.go @@ -52,13 +52,15 @@ type jobInstanceDefinition struct { job, instance, host, scheme, port string } -func makeResourceWithJobInstanceScheme(def *jobInstanceDefinition) pdata.Resource { +func makeResourceWithJobInstanceScheme(def *jobInstanceDefinition, noHost bool) pdata.Resource { resource := pdata.NewResource() attrs := resource.Attributes() // Using hardcoded values to assert on outward expectations so that // when variables change, these tests will fail and we'll have reports. attrs.UpsertString("service.name", def.job) - attrs.UpsertString("host.name", def.host) + if !noHost { + attrs.UpsertString("host.name", def.host) + } attrs.UpsertString("job", def.job) attrs.UpsertString("instance", def.instance) attrs.UpsertString("port", def.port) @@ -78,42 +80,42 @@ func TestCreateNodeAndResourcePromToOTLP(t *testing.T) { job: "job", instance: "localhost:8888", scheme: "http", want: makeResourceWithJobInstanceScheme(&jobInstanceDefinition{ "job", "localhost:8888", "localhost", "http", "8888", - }), + }, false), }, { name: "missing port", job: "job", instance: "myinstance", scheme: "https", want: makeResourceWithJobInstanceScheme(&jobInstanceDefinition{ "job", "myinstance", "myinstance", "https", "", - }), + }, false), }, { name: "blank scheme", job: "job", instance: "myinstance:443", scheme: "", want: makeResourceWithJobInstanceScheme(&jobInstanceDefinition{ "job", "myinstance:443", "myinstance", "", "443", - }), + }, false), }, { name: "blank instance, blank scheme", job: "job", instance: "", scheme: "", want: makeResourceWithJobInstanceScheme(&jobInstanceDefinition{ "job", "", "", "", "", - }), + }, false), }, { name: "blank instance, non-blank scheme", job: "job", instance: "", scheme: "http", want: makeResourceWithJobInstanceScheme(&jobInstanceDefinition{ "job", "", "", "http", "", - }), + }, false), }, { name: "0.0.0.0 address", job: "job", instance: "0.0.0.0:8888", scheme: "http", want: makeResourceWithJobInstanceScheme(&jobInstanceDefinition{ - "job", "0.0.0.0:8888", "127.0.0.1", "http", "8888", - }), + "job", "0.0.0.0:8888", "", "http", "8888", + }, true), }, } diff --git a/receiver/prometheusreceiver/internal/transaction.go b/receiver/prometheusreceiver/internal/transaction.go index c2cfeee8640f..0855fc365bb0 100644 --- a/receiver/prometheusreceiver/internal/transaction.go +++ b/receiver/prometheusreceiver/internal/transaction.go @@ -242,7 +242,11 @@ func createNodeAndResource(job, instance, scheme string) (*commonpb.Node, *resou if err != nil { host = instance } - host = sanitizeHost(host) + + if !isAllowedHost(host) { + host = "" + } + node := &commonpb.Node{ ServiceInfo: &commonpb.ServiceInfo{Name: job}, Identifier: &commonpb.ProcessIdentifier{ From 0c4a905eeaaa6677b0c46488ebba6a6770472a64 Mon Sep 17 00:00:00 2001 From: Pablo Baeyens Date: Mon, 6 Dec 2021 13:01:36 +0100 Subject: [PATCH 3/6] Fix unit tests; add localhost unit test --- .../prometheusreceiver/internal/prom_to_otlp_test.go | 11 +++++++++-- receiver/prometheusreceiver/internal/transaction.go | 12 ++++++------ 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/receiver/prometheusreceiver/internal/prom_to_otlp_test.go b/receiver/prometheusreceiver/internal/prom_to_otlp_test.go index 01b14dcb48b4..159bfb0e680f 100644 --- a/receiver/prometheusreceiver/internal/prom_to_otlp_test.go +++ b/receiver/prometheusreceiver/internal/prom_to_otlp_test.go @@ -77,9 +77,9 @@ func TestCreateNodeAndResourcePromToOTLP(t *testing.T) { }{ { name: "all attributes proper", - job: "job", instance: "localhost:8888", scheme: "http", + job: "job", instance: "hostname:8888", scheme: "http", want: makeResourceWithJobInstanceScheme(&jobInstanceDefinition{ - "job", "localhost:8888", "localhost", "http", "8888", + "job", "hostname:8888", "hostname", "http", "8888", }, false), }, { @@ -117,6 +117,13 @@ func TestCreateNodeAndResourcePromToOTLP(t *testing.T) { "job", "0.0.0.0:8888", "", "http", "8888", }, true), }, + { + name: "localhost", + job: "job", instance: "localhost:8888", scheme: "http", + want: makeResourceWithJobInstanceScheme(&jobInstanceDefinition{ + "job", "localhost:8888", "", "http", "8888", + }, true), + }, } for _, tt := range tests { diff --git a/receiver/prometheusreceiver/internal/transaction.go b/receiver/prometheusreceiver/internal/transaction.go index 0855fc365bb0..96f771762a03 100644 --- a/receiver/prometheusreceiver/internal/transaction.go +++ b/receiver/prometheusreceiver/internal/transaction.go @@ -243,16 +243,16 @@ func createNodeAndResource(job, instance, scheme string) (*commonpb.Node, *resou host = instance } - if !isAllowedHost(host) { - host = "" - } - node := &commonpb.Node{ ServiceInfo: &commonpb.ServiceInfo{Name: job}, - Identifier: &commonpb.ProcessIdentifier{ + } + + if isAllowedHost(host) { + node.Identifier = &commonpb.ProcessIdentifier{ HostName: host, - }, + } } + resource := &resourcepb.Resource{ Labels: map[string]string{ jobAttr: job, From f4da92f0865a27ddd07130081b476e26169409ef Mon Sep 17 00:00:00 2001 From: Pablo Baeyens Date: Mon, 6 Dec 2021 13:13:42 +0100 Subject: [PATCH 4/6] Rename to `isDiscernibleHost` --- receiver/prometheusreceiver/internal/prom_to_otlp.go | 12 ++++++------ receiver/prometheusreceiver/internal/transaction.go | 2 +- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/receiver/prometheusreceiver/internal/prom_to_otlp.go b/receiver/prometheusreceiver/internal/prom_to_otlp.go index a83758154753..4ec8c802fa2f 100644 --- a/receiver/prometheusreceiver/internal/prom_to_otlp.go +++ b/receiver/prometheusreceiver/internal/prom_to_otlp.go @@ -21,12 +21,12 @@ import ( conventions "go.opentelemetry.io/collector/model/semconv/v1.5.0" ) -// isAllowedHost returns if a host can be used as a value for the 'host.name' key. -// localhost-like hosts and unspecified (0.0.0.0) hosts are not allowed. -func isAllowedHost(host string) bool { +// isDiscernibleHost checks if a host can be used as a value for the 'host.name' key. +// localhost-like hosts and unspecified (0.0.0.0) hosts are not discernible. +func isDiscernibleHost(host string) bool { ip := net.ParseIP(host) if ip != nil { - // An IP is allowed if + // An IP is discernible if // - it's not local (e.g. belongs to 127.0.0.0/8 or ::1/128) and // - it's not unspecficied (e.g. the 0.0.0.0 address). return !ip.IsLoopback() && !ip.IsUnspecified() @@ -36,7 +36,7 @@ func isAllowedHost(host string) bool { return false } - // not an IP, not 'localhost', assume it is allowed. + // not an IP, not 'localhost', assume it is discernible. return true } @@ -48,7 +48,7 @@ func createNodeAndResourcePdata(job, instance, scheme string) pdata.Resource { resource := pdata.NewResource() attrs := resource.Attributes() attrs.UpsertString(conventions.AttributeServiceName, job) - if isAllowedHost(host) { + if isDiscernibleHost(host) { attrs.UpsertString(conventions.AttributeHostName, host) } attrs.UpsertString(jobAttr, job) diff --git a/receiver/prometheusreceiver/internal/transaction.go b/receiver/prometheusreceiver/internal/transaction.go index 96f771762a03..574125c98be9 100644 --- a/receiver/prometheusreceiver/internal/transaction.go +++ b/receiver/prometheusreceiver/internal/transaction.go @@ -247,7 +247,7 @@ func createNodeAndResource(job, instance, scheme string) (*commonpb.Node, *resou ServiceInfo: &commonpb.ServiceInfo{Name: job}, } - if isAllowedHost(host) { + if isDiscernibleHost(host) { node.Identifier = &commonpb.ProcessIdentifier{ HostName: host, } From 832eeab896e28f2ab25ceb7b3b6443db9a9a83dc Mon Sep 17 00:00:00 2001 From: Pablo Baeyens Date: Mon, 6 Dec 2021 16:02:20 +0100 Subject: [PATCH 5/6] Fix unit test panicking Unit test was expecting `host.name` attribute and panicking after not finding it. --- .../internal/prom_to_otlp.go | 3 ++- .../internal/prom_to_otlp_test.go | 4 ++-- .../metrics_receiver_helper_test.go | 19 ++++++++----------- 3 files changed, 12 insertions(+), 14 deletions(-) diff --git a/receiver/prometheusreceiver/internal/prom_to_otlp.go b/receiver/prometheusreceiver/internal/prom_to_otlp.go index 4ec8c802fa2f..50fb03a1e568 100644 --- a/receiver/prometheusreceiver/internal/prom_to_otlp.go +++ b/receiver/prometheusreceiver/internal/prom_to_otlp.go @@ -40,7 +40,8 @@ func isDiscernibleHost(host string) bool { return true } -func createNodeAndResourcePdata(job, instance, scheme string) pdata.Resource { +// CreateNodeAndResourcePdata creates the resource data added to OTLP payloads. +func CreateNodeAndResourcePdata(job, instance, scheme string) pdata.Resource { host, port, err := net.SplitHostPort(instance) if err != nil { host = instance diff --git a/receiver/prometheusreceiver/internal/prom_to_otlp_test.go b/receiver/prometheusreceiver/internal/prom_to_otlp_test.go index 159bfb0e680f..847e01f3242d 100644 --- a/receiver/prometheusreceiver/internal/prom_to_otlp_test.go +++ b/receiver/prometheusreceiver/internal/prom_to_otlp_test.go @@ -43,7 +43,7 @@ func TestCreateNodeAndResourceEquivalence(t *testing.T) { ) fromOCResource := mdFromOC.ResourceMetrics().At(0).Resource().Attributes().Sort() - byDirectOTLPResource := createNodeAndResourcePdata(job, instance, scheme).Attributes().Sort() + byDirectOTLPResource := CreateNodeAndResourcePdata(job, instance, scheme).Attributes().Sort() require.Equal(t, byDirectOTLPResource, fromOCResource) } @@ -129,7 +129,7 @@ func TestCreateNodeAndResourcePromToOTLP(t *testing.T) { for _, tt := range tests { tt := tt t.Run(tt.name, func(t *testing.T) { - got := createNodeAndResourcePdata(tt.job, tt.instance, tt.scheme) + got := CreateNodeAndResourcePdata(tt.job, tt.instance, tt.scheme) require.Equal(t, got, tt.want) }) } diff --git a/receiver/prometheusreceiver/metrics_receiver_helper_test.go b/receiver/prometheusreceiver/metrics_receiver_helper_test.go index 8bdd334fe026..1b0c2ba5481d 100644 --- a/receiver/prometheusreceiver/metrics_receiver_helper_test.go +++ b/receiver/prometheusreceiver/metrics_receiver_helper_test.go @@ -18,7 +18,6 @@ import ( "context" "fmt" "log" - "net" "net/http" "net/http/httptest" "net/url" @@ -36,6 +35,8 @@ import ( "go.opentelemetry.io/collector/consumer/consumertest" "go.opentelemetry.io/collector/model/pdata" "gopkg.in/yaml.v2" + + "github.com/open-telemetry/opentelemetry-collector-contrib/receiver/prometheusreceiver/internal" ) type mockPrometheusResponse struct { @@ -129,7 +130,6 @@ func setupMockPrometheus(openMetricsContentType bool, tds ...*testData) (*mockPr } mp := newMockPrometheus(endpoints, openMetricsContentType) u, _ := url.Parse(mp.srv.URL) - host, port, _ := net.SplitHostPort(u.Host) for i := 0; i < len(tds); i++ { job := make(map[string]interface{}) job["job_name"] = tds[i].name @@ -149,13 +149,7 @@ func setupMockPrometheus(openMetricsContentType bool, tds ...*testData) (*mockPr } // update attributes value (will use for validation) for _, t := range tds { - t.attributes = pdata.NewAttributeMap() - t.attributes.Insert("service.name", pdata.NewAttributeValueString(t.name)) - t.attributes.Insert("host.name", pdata.NewAttributeValueString(host)) - t.attributes.Insert("job", pdata.NewAttributeValueString(t.name)) - t.attributes.Insert("instance", pdata.NewAttributeValueString(u.Host)) - t.attributes.Insert("port", pdata.NewAttributeValueString(port)) - t.attributes.Insert("scheme", pdata.NewAttributeValueString("http")) + t.attributes = internal.CreateNodeAndResourcePdata(t.name, u.Host, "http").Attributes() } pCfg, err := promcfg.Load(string(cfg), false, gokitlog.NewNopLogger()) return mp, pCfg, err @@ -285,8 +279,11 @@ func doCompare(t *testing.T, name string, want pdata.AttributeMap, got *pdata.Re assert.Equal(t, expectedScrapeMetricCount, countScrapeMetricsRM(got)) assert.Equal(t, want.Len(), got.Resource().Attributes().Len()) for k, v := range want.AsRaw() { - value, _ := got.Resource().Attributes().Get(k) - assert.EqualValues(t, v, value.AsString()) + value, ok := got.Resource().Attributes().Get(k) + assert.True(t, ok, "%q attribute is missing", k) + if ok { + assert.EqualValues(t, v, value.AsString()) + } } for _, e := range expectations { e(t, got) From d2c73c12f55948fac67df55edde71ab893f860b0 Mon Sep 17 00:00:00 2001 From: Pablo Baeyens Date: Mon, 6 Dec 2021 16:09:46 +0100 Subject: [PATCH 6/6] Address review comments - Fix typo - Avoid double negative --- .../internal/prom_to_otlp.go | 2 +- .../internal/prom_to_otlp_test.go | 18 +++++++++--------- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/receiver/prometheusreceiver/internal/prom_to_otlp.go b/receiver/prometheusreceiver/internal/prom_to_otlp.go index 50fb03a1e568..18852947aa7d 100644 --- a/receiver/prometheusreceiver/internal/prom_to_otlp.go +++ b/receiver/prometheusreceiver/internal/prom_to_otlp.go @@ -28,7 +28,7 @@ func isDiscernibleHost(host string) bool { if ip != nil { // An IP is discernible if // - it's not local (e.g. belongs to 127.0.0.0/8 or ::1/128) and - // - it's not unspecficied (e.g. the 0.0.0.0 address). + // - it's not unspecified (e.g. the 0.0.0.0 address). return !ip.IsLoopback() && !ip.IsUnspecified() } diff --git a/receiver/prometheusreceiver/internal/prom_to_otlp_test.go b/receiver/prometheusreceiver/internal/prom_to_otlp_test.go index 847e01f3242d..a0ba9adf4c9d 100644 --- a/receiver/prometheusreceiver/internal/prom_to_otlp_test.go +++ b/receiver/prometheusreceiver/internal/prom_to_otlp_test.go @@ -52,13 +52,13 @@ type jobInstanceDefinition struct { job, instance, host, scheme, port string } -func makeResourceWithJobInstanceScheme(def *jobInstanceDefinition, noHost bool) pdata.Resource { +func makeResourceWithJobInstanceScheme(def *jobInstanceDefinition, hasHost bool) pdata.Resource { resource := pdata.NewResource() attrs := resource.Attributes() // Using hardcoded values to assert on outward expectations so that // when variables change, these tests will fail and we'll have reports. attrs.UpsertString("service.name", def.job) - if !noHost { + if hasHost { attrs.UpsertString("host.name", def.host) } attrs.UpsertString("job", def.job) @@ -80,49 +80,49 @@ func TestCreateNodeAndResourcePromToOTLP(t *testing.T) { job: "job", instance: "hostname:8888", scheme: "http", want: makeResourceWithJobInstanceScheme(&jobInstanceDefinition{ "job", "hostname:8888", "hostname", "http", "8888", - }, false), + }, true), }, { name: "missing port", job: "job", instance: "myinstance", scheme: "https", want: makeResourceWithJobInstanceScheme(&jobInstanceDefinition{ "job", "myinstance", "myinstance", "https", "", - }, false), + }, true), }, { name: "blank scheme", job: "job", instance: "myinstance:443", scheme: "", want: makeResourceWithJobInstanceScheme(&jobInstanceDefinition{ "job", "myinstance:443", "myinstance", "", "443", - }, false), + }, true), }, { name: "blank instance, blank scheme", job: "job", instance: "", scheme: "", want: makeResourceWithJobInstanceScheme(&jobInstanceDefinition{ "job", "", "", "", "", - }, false), + }, true), }, { name: "blank instance, non-blank scheme", job: "job", instance: "", scheme: "http", want: makeResourceWithJobInstanceScheme(&jobInstanceDefinition{ "job", "", "", "http", "", - }, false), + }, true), }, { name: "0.0.0.0 address", job: "job", instance: "0.0.0.0:8888", scheme: "http", want: makeResourceWithJobInstanceScheme(&jobInstanceDefinition{ "job", "0.0.0.0:8888", "", "http", "8888", - }, true), + }, false), }, { name: "localhost", job: "job", instance: "localhost:8888", scheme: "http", want: makeResourceWithJobInstanceScheme(&jobInstanceDefinition{ "job", "localhost:8888", "", "http", "8888", - }, true), + }, false), }, }