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

fix(kuma-dp): Fix envoy binary not found #695

Merged
merged 7 commits into from
May 1, 2020

Conversation

tharun208
Copy link
Contributor

@tharun208 tharun208 commented Apr 23, 2020

Check for envoy path and throw an error if not found during initializing envoy
Updated tests

Fix Envoy Binary Not Found

When envoy is not found Kuma-dp stops.

Full changelog

  • Checked for envoy path and if present creates an envoy instance.
  • Updated tests.

Issues resolved

Fix #686

Check for envoy path and throw error if not found during initializing envoy
Updated tests
@tharun208 tharun208 requested a review from a team April 23, 2020 17:13
@tharun208 tharun208 changed the title fix(kuma-dp): Fix kuma binay not found bug fix(kuma-dp): Fix envoy binary not found Apr 23, 2020
Copy link
Contributor

@subnetmarco subnetmarco left a comment

Choose a reason for hiding this comment

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

We need to implement two checks here:

  1. One to determine if envoy exists. If not then return an error that says something like could not find the "envoy" executable in your path
  2. One to return an error if running envoy fails for whatever reason (we have this here in this PR)
  3. And we could even add a check to determine that the version of envoy that we found (envoy --version) is compatible with the current version of Kuma.

For the last step, we will need to have a list of version ranges that we support (ie from 1.12 to 1.14) so that we can support minor releases that are being created in the meanwhile by Envoy without having to update Kuma. I will let @jakubdyszkiewicz chime in for this.

@tharun208
Copy link
Contributor Author

  1. could not find the "envoy" executable in your pat

We need to implement two checks here:

  1. One to determine if envoy exists. If not then return an error that says something like could not find the "envoy" executable in your path
  2. One to return an error if running envoy fails for whatever reason (we have this here in this PR)
  3. And we could even add a check to determine that the version of envoy that we found (envoy --version) is compatible with the current version of Kuma.

For the last step, we will need to have a list of version ranges that we support (ie from 1.12 to 1.14) so that we can support minor releases that are being created in the meanwhile by Envoy without having to update Kuma. I will let @jakubdyszkiewicz chime in for this.

  1. I added the envoy exists check and the error message.
  2. Running envoy fails is already present.

@tharun208
Copy link
Contributor Author

tharun208 commented Apr 23, 2020

  1. could not find the "envoy" executable in your pat

We need to implement two checks here:

  1. One to determine if envoy exists. If not then return an error that says something like could not find the "envoy" executable in your path
  2. One to return an error if running envoy fails for whatever reason (we have this here in this PR)
  3. And we could even add a check to determine that the version of envoy that we found (envoy --version) is compatible with the current version of Kuma.

For the last step, we will need to have a list of version ranges that we support (ie from 1.12 to 1.14) so that we can support minor releases that are being created in the meanwhile by Envoy without having to update Kuma. I will let @jakubdyszkiewicz chime in for this.

Hi Marco,

  1. I added the envoy not exists check and the error message.
  2. Running envoy fail check is already present.

@jakubdyszkiewicz
Copy link
Contributor

And we could even add a check to determine that the version of envoy that we found (envoy --version) is compatible with the current version of Kuma.

For the last step, we will need to have a list of version ranges that we support (ie from 1.12 to 1.14) so that we can support minor releases that are being created in the meanwhile by Envoy without having to update Kuma. I will let @jakubdyszkiewicz chime in for this.

I am hesitant about this step @subnetmarco
It is very much a use case to use your custom Envoy distro with Kuma. The version name then could be anything. Additionally, I'd allow users to experiment with new versions of Envoy, especially if this is a minor security release.

If we want to do it, we should only validate major version so in case of 1.12.1 we let 1.12.*. Additionally, there should be a flag to skip this validation.

@tharun208
Copy link
Contributor Author

And we could even add a check to determine that the version of envoy that we found (envoy --version) is compatible with the current version of Kuma.
For the last step, we will need to have a list of version ranges that we support (ie from 1.12 to 1.14) so that we can support minor releases that are being created in the meanwhile by Envoy without having to update Kuma. I will let @jakubdyszkiewicz chime in for this.

I am hesitant about this step @subnetmarco
It is very much a use case to use your custom Envoy distro with Kuma. The version name then could be anything. Additionally, I'd allow users to experiment with new versions of Envoy, especially if this is a minor security release.

If we want to do it, we should only validate major version so in case of 1.12.1 we let 1.12.*. Additionally, there should be a flag to skip this validation.

@subnetmarco are we going to add this check?

app/kuma-dp/cmd/run.go Outdated Show resolved Hide resolved
@subnetmarco
Copy link
Contributor

Okay then we can skip the Envoy version range validation for now, but I added another comment to my review that needs to be addressed.

Copy link
Contributor

@lobkovilya lobkovilya left a comment

Choose a reason for hiding this comment

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

Just small insignificant comments, overall looks good!

app/kuma-dp/pkg/dataplane/envoy/envoy_test.go Outdated Show resolved Hide resolved
app/kuma-dp/pkg/dataplane/envoy/envoy_test.go Outdated Show resolved Hide resolved
app/kuma-dp/pkg/dataplane/envoy/envoy_test.go Outdated Show resolved Hide resolved
app/kuma-dp/pkg/dataplane/envoy/envoy.go Outdated Show resolved Hide resolved
Copy link
Contributor

@lobkovilya lobkovilya left a comment

Choose a reason for hiding this comment

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

@tharun208 Great! Thank you for your contribution!

@lobkovilya lobkovilya merged commit 59bc291 into kumahq:master May 1, 2020
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.

When envoy is not found kuma-dp terminates in an inconsistent state
4 participants