-
Notifications
You must be signed in to change notification settings - Fork 7
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
Separate agent from cli #209
Conversation
Signed-off-by: Pablo Chacin <pablochacin@gmail.com>
Signed-off-by: Pablo Chacin <pablochacin@gmail.com>
af74501
to
cf571ee
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this gets us closer to a separation between CLI and agent, but I'm still not 100% in with the current approach. I Think the separation between our objects is still a bit blurry.
I've left comments with an approach that I think could be nicer, but it might as well be too much work for now if we plan to further refactor this into a daemon mode. LMK if you think we should postpone this discussion until then.
pkg/agent/agent.go
Outdated
} | ||
|
||
// do executes a command in the Agent | ||
func (r *Agent) do(ctx context.Context, action func(context.Context) error) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this action
parameter really wants to become an interface, as said above.
Signed-off-by: Pablo Chacin <pablochacin@gmail.com>
Signed-off-by: Pablo Chacin <pablochacin@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like how this is looking! I think we've converged into a nice solution that keeps Cobra isolated.
I've left three very minor comments, but it's looking great already!
if d.config.Transparent { | ||
trCfg := iptables.TrafficRedirectorConfig{ | ||
Executor: d.executor, | ||
} | ||
// Redirect traffic to the proxy | ||
tr := &iptables.TrafficRedirectionSpec{ | ||
Iface: d.config.Iface, | ||
DestinationPort: d.config.TargetPort, | ||
RedirectPort: d.config.RedirectPort, | ||
} | ||
|
||
redirector, err := iptables.NewTrafficRedirectorWithConfig(tr, trCfg) | ||
if err != nil { | ||
return err | ||
} | ||
|
||
if err := redirector.Start(); err != nil { | ||
return fmt.Errorf(" failed traffic redirection: %w", err) | ||
} | ||
|
||
defer func() { | ||
_ = redirector.Stop() | ||
}() | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another alternative to this, would be to make the redirector an interface, and have two implementations: iptables.Redirector
and a NoopRedirector
that does nothing. Then, instead of the big if
block here, we could pass the disruptor
a NoopRedirector
if we're just proxying and an iptables.Redirector
if we're running in transparent mode.
Dropping this here as an alternative approach in case you want to consider it. I don't have strong concerns against the simple if
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes sense. I will give it a try.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On second thought I will make it in a follow-up PR so as not to mix concerns here.
Signed-off-by: Pablo Chacin <pablochacin@gmail.com>
Description
Presently there is an intermix of the logic that process the CLI (mostly, parameter processing) and logic to inject faults in a target.
When combined with the complexity of the execution of cobra commands (pre and post-run functions across a hierarchy of commands, including a well-known bug), it makes it hard to reason about this logic, creating complex bugs such as #206.
This change set creates an Agent object that encapsulates the logic for controlling the execution of the fault injection, separated from the CLI.
These changes also point in the direction to run the agent as a grpc service with the CLI as a frontend for its API #52
Fixes #206 #205
Checklist: