Tunneling outbound traffic #37968
Tunneling outbound traffic #37968
Conversation
|
Hi @jewertow. Thanks for your PR. I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/ok-to-test |
e8041b6 to
2dfae53
Compare
I totally agree with you and that's why I don't want to support applying tunnel settings to services without associated virtual services. |
|
IMO its not our problem. For example, I can have a VS that matches sni=google.com and sends to wikipedia.org. It will very likely fail in most cases, and that is fine - the user configured it. The same applies here. The config is doing what they have configured. Their clients may reject it, and if so - they should not apply that config. Its far more confusing to have a valid config silently ignored vs having an invalid config give a clear TLS error. Especially when we consider all of this only applies to TLS but we are applying it to TCP for consistency. Note that "TCP" can also be TLS (if they do not configured it as TLS but actually send TLS). |
|
Agree with John. Also good to be consistent.
Eventually we want ALL connections in the mesh, including egress and
east-west to use either H2+CONNECT or H2 ( and H3 or H3 CONNECT at some
point later ).
…On Fri, May 27, 2022 at 12:14 PM John Howard ***@***.***> wrote:
IMO its not our problem. For example, I can have a VS that matches sni=
google.com and sends to wikipedia.org. It will very likely fail in most
cases, and that is fine - the user configured it. The same applies here.
The config is doing what they have configured. Their clients may reject it,
and if so - they should not apply that config.
Its far more confusing to have a valid config silently ignored vs having
an invalid config give a clear TLS error.
Especially when we consider all of this only applies to TLS but we are
applying it to TCP for consistency. Note that "TCP" can also be TLS (if
they do not configured it as TLS but actually send TLS).
—
Reply to this email directly, view it on GitHub
<#37968 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAUR2VJ7MSUE7AFNMQ5SCLVMENLBANCNFSM5Q3ME4FA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
eb0d633 to
1dda4bb
Compare
Signed-off-by: Jacek Ewertowski <jewertow@redhat.com>
e3b5bf9 to
94b396d
Compare
| @@ -136,6 +138,9 @@ func buildOutboundNetworkFiltersWithWeightedClusters(node *model.Proxy, routes [ | |||
|
|
|||
| // For weighted clusters set hash policy if any of the upstream destinations have sourceIP. | |||
| maybeSetHashPolicy(destinationRule, tcpProxy, "") | |||
| // In case of weighted clusters, tunneling config for a subset is ignored, | |||
| // because it is set on listener, not on a cluster. | |||
| tunnelingconfig.Apply(tcpProxy, destinationRule, "") | |||
There was a problem hiding this comment.
this is pretty unexpected since we are basically saying ij a weighted cluster, for some DR fields the first cluster applies to all of them. However, this behavior was introduced in #35014 and is only extended here...
| if tunnel == nil { | ||
| return | ||
| } | ||
| if tunnel.Protocol == "" { |
There was a problem hiding this comment.
Should default to connect, we can change the API
| @@ -0,0 +1,25 @@ | |||
| apiVersion: apps/v1 | |||
There was a problem hiding this comment.
fwiw for unrelated reasons I have been working on adding CONNECT support in the echo app. So we could probably replace this long term. No problem for short term
There was a problem hiding this comment.
Ok, I will get back to it once the CONNECT in echo is ready.
| @@ -0,0 +1,35 @@ | |||
| apiVersion: networking.istio.io/v1alpha3 | |||
There was a problem hiding this comment.
these types of files should likely be under testdata/ which go has some special handling for
Signed-off-by: Jacek Ewertowski <jewertow@redhat.com>
Signed-off-by: Jacek Ewertowski <jewertow@redhat.com>
|
/test integ-security-multicluster_istio |
| }) | ||
| } | ||
| if _, err := kubeClient.CoreV1().Services(externalNs).Create(context.TODO(), svc, metav1.CreateOptions{}); err != nil { | ||
| ctx.Fatalf("failed to create service external-forward-proxy: %s", err) |
There was a problem hiding this comment.
Its nice to be able to have things be idempotent since for local development running with no-cleanup is super super helpful (run tests in 1s instead of minutes). The following diff does it:
diff --git a/tests/integration/pilot/tunneling_test.go b/tests/integration/pilot/tunneling_test.go
index a21372b876..43d1c46162 100644
--- a/tests/integration/pilot/tunneling_test.go
+++ b/tests/integration/pilot/tunneling_test.go
@@ -27,6 +27,7 @@ import (
"time"
corev1 "k8s.io/api/core/v1"
+ kerrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/util/intstr"
@@ -230,7 +231,13 @@ func applyForwardProxyConfigMaps(ctx framework.TestContext, externalNs string) {
},
}
if _, err := kubeClient.CoreV1().ConfigMaps(externalNs).Create(context.TODO(), cfgMap, metav1.CreateOptions{}); err != nil {
- ctx.Fatalf("failed to create config map external-forward-proxy-config: %s", err)
+ if kerrors.IsAlreadyExists(err) {
+ if _, err := kubeClient.CoreV1().ConfigMaps(externalNs).Update(context.TODO(), cfgMap, metav1.UpdateOptions{}); err != nil {
+ ctx.Fatalf("failed to update config map external-forward-proxy-config: %s", err)
+ }
+ } else {
+ ctx.Fatalf("failed to create config map external-forward-proxy-config: %s", err)
+ }
}
}
@@ -257,7 +264,13 @@ func applyForwardProxyService(ctx framework.TestContext, externalNs string) {
})
}
if _, err := kubeClient.CoreV1().Services(externalNs).Create(context.TODO(), svc, metav1.CreateOptions{}); err != nil {
- ctx.Fatalf("failed to create service external-forward-proxy: %s", err)
+ if kerrors.IsAlreadyExists(err) {
+ if _, err := kubeClient.CoreV1().Services(externalNs).Update(context.TODO(), svc, metav1.UpdateOptions{}); err != nil {
+ ctx.Fatalf("failed to update service external-forward-proxy: %s", err)
+ }
+ } else {
+ ctx.Fatalf("failed to create service external-forward-proxy: %s", err)
+ }
}
}Alternatively, the reason we don't have this code in 100 places is most of our tests are just doing YAML apply. I don't mind this way though, either works.
side note -I wish client-go had CreateOrUpdate...
There was a problem hiding this comment.
Although since the deployment is already static it may make sense to do it all as YAML? it can be templated. Up to you, not sure how complex this logic ends up being in template logic, may be too much
| externalNs := apps.External.Namespace.Name() | ||
|
|
||
| applyForwardProxyConfigMaps(ctx, externalNs) | ||
| ctx.ConfigIstio().File(externalNs, "testdata/external-forward-proxy-deployment.yaml").ApplyOrFail(ctx) |
There was a problem hiding this comment.
| ctx.ConfigIstio().File(externalNs, "testdata/external-forward-proxy-deployment.yaml").ApplyOrFail(ctx) | |
| ctx.ConfigIstio().File(externalNs, "testdata/external-forward-proxy-deployment.yaml").ApplyOrFail(ctx, apply.CleanupConditionally) |
For running locally, helpful. Also consistent with the fact we are not cleaning up the configmap/service
| applyForwardProxyConfigMaps(ctx, externalNs) | ||
| ctx.ConfigIstio().File(externalNs, "testdata/external-forward-proxy-deployment.yaml").ApplyOrFail(ctx) | ||
| applyForwardProxyService(ctx, externalNs) | ||
| waitForPodsReadyOrFail(ctx, externalNs, "external-forward-proxy") |
There was a problem hiding this comment.
Do we need a readiness probe on the deployment to make this more reliable? The retries on the test may make this not a big issue as is though
Signed-off-by: Jacek Ewertowski <jewertow@redhat.com>
|
/retest |
…evisions
We are adding min istio version for tests related to below PRs as this functionalities were not there in previous revisions :-
a. gRPC fault
injection(istio#39295)
b. Ignore port number in domain
matching(istio#40475)
c. Tunneling outbound traffic :- new tunnel field got
introduced(istio#37968)
d. Fix consistent hash based on source IP for TCP
proxy(istio#38438)
e. Traffic policy load balancer API
changes(istio#39742)
…evisions (#40892) We are adding min istio version for tests related to below PRs as this functionalities were not there in previous revisions :- a. gRPC fault injection(#39295) b. Ignore port number in domain matching(#40475) c. Tunneling outbound traffic :- new tunnel field got introduced(#37968) d. Fix consistent hash based on source IP for TCP proxy(#38438) e. Traffic policy load balancer API changes(#39742)
…evisions
We are adding min istio version for tests related to below PRs as this functionalities were not there in previous revisions :-
a. gRPC fault
injection(istio#39295)
b. Ignore port number in domain
matching(istio#40475)
c. Tunneling outbound traffic :- new tunnel field got
introduced(istio#37968)
d. Fix consistent hash based on source IP for TCP
proxy(istio#38438)
e. Traffic policy load balancer API
changes(istio#39742)
…evisions (#40957) We are adding min istio version for tests related to below PRs as this functionalities were not there in previous revisions :- a. gRPC fault injection(#39295) b. Ignore port number in domain matching(#40475) c. Tunneling outbound traffic :- new tunnel field got introduced(#37968) d. Fix consistent hash based on source IP for TCP proxy(#38438) e. Traffic policy load balancer API changes(#39742) Co-authored-by: Hemendra Teli <hemendrat@google.com>
|
On Fri, May 27, 2022 at 8:18 AM Jacek Ewertowski ***@***.***> wrote:
Normal behavior - if intended destination is target.com client should
originate request for target.com, and sidecar/proxy handle that.
I totally agree with you and that's why I don't want to support applying
tunnel settings to services without associated virtual services.
My main goal is to simplify egress configurations - see my gateway
improvements proposal. The less a user has to configure the better.
Using the IP filters ( public IP addresses ) - we can automatically upgrade
target.com to connect, without any VirtualService or other config.
Also to redirect or tune the config - K8S Gateway SNIRoute attached to the
mesh is likely the future....
We'll likely need a few iterations :-), but I wouldn't require virtual
service.
… —
Reply to this email directly, view it on GitHub
<#37968 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAUR2XBPXNW5RM5ZA2JG3TVMDRTHANCNFSM5Q3ME4FA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Background context
This change aims to enable tunneling outbound traffic via external tunnel proxies. I described the idea in detail in this RFC.
The implementation differs slightly from what I described in the document, but I decided to make it as close to Envoy API as possible.
Important to note that this implementation enables tunneling outbound traffic through an egress gateway as well as directly through a sidecar proxy.
Related API change: istio/api#2283