Add `source.nat.ip` #7444

stuartnelson3 · 2022-03-02T17:03:19Z

Motivation/summary

as per the ECS team's recommendation, store the address provided in proxy headers forwarded, x-real-ip, x-forwarded-for under source.{ip,port} and client.{ip,port}; store the source ip addr in source.nat.ip

Checklist

Update CHANGELOG.asciidoc
Update package changelog.yml (only if changes to apmpackage have been made)
Documentation has been updated

How to test these changes

start apm-server
ingest events with an X-Forwarded-For header
confirm the document has source.nat.ip fields

Related issues

closes #7029

model.Source no longer equals model.Client, so you can't directly convert between them.

apmmachine · 2022-03-02T17:12:43Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Start Time: 2022-03-18T11:36:54.737+0000
Duration: 26 min 19 sec

Test stats 🧪

Test	Results
Failed	0
Passed	3926
Skipped	13
Total	3939

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.
/hey-apm : Run the hey-apm benchmark.
/package : Generate and publish the docker images.
/test windows : Build & tests on Windows.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

stuartnelson3 · 2022-03-02T17:16:01Z

Are there other places where I need to apply the request's metadata to the APMEvent, besides beater/otlp/clientmetadata.go and beater/api/mux.go?

model/modeldecoder/v2/decoder.go

simitt · 2022-03-08T13:30:30Z

Looking at https://github.com/elastic/ecs/blob/main/schemas/source.yml#L148 and #7029 (comment):

The ECS team recommend that we store the originating client's IP in source.ip and client.ip, and the network peer (proxy) IP in source.nat.ip. If there are no forwarded headers, then source.ip and client.ip will be the network peer IP, and there will be no source.nat.ip address. The naming is a bit odd, but it sounds like the best option we have at the moment.

My understanding from that is

request with Headers (forwarded, x-real-ip, x-forwarded-for), set the source.ip and client.ip to the IP address of these headers (in the defined order) and set source.nat.ip to the requests IP address, potentially expected to be pointing to a proxy.
request without Headers: source.ip and client.ip are set to the request's IP address, and the assumption is that no proxy was used in between, so leave the source.nat.ip empty.

I believe you summarized (and implemented) this the other way around. Please take another look at Andrew's comment and let me know if my understanding makes sense to you.

stuartnelson3 · 2022-03-08T15:25:23Z

@simitt thank you for the clarification!

stuartnelson3 · 2022-03-08T17:00:02Z

how do i run make check-approvals for systemtests/?

simitt · 2022-03-08T17:13:52Z

how do i run make check-approvals for systemtests/?

First run the systemtests and then run make check-approvals from root; if there are differences they should get picked up.

simitt

Looks good! (apart from the failing system tests)

axw

Please update doc comments & PR description (for commit message) before merging. Otherwise, also with the caveat of failing system test, LGTM. Thanks :)

axw · 2022-03-15T07:51:23Z

beater/interceptors/metadata.go

