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

connect: do not restrict auto envoy version to docker task driver #17041

Merged
merged 1 commit into from
May 1, 2023

Conversation

shoenig
Copy link
Member

@shoenig shoenig commented May 1, 2023

This PR updates the envoy_bootstrap_hook to no longer disable itself if
the task driver in use is not docker. In other words, make it work for
podman and other image based task drivers. The hook now only checks that

  1. the task is a connect sidecar
  2. the task.config block contains an "image" field

This PR updates the envoy_bootstrap_hook to no longer disable itself if
the task driver in use is not docker. In other words, make it work for
podman and other image based task drivers. The hook now only checks that

1. the task is a connect sidecar
2. the task.config block contains an "image" field
@shoenig shoenig force-pushed the connect-envoy-version-driver-check branch from eb65c02 to 0ec5aac Compare May 1, 2023 17:09
@shoenig shoenig added this to the 1.6.0 milestone May 1, 2023
@shoenig shoenig marked this pull request as ready for review May 1, 2023 18:21
@shoenig shoenig requested review from lgfa29 and gulducat May 1, 2023 18:22
Copy link
Member

@gulducat gulducat left a comment

Choose a reason for hiding this comment

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

just to phrase it differently, this is to allow this kind of config in a sidecar_task block

# job..group..service {
connect {
  sidecar_task {
    driver = "podman"
  }
}

without needing to also explicitly specify an envoy image for the sidecar task, right?

@shoenig
Copy link
Member Author

shoenig commented May 1, 2023

@gulducat yup, this combined with #17045 should get us to the point of using your config block. After that I'd like to go one step further, and adding a meta.connect.driver client config option (similar to the already existing meta.connect.image and meta.connect.log_level options) so that Podman oriented clusters can make use of a default connect{} block with no additional job level configuration.

@shoenig shoenig merged commit 0b3bd45 into main May 1, 2023
@shoenig shoenig deleted the connect-envoy-version-driver-check branch May 1, 2023 20:07
@lgfa29
Copy link
Contributor

lgfa29 commented May 1, 2023

Sorry, late to the party 😅

Would this approach cause problems for task drivers that use image for non-OCI paths, like the pot driver?

I think we document that Connect is currently only supported in Docker, so that's probably OK, but I wonder if it would make sense to create a new task driver capability.

@shoenig
Copy link
Member Author

shoenig commented May 2, 2023

The envoy version hook is also gated on the task being a Connect sidecar, which can only be true if the task is using bridge networking, which wouldn't apply to most task drivers.

Adding a new task driver capability is an interesting idea, but I suspect it would end up mirroring the same image/sidecar checks already in the hook.

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

Successfully merging this pull request may close these issues.

3 participants