diff --git a/internal/ingress/annotations/opentracing/main.go b/internal/ingress/annotations/opentracing/main.go index 7157f92e2..70d5504d5 100644 --- a/internal/ingress/annotations/opentracing/main.go +++ b/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 } diff --git a/internal/ingress/annotations/opentracing/main_test.go b/internal/ingress/annotations/opentracing/main_test.go index d47df85e0..f1e06b087 100644 --- a/internal/ingress/annotations/opentracing/main_test.go +++ b/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") - } } diff --git a/internal/ingress/controller/template/template.go b/internal/ingress/controller/template/template.go index 75d36a2c7..5a702d3cb 100644 --- a/internal/ingress/controller/template/template.go +++ b/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 "" +} diff --git a/internal/ingress/controller/template/template_test.go b/internal/ingress/controller/template/template_test.go index 28ab4212d..f93744e05 100644 --- a/internal/ingress/controller/template/template_test.go +++ b/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) + } + } +} diff --git a/internal/ingress/types_equals.go b/internal/ingress/types_equals.go index 34d3475d2..6eba4c3c2 100644 --- a/internal/ingress/types_equals.go +++ b/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 } diff --git a/rootfs/etc/nginx/template/nginx.tmpl b/rootfs/etc/nginx/template/nginx.tmpl index 5ee179516..d9468ae29 100755 --- a/rootfs/etc/nginx/template/nginx.tmpl +++ b/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 }};