Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow enabling/disabling opentracing for ingresses #4732

Merged

Conversation

willthames
Copy link
Contributor

What this PR does / why we need it:

Occasionally you might want to enable opentracing only for a specific set of ingresses, or disable a specific ingress (this appears to be the easiest way to not trace specific paths such as an external healthcheck). While enabling opentracing is possible using location snippets, disabling opentracing is not currently possible (because opentracing_propagate_context cannot be unset once set).

Special notes for your reviewer:

I would have liked the default for enabled to be the value for enable-opentracing from the ConfigMap as that would have simplified a lot of logic (I wouldn't need the Set property and the nginx template would have been more readable). I couldn't work out how to do that, but if it's possible, I would definitely be happy to refactor this change.

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Nov 1, 2019
@k8s-ci-robot
Copy link
Contributor

Hi @willthames. 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.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 1, 2019
@aledbf
Copy link
Member

aledbf commented Nov 25, 2019

/ok-to-test

@k8s-ci-robot 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 Nov 25, 2019
@codecov-io
Copy link

Codecov Report

Merging #4732 into master will decrease coverage by <.01%.
The diff coverage is 60%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4732      +/-   ##
==========================================
- Coverage   58.62%   58.61%   -0.01%     
==========================================
  Files          88       89       +1     
  Lines        6743     6761      +18     
==========================================
+ Hits         3953     3963      +10     
- Misses       2353     2361       +8     
  Partials      437      437
Impacted Files Coverage Δ
internal/ingress/types.go 0% <ø> (ø) ⬆️
internal/ingress/controller/controller.go 50.55% <100%> (+0.12%) ⬆️
internal/ingress/annotations/annotations.go 81.94% <100%> (+0.25%) ⬆️
internal/ingress/annotations/opentracing/main.go 53.84% <53.84%> (ø)
internal/ingress/controller/store/backend_ssl.go 42.85% <0%> (-0.45%) ⬇️
internal/ingress/controller/store/store.go 58.51% <0%> (-0.13%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 43fa61c...1d0af54. Read the comment docs.

@aledbf
Copy link
Member

aledbf commented Nov 26, 2019

@willthames please check the gofmt error

!!! 'gofmt -s' needs to be run on the following files: 
./internal/ingress/annotations/opentracing/main.go

By default you might want opentracing off, but on for a particular
ingress.

Similarly, you might want opentracing globally on, but disabled for
a specific endpoint. To achieve this, `opentracing_propagate_context`
cannot be set when combined with `opentracing off`

A new annotation, `enable-opentracing` allows more fine grained control
of opentracing for specific ingresses.
Ensure that AWS and Docker credentials don't get
accidentally added
@willthames willthames force-pushed the enable-opentracing-annotation branch from 1d0af54 to 6927d93 Compare November 27, 2019 01:07
@willthames
Copy link
Contributor Author

@aledbf that's done now, thanks!

@aledbf
Copy link
Member

aledbf commented Nov 27, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 27, 2019
@aledbf
Copy link
Member

aledbf commented Nov 27, 2019

@willthames thanks!

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aledbf, willthames

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:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 27, 2019
@k8s-ci-robot k8s-ci-robot merged commit b286c2a into kubernetes:master Nov 27, 2019
@Stono
Copy link
Contributor

Stono commented Jan 15, 2020

This is a great feature - thanks for adding it :-)
However we found a bug where the annotation doesn't seem to apply without a restart of the nginx controller (see #4933)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants