Skip to content

Commit

Permalink
Fix HTTPs on HTTP port passthrough (istio#28166)
Browse files Browse the repository at this point in the history 
* Fix HTTPs on HTTP port passthrough

* Add note
  • Loading branch information
@howardjohn
howardjohn authored Oct 23, 2020
1 parent fbe1ade commit 6a046ab
Show file tree
Hide file tree
Showing 8 changed files with 120 additions and 96 deletions.
12 changes: 10 additions & 2 deletions istioctl/pkg/writer/envoy/configdump/listener.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,11 +67,19 @@ func (l *ListenerFilter) Verify(listener *listener.Listener) bool {
return true
}

func getFilterChains(l *listener.Listener) []*listener.FilterChain {
res := l.FilterChains
if l.DefaultFilterChain != nil {
res = append(res, l.DefaultFilterChain)
}
return res
}

// retrieveListenerType classifies a Listener as HTTP|TCP|HTTP+TCP|UNKNOWN
func retrieveListenerType(l *listener.Listener) string {
nHTTP := 0
nTCP := 0
for _, filterChain := range l.GetFilterChains() {
for _, filterChain := range getFilterChains(l) {
for _, filter := range filterChain.GetFilters() {
if filter.Name == HTTPListener {
nHTTP++
Expand Down Expand Up @@ -179,7 +187,7 @@ var (
)

func retrieveListenerMatches(l *listener.Listener) []filterchain {
fChains := l.GetFilterChains()
fChains := getFilterChains(l)
resp := make([]filterchain, 0, len(fChains))
for _, filterChain := range fChains {
match := filterChain.FilterChainMatch
Expand Down
2 changes: 1 addition & 1 deletion pilot/pkg/networking/core/v1alpha3/gateway_simulation_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -588,7 +588,7 @@ type simulationTest struct {
calls []simulation.Expect
}

var debugMode = env.RegisterBoolVar("SIMULATION_DEBUG", false, "if enabled, will dump verbose output").Get()
var debugMode = env.RegisterBoolVar("SIMULATION_DEBUG", true, "if enabled, will dump verbose output").Get()

func runGatewayTest(t *testing.T, cases ...simulationTest) {
for _, tt := range cases {
Expand Down
55 changes: 38 additions & 17 deletions pilot/pkg/networking/core/v1alpha3/listener.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1345,6 +1345,7 @@ func (configgen *ConfigGeneratorImpl) buildSidecarOutboundListenerForPortOrUDS(n

// Support HTTP/1.0, HTTP/1.1 and HTTP/2
opt.match.ApplicationProtocols = append(opt.match.ApplicationProtocols, plaintextHTTPALPNs...)
opt.match.TransportProtocol = xdsfilters.RawBufferTransportProtocol
}

listenerOpts.needHTTPInspector = true
Expand Down Expand Up @@ -1419,6 +1420,7 @@ func (configgen *ConfigGeneratorImpl) buildSidecarOutboundListenerForPortOrUDS(n

// Support HTTP/1.0, HTTP/1.1 and HTTP/2
opt.match.ApplicationProtocols = append(opt.match.ApplicationProtocols, plaintextHTTPALPNs...)
opt.match.TransportProtocol = xdsfilters.RawBufferTransportProtocol
}

listenerOpts.filterChainOpts = append(listenerOpts.filterChainOpts, opts...)
Expand Down Expand Up @@ -1975,35 +1977,54 @@ func buildListener(opts buildListenerOpts) *listener.Listener {
return listener
}

func getMatchAllFilterChain(l *listener.Listener) (int, *listener.FilterChain) {
for i, fc := range l.FilterChains {
if isMatchAllFilterChain(fc) {
return i, fc
}
}
return 0, nil
}

// Create pass through filter chain for the listener assuming all the other filter chains are ready.
// The match member of pass through filter chain depends on the existing non-passthrough filter chain.
// TODO(lambdai): Calculate the filter chain match to replace the wildcard and replace appendListenerFallthroughRoute.
func (configgen *ConfigGeneratorImpl) appendListenerFallthroughRouteForCompleteListener(l *listener.Listener, node *model.Proxy, push *model.PushContext) {
for _, fc := range l.FilterChains {
if isMatchAllFilterChain(fc) {
// We can only have one wildcard match. If the filter chain already has one, skip it
// This happens in the case of HTTP, which will get a fallthrough route added later,
// or TCP, which is not supported
return
}
matchIndex, matchAll := getMatchAllFilterChain(l)
if matchAll != nil && !util.IsIstioVersionGE18(node) {
// We can only have one wildcard match. If the filter chain already has one, skip it
// This happens in the case of HTTP, which will get a fallthrough route added later,
// or TCP, which is not supported
// This check is skipped for Proxy's newer than 1.8, as we can use DefaultFilterChain
return
}

fallthroughNetworkFilters := buildOutboundCatchAllNetworkFiltersOnly(push, node)

mutable := &istionetworking.MutableObjects{
FilterChains: []istionetworking.FilterChain{
{
TCP: fallthroughNetworkFilters,
},
},
}

outboundPassThroughFilterChain := &listener.FilterChain{
FilterChainMatch: &listener.FilterChainMatch{},
Name: util.PassthroughFilterChain,
Filters: mutable.FilterChains[0].TCP,
Filters: fallthroughNetworkFilters,
}

// On proxy 1.8+, we can set a default filter chain. This allows us to avoid issues where
// traffic starts to match a filter chain but then doesn't match latter criteria, leading to
// dropped requests. See https://github.com/istio/istio/issues/26079 for details.
if util.IsIstioVersionGE18(node) {
// If there are multiple filter chains and a match all chain, move it to DefaultFilterChain
// This ensures it will always be used as the fallback
if matchAll != nil && len(l.FilterChains) > 1 {
copy(l.FilterChains[matchIndex:], l.FilterChains[matchIndex+1:]) // Shift l.FilterChains[i+1:] left one index.
l.FilterChains[len(l.FilterChains)-1] = nil // Erase last element (write zero value).
l.FilterChains = l.FilterChains[:len(l.FilterChains)-1] // Truncate slice.
l.DefaultFilterChain = matchAll
} else if matchAll == nil {
// Otherwise, if there is no match all already, set a passthrough match all
l.DefaultFilterChain = outboundPassThroughFilterChain
}
} else {
l.FilterChains = append(l.FilterChains, outboundPassThroughFilterChain)
}
l.FilterChains = append(l.FilterChains, outboundPassThroughFilterChain)
}

// buildCompleteFilterChain adds the provided TCP and HTTP filters to the provided Listener and serializes them.
Expand Down
108 changes: 55 additions & 53 deletions pilot/pkg/networking/core/v1alpha3/listener_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -742,7 +742,7 @@ func testOutboundListenerRoute(t *testing.T, services ...*model.Service) {
if l == nil {
t.Fatalf("expect listener %s", "1.2.3.4_8080")
}
f = l.FilterChains[1].Filters[0]
f = l.FilterChains[0].Filters[0]
cfg, _ = conversion.MessageToStruct(f.GetTypedConfig())
rds = cfg.Fields["rds"].GetStructValue().Fields["route_config_name"].GetStringValue()
if rds != "test1.com:8080" {
Expand All @@ -753,7 +753,7 @@ func testOutboundListenerRoute(t *testing.T, services ...*model.Service) {
if l == nil {
t.Fatalf("expect listener %s", "3.4.5.6_8080")
}
f = l.FilterChains[1].Filters[0]
f = l.FilterChains[0].Filters[0]
cfg, _ = conversion.MessageToStruct(f.GetTypedConfig())
rds = cfg.Fields["rds"].GetStructValue().Fields["route_config_name"].GetStringValue()
if rds != "test3.com:8080" {
Expand Down Expand Up @@ -781,7 +781,6 @@ func testOutboundListenerFilterTimeout(t *testing.T, services ...*model.Service)
}

func testOutboundListenerConflict(t *testing.T, services ...*model.Service) {
t.Helper()
oldestService := getOldestService(services...)
p := &fakePlugin{}
proxy := getProxy()
Expand All @@ -799,19 +798,18 @@ func testOutboundListenerConflict(t *testing.T, services ...*model.Service) {
t.Fatalf("expected tcp filter chain, found %s", listeners[0].FilterChains[1].Filters[0].Name)
}
} else if oldestProtocol != protocol.HTTP && oldestProtocol != protocol.TCP {
if len(listeners[0].FilterChains) != 2 {
t.Fatalf("expectd %d filter chains, found %d", 2, len(listeners[0].FilterChains))
} else {
if !isHTTPFilterChain(listeners[0].FilterChains[1]) {
t.Fatalf("expected http filter chain, found %s", listeners[0].FilterChains[1].Filters[0].Name)
}
if len(listeners[0].FilterChains) != 1 {
t.Fatalf("expectd %d filter chains, found %d", 1, len(listeners[0].FilterChains))
}
if !isHTTPFilterChain(listeners[0].FilterChains[0]) {
t.Fatalf("expected http filter chain, found %s", listeners[0].FilterChains[0].Filters[0].Name)
}

if !isTCPFilterChain(listeners[0].FilterChains[0]) {
t.Fatalf("expected tcp filter chain, found %s", listeners[0].FilterChains[2].Filters[0].Name)
}
if !isTCPFilterChain(listeners[0].DefaultFilterChain) {
t.Fatalf("expected tcp filter chain, found %s", listeners[0].DefaultFilterChain.Filters[0].Name)
}

verifyHTTPFilterChainMatch(t, listeners[0].FilterChains[1], model.TrafficDirectionOutbound, false)
verifyHTTPFilterChainMatch(t, listeners[0].FilterChains[0], model.TrafficDirectionOutbound, false)
verifyListenerFilters(t, listeners[0].ListenerFilters)

if !listeners[0].ContinueOnListenerFiltersTimeout || listeners[0].ListenerFiltersTimeout == nil {
Expand All @@ -820,16 +818,19 @@ func testOutboundListenerConflict(t *testing.T, services ...*model.Service) {
listeners[0].ListenerFiltersTimeout)
}

f := listeners[0].FilterChains[1].Filters[0]
f := listeners[0].FilterChains[0].Filters[0]
cfg, _ := conversion.MessageToStruct(f.GetTypedConfig())
rds := cfg.Fields["rds"].GetStructValue().Fields["route_config_name"].GetStringValue()
expect := fmt.Sprintf("%d", oldestService.Ports[0].Port)
if rds != expect {
t.Fatalf("expect routes %s, found %s", expect, rds)
}
} else {
if len(listeners[0].FilterChains) != 2 {
t.Fatalf("expectd %d filter chains, found %d", 2, len(listeners[0].FilterChains))
if len(listeners[0].FilterChains) != 1 {
t.Fatalf("expectd %d filter chains, found %d", 1, len(listeners[0].FilterChains))
}
if listeners[0].DefaultFilterChain == nil {
t.Fatalf("expected default filter chains, found none")
}

_ = getTCPFilterChain(t, listeners[0])
Expand All @@ -846,9 +847,17 @@ func testOutboundListenerConflict(t *testing.T, services ...*model.Service) {
}
}

func getFilterChains(l *listener.Listener) []*listener.FilterChain {
res := l.FilterChains
if l.DefaultFilterChain != nil {
res = append(res, l.DefaultFilterChain)
}
return res
}

func getTCPFilterChain(t *testing.T, l *listener.Listener) *listener.FilterChain {
t.Helper()
for _, fc := range l.FilterChains {
for _, fc := range getFilterChains(l) {
for _, f := range fc.Filters {
if f.Name == wellknown.TCPProxy {
return fc
Expand All @@ -861,7 +870,7 @@ func getTCPFilterChain(t *testing.T, l *listener.Listener) *listener.FilterChain

func getHTTPFilterChain(t *testing.T, l *listener.Listener) *listener.FilterChain {
t.Helper()
for _, fc := range l.FilterChains {
for _, fc := range getFilterChains(l) {
for _, f := range fc.Filters {
if f.Name == wellknown.HTTPConnectionManager {
return fc
Expand Down Expand Up @@ -995,9 +1004,7 @@ func verifyHTTPFilterChainMatch(t *testing.T, fc *listener.FilterChain, directio
len(plaintextHTTPALPNs), plaintextHTTPALPNs, fc.FilterChainMatch.ApplicationProtocols)
}

if direction == model.TrafficDirectionOutbound && fc.FilterChainMatch.TransportProtocol != "" {
t.Fatalf("exepct %q transport protocol, found %q", "", fc.FilterChainMatch.TransportProtocol)
} else if direction == model.TrafficDirectionInbound && fc.FilterChainMatch.TransportProtocol != xdsfilters.RawBufferTransportProtocol {
if fc.FilterChainMatch.TransportProtocol != xdsfilters.RawBufferTransportProtocol {
t.Fatalf("exepct %q transport protocol, found %q", xdsfilters.RawBufferTransportProtocol, fc.FilterChainMatch.TransportProtocol)
}
}
Expand Down Expand Up @@ -1105,21 +1112,20 @@ func testOutboundListenerConfigWithSidecar(t *testing.T, services ...*model.Serv
}

l := findListenerByPort(listeners, 8080)
if len(l.FilterChains) != 2 {
t.Fatalf("expectd %d filter chains, found %d", 2, len(l.FilterChains))
} else {
if !isHTTPFilterChain(l.FilterChains[1]) {
t.Fatalf("expected http filter chain, found %s", l.FilterChains[1].Filters[0].Name)
}

if !isTCPFilterChain(l.FilterChains[0]) {
t.Fatalf("expected tcp filter chain, found %s", l.FilterChains[0].Filters[0].Name)
}
if len(l.FilterChains) != 1 {
t.Fatalf("expectd %d filter chains, found %d", 1, len(l.FilterChains))
}
if !isHTTPFilterChain(l.FilterChains[0]) {
t.Fatalf("expected http filter chain, found %s", l.FilterChains[0].Filters[0].Name)
}

verifyHTTPFilterChainMatch(t, l.FilterChains[1], model.TrafficDirectionOutbound, false)
verifyListenerFilters(t, l.ListenerFilters)
if !isTCPFilterChain(l.DefaultFilterChain) {
t.Fatalf("expected tcp filter chain, found %s", l.DefaultFilterChain.Filters[0].Name)
}

verifyHTTPFilterChainMatch(t, l.FilterChains[0], model.TrafficDirectionOutbound, false)
verifyListenerFilters(t, l.ListenerFilters)

if l := findListenerByPort(listeners, 3306); !isMysqlListener(l) {
t.Fatalf("expected MySQL listener on port 3306, found %v", l)
}
Expand All @@ -1136,19 +1142,18 @@ func testOutboundListenerConfigWithSidecar(t *testing.T, services ...*model.Serv
}

l = findListenerByPort(listeners, 8888)
if len(l.FilterChains) != 2 {
t.Fatalf("expectd %d filter chains, found %d", 2, len(l.FilterChains))
} else {
if !isHTTPFilterChain(l.FilterChains[1]) {
t.Fatalf("expected http filter chain, found %s", l.FilterChains[0].Filters[0].Name)
}
if len(l.FilterChains) != 1 {
t.Fatalf("expected %d filter chains, found %d", 1, len(l.FilterChains))
}
if !isHTTPFilterChain(l.FilterChains[0]) {
t.Fatalf("expected http filter chain, found %s", l.FilterChains[0].Filters[0].Name)
}

if !isTCPFilterChain(l.FilterChains[0]) {
t.Fatalf("expected tcp filter chain, found %s", l.FilterChains[1].Filters[0].Name)
}
if !isTCPFilterChain(l.DefaultFilterChain) {
t.Fatalf("expected tcp filter chain, found %s", l.DefaultFilterChain.Filters[0].Name)
}

verifyHTTPFilterChainMatch(t, l.FilterChains[1], model.TrafficDirectionOutbound, false)
verifyHTTPFilterChainMatch(t, l.FilterChains[0], model.TrafficDirectionOutbound, false)
verifyListenerFilters(t, l.ListenerFilters)
}

Expand Down Expand Up @@ -1902,12 +1907,12 @@ func TestOutboundListenerConfig_TCPFailThrough(t *testing.T) {
buildService("test1.com", wildcardIP, protocol.HTTP, tnow)}
listeners := buildOutboundListeners(t, &fakePlugin{}, getProxy(), nil, nil, services...)

if len(listeners[0].FilterChains) != 2 {
t.Fatalf("expectd %d filter chains, found %d", 2, len(listeners[0].FilterChains))
if len(listeners[0].FilterChains) != 1 {
t.Fatalf("expectd %d filter chains, found %d", 1, len(listeners[0].FilterChains))
}

verifyHTTPFilterChainMatch(t, listeners[0].FilterChains[0], model.TrafficDirectionOutbound, false)
verifyPassThroughTCPFilterChain(t, listeners[0].FilterChains[1])
verifyPassThroughTCPFilterChain(t, listeners[0].DefaultFilterChain)
verifyListenerFilters(t, listeners[0].ListenerFilters)
}

Expand Down Expand Up @@ -2422,13 +2427,13 @@ func TestAppendListenerFallthroughRouteForCompleteListener(t *testing.T) {
t.Run(tests[idx].name, func(t *testing.T) {
configgen.appendListenerFallthroughRouteForCompleteListener(tests[idx].listener,
tests[idx].node, push)
if len(tests[idx].listener.FilterChains) != 1 {
t.Errorf("Expected exactly 1 filter chain")
if len(tests[idx].listener.FilterChains) != 0 {
t.Errorf("Expected exactly 0 filter chain")
}
if len(tests[idx].listener.FilterChains[0].Filters) != 1 {
if len(tests[idx].listener.DefaultFilterChain.Filters) != 1 {
t.Errorf("Expected exactly 1 network filter in the chain")
}
filter := tests[idx].listener.FilterChains[0].Filters[0]
filter := tests[idx].listener.DefaultFilterChain.Filters[0]
var tcpProxy tcp.TcpProxy
cfg := filter.GetTypedConfig()
_ = ptypes.UnmarshalAny(cfg, &tcpProxy)
Expand All @@ -2438,9 +2443,6 @@ func TestAppendListenerFallthroughRouteForCompleteListener(t *testing.T) {
if tcpProxy.GetCluster() != tests[idx].hostname {
t.Errorf("Expected cluster %s but got %s\n", tests[idx].hostname, tcpProxy.GetCluster())
}
if len(tests[idx].listener.FilterChains) != 1 {
t.Errorf("Expected exactly 1 filter chain on the tests[idx].listener")
}
})
}
}
Expand Down
21 changes: 0 additions & 21 deletions pilot/pkg/networking/core/v1alpha3/sidecar_simulation_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ import (
meshconfig "istio.io/api/mesh/v1alpha1"
"istio.io/istio/pilot/pkg/networking/util"
"istio.io/istio/pilot/pkg/simulation"
"istio.io/istio/pilot/pkg/util/sets"
"istio.io/istio/pilot/pkg/xds"
"istio.io/istio/pkg/config/mesh"
)
Expand Down Expand Up @@ -605,18 +604,6 @@ func TestPassthroughTraffic(t *testing.T) {
number: 86
protocol: HTTP2`

// TODO: https://github.com/istio/istio/issues/26079 this should be empty list
expectedFailures := sets.NewSet(
"http-tls-80-http/1.1",
"http-tls-85-http/1.1",
"http-tls-86-http/1.1",
)
withoutVipExpectedFailures := sets.NewSet(
"http-tls-80-http/1.1",
"http-tls-81-http/1.1",
"http-tls-85-http/1.1",
"http-tls-86-http/1.1",
)
isHTTPPort := func(p int) bool {
switch p {
case 80, 85, 86:
Expand Down Expand Up @@ -665,10 +652,6 @@ func TestPassthroughTraffic(t *testing.T) {
e.Result.ClusterMatched = ""
e.Result.VirtualHostMatched = util.BlackHole
}
if expectedFailures.Contains(name) {
e.Result.Error = simulation.ErrProtocolError
e.Result.ClusterMatched = ""
}
testCalls = append(testCalls, e)
}
sort.Slice(testCalls, func(i, j int) bool {
Expand Down Expand Up @@ -712,10 +695,6 @@ spec:
e.Result.Error = nil
e.Result.ClusterMatched = ""
}
if withoutVipExpectedFailures.Contains(name) {
e.Result.Error = simulation.ErrProtocolError
e.Result.ClusterMatched = ""
}
testCalls = append(testCalls, e)
}
sort.Slice(testCalls, func(i, j int) bool {
Expand Down
Loading

0 comments on commit 6a046ab

Please sign in to comment.