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

Sanitize spans with null process or empty service name #3631

Merged
merged 5 commits into from
May 10, 2022

Conversation

yurishkuro
Copy link
Member

@yurishkuro yurishkuro commented Apr 15, 2022

Sanitize spans with empty service name or null process. This should protect the pipeline from malformed spans submitted externally, as reported in #3578. It will not protect against malformed spans written directly to Kafka - we could do that too, but it changes the expectations of the deployment that Kafka is a public API into ingester, which would be a new guarantee and will require ingester to run preprocessing logic similar to collector.

@yurishkuro yurishkuro requested a review from a team as a code owner April 15, 2022 16:10
@codecov
Copy link

codecov bot commented Apr 15, 2022

Codecov Report

Merging #3631 (9d6cf8f) into main (998c9d4) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #3631      +/-   ##
==========================================
+ Coverage   96.49%   96.53%   +0.03%     
==========================================
  Files         266      267       +1     
  Lines       15622    15653      +31     
==========================================
+ Hits        15075    15111      +36     
+ Misses        457      453       -4     
+ Partials       90       89       -1     
Impacted Files Coverage Δ
cmd/collector/app/options.go 100.00% <ø> (ø)
cmd/agent/app/reporter/grpc/reporter.go 100.00% <100.00%> (ø)
...ctor/app/sanitizer/empty_service_name_sanitizer.go 100.00% <100.00%> (ø)
cmd/collector/app/sanitizer/sanitizer.go 100.00% <100.00%> (ø)
...d/collector/app/sanitizer/zipkin/span_sanitizer.go 100.00% <100.00%> (ø)
cmd/collector/app/span_handler_builder.go 100.00% <100.00%> (ø)
cmd/collector/app/span_processor.go 100.00% <100.00%> (ø)
plugin/storage/badger/spanstore/reader.go 95.50% <0.00%> (-0.71%) ⬇️
pkg/config/tlscfg/cert_watcher.go 94.73% <0.00%> (+2.10%) ⬆️
cmd/query/app/static_handler.go 97.60% <0.00%> (+3.59%) ⬆️

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 998c9d4...9d6cf8f. Read the comment docs.

@mergify
Copy link

mergify bot commented Apr 15, 2022

@codecov[bot] is not allowed to run commands

@yurishkuro
Copy link
Member Author

@albertteoh can you review, please?

Copy link
Contributor

@albertteoh albertteoh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall, just a question around the standard sanitizers variable.

"github.com/jaegertracing/jaeger/model"
)

var (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not const?

@@ -96,13 +96,18 @@ func newSpanProcessor(spanWriter spanstore.Writer, additional []ProcessSpan, opt
}
boundedQueue := queue.NewBoundedQueue(options.queueSize, droppedItemHandler)

sanitizers := append([]sanitizer.SanitizeSpan{}, sanitizer.StandardSanitizers...)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sanitizer.StandardSanitizers is a globally accessible slice that can be mutated. What do you think about unexporting sanitizer.StandardSanitizers and adding a sanitizer.GetStandardSanitizers() func?

Then (I don't think we need to append to an empty slice?):

sanitizers := sanitizer.GetStandardSanitizers()

@albertteoh
Copy link
Contributor

@yurishkuro, apologies for the late review, I've been away for a couple of weeks.

@@ -68,7 +69,7 @@ wait_for_it() {
''%{http_code}''
)
local counter=0
while [[ "$(curl ${params[@]} ${url})" != "200" && ${counter} -le 30 ]]; do
while [[ "$(curl ${params[@]} ${url})" != "200" && ${counter} -le 60 ]]; do
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1min (2*30) seems insufficient sometimes, we can wait longer if it takes OS longer to start

albertteoh
albertteoh previously approved these changes May 10, 2022
Copy link
Contributor

@albertteoh albertteoh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Seems some lines might need to be covered by tests though.

Signed-off-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: Yuri Shkuro <github@ysh.us>
@yurishkuro yurishkuro merged commit 7ad154e into jaegertracing:main May 10, 2022
@yurishkuro yurishkuro deleted the null-process branch May 10, 2022 17:58
albertteoh pushed a commit to albertteoh/jaeger that referenced this pull request Jul 13, 2022
…#3631)

* Sanitize spans with null process or empty service name

Signed-off-by: Yuri Shkuro <github@ysh.us>

* cleanup

Signed-off-by: Yuri Shkuro <github@ysh.us>

* review comments

Signed-off-by: Yuri Shkuro <github@ysh.us>

* wait longer

Signed-off-by: Yuri Shkuro <github@ysh.us>

* add tests

Signed-off-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: Albert Teoh <see.kwang.teoh@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants