aws / amazon-ecs-service-connect-agent

Amazon ECS Service Connect Agent
Apache License 2.0
27 stars 10 forks source link

Add ability to set envoy.reloadable_features_successful_active_health… #38

Closed thomashoffman closed 1 year ago

thomashoffman commented 1 year ago

…_check_uneject_host using env variable ENVOY_ACTIVE_HEALTH_CHECK_UNEJECT_HOST.

Summary

Add ability to set envoy.reloadable_features_successful_active_health_check_uneject_host using env variable ENVOY_ACTIVE_HEALTH_CHECK_UNEJECT_HOST.

https://www.envoyproxy.io/docs/envoy/latest/version_history/v1.26/v1.26.0.html

Implementation details

Added the ability to set that envoy flag based off an environment variable.

Testing

make test

New tests cover the changes:

Description for the changelog

Add ability to set envoy.reloadable_features_successful_active_health_check_uneject_host using env variable ENVOY_ACTIVE_HEALTH_CHECK_UNEJECT_HOST.

Licensing

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

karanvasnani commented 1 year ago

Thinking about the default behavior again, I'm leaning towards not enabling this by default and rather require customers to opt-in. When a customer decides to use both active and passive (outlier detection) health checking, they may have different use of those. With active health checks they may be wanting to monitor the backend service's health itself or its ability to serve requests while, with outlier detection they may want to detect any outages where even though the backend is healthy, one of its dependencies might be experiencing outages. Regardless, this is clearly a visible change in behavior and since we haven't had any ask from customers to alter the existing behavior, I think we should require customers to consciously opt-into it. Let me know what you think?

thomashoffman commented 1 year ago

I'm fine either way. If we disable it by default, then we should have a plan to swap that at some point down the line as the ability to revert to old behavior is most likely temporary.

suniltheta commented 1 year ago

Sure we can make it false by default. It is definitely going to be a breaking change for the customers. Either we do opt-in customers directly now (Envoy's default behavior) or continue to maintain the reloadable feature even after Envoy removes it (either after 6 months or a year).

Sometimes we notice the reloadable features stick around for a longer time (more than 1 year/ 4 minor releases) given the new feature caused issues that get reported.

Hopefully in this scenario as well there is similar sentiment about this change among the community. Watch out for updates related to this change.

I can commit to making this reloadable feature false by default on our end.

thomashoffman commented 1 year ago

Sure, this is definitely going to be a breaking change for the customers. Either we do opt-in customers directly now (Envoy's default behavior) or continue to maintain the reloadable feature even after Envoy removes it (either after 6 months or a year).

Sometimes we notice the reloadable features stick around for a longer time (more than 1 year/ 4 minor releases) given the new feature caused issues that get reported.

Hopefully in this scenario as well there is similar sentiment about this change among the community. Watch out for updates changes related to this change.

I can commit to making this reloadable feature false by default on our end.

Okay, I'll swap the logic and update the pull request.