Quick fixes for interception and debugging

galley/docker/Dockerfile.galley

@@ -1,3 +1,3 @@
-FROM scratch
+FROM istionightly/base_debug
 ADD galley /usr/local/bin/
 ENTRYPOINT ["/usr/local/bin/galley"]

install/kubernetes/helm/istio/templates/sidecar-injector-configmap.yaml

@@ -17,6 +17,7 @@ data:
       rewriteAppHTTPProbe: {{ .Values.sidecarInjectorWebhook.rewriteAppHTTPProbe }}
 {{- if or (not .Values.istio_cni.enabled) .Values.global.proxy.enableCoreDump }}
       initContainers:
+      {{ "[[ if ne (annotation .ObjectMeta `sidecar.istio.io/interceptionMode` .ProxyConfig.InterceptionMode) \"NONE\" ]]" }}
 {{- if not .Values.istio_cni.enabled }}
       - name: istio-init
 {{- if contains "/" .Values.global.proxy_init.image }}
@@ -56,6 +57,7 @@ data:
           {{- end }}
         restartPolicy: Always
 {{- end }}
+      {{ "[[ end -]]" }}
       {{- if eq .Values.global.proxy.enableCoreDump true }}
       - name: enable-core-dump
         args:

pilot/codecov.requirement

@@ -16,6 +16,7 @@ istio.io/istio/pilot/pkg/model:83 [85.8]
 istio.io/istio/pilot/pkg/model/test:0 [0]
 istio.io/istio/pilot/pkg/proxy:95 [98.8]
 istio.io/istio/pilot/pkg/proxy/envoy:72 [75.4]
+istio.io/istio/pilot/pkg/proxy/envoy/v2:30 [32]
 istio.io/istio/pilot/pkg/serviceregistry:0 [0]
 istio.io/istio/pilot/pkg/serviceregistry/consul:40 [43.6]
 istio.io/istio/pilot/pkg/serviceregistry/kube:60 [63.8]

pilot/pkg/kube/inject/inject.go

