change: make -error-on-replace more cooperating
#233
Conversation
With this change, the proxy wouldn't necessary return an error if
`-error-on-replace` is set and the incoming query has a label matcher
overlapping with the enforced label.
For instance when the enforced label is `namespace="foo"`, the following
expressions aren't rejected anymore:
- `up{namespace!="bar"}`
- `up{namespace=~"foo|bar"}`
- `up{namespace!~"bar"}`
The following expressions are rejected as before:
- `up{namespace=""}`
- `up{namespace="bar"}`
- `up{namespace=~"bar|fred"}`
- `up{namespace!~"foo"}`
Signed-off-by: Simon Pasquier <spasquie@redhat.com>
simonpasquier
force-pushed
the
enhance-error-on-replace
branch
from
2d50937
to
6a6cd98
Compare
|
@squat kindly asking for a review here. I added as many unit tests as possible to cover the different possibilities but I understand that the change isn't so easy to audit. |
injectproxy/enforce.go
Outdated
| // enforced matchers can return some result. If the combined | ||
| // matchers return no result, the function returns an error. | ||
| // | ||
| // For instance, when te enforced matcher is 'tenant="bar"': |
There was a problem hiding this comment.
| // For instance, when te enforced matcher is 'tenant="bar"': | |
| // For instance, when the enforced matcher is 'tenant="bar"': |
| true, | ||
| }, | ||
|
|
||
| // Not equal label selector in the expression. |
There was a problem hiding this comment.
there's some weird edge cases here that i'm not confident in because it depends so much on how the proxy's plumbing handles duplicate values. It's somewhat degenerate but I think it's possible but for an enforced matcher like foo|foo to be created. This would be disjoint with selectors like job!="foo" and we should return errors in those cases since we are replacing things, right?
In other words, just because the matcher labels.MatchRegexp and the selector is labels.MatchNotEqual doesn't mean ok. WDYT?
| false, | ||
| }, | ||
| { | ||
| `job=~"fred"`, |
There was a problem hiding this comment.
these are disjoint, no? so why don't we error here?
There was a problem hiding this comment.
Thanks for the verbosity @simonpasquier. Overall I think it's pretty easy to follow. I have a couple of questions that have to do with the correctness of some weird cases, but otherwise it looks good I think.
Signed-off-by: Simon Pasquier <spasquie@redhat.com>
With this change, the proxy wouldn't necessary return an error if
-error-on-replaceis set and the incoming query has a label matcher overlapping with the enforced label.For instance when the enforced label is
namespace="foo", the following expressions aren't rejected anymore:up{namespace!="bar"}up{namespace=~"foo|bar"}up{namespace!~"bar"}The following expressions are rejected as before:
up{namespace=""}up{namespace="bar"}up{namespace=~"bar|fred"}up{namespace!~"foo"}