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

NET-5132 - Configure multiport routing for connect proxies in TProxy mode #18606

Merged
merged 34 commits into from
Sep 12, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
f4239e2
mesh-controller: handle L4 protocols for a proxy without upstreams
ishustava Jul 23, 2023
ab6ce72
sidecar-controller: Support explicit destinations for L4 protocols an…
ishustava Jul 28, 2023
93631aa
endpoints-controller: add workload identity to the service endpoints …
ishustava Aug 3, 2023
f65f474
small fixes
ishustava Aug 4, 2023
7839d58
review comments
ishustava Aug 8, 2023
0b85a4b
Address PR comments
ishustava Aug 10, 2023
a281bdc
sidecar-proxy controller: Add support for transparent proxy
ishustava Aug 14, 2023
1d5f30a
PR review comments
ishustava Sep 7, 2023
4844da9
mesh-controller: handle L4 protocols for a proxy without upstreams
ishustava Jul 23, 2023
b1df340
sidecar-controller: Support explicit destinations for L4 protocols an…
ishustava Jul 28, 2023
cf81b48
endpoints-controller: add workload identity to the service endpoints …
ishustava Aug 3, 2023
7945918
small fixes
ishustava Aug 4, 2023
2a1f01b
review comments
ishustava Aug 8, 2023
2d486d8
Make sure endpoint refs route to mesh port instead of an app port
ishustava Aug 8, 2023
eae4bf6
Address PR comments
ishustava Aug 10, 2023
32928bc
fixing copyright
rboyer Aug 23, 2023
f95b1b0
tidy imports
rboyer Aug 23, 2023
3727bd4
sidecar-proxy controller: Add support for transparent proxy
ishustava Aug 14, 2023
28e00f6
tidy imports
rboyer Aug 23, 2023
df7b6cd
add copyright headers
rboyer Aug 23, 2023
22bbfca
Prefix sidecar proxy test files with source and destination.
hc-github-team-consul-core Aug 30, 2023
867f2a0
Merge branch 'main' into jm/prefix-tests
jmurret Sep 8, 2023
151d470
Update controller_test.go
jmurret Sep 8, 2023
840483f
NET-5132 - Configure multiport routing for connect proxies in TProxy …
jmurret Sep 8, 2023
47b9dbd
formatting golden files
jmurret Sep 8, 2023
4456a60
Merge branch 'main' into jm/NET-5132
jmurret Sep 8, 2023
2841214
reverting golden files and adding changes in manually. build implici…
jmurret Sep 9, 2023
71d5b13
fixing files that were incorrectly repeating the outbound listener
jmurret Sep 9, 2023
dcf54e6
PR comments
jmurret Sep 11, 2023
8ad8daf
extract AlpnProtocol naming convention to getAlpnProtocolFromPortName…
jmurret Sep 11, 2023
6f5850a
Merge branch 'main' into jm/NET-5132
jmurret Sep 11, 2023
e1392e0
removing address level filtering.
jmurret Sep 12, 2023
b6d84f5
adding license to resources_test.go
jmurret Sep 12, 2023
bb771ec
fixing test names
jmurret Sep 12, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 22 additions & 14 deletions agent/agent.go
Original file line number Diff line number Diff line change
Expand Up @@ -947,20 +947,28 @@ func (a *Agent) configureXDSServer(proxyWatcher xds.ProxyWatcher) {
// TODO(agentless): rather than asserting the concrete type of delegate, we
// should add a method to the Delegate interface to build a ConfigSource.
if server, ok := a.delegate.(*consul.Server); ok {
catalogCfg := catalogproxycfg.NewConfigSource(catalogproxycfg.Config{
NodeName: a.config.NodeName,
LocalState: a.State,
LocalConfigSource: proxyWatcher,
Manager: a.proxyConfig,
GetStore: func() catalogproxycfg.Store { return server.FSM().State() },
Logger: a.proxyConfig.Logger.Named("server-catalog"),
SessionLimiter: a.baseDeps.XDSStreamLimiter,
})
go func() {
<-a.shutdownCh
catalogCfg.Shutdown()
}()
proxyWatcher = catalogCfg
switch proxyWatcher.(type) {
Copy link
Member Author

Choose a reason for hiding this comment

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

this was found in testing that v2 wiring up was still using configSource with proxy tracker as the local cache.

case *proxytracker.ProxyTracker:
go func() {
<-a.shutdownCh
proxyWatcher.(*proxytracker.ProxyTracker).Shutdown()
}()
default:
catalogCfg := catalogproxycfg.NewConfigSource(catalogproxycfg.Config{
NodeName: a.config.NodeName,
LocalState: a.State,
LocalConfigSource: proxyWatcher,
Manager: a.proxyConfig,
GetStore: func() catalogproxycfg.Store { return server.FSM().State() },
Logger: a.proxyConfig.Logger.Named("server-catalog"),
SessionLimiter: a.baseDeps.XDSStreamLimiter,
})
go func() {
<-a.shutdownCh
catalogCfg.Shutdown()
}()
proxyWatcher = catalogCfg
}
}
a.xdsServer = xds.NewServer(
a.config.NodeName,
Expand Down
2 changes: 1 addition & 1 deletion agent/consul/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -909,7 +909,6 @@ func (s *Server) registerControllers(deps Deps, proxyUpdater ProxyUpdater) {
}
return &bundle, nil
},
ProxyUpdater: proxyUpdater,
// This function is adapted from server_connect.go:getCARoots.
TrustDomainFetcher: func() (string, error) {
_, caConfig, err := s.fsm.State().CAConfig(nil)
Expand All @@ -920,6 +919,7 @@ func (s *Server) registerControllers(deps Deps, proxyUpdater ProxyUpdater) {
return s.getTrustDomain(caConfig)
},
LocalDatacenter: s.config.Datacenter,
ProxyUpdater: proxyUpdater,
})
}

