Move opentracing configuration for location to go (#4965)

internal/ingress/annotations/opentracing/main.go

@@ -30,16 +30,14 @@ 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
-	} else if bd1.Enabled != bd2.Enabled {
+	if bd1.Enabled != bd2.Enabled {
 		return false
 	}
+
 	return true
 }
 
@@ -51,7 +49,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{Set: false, Enabled: false}, nil
+		return &Config{Enabled: false}, nil
 	}
-	return &Config{Set: true, Enabled: enabled}, nil
+
+	return &Config{Enabled: enabled}, nil
 }

internal/ingress/annotations/opentracing/main_test.go

@@ -74,9 +74,7 @@ func TestIngressAnnotationOpentracingSetTrue(t *testing.T) {
 	if !ok {
 		t.Errorf("expected a Config type")
 	}
-	if !openTracing.Set {
-		t.Errorf("expected annotation value to be set")
-	}
+
 	if !openTracing.Enabled {
 		t.Errorf("expected annotation value to be true, got false")
 	}
@@ -95,9 +93,7 @@ func TestIngressAnnotationOpentracingSetFalse(t *testing.T) {
 	if !ok {
 		t.Errorf("expected a Config type")
 	}
-	if !openTracing.Set {
-		t.Errorf("expected annotation value to be set")
-	}
+
 	if openTracing.Enabled {
 		t.Errorf("expected annotation value to be false, got true")
 	}
@@ -111,11 +107,8 @@ func TestIngressAnnotationOpentracingUnset(t *testing.T) {
 	ing.SetAnnotations(data)
 
 	val, _ := NewParser(&resolver.Mock{}).Parse(ing)
-	openTracing, ok := val.(*Config)
+	_, ok := val.(*Config)
 	if !ok {
 		t.Errorf("expected a Config type")
 	}
-	if openTracing.Set {
-		t.Errorf("expected annotation value to be unset")
-	}
 }

internal/ingress/controller/template/template.go

@@ -173,11 +173,11 @@ var (
 		"enforceRegexModifier":               enforceRegexModifier,
 		"stripLocationModifer":               stripLocationModifer,
 		"buildCustomErrorDeps":               buildCustomErrorDeps,
-		"opentracingPropagateContext":        opentracingPropagateContext,
 		"buildCustomErrorLocationsPerServer": buildCustomErrorLocationsPerServer,
 		"shouldLoadModSecurityModule":        shouldLoadModSecurityModule,
 		"buildHTTPListener":                  buildHTTPListener,
 		"buildHTTPSListener":                 buildHTTPSListener,
+		"buildOpentracingForLocation":        buildOpentracingForLocation,
 	}
 )
 
@@ -1064,18 +1064,16 @@ func buildCustomErrorLocationsPerServer(input interface{}) []errorLocation {
 	return errorLocations
 }
 
-func opentracingPropagateContext(loc interface{}) string {
-	location, ok := loc.(*ingress.Location)
-	if !ok {
-		klog.Errorf("expected a '*ingress.Location' type but %T was returned", loc)
-		return "opentracing_propagate_context"
+func opentracingPropagateContext(location *ingress.Location) string {
+	if location == nil {
+		return ""
 	}
 
 	if location.BackendProtocol == "GRPC" || location.BackendProtocol == "GRPCS" {
-		return "opentracing_grpc_propagate_context"
+		return "opentracing_grpc_propagate_context;"
 	}
 
-	return "opentracing_propagate_context"
+	return "opentracing_propagate_context;"
 }
 
 // shouldLoadModSecurityModule determines whether or not the ModSecurity module needs to be loaded.
@@ -1271,3 +1269,21 @@ func httpsListener(addresses []string, co string, tc config.TemplateConfig) []st
 
 	return out
 }
+
+func buildOpentracingForLocation(isOTEnabled bool, location *ingress.Location) string {
+	isOTEnabledInLoc := location.Opentracing.Enabled
+
+	if isOTEnabled {
+		if !isOTEnabledInLoc {
+			return "opentracing off;"
+		}
+
+		return opentracingPropagateContext(location)
+	}
+
+	if isOTEnabledInLoc {
+		return opentracingPropagateContext(location)
+	}
+
+	return ""
+}

internal/ingress/controller/template/template_test.go

@@ -39,6 +39,7 @@ import (
 	"k8s.io/ingress-nginx/internal/ingress/annotations/authreq"
 	"k8s.io/ingress-nginx/internal/ingress/annotations/influxdb"
 	"k8s.io/ingress-nginx/internal/ingress/annotations/modsecurity"
+	"k8s.io/ingress-nginx/internal/ingress/annotations/opentracing"
 	"k8s.io/ingress-nginx/internal/ingress/annotations/ratelimit"
 	"k8s.io/ingress-nginx/internal/ingress/annotations/rewrite"
 	"k8s.io/ingress-nginx/internal/ingress/controller/config"
@@ -883,14 +884,14 @@ func TestEscapeLiteralDollar(t *testing.T) {
 }
 
 func TestOpentracingPropagateContext(t *testing.T) {
-	tests := map[interface{}]string{
-		&ingress.Location{BackendProtocol: "HTTP"}:  "opentracing_propagate_context",
-		&ingress.Location{BackendProtocol: "HTTPS"}: "opentracing_propagate_context",
-		&ingress.Location{BackendProtocol: "GRPC"}:  "opentracing_grpc_propagate_context",
-		&ingress.Location{BackendProtocol: "GRPCS"}: "opentracing_grpc_propagate_context",
-		&ingress.Location{BackendProtocol: "AJP"}:   "opentracing_propagate_context",
-		&ingress.Location{BackendProtocol: "FCGI"}:  "opentracing_propagate_context",
-		"not a location": "opentracing_propagate_context",
+	tests := map[*ingress.Location]string{
+		{BackendProtocol: "HTTP"}:  "opentracing_propagate_context;",
+		{BackendProtocol: "HTTPS"}: "opentracing_propagate_context;",
+		{BackendProtocol: "GRPC"}:  "opentracing_grpc_propagate_context;",
+		{BackendProtocol: "GRPCS"}: "opentracing_grpc_propagate_context;",
+		{BackendProtocol: "AJP"}:   "opentracing_propagate_context;",
+		{BackendProtocol: "FCGI"}:  "opentracing_propagate_context;",
+		nil:                        "",
 	}
 
 	for loc, expectedDirective := range tests {
@@ -1280,3 +1281,27 @@ func TestShouldLoadModSecurityModule(t *testing.T) {
 		t.Errorf("Expected '%v' but returned '%v'", expected, actual)
 	}
 }
+
+func TestOpentracingForLocation(t *testing.T) {
+	testCases := []struct {
+		description string
+		globalOT    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;"},
+	}
+
+	for _, testCase := range testCases {
+		actual := buildOpentracingForLocation(testCase.globalOT, &ingress.Location{
+			Opentracing: opentracing.Config{Enabled: testCase.isOTInLoc},
+		})
+
+		if testCase.expected != actual {
+			t.Errorf("%v: expected '%v' but returned '%v'", testCase.description, testCase.expected, actual)
+		}
+	}
+}

internal/ingress/types_equals.go

@@ -442,6 +442,10 @@ func (l1 *Location) Equal(l2 *Location) bool {
 		return false
 	}
 
+	if !l1.Opentracing.Equal(&l2.Opentracing) {
+		return false
+	}
+
 	return true
 }
 

rootfs/etc/nginx/template/nginx.tmpl

@@ -967,18 +967,7 @@ stream {
             set $service_port   {{ $ing.ServicePort | quote }};
             set $location_path  {{ $location.Path | escapeLiteralDollar | quote }};
 
-            {{ if $all.Cfg.EnableOpentracing }}
-            {{ if and $location.Opentracing.Set (not $location.Opentracing.Enabled) }}
-            opentracing off;
-            {{ else }}
-            {{ opentracingPropagateContext $location }};
-            {{ end }}
-            {{ else }}
-            {{ if and $location.Opentracing.Set $location.Opentracing.Enabled }}
-            opentracing on;
-            {{ opentracingPropagateContext $location }};
-            {{ end }}
-            {{ end }}
+            {{ buildOpentracingForLocation $all.Cfg.EnableOpentracing $location }}
 
             {{ if $location.Mirror.URI }}
             mirror {{ $location.Mirror.URI }};