Skip to content

Commit

Permalink
PR feedback - LocalConnection and InboundConnection do not affect exp…
Browse files Browse the repository at this point in the history 
…osed routes. configure L7 route destinations. fix connection proto sequence numbers.
  • Loading branch information
@jmurret
jmurret committed Oct 13, 2023
1 parent 3b4201a commit 7b10439
Show file tree
Hide file tree
Showing 6 changed files with 193 additions and 21 deletions.
2 changes: 1 addition & 1 deletion internal/mesh/internal/controllers/sidecarproxy/builder/expose_paths.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ func (b *Builder) buildExposePaths(workload *pbcatalog.Workload) {
buildListener()

b.addExposePathsRoute(exposePath, clusterName).
addLocalAppCluster(clusterName, fmt.Sprintf("%d", exposePath.LocalPathPort)).
addLocalAppCluster(clusterName, nil).
addLocalAppStaticEndpoints(clusterName, exposePath.LocalPathPort)
}
}
Expand Down
25 changes: 16 additions & 9 deletions internal/mesh/internal/controllers/sidecarproxy/builder/local_app.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ func (b *Builder) BuildLocalApp(workload *pbcatalog.Workload, ctp *pbauth.Comput
if isL7(port.Protocol) {
b.addLocalAppRoute(routeName, clusterName)
}
b.addLocalAppCluster(clusterName, portName).
b.addLocalAppCluster(clusterName, &portName).
addLocalAppStaticEndpoints(clusterName, port.GetPort())
}
}
Expand Down Expand Up @@ -267,7 +267,7 @@ func (b *Builder) addInboundListener(name string, workload *pbcatalog.Workload)
// Add TLS inspection capability to be able to parse ALPN and/or SNI information from inbound connections.
listener.Capabilities = append(listener.Capabilities, pbproxystate.Capability_CAPABILITY_L4_TLS_INSPECTION)

if b.proxyCfg != nil && b.proxyCfg.DynamicConfig != nil && b.proxyCfg.DynamicConfig.InboundConnections != nil {
if b.proxyCfg.GetDynamicConfig() != nil && b.proxyCfg.GetDynamicConfig().InboundConnections != nil {
listener.BalanceConnections = pbproxystate.BalanceConnections(b.proxyCfg.DynamicConfig.InboundConnections.BalanceInboundConnections)
}
return b.NewListenerBuilder(listener)
Expand Down Expand Up @@ -301,9 +301,9 @@ func (l *ListenerBuilder) addInboundRouter(clusterName string, routeName string,

if ic != nil {
// MaxInboundConnections is uint32 that is used on:
// - router destinations MaxInboundConnection (uint64)
// - cluster circuit breakers UpstreamLimits.MaxConnections (uint32)
// It is cast to a uint64 here similarly as it is to the proxystateconverter code
// - router destinations MaxInboundConnection (uint64).
// - cluster circuit breakers UpstreamLimits.MaxConnections (uint32).
// It is cast to a uint64 here similarly as it is to the proxystateconverter code.
r.GetL4().MaxInboundConnections = uint64(ic.MaxInboundConnections)
}

Expand All @@ -326,6 +326,13 @@ func (l *ListenerBuilder) addInboundRouter(clusterName string, routeName string,
AlpnProtocols: []string{getAlpnProtocolFromPortName(portName)},
},
}

if ic != nil {
// MaxInboundConnections is cast to a uint64 here similarly as it is to the
// as the L4 case statement above and in proxystateconverter code.
r.GetL7().MaxInboundConnections = uint64(ic.MaxInboundConnections)
}

l.listener.Routers = append(l.listener.Routers, r)
}
return l
Expand Down Expand Up @@ -391,7 +398,7 @@ func isL7(protocol pbcatalog.Protocol) bool {
return false
}

