Fixes for k8s ingress (istio#11343)

* Fix ingress in pilot, writeback and multiple namespaces

* Fix tests, format

* Fix test - the generated service should be left in the namespace of ingress

* Additional test fixes, match the new 1.1 semantics

* Again make fmt and lint not matching

istioctl/cmd/istioctl/convert_ingress_test.go

@@ -23,6 +23,7 @@ import (
 )
 
 func TestConvertIngress(t *testing.T) {
+
 	tt := []struct {
 		in  []string
 		out string

istioctl/cmd/istioctl/testdata/v1alpha3/merged-gateway.yaml

@@ -3,10 +3,10 @@ kind: VirtualService
 metadata:
   creationTimestamp: null
   name: wild-simple-ingress-istio-autogenerated-k8s-ingress
-  namespace: istio-system
+  namespace: default
 spec:
   gateways:
-  - istio-autogenerated-k8s-ingress
+  - istio-system/istio-autogenerated-k8s-ingress
   hosts:
   - '*'
   http:

istioctl/cmd/istioctl/testdata/v1alpha3/merged-gateway.yaml.golden

@@ -3,10 +3,10 @@ kind: VirtualService
 metadata:
   creationTimestamp: null
   name: wild-simple-ingress-istio-autogenerated-k8s-ingress
-  namespace: istio-system
+  namespace: default
 spec:
   gateways:
-  - istio-autogenerated-k8s-ingress
+  - istio-system/istio-autogenerated-k8s-ingress
   hosts:
   - '*'
   http:

istioctl/cmd/istioctl/testdata/v1alpha3/myservice-gateway.yaml

@@ -3,10 +3,10 @@ kind: VirtualService
 metadata:
   creationTimestamp: null
   name: wild-simple-ingress-istio-autogenerated-k8s-ingress
-  namespace: istio-system
+  namespace: default
 spec:
   gateways:
-  - istio-autogenerated-k8s-ingress
+  - istio-system/istio-autogenerated-k8s-ingress
   hosts:
   - '*'
   http:

istioctl/cmd/istioctl/testdata/v1alpha3/myservice-gateway.yaml.golden

@@ -3,10 +3,10 @@ kind: VirtualService
 metadata:
   creationTimestamp: null
   name: wild-simple-ingress-istio-autogenerated-k8s-ingress
-  namespace: istio-system
+  namespace: default
 spec:
   gateways:
-  - istio-autogenerated-k8s-ingress
+  - istio-system/istio-autogenerated-k8s-ingress
   hosts:
   - '*'
   http:

pilot/pkg/config/kube/ingress/controller.go

@@ -18,6 +18,7 @@ package ingress
 
 import (
 	"errors"
+	"os"
 	"reflect"
 	"time"
 
@@ -32,6 +33,32 @@ import (
 	"istio.io/istio/pkg/log"
 )
 
+// In 1.0, the Gateway is defined in the namespace where the actual controller runs, and needs to be managed by
+// user.
+// The gateway is named by appending "-istio-autogenerated-k8s-ingress" to the name of the ingress.
+//
+// Currently the gateway namespace is hardcoded to istio-system (model.IstioIngressNamespace)
+//
+// VirtualServices are also auto-generated in the model.IstioIngressNamespace.
+//
+// The sync of Ingress objects to IP is done by status.go
+// the 'ingress service' name is used to get the IP of the Service
+// If ingress service is empty, it falls back to NodeExternalIP list, selected using the labels.
+// This is using 'namespace' of pilot - but seems to be broken (never worked), since it uses Pilot's pod labels
+// instead of the ingress labels.
+
+// Follows mesh.IngressControllerMode setting to enable - OFF|STRICT|DEFAULT.
+// STRICT requires "kubernetes.io/ingress.class" == mesh.IngressClass
+// DEFAULT allows Ingress without explicit class.
+
+// In 1.1:
+// - K8S_INGRESS_NS - namespace of the Gateway that will act as ingress.
+// - labels of the gateway set to "app=ingressgateway" for node_port, service set to 'ingressgateway' (matching default install)
+//   If we need more flexibility - we can add it (but likely we'll deprecate ingress support first)
+// -
+
+// Control needs RBAC permissions to write to Pods.
+
 type controller struct {
 	mesh         *meshconfig.MeshConfig
 	domainSuffix string
@@ -42,6 +69,11 @@ type controller struct {
 	handler  *kube.ChainHandler
 }
 
+var (
+	// TODO: move to features ( and remove in 1.2 )
+	ingressNamespace = os.Getenv("K8S_INGRESS_NS")
+)
+
 var (
 	errUnsupportedOp = errors.New("unsupported operation: the ingress config store is a read-only view")
 )
@@ -54,6 +86,10 @@ func NewController(client kubernetes.Interface, mesh *meshconfig.MeshConfig,
 	// queue requires a time duration for a retry delay after a handler error
 	queue := kube.NewQueue(1 * time.Second)
 
+	if ingressNamespace == "" {
+		ingressNamespace = model.IstioIngressNamespace
+	}
+
 	log.Infof("Ingress controller watching namespaces %q", options.WatchedNamespace)
 	informer := v1beta1.NewFilteredIngressInformer(client, options.WatchedNamespace, options.ResyncPeriod, cache.Indexers{}, nil)
 	informer.AddEventHandler(

pilot/pkg/config/kube/ingress/conversion.go

@@ -68,6 +68,8 @@ func ConvertIngressV1alpha3(ingress v1beta1.Ingress, domainSuffix string) model.
 
 	// FIXME this is a temporary hack until all test templates are updated
 	//for _, tls := range ingress.Spec.TLS {
+
+	// TODO: add secretName (converted to sdsName)
 	if len(ingress.Spec.TLS) > 0 {
 		tls := ingress.Spec.TLS[0] // FIXME
 		// TODO validation when multiple wildcard tls secrets are given
@@ -82,7 +84,7 @@ func ConvertIngressV1alpha3(ingress v1beta1.Ingress, domainSuffix string) model.
 			},
 			Hosts: tls.Hosts,
 			// While we accept multiple certs, we expect them to be mounted in
-			// /etc/istio/certs/namespace/secretname/tls.crt|tls.key
+			// /etc/istio/ingress-certs/tls.crt|tls.key|root-cert.pem
 			Tls: &networking.Server_TLSOptions{
 				HttpsRedirect: false,
 				Mode:          networking.Server_TLSOptions_SIMPLE,
@@ -110,7 +112,7 @@ func ConvertIngressV1alpha3(ingress v1beta1.Ingress, domainSuffix string) model.
 			Group:     model.Gateway.Group,
 			Version:   model.Gateway.Version,
 			Name:      ingress.Name + "-" + model.IstioIngressGatewayName,
-			Namespace: model.IstioIngressNamespace,
+			Namespace: ingressNamespace,
 			Domain:    domainSuffix,
 		},
 		Spec: gateway,
@@ -125,6 +127,9 @@ func ConvertIngressVirtualService(ingress v1beta1.Ingress, domainSuffix string,
 	// We need to merge all rules with a particular host across
 	// all ingresses, and return a separate VirtualService for each
 	// host.
+	if ingressNamespace == "" {
+		ingressNamespace = model.IstioIngressNamespace
+	}
 
 	for _, rule := range ingress.Spec.Rules {
 		if rule.HTTP == nil {
@@ -138,8 +143,10 @@ func ConvertIngressVirtualService(ingress v1beta1.Ingress, domainSuffix string,
 			host = "*"
 		}
 		virtualService := &networking.VirtualService{
-			Hosts:    []string{},
-			Gateways: []string{model.IstioIngressGatewayName},
+			Hosts: []string{},
+			// Note the name of the gateway is fixed - this is the Gateway that needs to be created by user (via helm
+			// or manually) with TLS secrets and explicit namespace (for security).
+			Gateways: []string{ingressNamespace + "/" + model.IstioIngressGatewayName},
 		}
 
 		virtualService.Hosts = []string{host}
@@ -167,7 +174,7 @@ func ConvertIngressVirtualService(ingress v1beta1.Ingress, domainSuffix string,
 				Group:     model.VirtualService.Group,
 				Version:   model.VirtualService.Version,
 				Name:      namePrefix + "-" + ingress.Name + "-" + model.IstioIngressGatewayName,
-				Namespace: model.IstioIngressNamespace,
+				Namespace: ingress.Namespace,
 				Domain:    domainSuffix,
 			},
 			Spec: virtualService,
@@ -187,7 +194,6 @@ func ConvertIngressVirtualService(ingress v1beta1.Ingress, domainSuffix string,
 	if ingress.Spec.Backend != nil {
 		log.Infof("Ignore default wildcard ingress, use VirtualService %s:%s",
 			ingress.Namespace, ingress.Name)
-
 	}
 }
 

pilot/pkg/config/kube/ingress/conversion_test.go

@@ -118,10 +118,6 @@ func TestConversion(t *testing.T) {
 	}
 
 	for n, cfg := range cfgs {
-		// Not clear if this is right - should probably be under input ns
-		if cfg.ConfigMeta.Namespace != "istio-system" {
-			t.Errorf("Expected istio-system namespace")
-		}
 
 		vs := cfg.Spec.(*networking.VirtualService)
 

pilot/pkg/config/kube/ingress/status.go

@@ -15,7 +15,6 @@
 package ingress
 
 import (
-	"errors"
 	"fmt"
 	"net"
 	"os"
@@ -54,9 +53,9 @@ type StatusSyncer struct {
 
 	ingressClass        string
 	defaultIngressClass string
-	ingressNameSpace    string
-	ingressService      string
-	pod                 *v1.Pod
+
+	// Name of service (ingressgateway default) to find the IP
+	ingressService string
 
 	queue    kube.Queue
 	informer cache.SharedIndexInformer
@@ -75,21 +74,8 @@ func (s *StatusSyncer) Run(stopCh <-chan struct{}) {
 // NewStatusSyncer creates a new instance
 func NewStatusSyncer(mesh *meshconfig.MeshConfig,
 	client kubernetes.Interface,
-	ingressNamespace string,
+	pilotNamespace string,
 	options kube.ControllerOptions) (*StatusSyncer, error) {
-	podName, exists := os.LookupEnv("POD_NAME")
-	if !exists {
-		return nil, errors.New("POD_NAME environment variable must be defined")
-	}
-	podNamespace, exists := os.LookupEnv("POD_NAMESPACE")
-	if !exists {
-		return nil, errors.New("POD_NAMESPACE environment variable must be defined")
-	}
-
-	pod, _ := client.CoreV1().Pods(podNamespace).Get(podName, meta_v1.GetOptions{})
-	if pod == nil {
-		return nil, fmt.Errorf("unable to get POD information")
-	}
 
 	// we need to use the defined ingress class to allow multiple leaders
 	// in order to update information about ingress status
@@ -121,9 +107,7 @@ func NewStatusSyncer(mesh *meshconfig.MeshConfig,
 		queue:               queue,
 		ingressClass:        ingressClass,
 		defaultIngressClass: defaultIngressClass,
-		ingressNameSpace:    ingressNamespace,
 		ingressService:      mesh.IngressService,
-		pod:                 pod,
 		handler:             handler,
 	}
 
@@ -155,9 +139,10 @@ func NewStatusSyncer(mesh *meshconfig.MeshConfig,
 		Component: "ingress-leader-elector",
 		Host:      hostname,
 	})
+	podName := os.Getenv("POD_NAME")
 
 	lock := resourcelock.ConfigMapLock{
-		ConfigMapMeta: meta_v1.ObjectMeta{Namespace: podNamespace, Name: electionID},
+		ConfigMapMeta: meta_v1.ObjectMeta{Namespace: pilotNamespace, Name: electionID},
 		Client:        client.CoreV1(),
 		LockConfig: resourcelock.ResourceLockConfig{
 			Identity:      podName,
@@ -182,7 +167,7 @@ func NewStatusSyncer(mesh *meshconfig.MeshConfig,
 
 	// Register handler at the beginning
 	handler.Append(func(obj interface{}, event model.Event) error {
-		addrs, err := st.runningAddresses()
+		addrs, err := st.runningAddresses(ingressNamespace)
 		if err != nil {
 			return err
 		}
@@ -225,11 +210,11 @@ func (s *StatusSyncer) updateStatus(status []v1.LoadBalancerIngress) error {
 
 // runningAddresses returns a list of IP addresses and/or FQDN where the
 // ingress controller is currently running
-func (s *StatusSyncer) runningAddresses() ([]string, error) {
+func (s *StatusSyncer) runningAddresses(ingressNs string) ([]string, error) {
 	addrs := []string{}
 
 	if s.ingressService != "" {
-		svc, err := s.client.CoreV1().Services(s.ingressNameSpace).Get(s.ingressService, meta_v1.GetOptions{})
+		svc, err := s.client.CoreV1().Services(ingressNs).Get(s.ingressService, meta_v1.GetOptions{})
 		if err != nil {
 			return nil, err
 		}
@@ -251,9 +236,10 @@ func (s *StatusSyncer) runningAddresses() ([]string, error) {
 		return addrs, nil
 	}
 
-	// get information about all the pods running the ingress controller
-	pods, err := s.client.CoreV1().Pods(s.pod.GetNamespace()).List(meta_v1.ListOptions{
-		LabelSelector: labels.SelectorFromSet(s.pod.GetLabels()).String(),
+	// get information about all the pods running the ingress controller (gateway)
+	pods, err := s.client.CoreV1().Pods(ingressNamespace).List(meta_v1.ListOptions{
+		// TODO: make it a const or maybe setting ( unless we remove k8s ingress support first)
+		LabelSelector: labels.SelectorFromSet(map[string]string{"app": "ingressgateway"}).String(),
 	})
 	if err != nil {
 		return nil, err

pilot/pkg/config/kube/ingress/status_test.go

@@ -58,10 +58,10 @@ func makeFakeClient() *fake.Clientset {
 		&v1.PodList{Items: []v1.Pod{
 			{
 				ObjectMeta: metav1.ObjectMeta{
-					Name:      "test",
-					Namespace: testNamespace,
+					Name:      "ingressgateway",
+					Namespace: "istio-system",
 					Labels: map[string]string{
-						"lable_sig": "test",
+						"app": "ingressgateway",
 					},
 				},
 				Spec: v1.PodSpec{
@@ -196,14 +196,19 @@ func TestConvertIngressControllerMode(t *testing.T) {
 	}
 }
 
-func TestRunningAddressesWithService(t *testing.T) {
+func TestRunningAddresses(t *testing.T) {
+	t.Run("service", testRunningAddressesWithService)
+	t.Run("hostname", testRunningAddressesWithHostname)
+}
+
+func testRunningAddressesWithService(t *testing.T) {
 	client := makeFakeClient()
 	syncer, err := makeStatusSyncer(t, client)
 	if err != nil {
 		t.Fatal(err)
 	}
 
-	address, err := syncer.runningAddresses()
+	address, err := syncer.runningAddresses(testNamespace)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -213,7 +218,7 @@ func TestRunningAddressesWithService(t *testing.T) {
 	}
 }
 
-func TestRunningAddressesWithHostname(t *testing.T) {
+func testRunningAddressesWithHostname(t *testing.T) {
 	client := makeFakeClient()
 	syncer, err := makeStatusSyncer(t, client)
 	if err != nil {
@@ -222,7 +227,7 @@ func TestRunningAddressesWithHostname(t *testing.T) {
 
 	syncer.ingressService = "istio-ingress-hostname"
 
-	address, err := syncer.runningAddresses()
+	address, err := syncer.runningAddresses(testNamespace)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -233,6 +238,7 @@ func TestRunningAddressesWithHostname(t *testing.T) {
 }
 
 func TestRunningAddressesWithPod(t *testing.T) {
+	ingressNamespace = "istio-system" // it is set in real pilot on newController.
 	client := makeFakeClient()
 	syncer, err := makeStatusSyncer(t, client)
 	if err != nil {
@@ -241,12 +247,12 @@ func TestRunningAddressesWithPod(t *testing.T) {
 
 	syncer.ingressService = ""
 
-	address, err := syncer.runningAddresses()
+	address, err := syncer.runningAddresses(ingressNamespace)
 	if err != nil {
 		t.Fatal(err)
 	}
 
 	if len(address) != 1 || address[0] != nodeIP {
-		t.Errorf("Address is not correctly set to node ip")
+		t.Errorf("Address is not correctly set to node ip %v %v", address, nodeIP)
 	}
 }