ip tagging: filter skeleton by junr03 · Pull Request #998 · envoyproxy/envoy

junr03 · 2017-05-19T19:19:38Z

Here is the skeleton for the ip tagging filter. Will write tests, and actual filter code once we have the trie.

ccaraman · 2017-05-19T19:48:58Z

+#include "common/http/filter/ip_tagging_filter.h"
+#include "common/json/config_schemas.h"
+
+namespace Envoy {


You can still write tests for loading the config under https://github.com/lyft/envoy/blob/master/test/server/config/http/config_test.cc

ccaraman · 2017-05-19T19:51:03Z

@junr03 I'll take a look later today.

ccaraman · 2017-05-22T23:37:42Z

+  static IpTaggingFilterStats generateStats(const std::string& prefix, Stats::Store& store);
+
+  //Trie ip_tags_;
+  Runtime::Loader& runtime_;


Runtime isn't needed for this filter at the moment.

ccaraman · 2017-05-22T23:38:40Z

+        "type" : "string",
+        "enum" : ["internal", "external", "both"]
+      },
+      "ip_tags" : {


ip_tags is supposed to be an array of ip_tag_name and ip_lists.

ccaraman · 2017-05-22T23:40:57Z

+            "items" : {
+              "type": "string",
+              "oneOf": [
+                { "format": "host-name" },


We don't have plans to support host-name for ip tagging.

In fact I think we need to change this to a regex match, because as far as I can tell there is no pre determined format for a CIDR range. And what this is is actually an array of cidr ranges, isn't it?

ccaraman · 2017-05-22T23:45:41Z

+/**
+ * All stats for the ip tagging filter. @see stats_macros.h
+ */
+// clang-format off


At the moment, there are no specific IP Tag stats. This can be removed for now.

ccaraman · 2017-05-24T18:41:38Z

+              "items" : {
+                "type": "string",
+                "oneOf": [
+                  { "pattern" : "^([0-9]{1,3}\.){3}[0-9]{1,3}(\/([0-9]|[1-2][0-9]|3[0-2]))?$" },


Why use these patterns instead of ipv6 and ipv4 like you had before?

from the comment above: In fact I think we need to change this to a regex match, because as far as I can tell there is no pre determined format for a CIDR range. And what this is is actually an array of cidr ranges, isn't it?

I stand corrected. The json schema format rule includes the range.

ccaraman · 2017-05-27T18:09:54Z

+extra data. This is useful to prevent against DDoS.
+
+**Note**: this filter is under active development, and currently does not perform any tagging on requests. In other
+words, installing this filter is a nop in the filter chain.


nit: no-op or no op

ccaraman · 2017-05-27T18:20:01Z

+  }
+
+request_type:
+  (optional, string) The type of requests the filter should apply to. ``external``, ``internal``, or ``both``.


Can you expand the about what is deemed internal/external? ie take it from the rl filter.

RomanDzhabarov · 2017-05-30T03:22:18Z

+  Defaults to ``both``.
+
+ip_tags:
+  (optional, array) Specifies the list of ip tags to set for a request.


(optional, array) + in other places.

RomanDzhabarov · 2017-05-30T03:22:29Z

+
+ip_list:
+  (required, list of strings) A list of IP address and subnet masks that will be tagged with the ``ip_tag_name``. Both
+  IPv4, and IPv6 CIDR addresses are allowed here.


no need for comma

RomanDzhabarov · 2017-05-30T03:23:25Z

+  }
+
+ip_tag_name:
+  (required, string) Specifies the ip tag name to apply. The names will be added to the request's ``x-envoy-ip-tag``


put some docs in place for the HTTP header "x-envoy-ip-tag"

RomanDzhabarov · 2017-05-30T03:24:05Z

+
+ip_tag_name:
+  (required, string) Specifies the ip tag name to apply. The names will be added to the request's ``x-envoy-ip-tag``
+  header if the request's XFF address belongs to a range present in the ``ip_list``


XFF can contain multiple addresses, can you put more details around if we look at the last hop address or not.

RomanDzhabarov · 2017-05-30T03:34:39Z

+namespace Server {
+namespace Configuration {
+
+HttpFilterFactoryCb IpTaggingFilterConfig::tryCreateFilterFactory(HttpFilterType type,


can you refactor this to use a new method for filter registration, see 3a466c3#diff-12a4439b6c76df424840b91ab1cf3789R14

RomanDzhabarov · 2017-05-30T20:47:08Z

+std::string IpTaggingFilterConfig::name() { return "ip_tagging"; }
+
+/**
+ * Static registration for the ip tagging filter. @see RegisterHttpFilterConfigFactory.


RegisterHttpFilterConfigFactory is not correct ;)

RomanDzhabarov · 2017-05-30T21:21:17Z

+              "items" : {
+                "type": "string",
+                "anyOf": [
+                  { "format" : "ipv4" },


can you add a test when format does not conform ipv4/ipv6?

on adding this test it looks like we are not actually enforcing this format. I verified with tests that exercise this https://github.com/lyft/envoy/blob/master/source/common/json/config_schemas.cc#L79. The schema validator lets the wrong string through, and it is envoy code that catches it. Specifically: https://github.com/lyft/envoy/blob/master/source/common/network/utility.cc#L28

RomanDzhabarov · 2017-05-30T21:22:30Z

+  Json::ObjectSharedPtr json_config = Json::Factory::loadFromString(json_string);
+  NiceMock<MockInstance> server;
+  IpTaggingFilterConfig factory;
+  EXPECT_THROW(factory.tryCreateFilterFactory(HttpFilterType::Decoder, "ip_tagging", *json_config,


createFilterFactory ?

RomanDzhabarov · 2017-06-01T18:39:33Z

+Ip tagging filter
+====================
+
+This is a filter which enables Envoy to tag requests with extra information such as location, cloud source and any


is a HTTP filter

RomanDzhabarov · 2017-06-01T18:40:10Z

+Ip tagging filter
+====================
+
+This is a filter which enables Envoy to tag requests with extra information such as location, cloud source and any


oxford comma?

ccaraman

LGTM

junr03 · 2017-06-01T20:54:13Z

@htuch this is just the skeleton for the forthcoming IP Tagging Filter described in #1001. We are going to add the Trie and functionality in subsequent PRs

htuch · 2017-06-02T13:50:14Z

@junr03 Sure. Can you keep in mind my request in #1001 that we make the trie generic enough to be useful in the Listener prefix range matching when we do LDS? Thanks.

mattklein123 · 2017-06-13T02:37:20Z

@junr03 @ccaraman even though this is not used yet and we are going to pause work for a little bit, can one of you please get coverage on the IP tagging filter files up to 100%. Thanks. :)

junr03 · 2017-06-13T16:46:53Z

@mattklein123 @ccaraman I can take care of that.

Description: This job seems to get cancelled frequently at the 45-minute mark, which just means another 45-minute cycle to run it again. Increase to 60 to hopefully get it to complete consistently. Signed-off-by: Mike Schore <mike.schore@gmail.com> Signed-off-by: JP Simard <jp@jpsim.com>

**Description** This removes the input api schema field in the filter config as it shouldn't be a config and currently it is not really used. The input schema is already determined by the specific API endpoint being used by the Envoy AI Gateway, such as the unified OpenAI API at `v1/chat/completions` or later the Anthropic Messages API we are supporting at `anthropic/v1/messages`. This removes the confusion when we are going to support other vendor pass through APIs like Anthropic. We may need to consider deprecating the input `APISchema` defined on `AIGatewayRoute` which is also not really used. Related Issue: #847 --------- Signed-off-by: Dan Sun <dsun20@bloomberg.net> Co-authored-by: Takeshi Yoneda <t.y.mathetake@gmail.com>

**Description** This removes the unused "schema" API from AIGatewayRoute as a preparation for transparent passthrough etc support for non-OpenAI SDK support as well as the prefix configuration #1002 **Related Issues/PRs (if applicable)** Follow up on #998 Necessary for #1002 --------- Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>

ip tagging: filter skeleton

689af40

ccaraman reviewed May 19, 2017

View reviewed changes

RomanDzhabarov assigned ccaraman May 22, 2017

ccaraman reviewed May 22, 2017

View reviewed changes

comments

61a794d

ccaraman reviewed May 24, 2017

View reviewed changes

Jose Nino added 10 commits May 24, 2017 13:58

correction

3e94d5d

some testing

70e473d

format

3c73c73

Merge branch 'master' into ip-tagging-filter

7176dc1

docs

db04ac6

fix

232ab06

remove baggage

133780f

nits

f3d3da2

format

291bb69

fix

8f6de66

ccaraman reviewed May 27, 2017

View reviewed changes

RomanDzhabarov reviewed May 30, 2017

View reviewed changes

Jose Nino added 3 commits May 30, 2017 12:23

address comments

3b96d32

Merge branch 'master' into ip-tagging-filter

2cef334

new filter registration

b0ed632

RomanDzhabarov reviewed May 30, 2017

View reviewed changes

Jose Nino added 2 commits May 30, 2017 13:49

comment

0843393

comment

bbb539d

RomanDzhabarov reviewed May 30, 2017

View reviewed changes

Jose Nino added 4 commits May 31, 2017 16:33

update

588c59c

update

970e112

fix

476c46a

fix

c43598b

RomanDzhabarov reviewed Jun 1, 2017

View reviewed changes

ccaraman previously approved these changes Jun 1, 2017

View reviewed changes

junr03 dismissed ccaraman’s stale review via 27900bc June 1, 2017 18:54

nit

27900bc

RomanDzhabarov approved these changes Jun 2, 2017

View reviewed changes

htuch approved these changes Jun 2, 2017

View reviewed changes

htuch merged commit a75dced into master Jun 2, 2017

htuch deleted the ip-tagging-filter branch June 2, 2017 13:54

mattklein123 unassigned ccaraman Sep 14, 2017

Conversation

junr03 commented May 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ccaraman commented May 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ccaraman left a comment

Choose a reason for hiding this comment

Uh oh!

junr03 commented Jun 1, 2017

Uh oh!

htuch commented Jun 2, 2017

Uh oh!

mattklein123 commented Jun 13, 2017

Uh oh!

junr03 commented Jun 13, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants