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

Fix IPv6 parsing errors in semconv.NetAttributesFromHTTPRequest #2285

Merged
merged 9 commits into from Oct 18, 2021

Conversation

evantorrie
Copy link
Contributor

fixes #2283

@codecov
Copy link

codecov bot commented Oct 13, 2021

Codecov Report

Merging #2285 (69b88ab) into main (43465e8) will increase coverage by 0.0%.
The diff coverage is 100.0%.

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #2285   +/-   ##
=====================================
  Coverage   72.6%   72.6%           
=====================================
  Files        172     172           
  Lines      11919   11898   -21     
=====================================
- Hits        8661    8646   -15     
+ Misses      3023    3019    -4     
+ Partials     235     233    -2     
Impacted Files Coverage Δ
semconv/v1.4.0/http.go 96.3% <100.0%> (+1.2%) ⬆️
...s/otlp/otlptrace/internal/connection/connection.go 16.2% <0.0%> (+1.5%) ⬆️

Standardize order of checks for IP, Name, Port
@garthk
Copy link
Contributor

garthk commented Oct 14, 2021

I don't recall touching any code relevant to TestNew_collectorConnectionDiesThenReconnects. Is that test flakey, by any chance, or do I need to dig deeper? Oh, sorry, wrong bug; I thought this was #2284. Thanks for picking this one up

Copy link
Contributor

@garthk garthk left a comment

Choose a reason for hiding this comment

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

I was expecting the implementation to shrink given net.SplitHostPort. Can we use it in the first line or two and take the fallback measures only if required—perhaps not at all? Under what circumstances would Go populate RemoteAddr without a port?

i.e. assume net.SplitHostPort(input) will succeed
Copy link
Member

@pellared pellared left a comment

Choose a reason for hiding this comment

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

👍

@evantorrie
Copy link
Contributor Author

@garthk I took your advice and refactored the code to assume the happy path succeeds, i.e. has a combination of both host and port. I do use the same routine to extract the hostIP/hostName and port for both request.RemoteAddr and the various permutations of request.Host, Host header and request URL. That means it needs to be a bit more robust than just assuming the Go runtime has set these reasonably (especially with things like parsing the Host header which comes direct from the "untrusted" client).

In any case, it looks like all the new IPv6 tests are passing and there are 20 fewer lines of code in http.go after this PR.

@MrAlias MrAlias merged commit 1f4b606 into open-telemetry:main Oct 18, 2021
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.

net.peer.ip not filled for IPv6
5 participants