Skip to content

Add more http.client_ip cases#1401

Merged
smola merged 3 commits intomainfrom
smola/client-ip-cases
Sep 12, 2023
Merged

Add more http.client_ip cases#1401
smola merged 3 commits intomainfrom
smola/client-ip-cases

Conversation

@smola
Copy link
Copy Markdown
Member

@smola smola commented Jul 13, 2023 •
edited
Loading

Description

  • Check that client_ip and associated tags are in the root span (not any span).
  • Check presence of all possible headers (Fastly and CloudFlare headers are tested separately, since support is less consistent across languages).
  • Check correct resolution for all header names.

Motivation

Workflow

  1. ⚠️⚠️ Create your PR as draft
  2. Follow the style guidelines of this project (See how to easily lint the code)
  3. Work on you PR until the CI passes (if something not related to your task is failing, you can ignore it)
  4. Mark it as ready for review

Once your PR is reviewed, you can merge it! ❤️

Reviewer checklist

  • If this PR modifies anything else than strictly the default scenario, then add the run-all-scenarios label (more info).
  • CI is green
    • If not, failing jobs are not related to this change (and you are 100% sure about this statement)
  • if any of build-some-image label is present
    1. is the image labl have been updated ?
    2. just before merging, locally build and push the image to hub.docker.com

@smola smola mentioned this pull request Jul 13, 2023
Merged
@smola smola changed the title Add more client_ip cases Add more http.client_ip cases Jul 13, 2023
@smola smola force-pushed the smola/client-ip-cases branch from 823dc85 to c69bf0b Compare July 13, 2023 13:43
@smola smola requested review from a team and robertpi July 13, 2023 13:45
@smola smola force-pushed the smola/client-ip-cases branch from b7128ed to 7101a6d Compare July 13, 2023 14:01
@smola
Add more http.client_ip cases
a7faa04 
* Check that client_ip and associated tags are in the root span (not
  _any_ span).
* Check presence of all possible headers.
* Test all possible header names
@smola smola force-pushed the smola/client-ip-cases branch from bf91b94 to a7faa04 Compare July 13, 2023 15:22
@smola smola marked this pull request as ready for review July 13, 2023 15:44
@smola smola requested a review from a team as a code owner July 13, 2023 15:44
simon-id
simon-id reviewed Jul 17, 2023
View reviewed changes
Comment thread tests/test_standard_tags.py
Comment on lines +276 to +327
def test_client_ip_with_appsec_event_and_vendor_headers(self):
"""Test that meta tag are correctly filled when an appsec event is present and ASM is enabled, with vendor headers"""
Copy link
Copy Markdown
Member

@simon-id simon-id Jul 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

that was never specified was it ? so let's make sure this is intended

Copy link
Copy Markdown
Collaborator

@cbeauchesne cbeauchesne Jul 19, 2023

Choose a reason for hiding this comment

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

@smola do you have a RFC link for this ?

Copy link
Copy Markdown
Member Author

@smola smola Jul 31, 2023

Choose a reason for hiding this comment

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

@simon-id @cbeauchesne The link is at the class level. These vendor headers were added in a subsequent update

Copy link
Copy Markdown
Member

@simon-id simon-id Jul 31, 2023

Choose a reason for hiding this comment

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

I know we're supposed to use these new headers for sourcing the client IP, but i don't think it said anywhere that we should add them all as tags in http.request.headers.... that's a different list no ?

Copy link
Copy Markdown
Member Author

@smola smola Aug 7, 2023

Choose a reason for hiding this comment

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

I don't think there's a separate list? And I'm not sure what would be the rationale for that. cc @robertpi what do you think?

Copy link
Copy Markdown
Member Author

@smola smola Sep 8, 2023

Choose a reason for hiding this comment

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

@simon-id There was, indeed, a separate list, and it did not include the new IP headers. This was just probably an overlook, and after discussing with @robertpi, I have updated the spec to make the list of collected headers a superset of headers used for IP resolution.

@smola smola requested review from cbeauchesne and simon-id July 31, 2023 10:45
@cbeauchesne
Copy link
Copy Markdown
Collaborator

cbeauchesne commented Sep 6, 2023

@smola Any ambition to merge this PR? :)

@smola smola force-pushed the smola/client-ip-cases branch from a4d0420 to 1bb470c Compare September 8, 2023 07:55
@smola
Copy link
Copy Markdown
Member Author

smola commented Sep 8, 2023

@cbeauchesne Yep. I solved conflicts now. But I'm on hold as I resolve some offline question about the spec.

@smola smola merged commit 2d88e60 into main Sep 12, 2023
@smola smola deleted the smola/client-ip-cases branch September 12, 2023 08:33
@smola smola mentioned this pull request Sep 19, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cbeauchesne cbeauchesne cbeauchesne approved these changes

@robertpi robertpi Awaiting requested review from robertpi

@simon-id simon-id Awaiting requested review from simon-id

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@smola @cbeauchesne @simon-id