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.

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

Launcher wrapper layer and the validator abstraction #611

Closed
2 tasks
jeniawhite opened this issue Dec 27, 2022 · 3 comments
Closed
2 tasks

Launcher wrapper layer and the validator abstraction #611

jeniawhite opened this issue Dec 27, 2022 · 3 comments
Labels
Team:Cloud Security Cloud Security team related technical debt

Comments

@jeniawhite
Copy link
Contributor

jeniawhite commented Dec 27, 2022

Motivation
Today our cloudbeat is wrapped by a launcher component that manages the reconfigurations and runs the beat itself.
This is different from other beats that do not have this wrapper logic and is also not part of the beat development generation code.
The goal of the launcher was to be agnostic to the underlying managed beat and to eventually be transformed into the beat development generation code.
Currently, this wrapper is being used only by our beat and there are no future plans for it to change (not that I'm aware of as of right now).
Part of the launcher includes the implementation of an abstraction layer of validator that has the purpose of validating configurations for the beat instance.
Our current implementation of the validator just calls the config constructor and doesn't do anything besides that.
We should discuss if we still want to have this whole abstraction layer or if it is redundant.
While working on this PR:

  • Remove runtime config logic #597
    I've had multiple commits regarding different ways how to tackle this configuration issue.
    We've decided to keep the validator and leave the checks inside the config for now.

Definition of done
What needs to be completed at the end of this task

  • Decide the fate of the abstraction and act accordingly
  • If we decide to leave it then create a plan/roadmap to utilize it and design everything accordingly
@jeniawhite jeniawhite added the Team:Cloud Security Cloud Security team related label Dec 27, 2022
@amirbenun
Copy link
Contributor

The functionality of a new beat generation no longer exists elastic/beats#28816.
This is actually good because we can focus our discussion on programming best practices with no other distractions.

I think we should try and follow the SOLID principles.
The "separation of concern" between the components should be kept in our case.
The listener is in charge of getting configuration updates from the fleet on managed environments.
The launcher then receives the new configuration, merge it with the old one, and instantiates a new beater.
Both of the classes have no business logic, they form an abstraction layer for the reconfiguration flow right before the Beater gets into action.

I am open to discussing cloudbeat specific requirements from the launcher, and discussing why we even need config validation as part of the reconfiguration.
But keep in mind, the validation step actually helps us separate the components. The Validator interface can be extended to return multiple types of errors, and the launcher can treat them differently (unhealthy state, or any other state we will have).

Would love to hear your thoughts as well. @jeniawhite @oren-zohar

@oren-zohar
Copy link
Collaborator

oren-zohar commented Dec 29, 2022

Based on both comments, I think focus on the following consideration first:

  • What's the value of decoupling the launcher from cloudbeat, if the entire role is launching cloudbeat?
  • Is it decoupled in the current design? It has some cloudbeat specific dependencies
  • Our validator implementation - is it clear what this component does? why isn't it doing the config validation?

validation step actually helps us separate the components

This isn't an intuitive aspect of its design. If this is the entire purpose, maybe we should rename it, or consider removing it completely. And again, we need to define clearly what is this component's role in cloudbeat flow (other than wrapping the config).

@amirbenun @jeniawhite

@amirbenun
Copy link
Contributor

What's the value of decoupling the launcher from cloudbeat, if the entire role is launching cloudbeat?

As I said keeping the SOLID principles, as I see it, those are different layers of cloudbeat.
The launcher is in charge of getting messages, somewhat a transport layer. I wouldn't want our transport layer to have business logic inside it.

Is it decoupled in the current design? It has some cloudbeat specific dependencies

Here is a pr that resolves it #628

Our validator implementation - is it clear what this component does? why isn't it doing the config validation?

I am open to discussing other naming for that class or a different implementation that also keeps the separation between the transport layer and the business logic.

@oren-zohar oren-zohar closed this as not planned Won't fix, can't repro, duplicate, stale Nov 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team:Cloud Security Cloud Security team related technical debt
Projects
None yet
Development

No branches or pull requests

4 participants