Geoip filter #24318

nezdolik · 2022-12-02T15:30:33Z

This change introduces IP Geolocation filter. Implementation according to design doc.

Commit Message: IP Geolocation filter
Additional Description:
Risk Level: Low (new extension)
Testing: unit tests
Docs Changes: Done
Release Notes: tbd
Platform Specific Features: NA
Fixes moby/moby#23736

Signed-off-by: Kateryna Nezdolii <[email protected]>

repokitteh-read-only · 2022-12-02T15:30:37Z

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #24318 was opened by nezdolik.

see: more, trace.

repokitteh-read-only · 2022-12-02T15:30:45Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @markdroth
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #24318 was opened by nezdolik.

see: more, trace.

nezdolik · 2023-04-06T21:16:03Z

tsan ci failure seems to be unrelated, will merge main and check if it helps

Signed-off-by: Kateryna Nezdolii <[email protected]>

nezdolik · 2023-04-11T13:08:56Z

@ravenblackx ci is fixed now. Think I have addressed all review comments, but please lmk if smth is missing.

ravenblackx · 2023-04-11T15:46:32Z

CODEOWNERS

@@ -291,6 +291,8 @@ extensions/filters/http/oauth2 @derekargueta @snowp
 /*/extensions/load_balancing_policies/round_robin @wbpcode @UNOWNED
 # Early header mutation
 /*/extensions/http/early_header_mutation/header_mutation @wbpcode @UNOWNED
