configmap: option to not trust incoming tracing spans #7045

timmysilv · 2021-04-13T01:57:40Z

What this PR does / why we need it:

nginx-opentracing allows a static configuration to not propagate incoming span headers. This gives the user the opportunity to choose whether to propagate the trace/span headers via configmap.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Which issue/s this PR fixes

How Has This Been Tested?

Checklist:

My change requires a change to the documentation.
I have updated the documentation accordingly.
I've read the CONTRIBUTION guide
I have added tests to cover my changes.
All new and existing tests passed.

k8s-ci-robot · 2021-04-13T01:57:48Z

Hi @timmysilv. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

rikatz · 2021-05-12T22:19:28Z

rootfs/etc/nginx/template/nginx.tmpl

@@ -1092,6 +1092,9 @@ stream {
            set $global_rate_limit_exceeding n;

            {{ buildOpentracingForLocation $all.Cfg.EnableOpentracing $location }}
+            {{ if not $all.Cfg.OpentracingTrustIncomingSpan }}


Is there a reason for this configuration to be global instead of location specific / per annotation?

Nope. It could be added to buildOpentracingForLocation() and use the same logic as isOTSetInLoc && isOTEnabledInLoc if that's preferred, so there can be both global and location-specific settings.

rikatz · 2021-05-12T22:19:37Z

/ok-to-test

rikatz · 2021-05-12T22:19:43Z

/assign
/cc

timmysilv · 2021-05-14T20:11:55Z

internal/ingress/controller/template/template_test.go

-		{"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},


deleted this line bc it was the same as the one before. Also added true, false, nil to each line since that results in the same behaviour as before this PR.

rikatz · 2021-06-27T22:11:10Z

internal/ingress/controller/template/template.go

@@ -1343,33 +1343,29 @@ 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

 	if isOTEnabled {
 		if isOTSetInLoc && !isOTEnabledInLoc {


I'm still trying to ramp up on this, but maybe... why we check isOTSetnLock and isOTEnabledInLoc, when isOTEnabledInLoc = false will always be false? And the default value is false anyway?

I would remove this and check only !isOTEnabledInLoc {return opentracing off; }

The default for isOTEnabledInLoc is false, so this scenario means:

OT is enabled globally

There is an OT annotation present on the specific Ingress

That annotation is set to false

The reason to check both is the (common) case where OT is enabled globally, and there is no annotation present on an Ingress. In that case the values will be: {isOTEnabled: true, isOTSetInLoc: false, isOTEnabledInLoc: false}, BUT it should be enabled in that location.

rikatz · 2021-06-27T22:11:52Z

internal/ingress/controller/template/template.go

-		}
-
-		return opc
+	} else if !isOTSetInLoc || !isOTEnabledInLoc {


Same here, I might be missing something but are we double checking things that might be a legacy behavior?

Agreed, in this case we should be able to just check !isOTEnabledInLoc. If it's set and set to false, don't enable. And if it's not set, the default for this bool would be false anyway. Also don't enable.

rikatz · 2021-06-27T22:15:04Z

rootfs/etc/nginx/template/nginx.tmpl

@@ -1094,7 +1094,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 }}


I think the main difference here is:
Opentracing can be enabled globally, while we can or cannot enable trusting incoming span locally right?

So my question is: what is the behavior, as the cluster admin we wanna do? Leave this decision to the user? If so, this should be exclusively local (and we don't need to assess this here).

If we want to have precedence, than we need to have a configmap that explicits if we trust incoming span or not, and bypass the verification of the location

This PR adds that configmap value. Without this PR (current state), all incoming spans are accepted as default behaviour. The change introduced here is the option to set a global "off" switch for that. The possible scenarios are:

Spans are trusted globally, but certain backend/ingress owners decide they don't want to trust any spans

Spans are not trusted globally, but certain backend/ingress owners decide that they do want to trust any spans

Seeing as we don't know every user of ingress-nginx, I'd prefer to keep it flexible. However, if you feel that there is a principle to be followed (for example, when trust is globally disabled then no one backend owner should have the right to enable it for just them), I am willing to change this PR to become more opinionated.

rikatz · 2021-06-27T22:17:06Z

@timmysilv I'm really ok with this :)

Just wondering what are the user stories: Should we block trusting span globally? Per user? both?

Please just give me some context so we can decide the best approach together.

rikatz · 2021-08-06T19:15:12Z

@timmysilv friendly ping :)

timmysilv · 2021-08-09T20:39:44Z

hi, sorry for the lack of clarity - I explained how this feature might be used by cluster owners and app owners in tandem in the inline comment responses.

rikatz · 2021-10-24T21:31:03Z

/lgtm
/approve
Thanks!

k8s-ci-robot · 2021-10-24T21:31:21Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rikatz, timmysilv

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [rikatz]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

* validate the sender of tracing spans * add location-specific setting

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 13, 2021

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 13, 2021

k8s-ci-robot requested review from cmluciano and ElvinEfendi April 13, 2021 01:58

timmysilv force-pushed the ot-trust-ips branch from 0c0d8a6 to c142b65 Compare April 28, 2021 18:30

k8s-ci-robot removed the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 28, 2021

timmysilv changed the title ~~validate the sender of tracing spans~~ configmap: option to not trust incoming tracing spans Apr 28, 2021

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Apr 28, 2021

rikatz reviewed May 12, 2021

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 12, 2021

k8s-ci-robot assigned rikatz May 12, 2021

k8s-ci-robot requested a review from rikatz May 12, 2021 22:19

Matthew Silverman added 2 commits May 14, 2021 13:47

validate the sender of tracing spans

9868e24

add location-specific setting

5178b64

timmysilv force-pushed the ot-trust-ips branch from c142b65 to 5178b64 Compare May 14, 2021 20:08

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 14, 2021

timmysilv commented May 14, 2021

View reviewed changes

rikatz requested changes Jun 27, 2021

View reviewed changes

rikatz changed the base branch from master to main July 16, 2021 13:27

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 24, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 24, 2021

k8s-ci-robot merged commit 7d5452d into kubernetes:main Oct 24, 2021

kskalski pushed a commit to kskalski/ingress-nginx that referenced this pull request Nov 28, 2021

configmap: option to not trust incoming tracing spans (kubernetes#7045)

6439ce7

* validate the sender of tracing spans * add location-specific setting

rchshld pushed a commit to joomcode/ingress-nginx that referenced this pull request May 19, 2023

configmap: option to not trust incoming tracing spans (kubernetes#7045)

6f2d981

* validate the sender of tracing spans * add location-specific setting

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

configmap: option to not trust incoming tracing spans #7045

configmap: option to not trust incoming tracing spans #7045

timmysilv commented Apr 13, 2021 •

edited

Loading

k8s-ci-robot commented Apr 13, 2021

rikatz May 12, 2021

timmysilv May 13, 2021

rikatz commented May 12, 2021

rikatz commented May 12, 2021

timmysilv May 14, 2021

rikatz Jun 27, 2021

timmysilv Jun 28, 2021

rikatz Jun 27, 2021

timmysilv Jun 28, 2021

rikatz Jun 27, 2021

timmysilv Jun 28, 2021

rikatz commented Jun 27, 2021

rikatz commented Aug 6, 2021

timmysilv commented Aug 9, 2021

rikatz commented Oct 24, 2021

k8s-ci-robot commented Oct 24, 2021

configmap: option to not trust incoming tracing spans #7045

configmap: option to not trust incoming tracing spans #7045

Conversation

timmysilv commented Apr 13, 2021 • edited Loading

What this PR does / why we need it:

Types of changes

Which issue/s this PR fixes

How Has This Been Tested?

Checklist:

k8s-ci-robot commented Apr 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rikatz commented May 12, 2021

rikatz commented May 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rikatz commented Jun 27, 2021

rikatz commented Aug 6, 2021

timmysilv commented Aug 9, 2021

rikatz commented Oct 24, 2021

k8s-ci-robot commented Oct 24, 2021

timmysilv commented Apr 13, 2021 •

edited

Loading