Repository: kubernetes/ingress-nginx
Pull request #6884 (2021-02-18 22:38:55)

jaeger-endpoint feature for non-agent trace collectors
<!--- Provide a general summary of your changes in the Title above --->
<!--- Please don't @-mention people in PR or commit messages (do so in an additional comment). --->
https://github.com/jaegertracing/jaeger-client-cpp supports both the compact jaeger.thrift format over UDP (meant for local tracing agents) as well as the binary jaeger.thrift format over HTTP, meant for collector deployments. This PR adds support for providing a collector endpoint when no tracing agents are deployed. I'll rebase and update #6872 accordingly (this should probably be merged first, but it doesn't really matter as long as the second one is updated).

## What this PR does / why we need it:
<!--- Why is this change required? What problem does it solve? -->
<!--- If it fixes an open issue, please link to the issue here. -->
Some cluster deployments may not have tracing agents on every host. If your infrastructure only has collectors listening over TCP, ingress-nginx cannot currently export compatible traces. This changes enables TCP endpoint support. As mentioned in the documentation change, when both are provided, this endpoint takes priority (that decision [is in the jaeger client](https://github.com/jaegertracing/jaeger-client-cpp/blob/5748442005c26320e8d793c2f984ba0594255af3/src/jaegertracing/reporters/Config.cpp#L40-L62), not in ingress-nginx).

## Types of changes
<!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: -->
- [ ] Bug fix (non-breaking change which fixes an issue)
- [x] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to change)

## Which issue/s this PR fixes
<!--
(optional, in `fixes #<issue number>` format, will close that issue when PR gets merged):

fixes #
-->

## How Has This Been Tested?
<!--- Please describe in detail how you tested your changes. -->
<!--- Include details of your testing environment, and the tests you ran to -->
<!--- see how your change affects other areas of the code, etc. -->
I have added e2e and unit test cases to show that tracing is enabled as expected. I also ran this code locally and verified that /etc/nginx/opentracing.json was updated accordingly (and that the collector I have running received traces over TCP as expected).

## Checklist:
<!--- Go over all the following points, and put an `x` in all the boxes that apply. -->
<!--- If you're unsure about any of these, don't hesitate to ask. We're here to help! -->
- [x] My change requires a change to the documentation.
- [x] I have updated the documentation accordingly.
- [x] I've read the [CONTRIBUTION](https://github.com/kubernetes/ingress-nginx/blob/master/CONTRIBUTING.md) guide
- [x] I have added tests to cover my changes.
- [ ] All new and existing tests passed.



Patch:
diff --git a/docs/user-guide/nginx-configuration/configmap.md b/docs/user-guide/nginx-configuration/configmap.md
index 0bead1c34..0af29d574 100755
--- a/docs/user-guide/nginx-configuration/configmap.md
+++ b/docs/user-guide/nginx-configuration/configmap.md
@@ -132,6 +132,7 @@ The following table shows a configuration option's name, type, and the default v
 |[zipkin-sample-rate](#zipkin-sample-rate)|float|1.0|
 |[jaeger-collector-host](#jaeger-collector-host)|string|""|
 |[jaeger-collector-port](#jaeger-collector-port)|int|6831|
+|[jaeger-endpoint](#jaeger-endpoint)|string|""|
 |[jaeger-service-name](#jaeger-service-name)|string|"nginx"|
 |[jaeger-sampler-type](#jaeger-sampler-type)|string|"const"|
 |[jaeger-sampler-param](#jaeger-sampler-param)|string|"1"|
@@ -845,6 +846,10 @@ Specifies the host to use when uploading traces. It must be a valid URL.
 
 Specifies the port to use when uploading traces. _**default:**_ 6831
 
+## jaeger-endpoint
+
+Specifies the endpoint to use when uploading traces to a collector. This takes priority over `jaeger-collector-host` if both are specified.
+
 ## jaeger-service-name
 
 Specifies the service name to use for any traces created. _**default:**_ nginx
diff --git a/docs/user-guide/third-party-addons/opentracing.md b/docs/user-guide/third-party-addons/opentracing.md
index 5fb08f432..1f4138250 100644
--- a/docs/user-guide/third-party-addons/opentracing.md
+++ b/docs/user-guide/third-party-addons/opentracing.md
@@ -30,6 +30,7 @@ jaeger-collector-host: jaeger-agent.default.svc.cluster.local
 datadog-collector-host: datadog-agent.default.svc.cluster.local
 ```
 NOTE: While the option is called `jaeger-collector-host`, you will need to point this to a `jaeger-agent`, and not the `jaeger-collector` component.
+Alternatively, you can set `jaeger-endpoint` and specify the full endpoint for uploading traces. This will use TCP and should be used for a collector rather than an agent.
 
 Next you will need to deploy a distributed tracing system which uses OpenTracing.
 [Zipkin](https://github.com/openzipkin/zipkin) and
@@ -57,6 +58,9 @@ zipkin-sample-rate
 # specifies the port to use when uploading traces, Default: 6831
 jaeger-collector-port
 
+# specifies the endpoint to use when uploading traces to a collector instead of an agent
+jaeger-endpoint
+
 # specifies the service name to use for any traces created, Default: nginx
 jaeger-service-name
 
diff --git a/internal/ingress/controller/config/config.go b/internal/ingress/controller/config/config.go
index 63bd65aac..35691cb55 100644
--- a/internal/ingress/controller/config/config.go
+++ b/internal/ingress/controller/config/config.go
@@ -555,6 +555,9 @@ type Configuration struct {
 	// Default: 6831
 	JaegerCollectorPort int `json:"jaeger-collector-port"`
 
+	// JaegerEndpoint specifies the enpoint to use when uploading traces to a collector over TCP
+	JaegerEndpoint string `json:"jaeger-endpoint"`
+
 	// JaegerServiceName specifies the service name to use for any traces created
 	// Default: nginx
 	JaegerServiceName string `json:"jaeger-service-name"`
diff --git a/internal/ingress/controller/nginx.go b/internal/ingress/controller/nginx.go
index baf0ff45e..cef285350 100644
--- a/internal/ingress/controller/nginx.go
+++ b/internal/ingress/controller/nginx.go
@@ -1039,6 +1039,7 @@ const jaegerTmpl = `{
 	"samplingServerURL": "{{ .JaegerSamplerHost }}:{{ .JaegerSamplerPort }}/sampling"
   },
   "reporter": {
+	"endpoint": "{{ .JaegerEndpoint }}",
 	"localAgentHostPort": "{{ .JaegerCollectorHost }}:{{ .JaegerCollectorPort }}"
   },
   "headers": {
@@ -1068,7 +1069,7 @@ func createOpentracingCfg(cfg ngx_config.Configuration) error {
 		if err != nil {
 			return err
 		}
-	} else if cfg.JaegerCollectorHost != "" {
+	} else if cfg.JaegerCollectorHost != "" || cfg.JaegerEndpoint != "" {
 		tmpl, err = template.New("jaeger").Parse(jaegerTmpl)
 		if err != nil {
 			return err
diff --git a/internal/ingress/controller/template/template.go b/internal/ingress/controller/template/template.go
index 014bdfdb9..6cdc1d7b2 100644
--- a/internal/ingress/controller/template/template.go
+++ b/internal/ingress/controller/template/template.go
@@ -1018,7 +1018,7 @@ func buildOpentracing(c interface{}, s interface{}) string {
 		buf.WriteString("opentracing_load_tracer /usr/local/lib64/libdd_opentracing.so /etc/nginx/opentracing.json;")
 	} else if cfg.ZipkinCollectorHost != "" {
 		buf.WriteString("opentracing_load_tracer /usr/local/lib/libzipkin_opentracing_plugin.so /etc/nginx/opentracing.json;")
-	} else if cfg.JaegerCollectorHost != "" {
+	} else if cfg.JaegerCollectorHost != "" || cfg.JaegerEndpoint != "" {
 		buf.WriteString("opentracing_load_tracer /usr/local/lib/libjaegertracing_plugin.so /etc/nginx/opentracing.json;")
 	}
 


Test patch:
diff --git a/internal/ingress/controller/template/template_test.go b/internal/ingress/controller/template/template_test.go
index abccb0d9c..1af988197 100644
--- a/internal/ingress/controller/template/template_test.go
+++ b/internal/ingress/controller/template/template_test.go
@@ -1163,6 +1163,16 @@ func TestBuildOpenTracing(t *testing.T) {
 		t.Errorf("Expected '%v' but returned '%v'", expected, actual)
 	}
 
+	cfgNoHost := config.Configuration{
+		EnableOpentracing: true,
+	}
+	expected = "\r\n"
+	actual = buildOpentracing(cfgNoHost, []*ingress.Server{})
+
+	if expected != actual {
+		t.Errorf("Expected '%v' but returned '%v'", expected, actual)
+	}
+
 	cfgJaeger := config.Configuration{
 		EnableOpentracing:   true,
 		JaegerCollectorHost: "jaeger-host.com",
@@ -1196,6 +1206,17 @@ func TestBuildOpenTracing(t *testing.T) {
 		t.Errorf("Expected '%v' but returned '%v'", expected, actual)
 	}
 
+	cfgJaegerEndpoint := config.Configuration{
+		EnableOpentracing: true,
+		JaegerEndpoint:    "http://jaeger-collector.com:14268/api/traces",
+	}
+	expected = "opentracing_load_tracer /usr/local/lib/libjaegertracing_plugin.so /etc/nginx/opentracing.json;\r\n"
+	actual = buildOpentracing(cfgJaegerEndpoint, []*ingress.Server{})
+
+	if expected != actual {
+		t.Errorf("Expected '%v' but returned '%v'", expected, actual)
+	}
+
 	cfgOpenTracing := config.Configuration{
 		EnableOpentracing:                true,
 		DatadogCollectorHost:             "datadog-host.com",
diff --git a/test/e2e/settings/opentracing.go b/test/e2e/settings/opentracing.go
index 923010294..498bc7a40 100644
--- a/test/e2e/settings/opentracing.go
+++ b/test/e2e/settings/opentracing.go
@@ -33,6 +33,7 @@ const (
 
 	jaegerCollectorHost = "jaeger-collector-host"
 	jaegerSamplerHost   = "jaeger-sampler-host"
+	// jaegerEndpoint      = "jaeger-endpoint"
 
 	datadogCollectorHost = "datadog-collector-host"
 
@@ -174,6 +175,20 @@ var _ = framework.IngressNginxDescribe("Configure OpenTracing", func() {
 		assert.NotContains(ginkgo.GinkgoT(), log, "Unexpected failure reloading the backend", "reloading nginx after a configmap change")
 	})
 
+	/*
+		ginkgo.It("should enable opentracing using jaeger with an HTTP endpoint", func() {
+			config := map[string]string{}
+			config[enableOpentracing] = "true"
+			config[jaegerEndpoint] = "http://127.0.0.1/api/traces"
+			f.SetNginxConfigMapData(config)
+
+			framework.Sleep(10 * time.Second)
+			log, err := f.NginxLogs()
+			assert.Nil(ginkgo.GinkgoT(), err, "obtaining nginx logs")
+			assert.NotContains(ginkgo.GinkgoT(), log, "Unexpected failure reloading the backend", "reloading nginx after a configmap change")
+		})
+	*/
+
 	ginkgo.It("should enable opentracing using datadog", func() {
 		config := map[string]string{}
 		config[enableOpentracing] = "true"