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

Support for containerized plugins #959

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

aamohd
Copy link
Contributor

@aamohd aamohd commented Feb 24, 2025

No description provided.

@aamohd aamohd force-pushed the aamohd/containerized-plugins branch from 27f419c to 68efa03 Compare February 26, 2025 15:58
@aamohd aamohd force-pushed the aamohd/containerized-plugins branch from 68efa03 to cade3d1 Compare February 26, 2025 16:06
@aamohd aamohd requested a review from j-lanson February 26, 2025 16:12
@aamohd aamohd marked this pull request as ready for review February 26, 2025 18:37
Copy link
Collaborator

@alilleybrinker alilleybrinker left a comment

Choose a reason for hiding this comment

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

Overall looks good, and very exciting to have this close to landing! Some comments, but I think this is on the right track.

@@ -168,16 +168,25 @@ impl<P: Plugin> PluginService for PluginServer<P> {
) -> QueryResult<Resp<Self::InitiateQueryProtocolStream>> {
let rx = req.into_inner();
// TODO: - make channel size configurable
let (tx, out_rx) = mpsc::channel::<QueryResult<InitiateQueryProtocolResp>>(10);
let (tx, out_rx) = mpsc::channel::<QueryResult<InitiateQueryProtocolResp>>(100);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why the increase to the channel size?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I ran it with a channel size of 10, I would sometimes get an error that the channel closed abruptly. I increased it by a factor of 10 but if there's another recommended size I should change it to, let me know

@@ -40,7 +40,7 @@ impl<P: Plugin> PluginServer<P> {
/// Run the plugin server on the provided port.
pub async fn listen(self, port: u16) -> Result<()> {
let service = PluginServiceServer::new(self);
let host = format!("127.0.0.1:{}", port).parse().unwrap();
let host = format!("0.0.0.0:{}", port).parse().unwrap();
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure this change is valid when running outside of a container; did you try it? If it's not, we may need to expose an SDK setting for what host to listen on, perhaps something like:

enum Host {
    // 127.0.0.1
    Loopback,
    // 0.0.0.0.0
    Any,
    // Any other IP address.
    Other(Ipv4Addr),
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just tried running normally with the 0.0.0.0 address and it was successful, but I can try to rework this so it isn't at 0.0.0.0 by default

Copy link
Collaborator

Choose a reason for hiding this comment

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

The question is whether we should be concerned about listening on all network interfaces and potentially enabling a remote process to connect to the plugin first and surreptitiously pose as hipcheck core.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Right, 0.0.0.0 inside a container is less concerning because what really matters on the host is the port bindings that the container runtime does. But for plugins running outside of the container we really ought to use the loopback address.

on arch="aarch64-apple-darwin" "activity-container-deploy.sh"
on arch="x86_64-apple-darwin" "activity-container-deploy.sh"
on arch="x86_64-unknown-linux-gnu" "activity-container-deploy.sh"
on arch="x86_64-pc-windows-msvc" "activity-container-deploy.sh"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is Windows able to run this shell script? You may need to also have a PowerShell script for Windows specifically.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

4 participants