From fc955dd527b5fce28b807cfafca40ed84a2e8b87 Mon Sep 17 00:00:00 2001 From: Costin Manolache Date: Mon, 11 Feb 2019 10:43:32 -0800 Subject: [PATCH] Fixes for k8s ingress (#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 | 1 + .../testdata/v1alpha3/merged-gateway.yaml | 4 +- .../v1alpha3/merged-gateway.yaml.golden | 4 +- .../testdata/v1alpha3/myservice-gateway.yaml | 4 +- .../v1alpha3/myservice-gateway.yaml.golden | 4 +- pilot/pkg/config/kube/ingress/controller.go | 36 +++++++++++++++++ pilot/pkg/config/kube/ingress/conversion.go | 18 ++++++--- .../config/kube/ingress/conversion_test.go | 4 -- pilot/pkg/config/kube/ingress/status.go | 40 ++++++------------- pilot/pkg/config/kube/ingress/status_test.go | 24 ++++++----- 10 files changed, 85 insertions(+), 54 deletions(-) diff --git a/istioctl/cmd/istioctl/convert_ingress_test.go b/istioctl/cmd/istioctl/convert_ingress_test.go index d8adaed65191..94da6a688044 100644 --- a/istioctl/cmd/istioctl/convert_ingress_test.go +++ b/istioctl/cmd/istioctl/convert_ingress_test.go @@ -23,6 +23,7 @@ import ( ) func TestConvertIngress(t *testing.T) { + tt := []struct { in []string out string diff --git a/istioctl/cmd/istioctl/testdata/v1alpha3/merged-gateway.yaml b/istioctl/cmd/istioctl/testdata/v1alpha3/merged-gateway.yaml index aebbf0651987..8e5507ae4b40 100644 --- a/istioctl/cmd/istioctl/testdata/v1alpha3/merged-gateway.yaml +++ b/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: diff --git a/istioctl/cmd/istioctl/testdata/v1alpha3/merged-gateway.yaml.golden b/istioctl/cmd/istioctl/testdata/v1alpha3/merged-gateway.yaml.golden index aebbf0651987..8e5507ae4b40 100644 --- a/istioctl/cmd/istioctl/testdata/v1alpha3/merged-gateway.yaml.golden +++ b/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: diff --git a/istioctl/cmd/istioctl/testdata/v1alpha3/myservice-gateway.yaml b/istioctl/cmd/istioctl/testdata/v1alpha3/myservice-gateway.yaml index 119e51803434..818081167cf1 100644 --- a/istioctl/cmd/istioctl/testdata/v1alpha3/myservice-gateway.yaml +++ b/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: diff --git a/istioctl/cmd/istioctl/testdata/v1alpha3/myservice-gateway.yaml.golden b/istioctl/cmd/istioctl/testdata/v1alpha3/myservice-gateway.yaml.golden index 119e51803434..818081167cf1 100644 --- a/istioctl/cmd/istioctl/testdata/v1alpha3/myservice-gateway.yaml.golden +++ b/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: diff --git a/pilot/pkg/config/kube/ingress/controller.go b/pilot/pkg/config/kube/ingress/controller.go index e596fdf56015..aa30c24e29e5 100644 --- a/pilot/pkg/config/kube/ingress/controller.go +++ b/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( diff --git a/pilot/pkg/config/kube/ingress/conversion.go b/pilot/pkg/config/kube/ingress/conversion.go index ce3e5c688e09..20ad17cd13c5 100644 --- a/pilot/pkg/config/kube/ingress/conversion.go +++ b/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) - } } diff --git a/pilot/pkg/config/kube/ingress/conversion_test.go b/pilot/pkg/config/kube/ingress/conversion_test.go index 0e043e0c40ab..f392d99d726d 100644 --- a/pilot/pkg/config/kube/ingress/conversion_test.go +++ b/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) diff --git a/pilot/pkg/config/kube/ingress/status.go b/pilot/pkg/config/kube/ingress/status.go index b20284059cf5..4625354f58b4 100644 --- a/pilot/pkg/config/kube/ingress/status.go +++ b/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 diff --git a/pilot/pkg/config/kube/ingress/status_test.go b/pilot/pkg/config/kube/ingress/status_test.go index e55201f7cbe7..776419ec495b 100644 --- a/pilot/pkg/config/kube/ingress/status_test.go +++ b/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) } }