-
Notifications
You must be signed in to change notification settings - Fork 5
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
source.remote_address well known attribute #111
Conversation
Signed-off-by: Eguzki Astiz Lezaun <eastizle@redhat.com>
Signed-off-by: Eguzki Astiz Lezaun <eastizle@redhat.com>
e3ce26c
to
7cdd953
Compare
Signed-off-by: Eguzki Astiz Lezaun <eastizle@redhat.com>
Signed-off-by: Eguzki Astiz Lezaun <eastizle@redhat.com>
Signed-off-by: Eguzki Astiz Lezaun <eastizle@redhat.com>
Signed-off-by: Eguzki Astiz Lezaun <eastizle@redhat.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, the changes look good, just going to go through verification
src/configuration.rs
Outdated
for action in &rule.actions { | ||
for datum in &action.data { | ||
let result = datum.item.compile(); | ||
if result.is_err() { | ||
return Err(result.err().unwrap()); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually one thing that's standing out to me is that we no longer keep the path as well as compiled version for PatternExpressions and don't pre-compile data items. I'm not 100%, does this conflict somewhat with the intention here cc @alexsnaps
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need to look into those changes properly, but that would also mean that we'd err out at evaluation time instead of at config parse time... that seems... not desirable 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed because I did not see any point in early compilation. The "compilation" is just about building the path out of the selector string, which can never fail. Hence, I removed.
Can you confirm my analysis?
Anyway, I am happy to add that back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doing it once, instead of on all invocations? Now we need to parse on every Policy::pattern_expression_applies
, right? And it will possibly fail when we move these to CEL expression as @adam-cattermole pointed out. But arguably that's my problem.
In any case, I'm a big proponent of doing things as early as possible, represent them in the proper way from there on in the system and do the least amount of work. This code was early parsing the "path" and keeping it in proper form to avoid paying the computation time on the data plane's path. These are my guiding principle in general, but more so here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for confirming.
Bringing that back
Signed-off-by: Eguzki Astiz Lezaun <eastizle@redhat.com>
Signed-off-by: Eguzki Astiz Lezaun <eastizle@redhat.com>
b2db161
to
2b6335f
Compare
Signed-off-by: Eguzki Astiz Lezaun <eastizle@redhat.com>
Comments addressed. Ready for review @adam-cattermole @alexsnaps |
Signed-off-by: Eguzki Astiz Lezaun <eastizle@redhat.com>
Signed-off-by: Eguzki Astiz Lezaun <eastizle@redhat.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes, verification worked great
What
This PR introduces a new Well Known Attribute called
source.remote_address
.Why
Envoy's
source.address
(well known) attribute includes the port asIP:PORT
. It is desirable to run rate limiting or auth policies relying only on the client address. Hence, thissource.remote_address
can be used to implement those policies.Attribute Value
This attribute evaluates to the
trusted client address
(IP address without port) as it is being defined by Envoy Doc.In summary, the
trusted client address
is calculated as follows: By default, Envoy gateway instances are configured to append the source IP address of the immediate downstream node’s connection of the incoming HTTP connections to theX-Forwarded-For
header. By default Envoy sets the number of trusted hops to 0,indicating that Envoy should use the address the connection is opened from,
rather than a value inside the X-Forwarded-For list.
Increasing this count will have Envoy use the
n
th value from the list, counting from the right.Where can I use it?
This attribute can be used on
selector
fields.As conditions:
As
SelectorItem
to generate descriptor entries.Fixes #97
Verification Steps
For the verification steps, this PR adds a new configuration to the existing local development environment. It's the new set of rules for
*.d.rlp.com
.And a new limit configuration
That configuration enables source based rate limiting on
*.d.rlp.com
subdomains, with only one "privileged" exception: IP "50.0.0.1" will not be rate limited.Port-forward envoy:
Alice (IP:
40.0.0.1
) has 2 requests per 10 seconds:Curl tool logs should show only two requests allowed for every 10 requests.
Bob (IP:
30.0.0.1
) has 2 requests per 10 seconds:Curl tool logs should show only two requests allowed for every 10 requests.
Charley (the Boss) with privileged IP
50.0.0.1
does not get rate limited.Curl tool logs should show all requests being accepted.