@@ -79,11 +81,15 @@ type ClientMetadataValues struct {
 	// SourceAddr holds the address of the (source) network peer, if known.
 	SourceAddr net.Addr

-	// ClientIP holds the IP address of the originating gRPC client, if known,
+	// SourceNATIP holds the IP address of the originating gRPC client, if known,


doc comments are still reversed

stuartnelson3 · 2022-03-17T07:20:35Z

@stuartnelson3 because systemtest has its own module, ./... in the repo root won't list it. You have to be in the systemtest directory.

@axw I'm running them from within the systemtest folder, but keep getting the error about not finding the apm package

~/w/a/systemtest (source-nat-ip %^)
# go test ./...
go: downloading github.com/docker/distribution v2.8.1+incompatible
2022/03/17 08:06:13 INFO: starting stack containers...
Pulling elasticsearch (docker.elastic.co/elasticsearch/elasticsearch:8.2.0-9bac538c-SNAPSHOT)...
8.2.0-9bac538c-SNAPSHOT: Pulling from elasticsearch/elasticsearch
Digest: sha256:096d3c0178276a9e4964a096efd6875dabd93391832474c90b864bddb21f39b5
Status: Downloaded newer image for docker.elastic.co/elasticsearch/elasticsearch:8.2.0-9bac538c-SNAPSHOT
Pulling kibana (docker.elastic.co/kibana/kibana:8.2.0-9bac538c-SNAPSHOT)...
8.2.0-9bac538c-SNAPSHOT: Pulling from kibana/kibana
Digest: sha256:2a94e73c164e4660e55ebeac348364f51403b277a3b92123fa136f5981ec7a8c
Status: Downloaded newer image for docker.elastic.co/kibana/kibana:8.2.0-9bac538c-SNAPSHOT
Pulling fleet-server (docker.elastic.co/beats/elastic-agent:8.2.0-9bac538c-SNAPSHOT)...
8.2.0-9bac538c-SNAPSHOT: Pulling from beats/elastic-agent
Digest: sha256:f019e4370810332ae13b3293572165a5826bb3b5efddbecdc76909e7aa59eefb
Status: Downloaded newer image for docker.elastic.co/beats/elastic-agent:8.2.0-9bac538c-SNAPSHOT
Recreating apm-server_elasticsearch_1 ... 
apm-server_package-registry_1 is up-to-date
Recreating apm-server_elasticsearch_1 ... done
Recreating apm-server_kibana_1        ... 
Recreating apm-server_kibana_1        ... done
Recreating apm-server_fleet-server_1  ... 
Recreating apm-server_fleet-server_1  ... done
2022/03/17 08:11:31 Waiting for fleet-server container (1da90ee5a622d28c4e7726ef67b2bd77a21a9d7c80b03e6060d0b083f943fcba) to become healthy
2022/03/17 08:11:51 INFO: cleaning up Elasticsearch...
2022/03/17 08:11:51 INFO: setting up fleet...
2022/03/17 08:11:54 could not find package 'apm'
FAIL	github.com/elastic/apm-server/systemtest	341.083s
ok  	github.com/elastic/apm-server/systemtest/apmservertest	84.052s
?   	github.com/elastic/apm-server/systemtest/benchtest	[no test files]
?   	github.com/elastic/apm-server/systemtest/cmd/apmbench	[no test files]
?   	github.com/elastic/apm-server/systemtest/cmd/runapm	[no test files]
?   	github.com/elastic/apm-server/systemtest/estest	[no test files]
?   	github.com/elastic/apm-server/systemtest/fleettest	[no test files]
FAIL

any ideas what I should check?

axw · 2022-03-17T08:56:34Z

@stuartnelson3 sorry, I just totally misread your error message before. I think you might need to run make build-package, and docker-compose restart package-registry.

stuartnelson3 · 2022-03-17T17:14:06Z

the systemtests are getting killed after 11min on my machine, i'll take another look into this tomorrow (or try running it on an aws vm)

cla-checker-service · 2022-03-17T17:56:05Z

💚 CLA has been signed

mergify · 2022-03-18T01:24:41Z

This pull request is now in conflicts. Could you fix it @stuartnelson3? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b source-nat-ip upstream/source-nat-ip
git merge upstream/main
git push upstream source-nat-ip

wasn't part of the original spec, and the ephemeral port was messing with systemtest approvals

apmmachine · 2022-03-18T11:37:15Z

💔 Build Failed

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Start Time: 2022-03-18T11:36:53.595+0000
Duration: 0 min 11 sec

Pipeline error

This error is likely related to the pipeline itself. Please go to the traditional console output here" You will see the error (either a wrong syntax or configuration)

…ging * upstream/main: ci: -SNAPSHOT suffix is required (elastic#7585) Add `source.nat.ip` (elastic#7444)

store the address provided in proxy headers forwarded, x-real-ip, x-forwarded-for under source.{ip,port} and client.{ip,port}; store the source ip addr in source.nat.ip (cherry picked from commit 27d244e) # Conflicts: # beater/api/mux.go # changelogs/head.asciidoc

axw · 2022-04-01T01:27:33Z

Verified with 8.2.0-BC1:

sent some RUM events with no special headers, confirmed the peer IP address was stored in source.ip and client.ip as-is, and no source.nat.ip address was recorded
sent some RUM events with X-Real-IP, confirmed the X-Real-IP address was stored in source.ip and client.ip, and the peer address was stored in source.nat.ip

mergify · 2022-04-01T05:41:21Z

This pull request does not have a backport label. Could you fix it @stuartnelson3? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

backport-7.x is the label to automatically backport to the 7.x branch.
backport-7./d is the label to automatically backport to the 7./d branch. /d is the digit

NOTE: backport-skip has been added to this pull request.

stuartnelson3 added 7 commits March 2, 2022 16:45

[grpc] add source.nat.ip

5b68e47

[request] add source.nat.{ip,port}

d9e7314

[apmpackage] add source.nat.{ip,port} ecs fields

369f2a3

add nat fields to apmevent.source

80a4336

[otlp] set source.nat.ip

ea16736

add question

2642ea1

[rummetadata] set source.nat.*

9d3f55e

stuartnelson3 added v8.2.0 backport-8.2 Automated backport with mergify labels Mar 2, 2022

stuartnelson3 requested a review from a team March 2, 2022 17:03

update tests

df78c54

model.Source no longer equals model.Client, so you can't directly convert between them.

stuartnelson3 commented Mar 2, 2022

View reviewed changes

model/modeldecoder/v2/decoder.go Outdated Show resolved Hide resolved

stuartnelson3 changed the title ~~Source nat ip~~ Add source.nat.{ip,port} Mar 2, 2022

stuartnelson3 added 3 commits March 4, 2022 12:26

uncomment code

f16ade0

ignore source.nat in metadata header

61499d7

Merge branch 'main' into source-nat-ip

5e6bb43

stuartnelson3 added 5 commits March 8, 2022 15:11

fix ip/port assignation logic

2a0db52

[rumv3] update metadata test

d41e18a

update approvals

4bed90f

update changelog

d0628ce

[apmpackage] update changelog

9e03be7

stuartnelson3 marked this pull request as ready for review March 8, 2022 15:25

stuartnelson3 added 2 commits March 8, 2022 16:49

make check-approvals

ca7deb8

only set nat if there are fields

9525040

simitt approved these changes Mar 15, 2022

View reviewed changes

axw reviewed Mar 15, 2022

View reviewed changes

simitt mentioned this pull request Mar 15, 2022

Introduce configuration to disable use of HTTP headers for identifying client.ip #6424

Open

Merge branch 'main' into source-nat-ip

daf005c

correct struct comments

ae9f04e

stuartnelson3 force-pushed the source-nat-ip branch from 3f5205f to ae9f04e Compare March 17, 2022 17:56

make check-approvals

ed7b76f

stuartnelson3 added 4 commits March 18, 2022 09:31

remove source.nat.port

96eb4e5

wasn't part of the original spec, and the ephemeral port was messing with systemtest approvals

Merge branch 'main' into source-nat-ip

145c495

errant commas

c315cee

Merge branch 'main' into source-nat-ip

ddf7029

stuartnelson3 changed the title ~~Add source.nat.{ip,port}~~ Add source.nat.ip Mar 18, 2022

stuartnelson3 merged commit 27d244e into elastic:main Mar 18, 2022

stuartnelson3 deleted the source-nat-ip branch March 18, 2022 12:08

v1v added a commit to v1v/apm-server that referenced this pull request Mar 18, 2022

Merge remote-tracking branch 'upstream/main' into feature/support-sta…

14ba7d5

…ging * upstream/main: ci: -SNAPSHOT suffix is required (elastic#7585) Add `source.nat.ip` (elastic#7444)

marclop added the test-plan label Mar 30, 2022

axw self-assigned this Mar 31, 2022

mergify bot mentioned this pull request Mar 31, 2022

[8.2] Add source.nat.ip (backport #7444) #7760

Closed

axw added test-plan-ok and removed backport-8.2 Automated backport with mergify labels Apr 1, 2022

mergify bot added the backport-skip Skip notification from the automated backport with mergify label Apr 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `source.nat.ip` #7444

Add `source.nat.ip` #7444

stuartnelson3 commented Mar 2, 2022 •

edited

Loading

apmmachine commented Mar 2, 2022 •

edited

Loading

Build stats

Test stats 🧪

stuartnelson3 commented Mar 2, 2022

simitt commented Mar 8, 2022

stuartnelson3 commented Mar 8, 2022

stuartnelson3 commented Mar 8, 2022

simitt commented Mar 8, 2022 •

edited

Loading

simitt left a comment •

edited

Loading

axw left a comment

axw Mar 15, 2022

stuartnelson3 commented Mar 17, 2022

axw commented Mar 17, 2022

stuartnelson3 commented Mar 17, 2022

cla-checker-service bot commented Mar 17, 2022 •

edited

Loading

mergify bot commented Mar 18, 2022

apmmachine commented Mar 18, 2022

Build stats

axw commented Apr 1, 2022

mergify bot commented Apr 1, 2022

Add source.nat.ip #7444

Add source.nat.ip #7444

Conversation

stuartnelson3 commented Mar 2, 2022 • edited Loading

Motivation/summary

Checklist

How to test these changes

Related issues

apmmachine commented Mar 2, 2022 • edited Loading

💚 Build Succeeded

Build stats

Test stats 🧪

🤖 GitHub comments

stuartnelson3 commented Mar 2, 2022

simitt commented Mar 8, 2022

stuartnelson3 commented Mar 8, 2022

stuartnelson3 commented Mar 8, 2022

simitt commented Mar 8, 2022 • edited Loading

simitt left a comment • edited Loading

Choose a reason for hiding this comment

axw left a comment

Choose a reason for hiding this comment

axw Mar 15, 2022

Choose a reason for hiding this comment

stuartnelson3 commented Mar 17, 2022

axw commented Mar 17, 2022

stuartnelson3 commented Mar 17, 2022

cla-checker-service bot commented Mar 17, 2022 • edited Loading

mergify bot commented Mar 18, 2022

apmmachine commented Mar 18, 2022

💔 Build Failed

Build stats

Pipeline error

axw commented Apr 1, 2022

mergify bot commented Apr 1, 2022

Add `source.nat.ip` #7444

Add `source.nat.ip` #7444

stuartnelson3 commented Mar 2, 2022 •

edited

Loading

apmmachine commented Mar 2, 2022 •

edited

Loading

simitt commented Mar 8, 2022 •

edited

Loading

simitt left a comment •

edited

Loading

cla-checker-service bot commented Mar 17, 2022 •

edited

Loading