Skip to content
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.

Sign up for GitHub

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

Jump to bottom

k8s server triggers filtering #1351

Merged
merged 1 commit into from Apr 1, 2024
Merged

k8s server triggers filtering #1351

merged 1 commit into from Apr 1, 2024

Conversation

RobertSzefler
Copy link
Contributor

@RobertSzefler RobertSzefler commented Mar 19, 2024 •
edited

Implements scope matching for k8s server triggers.

@RobertSzefler RobertSzefler force-pushed the feature/main-1416/enhanced-k8s-api-server-triggers-filtering branch 9 times, most recently from cd98bce to ba1abd0 Compare March 24, 2024 10:11
@RobertSzefler RobertSzefler marked this pull request as ready for review March 24, 2024 11:11
@RobertSzefler RobertSzefler force-pushed the feature/main-1416/enhanced-k8s-api-server-triggers-filtering branch from 7733afd to 121c6d8 Compare March 25, 2024 09:12
@RobertSzefler RobertSzefler mentioned this pull request Mar 25, 2024
Merged
@RobertSzefler RobertSzefler force-pushed the feature/main-1416/enhanced-k8s-api-server-triggers-filtering branch 10 times, most recently from 2cb6bea to 619b0aa Compare March 28, 2024 15:44
arikalon1
arikalon1 reviewed Mar 28, 2024
View reviewed changes
Copy link
Contributor

@arikalon1 arikalon1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice work

left quite a few comments, as this is a complex feature

scripts/generate_kubernetes_code.py Outdated
@@ -368,13 +372,16 @@ def autogenerate_triggers(f: TextIO):
textwrap.dedent(
f"""\
class {resource}{get_trigger_class_name(trigger_name)}Trigger(K8sBaseTrigger):
def __init__(self, name_prefix: str = None, namespace_prefix: str = None, labels_selector: str = None):
def __init__(
self, name_prefix: str = None, namespace_prefix: str = None, labels_selector: str = None, scope: Optional[ScopeParamsAsDict] = None
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the wrong type
there are redundant wrappers
I think it should be: Optional[ScopeParams]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

src/robusta/integrations/kubernetes/base_triggers.py Outdated
@@ -93,8 +103,80 @@ def should_fire(self, event: TriggerEvent, playbook_id: str, build_context: Dict
if label_value != obj_labels.get(label_key, ""):
return False

if self.scope:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the logic here is wrong
same bug we had on scopes matchers

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

src/robusta/integrations/kubernetes/base_triggers.py Outdated
return True

def attribute_map(self, aux_obj=None) -> Dict[str, Union[str, Dict[str, str]]]:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't get this aux_obj

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed in favor of get_scope_matching_data and refactored later to just be a data attribute on BaseScopeMatcher-derived classes.

src/robusta/utils/scope.py Outdated
ScopeParamsAsDict = Dict[str, Optional[List[ScopeIncludeExcludeParams]]]


class BaseScopeMatching(ABC):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this
IMO this should be a utility class

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactored into a utility class.

src/robusta/utils/scope.py Outdated
return False
return True

def scope_attribute_matches(self, attr_name: str, attr_matchers: List[str], aux_obj):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this code at all

src/robusta/utils/scope.py Outdated
def scope_inc_exc_matches(self, scope_inc_exc: Optional[list]):
return any(self.scope_matches(scope) for scope in scope_inc_exc)

def scope_matches(self, scope: Dict[str, List[str]]):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

type here is ScopeIncludeExcludeParams ?
please add return type

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added

src/robusta/utils/scope.py Outdated
return False
return True

def scope_attribute_matches(self, attr_name: str, attr_matchers: List[str]):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add return type

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added

src/robusta/utils/scope.py Outdated
return False
attr_value = data[attr_name]
for attr_matcher in attr_matchers:
if hasattr(self, f"scope_match_{attr_name}"):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not clear
I would do:

if attr_name == "attributes":
  return self.match_path_attributes(...)

Then, in match_path_attributes, for each attribute path, run get_attribute_by_path, and do the matching logic here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

src/robusta/utils/scope.py Outdated
return True
return False

def match_labels_annotations(self, labels_match_expr: str, labels: Dict[str, str]):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return type

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

src/robusta/utils/scope.py Outdated
return False
return True

def label_matches(self, label_match: str, labels: Dict[str, str]):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return type

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@RobertSzefler RobertSzefler force-pushed the feature/main-1416/enhanced-k8s-api-server-triggers-filtering branch 6 times, most recently from 49ee6b2 to 1e4c06e Compare April 1, 2024 09:09
@RobertSzefler RobertSzefler force-pushed the feature/main-1416/enhanced-k8s-api-server-triggers-filtering branch from 1e4c06e to 67d154b Compare April 1, 2024 09:11
arikalon1
arikalon1 approved these changes Apr 1, 2024
View reviewed changes
Copy link
Contributor

@arikalon1 arikalon1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice work

@arikalon1 arikalon1 merged commit c496509 into master Apr 1, 2024
16 checks passed
@arikalon1 arikalon1 deleted the feature/main-1416/enhanced-k8s-api-server-triggers-filtering branch April 1, 2024 09:30
This was linked to issues Apr 5, 2024
Support filtering on more fields in events of type 'Normal' #1219
Closed
Additional filter options for on_event triggers #1206
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arikalon1 arikalon1 arikalon1 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support filtering on more fields in events of type 'Normal' Additional filter options for on_event triggers
2 participants
@RobertSzefler @arikalon1