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-cp): delete an empty TimeoutConfigurer #4554

Merged
merged 3 commits into from
Jul 8, 2022

Conversation

lobkovilya
Copy link
Contributor

Summary

ProvidedEndpointCluster and PassThroughCluster created clusters with an empty TimeoutConfigurer that was overriding values from the Timeout policy.

Full changelog

  • delete TimeoutConfigurer from ProvidedEndpointCluster
  • delete TimeoutConfigurer from PassThroughCluster
  • add Configure(envoy_clusters.DefaultTimeout()). where we don't have Timeout policy.
  • add unit tests

Issues resolved

Fix #4475

Documentation

Testing

  • Unit tests
  • E2E tests
  • Manual testing on Universal
  • Manual testing on Kubernetes

Backwards compatibility

  • Update UPGRADE.md with any steps users will need to take when upgrading.
  • Add backport-to-stable label if the code follows our backporting policy

Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>
Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>
Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>
@lobkovilya lobkovilya requested a review from a team as a code owner July 6, 2022 07:56
Copy link
Contributor

@michaelbeaumont michaelbeaumont left a comment

Choose a reason for hiding this comment

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

Is there a way to not have to "remember" to add .Configure(DefaultTimeout) for every cluster?

@lobkovilya
Copy link
Contributor Author

Is there a way to not have to "remember" to add .Configure(DefaultTimeout) for every cluster?

@michaelbeaumont I was thinking about that. I see a few options:

  1. we can panic if you're trying to create a cluster without timeouts
  2. we can add Timeout as a parameter to every cluster's constructor like ProvidedEndpointCluster(name string, hasIPv6 bool, timeout mesh_proto.Timeout, endpoints ...core_xds.Endpoint). But then it's still easy to forget about timeouts when you create a new type of cluster
  3. we can leave it as it is; the worst-case – configuration will be rejected by Envoy because connectTimeout is mandatory

@michaelbeaumont
Copy link
Contributor

  1. we can leave it as it is; the worst-case – configuration will be rejected by Envoy because connectTimeout is mandatory

ah ok that I wasn't aware of, yeah maybe that's enough then.

@lobkovilya lobkovilya merged commit d220039 into master Jul 8, 2022
@lobkovilya lobkovilya deleted the fix/provided-endpoint branch July 8, 2022 11:19
mergify bot pushed a commit that referenced this pull request Jul 8, 2022
Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>
(cherry picked from commit d220039)

# Conflicts:
#	pkg/plugins/runtime/gateway/gateway_route_generator_test.go
#	pkg/plugins/runtime/gateway/testdata/http/cross-mesh-gateway.yaml
#	pkg/plugins/runtime/gateway/testdata/https/cross-mesh-gateway.yaml
lobkovilya added a commit that referenced this pull request Jul 12, 2022
Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>
# Conflicts:
#	pkg/plugins/runtime/gateway/gateway_route_generator_test.go
#	pkg/plugins/runtime/gateway/testdata/http/cross-mesh-gateway.yaml
#	pkg/plugins/runtime/gateway/testdata/https/cross-mesh-gateway.yaml
#	pkg/xds/generator/direct_access_proxy_generator.go
#	pkg/xds/generator/outbound_proxy_generator_test.go
#	pkg/xds/generator/testdata/outbound-proxy/06.envoy.golden.yaml
lobkovilya added a commit that referenced this pull request Jul 12, 2022
Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>
# Conflicts:
#	pkg/plugins/runtime/gateway/gateway_route_generator_test.go
#	pkg/plugins/runtime/gateway/testdata/http/cross-mesh-gateway.yaml
#	pkg/plugins/runtime/gateway/testdata/https/cross-mesh-gateway.yaml
mergify bot added a commit that referenced this pull request Jul 13, 2022
* fix(kuma-cp): delete an empty TimeoutConfigurer (#4554)

Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>
# Conflicts:
#	pkg/plugins/runtime/gateway/gateway_route_generator_test.go
#	pkg/plugins/runtime/gateway/testdata/http/cross-mesh-gateway.yaml
#	pkg/plugins/runtime/gateway/testdata/https/cross-mesh-gateway.yaml

* fix test

Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>

Co-authored-by: Ilya Lobkov <ilya.lobkov@konghq.com>
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.

timeoutConfigurer is overriden for ExternalServices
2 participants