func (b *Builder) addLocalAppCluster(clusterName, portName string) *Builder {
func (b *Builder) addLocalAppCluster(clusterName string, portName *string) *Builder {
// Make cluster for this router destination.
cluster := &pbproxystate.Cluster{
Group: &pbproxystate.Cluster_EndpointGroup{
Expand All @@ -404,8 +411,8 @@ func (b *Builder) addLocalAppCluster(clusterName, portName string) *Builder {
}

// configure inbound connections or connection timeout if either is defined
if b.proxyCfg.GetDynamicConfig() != nil {
lc, lcOK := b.proxyCfg.DynamicConfig.LocalConnection[portName]
if b.proxyCfg.GetDynamicConfig() != nil && portName != nil {
lc, lcOK := b.proxyCfg.DynamicConfig.LocalConnection[*portName]

if lcOK || b.proxyCfg.DynamicConfig.InboundConnections != nil {
cluster.GetEndpointGroup().GetStatic().Config = &pbproxystate.StaticEndpointGroupConfig{}
Expand All @@ -429,7 +436,7 @@ func (b *Builder) addLocalAppCluster(clusterName, portName string) *Builder {
}

func (b *Builder) addBlackHoleCluster() *Builder {
return b.addLocalAppCluster(xdscommon.BlackHoleClusterName, "")
return b.addLocalAppCluster(xdscommon.BlackHoleClusterName, nil)
}

func (b *Builder) addLocalAppStaticEndpoints(clusterName string, port uint32) {
Expand Down
21 changes: 20 additions & 1 deletion internal/mesh/internal/controllers/sidecarproxy/builder/local_app_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,9 @@ func TestBuildLocalApp_WithProxyConfiguration(t *testing.T) {
},
},
},
// source/local-and-inbound-connections shows that configuring LocalCOnnection
// and InboundConnections in DynamicConfig will set fields on standard clusters and routes,
// but will not set fields on exposed path clusters and routes.
"source/local-and-inbound-connections": {
workload: &pbcatalog.Workload{
Addresses: []*pbcatalog.WorkloadAddress{
Expand All @@ -152,7 +155,7 @@ func TestBuildLocalApp_WithProxyConfiguration(t *testing.T) {
Ports: map[string]*pbcatalog.WorkloadPort{
"port1": {Port: 8080, Protocol: pbcatalog.Protocol_PROTOCOL_TCP},
"port2": {Port: 20000, Protocol: pbcatalog.Protocol_PROTOCOL_MESH},
"port3": {Port: 8081, Protocol: pbcatalog.Protocol_PROTOCOL_TCP},
"port3": {Port: 8081, Protocol: pbcatalog.Protocol_PROTOCOL_HTTP},
},
},
proxyCfg: &pbmesh.ComputedProxyConfiguration{
Expand All @@ -169,6 +172,22 @@ func TestBuildLocalApp_WithProxyConfiguration(t *testing.T) {
MaxInboundConnections: 123,
BalanceInboundConnections: pbmesh.BalanceConnections(pbproxystate.BalanceConnections_BALANCE_CONNECTIONS_EXACT),
},
ExposeConfig: &pbmesh.ExposeConfig{
ExposePaths: []*pbmesh.ExposePath{
{
ListenerPort: 1234,
Path: "/health",
LocalPathPort: 9090,
Protocol: pbmesh.ExposePathProtocol_EXPOSE_PATH_PROTOCOL_HTTP,
},
{
ListenerPort: 1235,
Path: "GetHealth",
LocalPathPort: 9091,
Protocol: pbmesh.ExposePathProtocol_EXPOSE_PATH_PROTOCOL_HTTP2,
},
},
},
},
},
},
Expand Down
154 changes: 150 additions & 4 deletions ...nal/controllers/sidecarproxy/builder/testdata/source/local-and-inbound-connections.golden
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,18 @@
{
"proxyState": {
"clusters": {
"exposed_cluster_9090": {
"endpointGroup": {
"static": {}
},
"name": "exposed_cluster_9090"
},
"exposed_cluster_9091": {
"endpointGroup": {
"static": {}
},
"name": "exposed_cluster_9091"
},
"local_app:port1": {
"endpointGroup": {
"static": {
Expand Down Expand Up @@ -33,6 +45,26 @@
}
},
"endpoints": {
"exposed_cluster_9090": {
"endpoints": [
{
"hostPort": {
"host": "127.0.0.1",
"port": 9090
}
}
]
},
"exposed_cluster_9091": {
"endpoints": [
{
"hostPort": {
"host": "127.0.0.1",
"port": 9091
}
}
]
},
"local_app:port1": {
"endpoints": [
{
Expand Down Expand Up @@ -116,12 +148,13 @@
}
}
},
"l4": {
"cluster": {
"name": "local_app:port3"
"l7": {
"route": {
"name": "public_listener:port3"
},
"maxInboundConnections": 123,
"statPrefix": "public_listener",
"staticRoute": true,
"trafficPermissions": {}
},
"match": {
Expand All @@ -131,8 +164,121 @@
}
}
]
},
{
"direction": "DIRECTION_INBOUND",
"hostPort": {
"host": "10.0.0.1",
"port": 9090
},
"name": "exposed_path_health",
"routers": [
{
"l7": {
"route": {
"name": "exposed_path_filter_health_1234"
},
"statPrefix": "exposed_path_filter_health_1234",
"staticRoute": true
}
}
]
},
{
"direction": "DIRECTION_INBOUND",
"hostPort": {
"host": "10.0.0.1",
"port": 9091
},
"name": "exposed_path_GetHealth",
"routers": [
{
"l7": {
"protocol": "L7_PROTOCOL_HTTP2",
"route": {
"name": "exposed_path_filter_GetHealth_1235"
},
"statPrefix": "exposed_path_filter_GetHealth_1235",
"staticRoute": true
}
}
]
}
]
],
"routes": {
"exposed_path_filter_GetHealth_1235": {
"virtualHosts": [
{
"domains": [
"*"
],
"name": "exposed_path_filter_GetHealth_1235",
"routeRules": [
{
"destination": {
"cluster": {
"name": "exposed_cluster_9091"
}
},
"match": {
"pathMatch": {
"exact": "GetHealth"
}
}
}
]
}
]
},
"exposed_path_filter_health_1234": {
"virtualHosts": [
{
"domains": [
"*"
],
"name": "exposed_path_filter_health_1234",
"routeRules": [
{
"destination": {
"cluster": {
"name": "exposed_cluster_9090"
}
},
"match": {
"pathMatch": {
"exact": "/health"
}
}
}
]
}
]
},
"public_listener:port3": {
"virtualHosts": [
{
"domains": [
"*"
],
"name": "public_listener:port3",
"routeRules": [
{
"destination": {
"cluster": {
"name": "local_app:port3"
}
},
"match": {
"pathMatch": {
"prefix": "/"
}
}
}
]
}
]
}
}
},
"requiredLeafCertificates": {
"test-identity": {
Expand Down
8 changes: 4 additions & 4 deletions proto-public/pbmesh/v2beta1/connection.pb.go
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions proto-public/pbmesh/v2beta1/connection.proto
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@ message ConnectionConfig {

// Referenced by ProxyConfiguration
message InboundConnectionsConfig {
uint32 max_inbound_connections = 12;
BalanceConnections balance_inbound_connections = 13;
uint32 max_inbound_connections = 1;
BalanceConnections balance_inbound_connections = 2;
}

// +kubebuilder:validation:Enum=BALANCE_CONNECTIONS_DEFAULT;BALANCE_CONNECTIONS_EXACT
Expand Down

0 comments on commit 7b10439

Please sign in to comment.