Expand Down
3 changes: 2 additions & 1 deletion agent/xds/delta.go
Original file line number Diff line number Diff line change
Expand Up @@ -428,8 +428,9 @@ func newResourceIDFromEnvoyNode(node *envoy_config_core_v3.Node) *pbresource.ID
Tenancy: &pbresource.Tenancy{
Namespace: entMeta.NamespaceOrDefault(),
Partition: entMeta.PartitionOrDefault(),
PeerName: "local",
},
Type: mesh.ProxyStateTemplateConfigurationV1Alpha1Type,
Type: mesh.ProxyStateTemplateV1AlphaType,
}
}

Expand Down
12 changes: 12 additions & 0 deletions agent/xdsv2/listener_resources.go
Original file line number Diff line number Diff line change
Expand Up @@ -275,6 +275,14 @@ func makeEnvoyFilterChainMatch(routerMatch *pbproxystate.Match) *envoy_listener_
}
envoyFilterChainMatch.SourcePrefixRanges = ranges
}
if len(routerMatch.AlpnProtocols) > 0 {
sort.Strings(routerMatch.AlpnProtocols)
var alpnProtocols []string
for _, protocol := range routerMatch.AlpnProtocols {
alpnProtocols = append(alpnProtocols, protocol)
}
envoyFilterChainMatch.ApplicationProtocols = alpnProtocols
}
}
return envoyFilterChainMatch
}
Expand Down Expand Up @@ -527,6 +535,10 @@ func (pr *ProxyResources) makeEnvoyTLSParameters(defaultParams *pbproxystate.TLS
}

