From 7d5452d00b7584195c4f9239f8938447a2a1b3f5 Mon Sep 17 00:00:00 2001 From: Matthew Silverman Date: Sun, 24 Oct 2021 17:36:21 -0400 Subject: [PATCH] configmap: option to not trust incoming tracing spans (#7045) * validate the sender of tracing spans * add location-specific setting --- .../nginx-configuration/annotations.md | 10 ++++++ .../third-party-addons/opentracing.md | 12 +++++++ .../ingress/annotations/opentracing/main.go | 23 +++++++++--- .../annotations/opentracing/main_test.go | 23 ++++++++++++ internal/ingress/controller/config/config.go | 6 ++++ .../ingress/controller/template/template.go | 28 +++++++-------- .../controller/template/template_test.go | 36 ++++++++++++------- rootfs/etc/nginx/template/nginx.tmpl | 2 +- test/e2e/settings/opentracing.go | 18 +++++++++- 9 files changed, 124 insertions(+), 34 deletions(-) diff --git a/docs/user-guide/nginx-configuration/annotations.md b/docs/user-guide/nginx-configuration/annotations.md index d8e6010ab..ca515f9e2 100755 --- a/docs/user-guide/nginx-configuration/annotations.md +++ b/docs/user-guide/nginx-configuration/annotations.md @@ -112,6 +112,7 @@ You can add these Kubernetes annotations to specific Ingress objects to customiz |[nginx.ingress.kubernetes.io/connection-proxy-header](#connection-proxy-header)|string| |[nginx.ingress.kubernetes.io/enable-access-log](#enable-access-log)|"true" or "false"| |[nginx.ingress.kubernetes.io/enable-opentracing](#enable-opentracing)|"true" or "false"| +|[nginx.ingress.kubernetes.io/opentracing-trust-incoming-span](#opentracing-trust-incoming-span)|"true" or "false"| |[nginx.ingress.kubernetes.io/enable-influxdb](#influxdb)|"true" or "false"| |[nginx.ingress.kubernetes.io/influxdb-measurement](#influxdb)|string| |[nginx.ingress.kubernetes.io/influxdb-port](#influxdb)|string| @@ -768,6 +769,15 @@ to enable it or disable it for a specific ingress (e.g. to turn off tracing of e nginx.ingress.kubernetes.io/enable-opentracing: "true" ``` +### Opentracing Trust Incoming Span + +The option to trust incoming trace spans can be enabled or disabled globally through the ConfigMap but this will +sometimes need to be overriden to enable it or disable it for a specific ingress (e.g. only enable on a private endpoint) + +```yaml +nginx.ingress.kubernetes.io/opentracing-trust-incoming-span: "true" +``` + ### X-Forwarded-Prefix Header To add the non-standard `X-Forwarded-Prefix` header to the upstream request with a string value, the following annotation can be used: diff --git a/docs/user-guide/third-party-addons/opentracing.md b/docs/user-guide/third-party-addons/opentracing.md index 916b29637..6c2918046 100644 --- a/docs/user-guide/third-party-addons/opentracing.md +++ b/docs/user-guide/third-party-addons/opentracing.md @@ -46,6 +46,9 @@ opentracing-operation-name # specifies specifies the name to use for the location span opentracing-location-operation-name +# sets whether or not to trust incoming tracing spans +opentracing-trust-incoming-span + # specifies the port to use when uploading traces, Default: 9411 zipkin-collector-port @@ -114,6 +117,15 @@ datadog-sample-rate All these options (including host) allow environment variables, such as `$HOSTNAME` or `$HOST_IP`. In the case of Jaeger, if you have a Jaeger agent running on each machine in your cluster, you can use something like `$HOST_IP` (which can be 'mounted' with the `status.hostIP` fieldpath, as described [here](https://kubernetes.io/docs/tasks/inject-data-application/downward-api-volume-expose-pod-information/#capabilities-of-the-downward-api)) to make sure traces will be sent to the local agent. + +Note that you can also set whether to trust incoming spans (global default is true) per-location using annotations like the following: +``` +kind: Ingress +metadata: + annotations: + nginx.ingress.kubernetes.io/opentracing-trust-incoming-span: "true" +``` + ## Examples The following examples show how to deploy and test different distributed tracing systems. These example can be performed using Minikube. diff --git a/internal/ingress/annotations/opentracing/main.go b/internal/ingress/annotations/opentracing/main.go index 2ed4a2182..17ba7eb9f 100644 --- a/internal/ingress/annotations/opentracing/main.go +++ b/internal/ingress/annotations/opentracing/main.go @@ -29,8 +29,10 @@ type opentracing struct { // Config contains the configuration to be used in the Ingress type Config struct { - Enabled bool `json:"enabled"` - Set bool `json:"set"` + Enabled bool `json:"enabled"` + Set bool `json:"set"` + TrustEnabled bool `json:"trust-enabled"` + TrustSet bool `json:"trust-set"` } // Equal tests for equality between two Config types @@ -43,6 +45,14 @@ func (bd1 *Config) Equal(bd2 *Config) bool { return false } + if bd1.TrustSet != bd2.TrustSet { + return false + } + + if bd1.TrustEnabled != bd2.TrustEnabled { + return false + } + return true } @@ -54,8 +64,13 @@ 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{}, nil } - return &Config{Set: true, Enabled: enabled}, nil + trustSpan, err := parser.GetBoolAnnotation("opentracing-trust-incoming-span", ing) + if err != nil { + return &Config{Set: true, Enabled: enabled}, nil + } + + return &Config{Set: true, Enabled: enabled, TrustSet: true, TrustEnabled: trustSpan}, nil } diff --git a/internal/ingress/annotations/opentracing/main_test.go b/internal/ingress/annotations/opentracing/main_test.go index 77e29cb5d..7bd9d31ff 100644 --- a/internal/ingress/annotations/opentracing/main_test.go +++ b/internal/ingress/annotations/opentracing/main_test.go @@ -106,6 +106,29 @@ func TestIngressAnnotationOpentracingSetFalse(t *testing.T) { } } +func TestIngressAnnotationOpentracingTrustSetTrue(t *testing.T) { + ing := buildIngress() + + data := map[string]string{} + data[parser.GetAnnotationWithPrefix("enable-opentracing")] = "true" + data[parser.GetAnnotationWithPrefix("opentracing-trust-incoming-span")] = "true" + ing.SetAnnotations(data) + + val, _ := NewParser(&resolver.Mock{}).Parse(ing) + openTracing, ok := val.(*Config) + if !ok { + t.Errorf("expected a Config type") + } + + if !openTracing.Enabled { + t.Errorf("expected annotation value to be true, got false") + } + + if !openTracing.TrustEnabled { + t.Errorf("expected annotation value to be true, got false") + } +} + func TestIngressAnnotationOpentracingUnset(t *testing.T) { ing := buildIngress() diff --git a/internal/ingress/controller/config/config.go b/internal/ingress/controller/config/config.go index a29c1b094..b1a5fc8c4 100644 --- a/internal/ingress/controller/config/config.go +++ b/internal/ingress/controller/config/config.go @@ -537,6 +537,11 @@ type Configuration struct { // OpentracingOperationName specifies a custom name for the location span OpentracingLocationOperationName string `json:"opentracing-location-operation-name"` + // OpentracingTrustIncomingSpan sets whether or not to trust incoming trace spans + // If false, incoming span headers will be rejected + // Default: true + OpentracingTrustIncomingSpan bool `json:"opentracing-trust-incoming-span"` + // ZipkinCollectorHost specifies the host to use when uploading traces ZipkinCollectorHost string `json:"zipkin-collector-host"` @@ -874,6 +879,7 @@ func NewDefault() Configuration { LimitConnZoneVariable: defaultLimitConnZoneVariable, BindAddressIpv4: defBindAddress, BindAddressIpv6: defBindAddress, + OpentracingTrustIncomingSpan: true, ZipkinCollectorPort: 9411, ZipkinServiceName: "nginx", ZipkinSampleRate: 1.0, diff --git a/internal/ingress/controller/template/template.go b/internal/ingress/controller/template/template.go index e5da5b993..7ed297fa6 100644 --- a/internal/ingress/controller/template/template.go +++ b/internal/ingress/controller/template/template.go @@ -1440,7 +1440,7 @@ func httpsListener(addresses []string, co string, tc config.TemplateConfig) []st return out } -func buildOpentracingForLocation(isOTEnabled bool, location *ingress.Location) string { +func buildOpentracingForLocation(isOTEnabled bool, isOTTrustSet bool, location *ingress.Location) string { isOTEnabledInLoc := location.Opentracing.Enabled isOTSetInLoc := location.Opentracing.Set @@ -1448,25 +1448,21 @@ func buildOpentracingForLocation(isOTEnabled bool, location *ingress.Location) s if isOTSetInLoc && !isOTEnabledInLoc { return "opentracing off;" } - - opc := opentracingPropagateContext(location) - if opc != "" { - opc = fmt.Sprintf("opentracing on;\n%v", opc) - } - - return opc + } else if !isOTSetInLoc || !isOTEnabledInLoc { + return "" } - if isOTSetInLoc && isOTEnabledInLoc { - opc := opentracingPropagateContext(location) - if opc != "" { - opc = fmt.Sprintf("opentracing on;\n%v", opc) - } - - return opc + opc := opentracingPropagateContext(location) + if opc != "" { + opc = fmt.Sprintf("opentracing on;\n%v", opc) } - return "" + if (!isOTTrustSet && !location.Opentracing.TrustSet) || + (location.Opentracing.TrustSet && !location.Opentracing.TrustEnabled) { + opc = opc + "\nopentracing_trust_incoming_span off;" + } + + return opc } // shouldLoadOpentracingModule determines whether or not the Opentracing module needs to be loaded. diff --git a/internal/ingress/controller/template/template_test.go b/internal/ingress/controller/template/template_test.go index 96dc9acae..cfb65c08e 100644 --- a/internal/ingress/controller/template/template_test.go +++ b/internal/ingress/controller/template/template_test.go @@ -1637,32 +1637,44 @@ func TestShouldLoadModSecurityModule(t *testing.T) { func TestOpentracingForLocation(t *testing.T) { trueVal := true + falseVal := false loadOT := `opentracing on; opentracing_propagate_context;` + loadOTUntrustedSpan := `opentracing on; +opentracing_propagate_context; +opentracing_trust_incoming_span off;` testCases := []struct { - description string - globalOT bool - isSetInLoc bool - isOTInLoc *bool - expected string + description string + globalOT bool + isSetInLoc bool + isOTInLoc *bool + globalTrust bool + isTrustSetInLoc bool + isTrustInLoc *bool + expected string }{ - {"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}, + {"globally enabled, without annotation", true, false, nil, true, false, nil, loadOT}, + {"globally enabled and enabled in location", true, true, &trueVal, true, false, nil, loadOT}, + {"globally disabled and not enabled in location", false, false, nil, true, false, nil, ""}, + {"globally disabled but enabled in location", false, true, &trueVal, true, false, nil, loadOT}, + {"globally trusted, not trusted in location", true, false, nil, true, true, &falseVal, loadOTUntrustedSpan}, + {"not globally trusted, trust set in location", true, false, nil, false, true, &trueVal, loadOT}, + {"not globally trusted, trust not set in location", true, false, nil, false, false, nil, loadOTUntrustedSpan}, } for _, testCase := range testCases { il := &ingress.Location{ - Opentracing: opentracing.Config{Set: testCase.isSetInLoc}, + Opentracing: opentracing.Config{Set: testCase.isSetInLoc, TrustSet: testCase.isTrustSetInLoc}, } if il.Opentracing.Set { il.Opentracing.Enabled = *testCase.isOTInLoc } + if il.Opentracing.TrustSet { + il.Opentracing.TrustEnabled = *testCase.isTrustInLoc + } - actual := buildOpentracingForLocation(testCase.globalOT, il) + actual := buildOpentracingForLocation(testCase.globalOT, testCase.globalTrust, il) if testCase.expected != actual { t.Errorf("%v: expected '%v' but returned '%v'", testCase.description, testCase.expected, actual) diff --git a/rootfs/etc/nginx/template/nginx.tmpl b/rootfs/etc/nginx/template/nginx.tmpl index 32b02f72a..bfe0703ff 100755 --- a/rootfs/etc/nginx/template/nginx.tmpl +++ b/rootfs/etc/nginx/template/nginx.tmpl @@ -1096,7 +1096,7 @@ stream { set $location_path {{ $ing.Path | escapeLiteralDollar | quote }}; set $global_rate_limit_exceeding n; - {{ buildOpentracingForLocation $all.Cfg.EnableOpentracing $location }} + {{ buildOpentracingForLocation $all.Cfg.EnableOpentracing $all.Cfg.OpentracingTrustIncomingSpan $location }} {{ if $location.Mirror.Source }} mirror {{ $location.Mirror.Source }}; diff --git a/test/e2e/settings/opentracing.go b/test/e2e/settings/opentracing.go index 0fe4e62f6..3ac16be78 100644 --- a/test/e2e/settings/opentracing.go +++ b/test/e2e/settings/opentracing.go @@ -29,7 +29,8 @@ import ( ) const ( - enableOpentracing = "enable-opentracing" + enableOpentracing = "enable-opentracing" + opentracingTrustIncomingSpan = "opentracing-trust-incoming-span" zipkinCollectorHost = "zipkin-collector-host" @@ -81,6 +82,21 @@ var _ = framework.IngressNginxDescribe("Configure OpenTracing", func() { }) }) + ginkgo.It("should include opentracing_trust_incoming_span off directive when disabled", func() { + config := map[string]string{} + config[enableOpentracing] = "true" + config[opentracingTrustIncomingSpan] = "false" + config[zipkinCollectorHost] = "127.0.0.1" + f.SetNginxConfigMapData(config) + + f.EnsureIngress(framework.NewSingleIngress(enableOpentracing, "/", enableOpentracing, f.Namespace, "http-svc", 80, nil)) + + f.WaitForNginxConfiguration( + func(cfg string) bool { + return strings.Contains(cfg, "opentracing_trust_incoming_span off") + }) + }) + ginkgo.It("should not exists opentracing_operation_name directive when is empty", func() { config := map[string]string{} config[enableOpentracing] = "true"