Fix enable opentracing per location (#4983)

internal/ingress/annotations/opentracing/main.go

@@ -30,10 +30,15 @@ type opentracing struct {
 // Config contains the configuration to be used in the Ingress
 type Config struct {
 	Enabled bool `json:"enabled"`
+	Set     bool `json:"set"`
 }
 
 // Equal tests for equality between two Config types
 func (bd1 *Config) Equal(bd2 *Config) bool {
+	if bd1.Set != bd2.Set {
+		return false
+	}
+
 	if bd1.Enabled != bd2.Enabled {
 		return false
 	}
@@ -49,8 +54,8 @@ func NewParser(r resolver.Resolver) parser.IngressAnnotation {
 func (s opentracing) Parse(ing *networking.Ingress) (interface{}, error) {
 	enabled, err := parser.GetBoolAnnotation("enable-opentracing", ing)
 	if err != nil {
-		return &Config{Enabled: false}, nil
+		return &Config{Set: false, Enabled: false}, nil
 	}
 
-	return &Config{Enabled: enabled}, nil
+	return &Config{Set: true, Enabled: enabled}, nil
 }

internal/ingress/controller/nginx.go

@@ -662,11 +662,9 @@ func (n *NGINXController) OnUpdate(ingressCfg ingress.Configuration) error {
 		return err
 	}
 
-	if cfg.EnableOpentracing {
-		err := createOpentracingCfg(cfg)
-		if err != nil {
-			return err
-		}
+	err = createOpentracingCfg(cfg)
+	if err != nil {
+		return err
 	}
 
 	err = n.testTemplate(content)

internal/ingress/controller/template/template.go

@@ -178,6 +178,7 @@ var (
 		"buildHTTPListener":                  buildHTTPListener,
 		"buildHTTPSListener":                 buildHTTPSListener,
 		"buildOpentracingForLocation":        buildOpentracingForLocation,
+		"shouldLoadOpentracingModule":        shouldLoadOpentracingModule,
 	}
 )
 
@@ -928,14 +929,20 @@ func randomString() string {
 	return string(b)
 }
 
-func buildOpentracing(input interface{}) string {
-	cfg, ok := input.(config.Configuration)
+func buildOpentracing(c interface{}, s interface{}) string {
+	cfg, ok := c.(config.Configuration)
 	if !ok {
-		klog.Errorf("expected a 'config.Configuration' type but %T was returned", input)
+		klog.Errorf("expected a 'config.Configuration' type but %T was returned", c)
 		return ""
 	}
 
-	if !cfg.EnableOpentracing {
+	servers, ok := s.([]*ingress.Server)
+	if !ok {
+		klog.Errorf("expected an '[]*ingress.Server' type but %T was returned", s)
+		return ""
+	}
+
+	if !shouldLoadOpentracingModule(cfg, servers) {
 		return ""
 	}
 
@@ -1272,18 +1279,60 @@ func httpsListener(addresses []string, co string, tc config.TemplateConfig) []st
 
 func buildOpentracingForLocation(isOTEnabled bool, location *ingress.Location) string {
 	isOTEnabledInLoc := location.Opentracing.Enabled
+	isOTSetInLoc := location.Opentracing.Set
 
 	if isOTEnabled {
-		if !isOTEnabledInLoc {
+		if isOTSetInLoc && !isOTEnabledInLoc {
 			return "opentracing off;"
 		}
 
-		return opentracingPropagateContext(location)
+		opc := opentracingPropagateContext(location)
+		if opc != "" {
+			opc = fmt.Sprintf("opentracing on;\n%v", opc)
+		}
+
+		return opc
 	}
 
-	if isOTEnabledInLoc {
-		return opentracingPropagateContext(location)
+	if isOTSetInLoc && isOTEnabledInLoc {
+		opc := opentracingPropagateContext(location)
+		if opc != "" {
+			opc = fmt.Sprintf("opentracing on;\n%v", opc)
+		}
+
+		return opc
 	}
 
 	return ""
 }
+
+// shouldLoadOpentracingModule determines whether or not the Opentracing module needs to be loaded.
+// First, it checks if `enable-opentracing` is set in the ConfigMap. If it is not, it iterates over all locations to
+// check if Opentracing is enabled by the annotation `nginx.ingress.kubernetes.io/enable-opentracing`.
+func shouldLoadOpentracingModule(c interface{}, s interface{}) bool {
+	cfg, ok := c.(config.Configuration)
+	if !ok {
+		klog.Errorf("expected a 'config.Configuration' type but %T was returned", c)
+		return false
+	}
+
+	servers, ok := s.([]*ingress.Server)
+	if !ok {
+		klog.Errorf("expected an '[]*ingress.Server' type but %T was returned", s)
+		return false
+	}
+
+	if cfg.EnableOpentracing {
+		return true
+	}
+
+	for _, server := range servers {
+		for _, location := range server.Locations {
+			if location.Opentracing.Enabled {
+				return true
+			}
+		}
+	}
+
+	return false
+}

internal/ingress/controller/template/template_test.go

@@ -1153,7 +1153,7 @@ func TestBuildInfluxDB(t *testing.T) {
 func TestBuildOpenTracing(t *testing.T) {
 	invalidType := &ingress.Ingress{}
 	expected := ""
-	actual := buildOpentracing(invalidType)
+	actual := buildOpentracing(invalidType, []*ingress.Server{})
 
 	if expected != actual {
 		t.Errorf("Expected '%v' but returned '%v'", expected, actual)
@@ -1164,7 +1164,7 @@ func TestBuildOpenTracing(t *testing.T) {
 		JaegerCollectorHost: "jaeger-host.com",
 	}
 	expected = "opentracing_load_tracer /usr/local/lib/libjaegertracing_plugin.so /etc/nginx/opentracing.json;\r\n"
-	actual = buildOpentracing(cfgJaeger)
+	actual = buildOpentracing(cfgJaeger, []*ingress.Server{})
 
 	if expected != actual {
 		t.Errorf("Expected '%v' but returned '%v'", expected, actual)
@@ -1175,7 +1175,7 @@ func TestBuildOpenTracing(t *testing.T) {
 		ZipkinCollectorHost: "zipkin-host.com",
 	}
 	expected = "opentracing_load_tracer /usr/local/lib/libzipkin_opentracing.so /etc/nginx/opentracing.json;\r\n"
-	actual = buildOpentracing(cfgZipkin)
+	actual = buildOpentracing(cfgZipkin, []*ingress.Server{})
 
 	if expected != actual {
 		t.Errorf("Expected '%v' but returned '%v'", expected, actual)
@@ -1186,7 +1186,7 @@ func TestBuildOpenTracing(t *testing.T) {
 		DatadogCollectorHost: "datadog-host.com",
 	}
 	expected = "opentracing_load_tracer /usr/local/lib64/libdd_opentracing.so /etc/nginx/opentracing.json;\r\n"
-	actual = buildOpentracing(cfgDatadog)
+	actual = buildOpentracing(cfgDatadog, []*ingress.Server{})
 
 	if expected != actual {
 		t.Errorf("Expected '%v' but returned '%v'", expected, actual)
@@ -1283,25 +1283,89 @@ func TestShouldLoadModSecurityModule(t *testing.T) {
 }
 
 func TestOpentracingForLocation(t *testing.T) {
+	trueVal := true
+
+	loadOT := `opentracing on;
+opentracing_propagate_context;`
 	testCases := []struct {
 		description string
 		globalOT    bool
-		isOTInLoc   bool
+		isSetInLoc  bool
+		isOTInLoc   *bool
 		expected    string
 	}{
-		{"globally enabled but not in location", true, false, "opentracing off;"},
-		{"globally enabled and enabled in location", true, true, "opentracing_propagate_context;"},
-		{"globally disabled and not enabled in location", false, false, ""},
-		{"globally disabled but enabled in location", false, true, "opentracing_propagate_context;"},
+		{"globally enabled, without annotation", true, false, nil, loadOT},
+		{"globally enabled and enabled in location", true, true, &trueVal, loadOT},
+		{"globally disabled and not enabled in location", false, false, nil, ""},
+		{"globally disabled but enabled in location", false, true, &trueVal, loadOT},
+		{"globally disabled, enabled in location but false", false, true, &trueVal, loadOT},
 	}
 
 	for _, testCase := range testCases {
-		actual := buildOpentracingForLocation(testCase.globalOT, &ingress.Location{
-			Opentracing: opentracing.Config{Enabled: testCase.isOTInLoc},
-		})
+		il := &ingress.Location{
+			Opentracing: opentracing.Config{Set: testCase.isSetInLoc},
+		}
+		if il.Opentracing.Set {
+			il.Opentracing.Enabled = *testCase.isOTInLoc
+		}
+
+		actual := buildOpentracingForLocation(testCase.globalOT, il)
 
 		if testCase.expected != actual {
 			t.Errorf("%v: expected '%v' but returned '%v'", testCase.description, testCase.expected, actual)
 		}
 	}
 }
+
+func TestShouldLoadOpentracingModule(t *testing.T) {
+	// ### Invalid argument type tests ###
+	// The first tests should return false.
+	expected := false
+
+	invalidType := &ingress.Ingress{}
+	actual := shouldLoadOpentracingModule(config.Configuration{}, invalidType)
+	if expected != actual {
+		t.Errorf("Expected '%v' but returned '%v'", expected, actual)
+	}
+
+	actual = shouldLoadOpentracingModule(invalidType, []*ingress.Server{})
+	if expected != actual {
+		t.Errorf("Expected '%v' but returned '%v'", expected, actual)
+	}
+
+	// ### Functional tests ###
+	actual = shouldLoadOpentracingModule(config.Configuration{}, []*ingress.Server{})
+	if expected != actual {
+		t.Errorf("Expected '%v' but returned '%v'", expected, actual)
+	}
+
+	// All further tests should return true.
+	expected = true
+
+	configuration := config.Configuration{EnableOpentracing: true}
+	actual = shouldLoadOpentracingModule(configuration, []*ingress.Server{})
+	if expected != actual {
+		t.Errorf("Expected '%v' but returned '%v'", expected, actual)
+	}
+
+	servers := []*ingress.Server{
+		{
+			Locations: []*ingress.Location{
+				{
+					Opentracing: opentracing.Config{
+						Enabled: true,
+					},
+				},
+			},
+		},
+	}
+	actual = shouldLoadOpentracingModule(config.Configuration{}, servers)
+	if expected != actual {
+		t.Errorf("Expected '%v' but returned '%v'", expected, actual)
+	}
+
+	actual = shouldLoadOpentracingModule(configuration, servers)
+	if expected != actual {
+		t.Errorf("Expected '%v' but returned '%v'", expected, actual)
+	}
+}

rootfs/etc/nginx/template/nginx.tmpl

@@ -20,7 +20,7 @@ load_module /etc/nginx/modules/ngx_http_geoip2_module.so;
 load_module /etc/nginx/modules/ngx_http_modsecurity_module.so;
 {{ end }}
 
-{{ if $cfg.EnableOpentracing }}
+{{ if (shouldLoadOpentracingModule $cfg $servers) }}
 load_module /etc/nginx/modules/ngx_http_opentracing_module.so;
 {{ end }}
 
@@ -224,11 +224,7 @@ http {
     limit_req_status                {{ $cfg.LimitReqStatusCode }};
     limit_conn_status               {{ $cfg.LimitConnStatusCode }};
 
-    {{ if $cfg.EnableOpentracing }}
-    opentracing on;
-    {{ end }}
-
-    {{ buildOpentracing $cfg }}
+    {{ buildOpentracing $cfg $servers }}
 
     include /etc/nginx/mime.types;
     default_type text/html;