@@ -32,7 +32,9 @@ import (
 
 	"github.com/ghodss/yaml"
 	"github.com/gogo/protobuf/types"
+
 	multierror "github.com/hashicorp/go-multierror"
+
 	"k8s.io/api/batch/v2alpha1"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -42,6 +44,7 @@ import (
 	yamlDecoder "k8s.io/apimachinery/pkg/util/yaml"
 
 	meshconfig "istio.io/api/mesh/v1alpha1"
+	"istio.io/istio/pilot/pkg/model"
 	"istio.io/istio/pkg/log"
 )
 
@@ -301,8 +304,9 @@ func validateInterceptionMode(mode string) error {
 	switch mode {
 	case meshconfig.ProxyConfig_REDIRECT.String():
 	case meshconfig.ProxyConfig_TPROXY.String():
+	case string(model.InterceptionNone): // not a global mesh config - must be enabled for each sidecar
 	default:
-		return fmt.Errorf("interceptionMode invalid: %v", mode)
+		return fmt.Errorf("interceptionMode invalid, use REDIRECT,TPROXY,NONE: %v", mode)
 	}
 	return nil
 }

pilot/pkg/model/context.go

@@ -488,6 +488,10 @@ const (
 	// NodeConfigNamespace is the name of the metadata variable that carries info about
 	// the config namespace associated with the proxy
 	NodeConfigNamespace = "CONFIG_NAMESPACE"
+
+	// NodeMetadataUID is the user ID running envoy. Pilot can check if envoy runs as root, and may generate
+	// different configuration. If not set, the default istio-proxy UID (1337) is assumed.
+	NodeMetadataUID = "UID"
 )
 
 // TrafficInterceptionMode indicates how traffic to/from the workload is captured and

pilot/pkg/model/sidecar.go

@@ -214,6 +214,7 @@ func convertIstioListenerToWrapper(ps *PushContext, sidecarConfig *Config,
 	}
 
 	if istioListener.Hosts != nil {
+		// TODO: BUG, this means you can't have 2 explicit imports for same namespace ( override each other )
 		for _, h := range istioListener.Hosts {
 			parts := strings.SplitN(h, "/", 2)
 			if parts[0] == currentNamespace {

pilot/pkg/networking/core/v1alpha3/cluster.go

@@ -424,14 +424,15 @@ func (configgen *ConfigGeneratorImpl) buildInboundClusters(env *model.Environmen
 	// If the node has no sidecarScope and has interception mode set to NONE, then we should skip the inbound
 	// clusters, because there would be no corresponding inbound listeners
 	sidecarScope := proxy.SidecarScope
+	noneMode := proxy.GetInterceptionMode() == model.InterceptionNone
 
 	if sidecarScope == nil || !sidecarScope.HasCustomIngressListeners {
 		// No user supplied sidecar scope or the user supplied one has no ingress listeners
 
 		// We should not create inbound listeners in NONE mode based on the service instances
 		// Doing so will prevent the workloads from starting as they would be listening on the same port
 		// Users are required to provide the sidecar config to define the inbound listeners
-		if proxy.GetInterceptionMode() == model.InterceptionNone {
+		if noneMode {
 			return nil
 		}
 
@@ -459,7 +460,12 @@ func (configgen *ConfigGeneratorImpl) buildInboundClusters(env *model.Environmen
 			clusters = append(clusters, mgmtCluster)
 		}
 	} else {
+		if instances == nil || len(instances) == 0 {
+			return clusters
+		}
 		rule := sidecarScope.Config.Spec.(*networking.Sidecar)
+		// Will generate inbound listeners based on ingress listed ports.
+		// TODO: verify that Ingress contains the ports defined by app ( no need to duplicate )
 		for _, ingressListener := range rule.Ingress {
 			// LDS would have setup the inbound clusters
 			// as inbound|portNumber|portName|Hostname
@@ -486,13 +492,24 @@ func (configgen *ConfigGeneratorImpl) buildInboundClusters(env *model.Environmen
 				}
 			}
 
-			// First create a copy of a service instance
-			instance := &model.ServiceInstance{
-				Endpoint:       instances[0].Endpoint,
-				Service:        instances[0].Service,
-				Labels:         instances[0].Labels,
-				ServiceAccount: instances[0].ServiceAccount,
+			// TODO: Sidecar config has port number and name - but the port number is the targetPort.
+			// Right now we don't track the targetPort in the ServiceInstance - only the service port.
+			// For now we'll match on name ( and require the name of the port to be unique inside a namespace )
+			// Proper fix is to track targetPort, or use service port number in Sidecar
+			instance := configgen.findInstance(instances, ingressListener)
+
+			if instance == nil {
+				// We didn't find a matching port
+				continue
 			}
+			//// TODO: this can't be correct, what happens with the other instances ? Likely works for test with single svc only
+			//// First create a copy of a service instance
+			//instance := &model.ServiceInstance{
+			//	Endpoint:       instances[0].Endpoint,
+			//	Service:        instances[0].Service,
+			//	Labels:         instances[0].Labels,
+			//	ServiceAccount: instances[0].ServiceAccount,
+			//}
 
 			// Update the values here so that the plugins use the right ports
 			// uds values
@@ -506,7 +523,7 @@ func (configgen *ConfigGeneratorImpl) buildInboundClusters(env *model.Environmen
 			pluginParams := &plugin.InputParams{
 				Env:             env,
 				Node:            proxy,
-				ServiceInstance: instances[0],
+				ServiceInstance: instance,
 				Port:            listenPort,
 				Push:            push,
 				Bind:            bind,
@@ -519,6 +536,41 @@ func (configgen *ConfigGeneratorImpl) buildInboundClusters(env *model.Environmen
 	return clusters
 }
 
+func (configgen *ConfigGeneratorImpl) findInstance(instances []*model.ServiceInstance,
+	ingressListener *networking.IstioIngressListener) *model.ServiceInstance {
+	var instance *model.ServiceInstance
+	// Search by port
+	for _, realinstance := range instances {
+		if realinstance.Endpoint.Port == int(ingressListener.Port.Number) {
+			instance = &model.ServiceInstance{
+				Endpoint:       realinstance.Endpoint,
+				Service:        realinstance.Service,
+				Labels:         realinstance.Labels,
+				ServiceAccount: realinstance.ServiceAccount,
+			}
+			return instance
+		}
+	}
+
+	// search by name
+	for _, realinstance := range instances {
+		// TODO: UDS may not work, we could use the port name
+		for _, iport := range realinstance.Service.Ports {
+			if iport.Name == ingressListener.Port.Name {
+				instance = &model.ServiceInstance{
+					Endpoint:       realinstance.Endpoint,
+					Service:        realinstance.Service,
+					Labels:         realinstance.Labels,
+					ServiceAccount: realinstance.ServiceAccount,
+				}
+				return instance
+			}
+		}
+	}
+
+	return instance
+}
+
 func (configgen *ConfigGeneratorImpl) buildInboundClusterForPortOrUDS(pluginParams *plugin.InputParams) *apiv2.Cluster {
 	instance := pluginParams.ServiceInstance
 	clusterName := model.BuildSubsetKey(model.TrafficDirectionInbound, instance.Endpoint.ServicePort.Name,

pilot/pkg/networking/core/v1alpha3/configgen.go

@@ -54,6 +54,7 @@ func (configgen *ConfigGeneratorImpl) BuildSharedPushState(env *model.Environmen
 	namespaceMap[""] = struct{}{}
 
 	// generate outbound clusters for all namespaces in parallel.
+	// TODO: for large number of clusters this may result in OOM, needs rate control !!!
 	wg := &sync.WaitGroup{}
 	mutex := &sync.Mutex{}
 	wg.Add(len(namespaceMap))

pilot/pkg/networking/core/v1alpha3/listener.go

@@ -30,7 +30,7 @@ import (
 	accesslog "github.com/envoyproxy/go-control-plane/envoy/config/filter/accesslog/v2"
 	http_conn "github.com/envoyproxy/go-control-plane/envoy/config/filter/network/http_connection_manager/v2"
 	tcp_proxy "github.com/envoyproxy/go-control-plane/envoy/config/filter/network/tcp_proxy/v2"
-	envoy_type "github.com/envoyproxy/go-control-plane/envoy/type"
+	"github.com/envoyproxy/go-control-plane/envoy/type"
 	xdsutil "github.com/envoyproxy/go-control-plane/pkg/util"
 	google_protobuf "github.com/gogo/protobuf/types"
 	"github.com/prometheus/client_golang/prometheus"
@@ -395,7 +395,9 @@ func (configgen *ConfigGeneratorImpl) buildSidecarInboundListeners(env *model.En
 
 	} else {
 		rule := sidecarScope.Config.Spec.(*networking.Sidecar)
-
+		if len(proxyInstances) == 0 {
+			return listeners // no services defined.
+		}
 		for _, ingressListener := range rule.Ingress {
 			// determine the bindToPort setting for listeners
 			bindToPort := false
@@ -420,6 +422,16 @@ func (configgen *ConfigGeneratorImpl) buildSidecarInboundListeners(env *model.En
 				Name:     ingressListener.Port.Name,
 			}
 
+			// if app doesn't have a declared ServicePort, but a sidecar ingress is defined - we can't generate a listener
+			// for that port since we don't know what policies or configs apply to it ( many are based on service matching).
+			// Sidecar doesn't include all the info needed to configure a port.
+			instance := configgen.findInstance(proxyInstances, ingressListener)
+
+			if instance == nil {
+				// We didn't find a matching port
+				continue
+			}
+
 			bind := ingressListener.Bind
 			// if bindToPort is true, we set the bind address if empty to 0.0.0.0 - this is an inbound port.
 			if len(bind) == 0 && bindToPort {
@@ -428,7 +440,7 @@ func (configgen *ConfigGeneratorImpl) buildSidecarInboundListeners(env *model.En
 				// auto infer the IP from the proxyInstances
 				// We assume all endpoints in the proxy instances have the same IP
 				// as they should all be pointing to the same network endpoint
-				bind = proxyInstances[0].Endpoint.Address
+				bind = instance.Endpoint.Address
 			}
 
 			listenerOpts := buildListenerOpts{
@@ -441,21 +453,6 @@ func (configgen *ConfigGeneratorImpl) buildSidecarInboundListeners(env *model.En
 				bindToPort:     bindToPort,
 			}
 
-			// Construct a dummy service instance for this port so that the rest of the code doesn't freak out
-			// due to a missing instance. Technically this instance is not a service instance as it corresponds to
-			// some workload listener. But given that we force all workloads to be part of atleast one service,
-			// lets create a service instance for this workload based on the first service associated with the workload.
-			// TODO: We are arbitrarily using the first proxyInstance. When a workload has multiple services bound to it,
-			// what happens? We could run the loop for every instance but we would have the same listeners.
-
-			// First create a copy of a service instance
-			instance := &model.ServiceInstance{
-				Endpoint:       proxyInstances[0].Endpoint,
-				Service:        proxyInstances[0].Service,
-				Labels:         proxyInstances[0].Labels,
-				ServiceAccount: proxyInstances[0].ServiceAccount,
-			}
-
 			// Update the values here so that the plugins use the right ports
 			// uds values
 			// TODO: all plugins need to be updated to account for the fact that
@@ -690,16 +687,9 @@ func (configgen *ConfigGeneratorImpl) buildSidecarOutboundListeners(env *model.E
 		meshGateway := map[string]bool{model.IstioMeshGateway: true}
 		virtualServices := push.VirtualServices(node, meshGateway)
 
-		// determine the bindToPort setting for listeners
+		// bindToPort will be false - 'none' mode requires Sidecar.
 		bindToPort := false
-		if noneMode {
-			bindToPort = true
-		}
-
 		bind := ""
-		if bindToPort {
-			bind = LocalhostAddress
-		}
 		for _, service := range services {
 			for _, servicePort := range service.Ports {
 				// if the workload has NONE mode interception, then we generate TCP ports only
@@ -754,7 +744,7 @@ func (configgen *ConfigGeneratorImpl) buildSidecarOutboundListeners(env *model.E
 
 			// determine the bindToPort setting for listeners
 			bindToPort := false
-			if node.GetInterceptionMode() == model.InterceptionNone {
+			if noneMode {
 				// dont care what the listener's capture mode setting is. The proxy does not use iptables
 				bindToPort = true
 			} else {
@@ -802,7 +792,6 @@ func (configgen *ConfigGeneratorImpl) buildSidecarOutboundListeners(env *model.E
 						bind = WildcardAddress
 					}
 				}
-
 				listenerOpts := buildListenerOpts{
 					env:            env,
 					proxy:          node,
@@ -857,6 +846,9 @@ func (configgen *ConfigGeneratorImpl) buildSidecarOutboundListeners(env *model.E
 				}
 				for _, service := range services {
 					for _, servicePort := range service.Ports {
+						if !validatePort(node, servicePort.Port, bindToPort) {
+							continue // port is not valid for this node, skipping
+						}
 						listenerOpts := buildListenerOpts{
 							env:            env,
 							proxy:          node,
@@ -904,6 +896,24 @@ func (configgen *ConfigGeneratorImpl) buildSidecarOutboundListeners(env *model.E
 	return append(tcpListeners, httpListeners...)
 }
 
+// validatePort may remap privileged ports. Proxy doesn't run as root, so if bind=true ports <1024 will
+// not be accepted, envoy will reject the full config.
+func validatePort(node *model.Proxy, i int, bindToPort bool) bool {
+	if !bindToPort {
+		return true // all good, iptables doesn't care
+	}
+	if i > 1024 {
+		return true
+	}
+	remapBase := node.Metadata[model.NodeMetadataRemapBase]
+	if remapBase == "0" {
+		return true
+	}
+
+	// TODO: add some setting or override
+	return false
+}
+
 // buildSidecarOutboundListenerForPortOrUDS builds a single listener and
 // adds it to the listenerMap provided by the caller.  Listeners are added
 // if one doesn't already exist. HTTP listeners on same port are ignored

pilot/pkg/networking/core/v1alpha3/listener_test.go

@@ -188,7 +188,7 @@ func testInboundListenerConfigWithSidecar(t *testing.T, services ...*model.Servi
 			Ingress: []*networking.IstioIngressListener{
 				{
 					Port: &networking.Port{
-						Number:   80,
+						Number:   8080,
 						Protocol: "HTTP",
 						Name:     "uds",
 					},
@@ -411,6 +411,7 @@ func buildService(hostname string, ip string, protocol model.Protocol, creationT
 func buildEndpoint(service *model.Service) model.NetworkEndpoint {
 	return model.NetworkEndpoint{
 		ServicePort: service.Ports[0],
+		Port:        8080,
 	}
 }