namespace_labels matching for the sink scope mechanism #1390
Conversation
RobertSzefler
force-pushed
the
feature/filter_alerts_namespace_labels
branch
3 times, most recently
from
efdd301
to
60e4730
Compare
RobertSzefler
force-pushed
the
feature/filter_alerts_namespace_labels
branch
2 times, most recently
from
12fe368
to
495f5fa
Compare
scripts/generate_kubernetes_code.py
Outdated
| @@ -376,6 +378,13 @@ def autogenerate_triggers(f: TextIO): | |||
| for resource in KUBERNETES_RESOURCES: | |||
| f.write(f"# {resource} Triggers\n") | |||
| for trigger_name, operation_type in sorted(TRIGGER_TYPES.items()): | |||
| if f"{resource}{get_trigger_class_name(trigger_name)}" in [ | |||
There was a problem hiding this comment.
As was discussed previously, only the event types listed here should have change_filters set to DEFAULT_CHANGE_FILTERS, the reason being change tracking for the other events makes no sense, right?
I think this change was previously made to the code under src/robusta/integrations/kubernetes/autogenerated/, and all the changes in generate_kubernetes_code.py in this PR are introduced just to make sure the autogenerated code after running the script is exactly the same as the code in the repo.
There was a problem hiding this comment.
As discussed, my reasoning here was actually wrong and all of these classes should have change_filters default set to None, which has been fixed.
| try: | ||
| return {ns.metadata.name: ns.metadata for ns in core_v1.list_namespace().items} | ||
| except ApiException: | ||
| logging.error("failed to list namespaces") |
There was a problem hiding this comment.
please log the stack trace
src/robusta/utils/scope.py
Outdated
| return False | ||
| attr_value = data[attr_name] | ||
| for attr_matcher in attr_matchers: | ||
| if attr_name == "attributes": | ||
| return self.scope_match_attributes(attr_matcher, attr_value) | ||
| if hasattr(self, f"match_{attr_name}"): |
There was a problem hiding this comment.
please leave it more explicit
I think it's best to load the namespace labels only here, of there's actually a matcher that uses it
It will be used on a fraction of the users only
There was a problem hiding this comment.
That way we would be accumulating abstract methods in the base class, each of which is implemented in some random derived class. Are you sure we want to go that way? The mechanism reintroduced here, match_{whatever} IMO elegantly implements the matching of attributes that is not explicitly implemented in the base class, basing on attribute name, so that a derived class can just implement match_something without the base class knowing about it (and without the need to touch the base class code at all).
There was a problem hiding this comment.
Okay, for reasons of clarity changed the scope_match_* methods/calls back to be explicit.
| @@ -169,7 +173,12 @@ def matches(self, match_requirements: Dict[str, Union[str, List[str]]], scope_re | |||
| # 1. "scope" check | |||
| accept = True | |||
| if scope_requirements is not None: | |||
| matcher = FilterableScopeMatcher(self.attribute_map) | |||
| data = self.attribute_map | |||
There was a problem hiding this comment.
I think it would be best to load and cache the namespaces, only if there's a matcher that uses it
There was a problem hiding this comment.
as discussed, always listing the namespaces does not incur an unacceptable time penalty, so we leave it
RobertSzefler
force-pushed
the
feature/filter_alerts_namespace_labels
branch
from
495f5fa
to
8bcf1f3
Compare
RobertSzefler
force-pushed
the
feature/filter_alerts_namespace_labels
branch
from
84de915
to
f5003d1
Compare
RobertSzefler
force-pushed
the
feature/filter_alerts_namespace_labels
branch
from
9773eee
to
a630c8f
Compare
Implements the matching of labels assigned to the namespace for the sink scope mechanism.
Includes some fixes in the k8s trigger autogeneration code.