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

contrib/internal/httptrace: record errors in span tags while retrieving client IP #1346

Merged
merged 8 commits into from Jun 23, 2022

Conversation

Hellzy
Copy link
Contributor

@Hellzy Hellzy commented Jun 17, 2022

This change follows the RFC update: https://datadoghq.atlassian.net/wiki/spaces/APS/pages/2118779066/Client+IP+addresses+resolution.
In case multiple IP related headers are found in a request, we need to record information for the backend in the span tags.

Added tags:

  • http.request.headers.*
  • _dd.multiple-ip-headers

Changes:

  • Change the behaviour of getClientIP so that it builds the list of IP headers encountered while retrieving the client IP.
  • If the list compiled by getClientIP holds more than one element, the new tags are set with information from this list
  • genClientIPSpanTags abstracts this decision logic and returns a map of tag key/values to set for the span
  • Add an env var to disable client ip collection altogether

@Hellzy Hellzy added this to the 1.39.0 milestone Jun 17, 2022
@Hellzy Hellzy marked this pull request as ready for review June 17, 2022 15:14
@Hellzy Hellzy requested a review from a team June 17, 2022 15:14
contrib/internal/httptrace/httptrace.go Outdated Show resolved Hide resolved
contrib/internal/httptrace/httptrace.go Outdated Show resolved Hide resolved
@Hellzy Hellzy force-pushed the francois.mazeau/backend-waf-client-ip branch from e3ea201 to 6d1c0dc Compare June 20, 2022 15:25
@Hellzy Hellzy requested a review from knusbaum June 20, 2022 15:26
contrib/internal/httptrace/httptrace.go Outdated Show resolved Hide resolved
contrib/internal/httptrace/httptrace.go Outdated Show resolved Hide resolved
contrib/internal/httptrace/httptrace.go Outdated Show resolved Hide resolved
contrib/internal/httptrace/httptrace.go Outdated Show resolved Hide resolved
Copy link
Contributor

@knusbaum knusbaum left a comment

Choose a reason for hiding this comment

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

I think there's still a few corner cases here that give strange results and still think this can be greatly simplified.

I'm proposing something more along these lines:

// clientIPTags generates the client IP related tags. If there is exactly one, valid IP header, the
// IP will be set in the ext.HTTPClientIP tag. Otherwise, any IP headers will be added as tags with
// ext.HTTPRequestHeaders.<header> keys. If there are no IP headers, the request's RemoteAddr field
// will be used if valid.
func clientIPTags(r *http.Request) []ddtrace.StartSpanOption {
	ipHeaders := defaultIPHeaders
	if len(clientIPHeader) > 0 {
		ipHeaders = []string{clientIPHeader}
	}

	var ips []string
	var tags []ddtrace.StartSpanOption
	for _, hdr := range ipHeaders {
		if v := r.Header.Get(hdr); v != "" {
			ips = append(ips, v)
			tags = append(tags, tracer.Tag(ext.HTTPRequestHeaders+"."+hdr, v))
		}
	}

	if len(ips) == 0 {
		// Only use the server-detected remote address if the client didn't provide any ip headers.
		ips = append(ips, r.RemoteAddr)
	}
	if len(ips) == 1 {
		for _, s := range strings.Split(ips[0], ",") {
			ip := parseIP(strings.TrimSpace(s))
			if ip.IsValid() && isGlobal(ip) {
				return []ddtrace.StartSpanOption{tracer.Tag(ext.HTTPClientIP, ip.String())}
			}
		}
		return tags
	}
	return append(tags, tracer.Tag(ext.MultipleIPHeaders, strings.Join(ips, ",")))
}

I still question why we should return the ext.HTTPRequestHeaders list only when there is not a valid IP. Is the name of the header not important when the IP is valid?
Similarly, why do we handle r.RemoteAddr so differently? It can only ever appear in the ext.HTTPClientIP tag, but never anywhere else.

contrib/internal/httptrace/httptrace.go Outdated Show resolved Hide resolved
contrib/internal/httptrace/httptrace.go Outdated Show resolved Hide resolved
@knusbaum
Copy link
Contributor

@Hellzy My previous review was posted before I saw the new changes. I'll review the new changes once this is ready.

@Hellzy Hellzy force-pushed the francois.mazeau/backend-waf-client-ip branch from 5f70211 to 24e9d7b Compare June 22, 2022 20:19
@Hellzy Hellzy force-pushed the francois.mazeau/backend-waf-client-ip branch from 24e9d7b to 9c3ff4b Compare June 22, 2022 20:27
@knusbaum
Copy link
Contributor

Pending approval from @katiehockman

@Hellzy
Copy link
Contributor Author

Hellzy commented Jun 23, 2022

Pending approval from @katiehockman

Checked w/ Katie, merging.

@Hellzy Hellzy merged commit b297ad7 into main Jun 23, 2022
@Hellzy Hellzy deleted the francois.mazeau/backend-waf-client-ip branch June 23, 2022 13:41
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.

None yet

3 participants