Skip to content

Commit

Permalink
Fix Sidecar egress TCP listener doesn't use auto allocated address (i…
Browse files Browse the repository at this point in the history 
…stio#41311)

* Fix Sidecar egress TCP listener doesn't use auto allocated address when Port is specified

When proxy metadata DNSCapture and DNSAutoAllocate are enabled, if Sidecar egress listener
for TCP service is defined with port number, the generated sidecar outbound listener
doesn't bind to auto allocated address(240.240.x.x).

* actual fix code

* address comments
  • Loading branch information
@l8huang
l8huang authored Oct 12, 2022
1 parent 43e3ec8 commit b8876c8
Show file tree
Hide file tree
Showing 2 changed files with 133 additions and 27 deletions.
50 changes: 23 additions & 27 deletions pilot/pkg/networking/core/v1alpha3/listener.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -316,6 +316,28 @@ func (lb *ListenerBuilder) buildSidecarOutboundListeners(node *model.Proxy,
}
}

// If capture mode is NONE i.e., bindToPort is true, and
// Bind IP + Port is specified, we will bind to the specified IP and Port.
// This specified IP is ideally expected to be a loopback IP.
//
// If capture mode is NONE i.e., bindToPort is true, and
// only Port is specified, we will bind to the default loopback IP
// 127.0.0.1 and the specified Port.
//
// If capture mode is NONE, i.e., bindToPort is true, and
// only Bind IP is specified, we will bind to the specified IP
// for each port as defined in the service registry.
//
// If captureMode is not NONE, i.e., bindToPort is false, then
// we will bind to user specified IP (if any) or to the VIPs of services in
// this egress listener.
var bind string
if egressListener.IstioListener != nil && egressListener.IstioListener.Bind != "" {
bind = egressListener.IstioListener.Bind
} else if bindToPort {
bind = actualLocalHosts[0]
}

if egressListener.IstioListener != nil &&
egressListener.IstioListener.Port != nil {
// We have a non catch all listener on some user specified port
Expand All @@ -338,35 +360,17 @@ func (lb *ListenerBuilder) buildSidecarOutboundListeners(node *model.Proxy,
Name: egressListener.IstioListener.Port.Name,
}

// If capture mode is NONE i.e., bindToPort is true, and
// Bind IP + Port is specified, we will bind to the specified IP and Port.
// This specified IP is ideally expected to be a loopback IP.
//
// If capture mode is NONE i.e., bindToPort is true, and
// only Port is specified, we will bind to the default loopback IP
// 127.0.0.1 and the specified Port.
//
// If capture mode is NONE, i.e., bindToPort is true, and
// only Bind IP is specified, we will bind to the specified IP
// for each port as defined in the service registry.
//
// If captureMode is not NONE, i.e., bindToPort is false, then
// we will bind to user specified IP (if any) or to the VIPs of services in
// this egress listener.
var extraBind []string
bind := egressListener.IstioListener.Bind
if bind == "" {
if egressListener.IstioListener.Bind == "" {
if bindToPort {
// the first local host would be the binding address and
// the rest would be the additional addresses
bind = actualLocalHosts[0]
if len(actualLocalHosts) > 1 {
extraBind = actualLocalHosts[1:]
}
} else {
// the first wildcard address would be the binding address and
// the rest would be the additional addresses
bind = actualWildcards[0]
if len(actualWildcards) > 1 {
extraBind = actualWildcards[1:]
}
Expand Down Expand Up @@ -413,14 +417,6 @@ func (lb *ListenerBuilder) buildSidecarOutboundListeners(node *model.Proxy,
e.locked = true
}

var bind string
if egressListener.IstioListener != nil && egressListener.IstioListener.Bind != "" {
bind = egressListener.IstioListener.Bind
}
if bindToPort && bind == "" {
bind = actualLocalHosts[0]
}

// Build ListenerOpts and PluginParams once and reuse across all Services to avoid unnecessary allocations.
listenerOpts := buildListenerOpts{
push: push,
Expand Down
110 changes: 110 additions & 0 deletions pilot/pkg/networking/core/v1alpha3/listener_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2999,3 +2999,113 @@ func TestFilterChainMatchEqual(t *testing.T) {
})
}
}

func TestOutboundListenerConfig_WithAutoAllocatedAddress(t *testing.T) {
const tcpPort = 79
services := []*model.Service{
{
CreationTime: tnow.Add(1 * time.Second),
Hostname: host.Name("test1.com"),
DefaultAddress: wildcardIPv4,
AutoAllocatedIPv4Address: "240.240.0.100",
Ports: model.PortList{
&model.Port{
Name: "tcp",
Port: tcpPort,
Protocol: protocol.TCP,
},
},
Resolution: model.DNSLB,
Attributes: model.ServiceAttributes{
Namespace: "default",
},
},
}

sidecarConfig := &config.Config{
Meta: config.Meta{
Name: "sidecar-with-tcp",
Namespace: "not-default",
GroupVersionKind: gvk.Sidecar,
},
Spec: &networking.Sidecar{
Egress: []*networking.IstioEgressListener{
{
Hosts: []string{"default/*"},
},
},
},
}

sidecarConfigWithPort := &config.Config{
Meta: config.Meta{
Name: "sidecar-with-tcp-port",
Namespace: "not-default",
GroupVersionKind: gvk.Sidecar,
},
Spec: &networking.Sidecar{
Egress: []*networking.IstioEgressListener{
{
Hosts: []string{"default/*"},
Port: &networking.Port{
Number: tcpPort,
Protocol: "TCP",
Name: "tcp",
},
},
},
},
}

tests := []struct {
name string
services []*model.Service
sidecar *config.Config
numListenersOnServicePort int
useAutoAllocatedAddress bool
}{
{
name: "egress tcp with auto allocated address",
services: services,
sidecar: sidecarConfig,
numListenersOnServicePort: 1,
useAutoAllocatedAddress: true,
},
{
name: "egress tcp and port with auto allocated address",
services: services,
sidecar: sidecarConfigWithPort,
numListenersOnServicePort: 1,
useAutoAllocatedAddress: true,
},
}

proxy := getProxy()
proxy.Metadata.DNSCapture = true
proxy.Metadata.DNSAutoAllocate = true

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
listeners := buildOutboundListeners(t, proxy, tt.sidecar, nil, services...)

listenersToCheck := make([]string, 0)
for _, l := range listeners {
if l.Address.GetSocketAddress().GetPortValue() == tcpPort {
listenersToCheck = append(listenersToCheck, l.Address.GetSocketAddress().GetAddress())
}
}

if len(listenersToCheck) != tt.numListenersOnServicePort {
t.Errorf("Expected %d listeners, got %d (%v)", tt.numListenersOnServicePort, len(listenersToCheck), listenersToCheck)
}

if tt.useAutoAllocatedAddress {
for _, addr := range listenersToCheck {
if !strings.HasPrefix(addr, "240.240") {
t.Errorf("Expected %d listeners on service port 79, got %d (%v)", tt.numListenersOnServicePort, len(listenersToCheck), listenersToCheck)
}
}
}
})
}
}

0 comments on commit b8876c8

Please sign in to comment.