kuma-cp: generate HTTP-specific outbound listeners for services tagged with protocol: http
#585
Conversation
yskopets
force-pushed
the
feature/generate-outbound-listeners-for-http-traffic
branch
7 times, most recently
from
bb1cb8a to
f24dbd0
Compare
yskopets
changed the base branch from
refactoring/refactor-envoy-config-generators
to
ci/refactor-e2e-minikube-setup
yskopets
changed the base branch from
ci/refactor-e2e-minikube-setup
to
refactoring/introduce-filter-chain-builder
yskopets
force-pushed
the
feature/generate-outbound-listeners-for-http-traffic
branch
3 times, most recently
from
676917a to
e3ddf95
Compare
pkg/xds/generator/protocol.go
Outdated
| // a common protocol between HTTP and TCP is TCP, | ||
| // a common protocol between GRPC and HTTP2 is HTTP2, | ||
| // a common protocol between HTTP and HTTP2 is TCP. | ||
| func GetCommonProtocol(one, another mesh_core.Protocol) mesh_core.Protocol { |
There was a problem hiding this comment.
Why this function is public?
I found that we expose many methods as public even if those are used only in one place internally. Then we write tests to those methods which creates tests duplication and makes refactor harder.
In this case, tests for InterServiceProtocol would be enough and it would cover GetCommonProtocol just fine.
There was a problem hiding this comment.
It's been made public mostly for convenience in unit testing.
We don't write unit tests only because a function is public. We write unit tests when we want to test a function in isolation (because we see a value in doing it this way instead of testing indirectly).
Exporting a function makes unit testing simpler. However, it's not a pre-requirement.
I've renamed GetCommonProtocol into getCommonProtocol and updated unit test.
There was a problem hiding this comment.
I write tests to check functionality, not functions. At least that is what I did until I started writing in Go. The convention of putting tests next to the file kind of tries to force you to write tests to functions.
The problem I have with this approach is that when I want to change the functionality I need to now change tests of two functions. Refactor is harder because if I change anything in the code I need to change functions tests since they are 1:1. Tests should help me refactor code whenever I want, not make it harder.
Exporting a function makes unit testing simpler. However, it's not a pre-requirement.
I don't think we should ever make function public just for the sake of simpler testing. Proper encapsulation makes my reasoning about code much better. I instantly know something is local to this package. Once I see something is public I check where in the other parts of the system it is used.
| @@ -162,6 +162,5 @@ networking: | |||
| - interface: {{ IP }}:{{ PUBLIC_PORT }}:{{ LOCAL_PORT }} | |||
| tags: | |||
| service: kuma-example-backend | |||
| protocol: http | |||
There was a problem hiding this comment.
What was a problem here?
There was a problem hiding this comment.
e2e test is expecting TcpProxy to be configured.
But adding protocol: http turns TcpProxy into HttpConnectionManager.
We need to test both scenarios:
- when a user does add
protocol: http - when a user does NOT add
protocol: http
That's why I removed protocol: http to restore original e2e test for TCP case.
I will add an e2e test for HTTP case in a separate PR.
yskopets
force-pushed
the
feature/generate-outbound-listeners-for-http-traffic
branch
from
0c06b96 to
7f50e0a
Compare
yskopets
force-pushed
the
refactoring/introduce-filter-chain-builder
branch
from
9675229 to
38f9afd
Compare
yskopets
force-pushed
the
feature/generate-outbound-listeners-for-http-traffic
branch
from
7f50e0a to
55077e2
Compare
yskopets
changed the base branch from
refactoring/introduce-filter-chain-builder
to
master
devadvocado
deleted the
feature/generate-outbound-listeners-for-http-traffic
branch
Summary
protocol: http