diff --git a/internal/ingress/annotations/opentracing/main.go b/internal/ingress/annotations/opentracing/main.go index 70d5504d53..875d695f70 100644 --- a/internal/ingress/annotations/opentracing/main.go +++ b/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 } diff --git a/internal/ingress/controller/nginx.go b/internal/ingress/controller/nginx.go index d393e084e2..6d5ff1cccc 100644 --- a/internal/ingress/controller/nginx.go +++ b/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) diff --git a/internal/ingress/controller/template/template.go b/internal/ingress/controller/template/template.go index 5a702d3cb7..a7f562b558 100644 --- a/internal/ingress/controller/template/template.go +++ b/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", c) + return "" + } + + servers, ok := s.([]*ingress.Server) if !ok { - klog.Errorf("expected a 'config.Configuration' type but %T was returned", input) + klog.Errorf("expected an '[]*ingress.Server' type but %T was returned", s) return "" } - if !cfg.EnableOpentracing { + 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 +} diff --git a/internal/ingress/controller/template/template_test.go b/internal/ingress/controller/template/template_test.go index f93744e05b..c134196cb8 100644 --- a/internal/ingress/controller/template/template_test.go +++ b/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) + } +} diff --git a/rootfs/etc/nginx/template/nginx.tmpl b/rootfs/etc/nginx/template/nginx.tmpl index c937a73478..2ccdaa9216 100755 --- a/rootfs/etc/nginx/template/nginx.tmpl +++ b/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; @@ -1021,13 +1017,13 @@ stream { set $proxy_upstream_name {{ buildUpstreamName $location | quote }}; set $proxy_host $proxy_upstream_name; set $pass_access_scheme $scheme; - + {{ if $all.Cfg.UseProxyProtocol }} set $pass_server_port $proxy_protocol_server_port; {{ else }} set $pass_server_port $server_port; {{ end }} - + set $best_http_host $http_host; set $pass_port $pass_server_port;