Added new operator to check IP within range #185
Conversation
Codecov Report
@@ Coverage Diff @@
## main #185 +/- ##
=========================================
Coverage 22.31% 22.31%
Complexity 75 75
=========================================
Files 68 68
Lines 1788 1788
Branches 54 54
=========================================
Hits 399 399
Misses 1380 1380
Partials 9 9
Flags with carried forward coverage won't be shown. Click here to find out more. 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| OPERATOR_EQUALS, | ||
| OPERATOR_MATCHES_REGEX; | ||
| OPERATOR_MATCHES_REGEX, | ||
| OPERATOR_CIDR; |
There was a problem hiding this comment.
Where the negative version of this operator? When I was discussing this with amod, we agreed that we need two operators, not sure if that got lost in translation. Looks like config side already got merged, so we probably need to break it to clean up name. Please keep me on the reviews for label rules in the future.
There was a problem hiding this comment.
Will we have a use case where if certain APIs don't fall under given CIDR, we will add another/counter label to them?
cc @skjindal93
There was a problem hiding this comment.
Sure, that kind of is the external/internal use case:
If attr.ip NOT_IN_IP_RANGE <internal ip range>, then mark external
The inverse of that (If attr.ip IN_IP_RANGE <internal ip range>, then mark internal) doesn't necessarily hold, since external APIs can be called internally too.
|
Just realised I have to work on converters for StringCondition values as well 😅 |
|
|
||
| String OPERATOR_KEY = "operator"; | ||
| String VALUE_KEY = "value"; | ||
| String STRING_CONDITION_VALUE_KEY = "stringConditionValue"; |
There was a problem hiding this comment.
Please revert this, it's a breaking change. Instead, accept a list of values as a sibling to existing value and remove the non null on value (still technically breaking, but less critical as it'd only break on trying to read a new value with an old query).
You may also want to include a nullable enum that specifies which field is set - your StringConditionValueType. It should always be set on return, but on request you would need to default it to the singular form for backwards compat. Hope that makes sense
|
|
||
| @GraphQLField | ||
| @GraphQLNonNull | ||
| @Nullable |
There was a problem hiding this comment.
Another alternative would be to have value be typed Object and accept either a string or list of strings in it. We're not consistent, we've taken both approaches in different places. Up to you
There was a problem hiding this comment.
If no harm in this approach, will keep it this way.
There was a problem hiding this comment.
no harm, the difference is how the callers would use it. In the read direction: whether they have to ask for 3 fields (value, values, type), then use type to determine which field should be read; or they ask for one field (value) and inspect the runtime value and use it accordingly. write direction: whether they have to set two fields (either value or values, then assigning type correctly depending on), or just assigning value with either one (in which case GQL now has to inspect it and determine which it is.
So the explicit is more verbose, but they both accomplish same thing.
| @JsonProperty(VALUE_KEY) | ||
| String value; | ||
|
|
||
| @JsonProperty(VALUES_KEY) |
There was a problem hiding this comment.
I guess it's pre-existing, but why did this class get implemented twice? Seems like a mistake here.
There was a problem hiding this comment.
private static class LabelApplicationRuleArgument is missing in one of them. Should I remove one of them, the one without LabelApplicationRuleArgument class?
There was a problem hiding this comment.
Sure. I'm guessing there's a misunderstanding on how deserialization works, but there's only one jackson mapper so no interface needs to be implemented twice
Description
Please include a summary of the change, motivation and context.
Testing
Please describe the tests that you ran to verify your changes. Please summarize what did you test and what needs to be tested e.g. deployed and tested helm chart locally.
Checklist:
Documentation
Make sure that you have documented corresponding changes in this repository or hypertrace docs repo if required.