func (pr *ProxyResources) makeEnvoyTransportSocket(ts *pbproxystate.TransportSocket) (*envoy_core_v3.TransportSocket, error) {
// TODO(JM): did this just make tests pass. Figure out whether proxyState.Tls will always be available.
if pr.proxyState.Tls == nil {
return nil, nil
}
if ts == nil {
return nil, nil
}
Expand Down
111 changes: 111 additions & 0 deletions agent/xdsv2/resources_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
// Copyright (c) HashiCorp, Inc.
// SPDX-License-Identifier: BUSL-1.1

package xdsv2

import (
envoy_cluster_v3 "github.com/envoyproxy/go-control-plane/envoy/config/cluster/v3"
envoy_listener_v3 "github.com/envoyproxy/go-control-plane/envoy/config/listener/v3"
"github.com/hashicorp/consul/agent/xds/response"
"github.com/hashicorp/consul/envoyextensions/xdscommon"
proxytracker "github.com/hashicorp/consul/internal/mesh/proxy-tracker"
meshv1alpha1 "github.com/hashicorp/consul/proto-public/pbmesh/v1alpha1"
"github.com/hashicorp/consul/sdk/testutil"
"os"
"path/filepath"
"sort"
"testing"

"github.com/stretchr/testify/require"
"google.golang.org/protobuf/encoding/protojson"
"google.golang.org/protobuf/proto"
)

func TestResources_ImplicitDestinations(t *testing.T) {

cases := map[string]struct {
}{
"l4-single-implicit-destination-tproxy": {},
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does it only have one test case?

Copy link
Member Author

Choose a reason for hiding this comment

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

see my comment below. when I implemented the AlpnProtocol router match, I totally missed adding in logic for the proxyState -> xds conversion. so no tests failed and it was not until there was generated xds that you can verify it. There is no other place to put this kind of test so I added it here. I did not think we need to recreate all scenarios here since there should be less logic in this step of the conversion. I don't think we need many tests here, but we need something to able to assert that IR proto changes we make then properly get converted to XDS. Happy to entertain other ideas!

}

for name := range cases {
goldenValueInput := goldenValueJSON(t, name, "input")

proxyTemplate := jsonToProxyTemplate(t, goldenValueInput)
generator := NewResourceGenerator(testutil.Logger(t))

resources, err := generator.AllResourcesFromIR(&proxytracker.ProxyState{ProxyState: proxyTemplate.ProxyState})
require.NoError(t, err)

verifyClusterResourcesToGolden(t, resources, name)
verifyListenerResourcesToGolden(t, resources, name)

}
}

func verifyClusterResourcesToGolden(t *testing.T, resources map[string][]proto.Message, testName string) {
clusters := resources[xdscommon.ClusterType]

// The order of clusters returned via CDS isn't relevant, so it's safe
// to sort these for the purposes of test comparisons.
sort.Slice(clusters, func(i, j int) bool {
return clusters[i].(*envoy_cluster_v3.Cluster).Name < clusters[j].(*envoy_cluster_v3.Cluster).Name
})

resp, err := response.CreateResponse(xdscommon.ClusterType, "00000001", "00000001", clusters)
require.NoError(t, err)
gotJSON := protoToJSON(t, resp)

expectedJSON := goldenValue(t, filepath.Join("clusters", testName), "output")
require.JSONEq(t, expectedJSON, gotJSON)
}

func verifyListenerResourcesToGolden(t *testing.T, resources map[string][]proto.Message, testName string) {
listeners := resources[xdscommon.ListenerType]

// The order of clusters returned via CDS isn't relevant, so it's safe
// to sort these for the purposes of test comparisons.
sort.Slice(listeners, func(i, j int) bool {
return listeners[i].(*envoy_listener_v3.Listener).Name < listeners[j].(*envoy_listener_v3.Listener).Name
})

resp, err := response.CreateResponse(xdscommon.ListenerType, "00000001", "00000001", listeners)
require.NoError(t, err)
gotJSON := protoToJSON(t, resp)

expectedJSON := goldenValue(t, filepath.Join("listeners", testName), "output")
require.JSONEq(t, expectedJSON, gotJSON)
}

func protoToJSON(t *testing.T, pb proto.Message) string {
t.Helper()
m := protojson.MarshalOptions{
Indent: " ",
}
gotJSON, err := m.Marshal(pb)
require.NoError(t, err)
return string(gotJSON)
}

func jsonToProxyTemplate(t *testing.T, json []byte) *meshv1alpha1.ProxyStateTemplate {
t.Helper()
um := protojson.UnmarshalOptions{}
proxyTemplate := &meshv1alpha1.ProxyStateTemplate{}
err := um.Unmarshal(json, proxyTemplate)
require.NoError(t, err)
return proxyTemplate
}

func goldenValueJSON(t *testing.T, goldenFile, inputOutput string) []byte {
t.Helper()
goldenPath := filepath.Join("testdata", inputOutput, goldenFile) + ".golden"

content, err := os.ReadFile(goldenPath)
require.NoError(t, err)
return content
}

func goldenValue(t *testing.T, goldenFile, inputOutput string) string {
jmurret marked this conversation as resolved.
Show resolved Hide resolved
t.Helper()
return string(goldenValueJSON(t, goldenFile, inputOutput))
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
{
"proxyState": {
"identity": {
"tenancy": {
"partition": "default",
"namespace": "default",
"peerName": "local"
},
"name": "test-identity"
},
"listeners": [
{
"name": "outbound_listener",
"direction": "DIRECTION_OUTBOUND",
"hostPort": {
"host": "127.0.0.1",
"port": 15001
},
"routers": [
{
"match": {
"prefixRanges": [
{
"addressPrefix": "1.1.1.1",
"prefixLen": 32
}
],
"destinationPort": 8080
},
"l4": {
"name": "tcp.api-1.default.dc1.internal.foo.consul",
"statPrefix": "upstream.tcp.api-1.default.default.dc1"
}
}
],
"capabilities": [
"CAPABILITY_TRANSPARENT"
]
}
],
"clusters": {
"tcp.api-1.default.dc1.internal.foo.consul": {
"endpointGroup": {
"dynamic": {
"config": {
"disablePanicThreshold": true
},
"outboundTls": {
"outboundMesh": {
"identityKey": "test-identity",
"validationContext": {
"spiffeIds": [
"spiffe://foo.consul/ap/default/ns/default/identity/api1-identity"
]
},
"sni": "api-1.default.dc1.internal.foo.consul"
},
"alpnProtocols": [
"consul~tcp"
]
}
}
}
}
}
},
"requiredEndpoints": {
"api-1.default.dc1.internal.foo.consul": {
"id": {
"name": "api-1",
"type": {
"group": "catalog",
"groupVersion": "v1alpha1",
"kind": "ServiceEndpoints"
},
"tenancy": {
"partition": "default",
"namespace": "default",
"peerName": "local"
}
},
"port": "mesh"
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a copy-paste of the existing tests? If so, how were you thinking we'd keep these from diverging?

Copy link
Member Author

Choose a reason for hiding this comment

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

The other tests are golden files of proxyState, testing . This is a golden files for catalog/mesh vs -> proxy State.

This test and file is for testing proxyState -> xds conversion. We have xds golden file tests that test proxyState -> xds, but they are proxy state converter tests, so adding those there did not seem appropriate. I added there was nothing testing that an ALPN router actually got converted to XDS. No tests failed and it was not until running an integration test that I noticed this it was even missing. So I added this to ensure that it happened. This type of testing will continue to be needed as we expand the types of Matches we support as we add more functionality.

Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
{
"versionInfo": "00000001",
"resources": [
{
"@type": "type.googleapis.com/envoy.config.cluster.v3.Cluster",
"name": "tcp.api-1.default.dc1.internal.foo.consul",
"type": "EDS",
"edsClusterConfig": {
"edsConfig": {
"ads": {},
"resourceApiVersion": "V3"
}
}
}
],
"typeUrl": "type.googleapis.com/envoy.config.cluster.v3.Cluster",
"nonce": "00000001"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
{
"versionInfo": "00000001",
"resources": [
{
"@type": "type.googleapis.com/envoy.config.listener.v3.Listener",
"name": "outbound_listener",
"address": {
"socketAddress": {
"address": "127.0.0.1",
"portValue": 15001
}
},
"filterChains": [
{
"filterChainMatch": {
"destinationPort": 8080,
"prefixRanges": [
{
"addressPrefix": "1.1.1.1",
"prefixLen": 32
}
]
},
"filters": [
{
"name": "envoy.filters.network.tcp_proxy",
"typedConfig": {
"@type": "type.googleapis.com/envoy.extensions.filters.network.tcp_proxy.v3.TcpProxy",
"statPrefix": "upstream.tcp.api-1.default.default.dc1",
"cluster": "tcp.api-1.default.dc1.internal.foo.consul"
}
}
]
}
],
"listenerFilters": [
{
"name": "envoy.filters.listener.original_dst",
"typedConfig": {
"@type": "type.googleapis.com/envoy.extensions.filters.listener.original_dst.v3.OriginalDst"
}
}
],
"trafficDirection": "OUTBOUND"
}
],
"typeUrl": "type.googleapis.com/envoy.config.listener.v3.Listener",
"nonce": "00000001"
}
19 changes: 9 additions & 10 deletions internal/mesh/exports.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,16 +34,15 @@ var (

// Resource Types for the v1alpha1 version.

ProxyConfigurationV1Alpha1Type = types.ProxyConfigurationV1Alpha1Type
UpstreamsV1Alpha1Type = types.UpstreamsV1Alpha1Type
UpstreamsConfigurationV1Alpha1Type = types.UpstreamsConfigurationV1Alpha1Type
ProxyStateTemplateConfigurationV1Alpha1Type = types.ProxyStateTemplateV1Alpha1Type
HTTPRouteV1Alpha1Type = types.HTTPRouteV1Alpha1Type
GRPCRouteV1Alpha1Type = types.GRPCRouteV1Alpha1Type
TCPRouteV1Alpha1Type = types.TCPRouteV1Alpha1Type
DestinationPolicyV1Alpha1Type = types.DestinationPolicyV1Alpha1Type
ComputedRoutesV1Alpha1Type = types.ComputedRoutesV1Alpha1Type
ProxyStateTemplateV1AlphaType = types.ProxyStateTemplateV1Alpha1Type
ProxyConfigurationV1Alpha1Type = types.ProxyConfigurationV1Alpha1Type
UpstreamsV1Alpha1Type = types.UpstreamsV1Alpha1Type
UpstreamsConfigurationV1Alpha1Type = types.UpstreamsConfigurationV1Alpha1Type
HTTPRouteV1Alpha1Type = types.HTTPRouteV1Alpha1Type
GRPCRouteV1Alpha1Type = types.GRPCRouteV1Alpha1Type
TCPRouteV1Alpha1Type = types.TCPRouteV1Alpha1Type
DestinationPolicyV1Alpha1Type = types.DestinationPolicyV1Alpha1Type
ComputedRoutesV1Alpha1Type = types.ComputedRoutesV1Alpha1Type
ProxyStateTemplateV1AlphaType = types.ProxyStateTemplateV1Alpha1Type

// Resource Types for the latest version.

Expand Down
Loading