+# IP Geolocation
+/*/extensions/filters/http/geoip @nezdolik @mattklein123


I guess. It's not really my field, but it would make sense since having reviewed some of it I now have the best context to review the rest of it.

Signed-off-by: Kateryna Nezdolii <[email protected]>

nezdolik · 2023-04-12T11:55:02Z

cc @sc0ttbeardsley i have started working on plugging the maxmind geoip provider for this filter in a separate change. If your company uses some other geolocation provider, you will need to submit a patch for it.

nezdolik · 2023-04-12T11:58:49Z

@mattklein123 could you please take the final look? :)

mattklein123

Thanks a few high level API comments. Can you also add a release note? Or do we want to wait on that until the maxmind provider is added per my other comment?

/wait

mattklein123 · 2023-04-12T17:44:43Z

api/envoy/extensions/filters/http/geoip/v3/geoip.proto

+  GeolocationHeadersToAdd geo_headers_to_add = 3 [(validate.rules).message = {required: true}];
+
+  // Geolocation provider specific configuration.
+  GeolocationProvider provider = 4 [(validate.rules).message = {required: true}];


This should use TypedExtensionConfig I think?

Yeap, did not know about that core message TypedExtensionConfig

mattklein123 · 2023-04-12T17:45:38Z

api/envoy/extensions/filters/http/geoip/v3/geoip.proto

+  // If set to true, the `xff_num_trusted_hops` field will be used to determine
+  // trusted client address from `x-forwarded-for` header.
+  // Otherwise, the immediate downstream connection source address will be used.
+  bool use_xff = 1;
+
+  // The number of additional ingress proxy hops from the right side of the
+  // :ref:`config_http_conn_man_headers_x-forwarded-for` HTTP header to trust when
+  // determining the origin client's IP address. The default is zero if this option
+  // is not specified. See the documentation for
+  // :ref:`config_http_conn_man_headers_x-forwarded-for` for more information.
+  uint32 xff_num_trusted_hops = 2;


I would refactor this into an XffConfig message, with the presence of the message indicating that XFF should be consulted, and the num_trusted_hops inside of it. This will be a bit clearer from an API perspective.

mattklein123 · 2023-04-12T17:46:37Z

api/envoy/extensions/filters/http/geoip/v3/geoip.proto

+    // If set, the IP address will be checked if it belongs to any type of anonymization network (e.g. VPN, public proxy etc)
+    // and header will be populated with the check result.


Can you specify the concrete values? Is it true/false? Something else? Same for all of the ones that aren't obviously strings.

mattklein123 · 2023-04-12T17:48:29Z

docs/root/configuration/http/http_filters/geoip_filter.rst

+    provider:
+        name: "envoy.geoip_providers.maxmind"


This doesn't actually exist. Is this going to be added in some future PR?

That's correct. I have initially suggested to split this work into implementing filter itself and then implementing the provider.

Signed-off-by: Kateryna Nezdolii <[email protected]>

nezdolik · 2023-04-17T09:24:17Z

/retest

repokitteh-read-only · 2023-04-17T09:24:24Z

Retrying Azure Pipelines:
Check envoy-presubmit isn't fully completed, but will still attempt retrying.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #24318 (comment) was created by @nezdolik.

see: more, trace.

phlax · 2023-04-17T09:28:22Z

not sure what is causing the docker fail - but ticket is here moby/moby#26634 - ive reported upstream also moby/buildkit#3801

nezdolik · 2023-04-17T10:04:53Z

@mattklein123 regarding release note, i think is better to wait until provider is implemented.

mattklein123

Thanks!

Kateryna Nezdolii added 2 commits December 2, 2022 10:43

Add geoip filter

e068e00

Signed-off-by: Kateryna Nezdolii <[email protected]>

Add api protos

4177cf8

Signed-off-by: Kateryna Nezdolii <[email protected]>

repokitteh-read-only bot added the api label Dec 2, 2022

repokitteh-read-only bot assigned markdroth Dec 2, 2022

nezdolik force-pushed the geoip branch 3 times, most recently from 1b13a11 to c1cb979 Compare December 6, 2022 23:17

nezdolik force-pushed the geoip branch 18 times, most recently from 569c360 to c162dd4 Compare December 22, 2022 15:16

nezdolik force-pushed the geoip branch 3 times, most recently from ad8bf60 to 1e9846b Compare December 27, 2022 18:48

repokitteh-read-only bot removed the waiting label Apr 6, 2023

Merge remote-tracking branch 'origin/main'

c012d13

Signed-off-by: Kateryna Nezdolii <[email protected]>

ravenblackx added the waiting label Apr 7, 2023

ravenblackx previously approved these changes Apr 11, 2023

View reviewed changes

Update codeowners

1cc4676

Signed-off-by: Kateryna Nezdolii <[email protected]>

nezdolik dismissed ravenblackx’s stale review via 1cc4676 April 11, 2023 18:08

repokitteh-read-only bot removed the waiting label Apr 11, 2023

ravenblackx previously approved these changes Apr 11, 2023

View reviewed changes

mattklein123 self-assigned this Apr 12, 2023

mattklein123 requested changes Apr 12, 2023

View reviewed changes

repokitteh-read-only bot added the waiting label Apr 12, 2023

Apply review comments

55b2de9

Signed-off-by: Kateryna Nezdolii <[email protected]>

nezdolik dismissed ravenblackx’s stale review via 55b2de9 April 14, 2023 21:27

repokitteh-read-only bot removed the waiting label Apr 14, 2023

Fix api field ref

4d40985

Signed-off-by: Kateryna Nezdolii <[email protected]>

mattklein123 approved these changes Apr 18, 2023

View reviewed changes

repokitteh-read-only bot removed the api label Apr 18, 2023

mattklein123 merged commit 46aecfc into envoyproxy:main Apr 18, 2023

nezdolik deleted the geoip branch April 18, 2023 20:58

daum3ns mentioned this pull request Jun 1, 2023

IP Geolocation Filter #23736

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Geoip filter #24318

Geoip filter #24318

nezdolik commented Dec 2, 2022 •

edited

Loading

repokitteh-read-only bot commented Dec 2, 2022

repokitteh-read-only bot commented Dec 2, 2022

nezdolik commented Apr 6, 2023 •

edited

Loading

nezdolik commented Apr 11, 2023

ravenblackx Apr 11, 2023

nezdolik commented Apr 12, 2023 •

edited

Loading

nezdolik commented Apr 12, 2023

mattklein123 left a comment

mattklein123 Apr 12, 2023

nezdolik Apr 13, 2023

mattklein123 Apr 12, 2023

mattklein123 Apr 12, 2023

mattklein123 Apr 12, 2023

nezdolik Apr 13, 2023

nezdolik commented Apr 17, 2023

repokitteh-read-only bot commented Apr 17, 2023

phlax commented Apr 17, 2023

nezdolik commented Apr 17, 2023

mattklein123 left a comment

		// If set, the IP address will be checked if it belongs to any type of anonymization network (e.g. VPN, public proxy etc)
		// and header will be populated with the check result.

Geoip filter #24318

Geoip filter #24318

Conversation

nezdolik commented Dec 2, 2022 • edited Loading

repokitteh-read-only bot commented Dec 2, 2022

repokitteh-read-only bot commented Dec 2, 2022

nezdolik commented Apr 6, 2023 • edited Loading

nezdolik commented Apr 11, 2023

ravenblackx Apr 11, 2023

Choose a reason for hiding this comment

nezdolik commented Apr 12, 2023 • edited Loading

nezdolik commented Apr 12, 2023

mattklein123 left a comment

Choose a reason for hiding this comment

mattklein123 Apr 12, 2023

Choose a reason for hiding this comment

nezdolik Apr 13, 2023

Choose a reason for hiding this comment

mattklein123 Apr 12, 2023

Choose a reason for hiding this comment

mattklein123 Apr 12, 2023

Choose a reason for hiding this comment

mattklein123 Apr 12, 2023

Choose a reason for hiding this comment

nezdolik Apr 13, 2023

Choose a reason for hiding this comment

nezdolik commented Apr 17, 2023

repokitteh-read-only bot commented Apr 17, 2023

phlax commented Apr 17, 2023

nezdolik commented Apr 17, 2023

mattklein123 left a comment

Choose a reason for hiding this comment

nezdolik commented Dec 2, 2022 •

edited

Loading

nezdolik commented Apr 6, 2023 •

edited

Loading

nezdolik commented Apr 12, 2023 •

edited

Loading