From 5ead169be0fd41608a130ab485c74777970c08e1 Mon Sep 17 00:00:00 2001 From: pastequo Date: Thu, 14 Dec 2023 15:32:43 +0100 Subject: [PATCH] feat: New istio config format (#1247) --- go.mod | 2 +- pkg/reconciler/ingress/config/istio.go | 178 +++++++++++-- pkg/reconciler/ingress/config/istio_test.go | 264 ++++++++++++++++++-- 3 files changed, 394 insertions(+), 50 deletions(-) diff --git a/go.mod b/go.mod index 8150bcccf4..e4ce4318e7 100644 --- a/go.mod +++ b/go.mod @@ -7,6 +7,7 @@ require ( go.uber.org/zap v1.26.0 golang.org/x/sync v0.6.0 google.golang.org/protobuf v1.32.0 + gopkg.in/yaml.v3 v3.0.1 istio.io/api v1.20.2 istio.io/client-go v1.20.2 k8s.io/api v0.28.5 @@ -83,7 +84,6 @@ require ( google.golang.org/grpc v1.61.0 // indirect gopkg.in/inf.v0 v0.9.1 // indirect gopkg.in/yaml.v2 v2.4.0 // indirect - gopkg.in/yaml.v3 v3.0.1 // indirect k8s.io/apiextensions-apiserver v0.28.5 // indirect k8s.io/code-generator v0.28.5 // indirect k8s.io/gengo v0.0.0-20221011193443-fad74ee6edd9 // indirect diff --git a/pkg/reconciler/ingress/config/istio.go b/pkg/reconciler/ingress/config/istio.go index 70702801d8..6641eeafbd 100644 --- a/pkg/reconciler/ingress/config/istio.go +++ b/pkg/reconciler/ingress/config/istio.go @@ -21,6 +21,7 @@ import ( "sort" "strings" + "gopkg.in/yaml.v3" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/util/validation" "knative.dev/pkg/network" @@ -38,6 +39,12 @@ const ( // localGatewayKeyPrefix is the prefix of all keys to configure Istio gateways for public & private Ingresses. localGatewayKeyPrefix = "local-gateway." + // externalGatewaysKey is the configmap key to configure Istio gateways for public Ingresses. + externalGatewaysKey = "external-gateways" + + // localGatewaysKey is the configmap key to configure Istio gateways for private Ingresses. + localGatewaysKey = "local-gateways" + // KnativeIngressGateway is the name of the ingress gateway KnativeIngressGateway = "knative-ingress-gateway" @@ -71,7 +78,7 @@ func defaultLocalGateways() []Gateway { type Gateway struct { Namespace string Name string - ServiceURL string + ServiceURL string `yaml:"service"` } // QualifiedName returns gateway name in '{namespace}/{name}' format. @@ -79,6 +86,26 @@ func (g Gateway) QualifiedName() string { return g.Namespace + "/" + g.Name } +func (g Gateway) Validate() error { + if g.Namespace == "" { + return fmt.Errorf("missing namespace") + } + + if g.Name == "" { + return fmt.Errorf("missing name") + } + + if g.ServiceURL == "" { + return fmt.Errorf("missing service") + } + + if errs := validation.IsDNS1123Subdomain(strings.TrimSuffix(g.ServiceURL, ".")); len(errs) > 0 { + return fmt.Errorf("invalid gateway service format: %v", errs) + } + + return nil +} + // Istio contains istio related configuration defined in the // istio config map. type Istio struct { @@ -89,7 +116,123 @@ type Istio struct { LocalGateways []Gateway } -func parseGateways(configMap *corev1.ConfigMap, prefix string) ([]Gateway, error) { +func (i Istio) Validate() error { + for _, gtw := range i.IngressGateways { + if err := gtw.Validate(); err != nil { + return fmt.Errorf("invalid gateway: %w", err) + } + } + + for _, gtw := range i.LocalGateways { + if err := gtw.Validate(); err != nil { + return fmt.Errorf("invalid local gateway: %w", err) + } + } + + return nil +} + +// NewIstioFromConfigMap creates an Istio config from the supplied ConfigMap +func NewIstioFromConfigMap(configMap *corev1.ConfigMap) (*Istio, error) { + ret := &Istio{} + var err error + + oldFormatDefined := isOldFormatDefined(configMap) + newFormatDefined := isNewFormatDefined(configMap) + + switch { + case newFormatDefined && oldFormatDefined: + return nil, fmt.Errorf( + "invalid configmap: %q or %q can not be defined simultaneously with %q or %q", + localGatewaysKey, externalGatewaysKey, gatewayKeyPrefix, localGatewayKeyPrefix, + ) + case newFormatDefined: + ret, err = parseNewFormat(configMap) + if err != nil { + return nil, fmt.Errorf("failed to parse configmap: %w", err) + } + case oldFormatDefined: + ret = parseOldFormat(configMap) + } + + defaultValues(ret) + + err = ret.Validate() + if err != nil { + return nil, fmt.Errorf("invalid configuration: %w", err) + } + + return ret, nil +} + +func isNewFormatDefined(configMap *corev1.ConfigMap) bool { + _, hasGateway := configMap.Data[externalGatewaysKey] + _, hasLocalGateway := configMap.Data[localGatewaysKey] + + return hasGateway || hasLocalGateway +} + +func isOldFormatDefined(configMap *corev1.ConfigMap) bool { + for key := range configMap.Data { + if strings.HasPrefix(key, gatewayKeyPrefix) || strings.HasPrefix(key, localGatewayKeyPrefix) { + return true + } + } + + return false +} + +func parseNewFormat(configMap *corev1.ConfigMap) (*Istio, error) { + ret := &Istio{} + + gatewaysStr, hasGateway := configMap.Data[externalGatewaysKey] + + if hasGateway { + gateways, err := parseNewFormatGateways(gatewaysStr) + if err != nil { + return ret, fmt.Errorf("failed to parse %q gateways: %w", externalGatewaysKey, err) + } + + ret.IngressGateways = gateways + } + + localGatewaysStr, hasLocalGateway := configMap.Data[localGatewaysKey] + + if hasLocalGateway { + localGateways, err := parseNewFormatGateways(localGatewaysStr) + if err != nil { + return ret, fmt.Errorf("failed to parse %q gateways: %w", localGatewaysKey, err) + } + + ret.LocalGateways = localGateways + } + + if len(ret.LocalGateways) > 1 { + return ret, fmt.Errorf("only one local gateway can be defined: current %q value is %q", localGatewaysKey, localGatewaysStr) + } + + return ret, nil +} + +func parseNewFormatGateways(data string) ([]Gateway, error) { + ret := make([]Gateway, 0) + + err := yaml.Unmarshal([]byte(data), &ret) + if err != nil { + return ret, fmt.Errorf("failed to unmarshal: %w", err) + } + + return ret, nil +} + +func parseOldFormat(configMap *corev1.ConfigMap) *Istio { + return &Istio{ + IngressGateways: parseOldFormatGateways(configMap, gatewayKeyPrefix), + LocalGateways: parseOldFormatGateways(configMap, localGatewayKeyPrefix), + } +} + +func parseOldFormatGateways(configMap *corev1.ConfigMap, prefix string) []Gateway { urls := map[string]string{} gatewayNames := []string{} for k, v := range configMap.Data { @@ -97,9 +240,7 @@ func parseGateways(configMap *corev1.ConfigMap, prefix string) ([]Gateway, error continue } gatewayName, serviceURL := k[len(prefix):], v - if errs := validation.IsDNS1123Subdomain(strings.TrimSuffix(serviceURL, ".")); len(errs) > 0 { - return nil, fmt.Errorf("invalid gateway format: %v", errs) - } + gatewayNames = append(gatewayNames, gatewayName) urls[gatewayName] = serviceURL } @@ -121,28 +262,15 @@ func parseGateways(configMap *corev1.ConfigMap, prefix string) ([]Gateway, error ServiceURL: urls[gatewayName], } } - return gateways, nil + return gateways } -// NewIstioFromConfigMap creates an Istio config from the supplied ConfigMap -func NewIstioFromConfigMap(configMap *corev1.ConfigMap) (*Istio, error) { - gateways, err := parseGateways(configMap, gatewayKeyPrefix) - if err != nil { - return nil, err - } - if len(gateways) == 0 { - gateways = defaultIngressGateways() - } - localGateways, err := parseGateways(configMap, localGatewayKeyPrefix) - if err != nil { - return nil, err - } - if len(localGateways) == 0 { - localGateways = defaultLocalGateways() +func defaultValues(conf *Istio) { + if len(conf.IngressGateways) == 0 { + conf.IngressGateways = defaultIngressGateways() } - return &Istio{ - IngressGateways: gateways, - LocalGateways: localGateways, - }, nil + if len(conf.LocalGateways) == 0 { + conf.LocalGateways = defaultLocalGateways() + } } diff --git a/pkg/reconciler/ingress/config/istio_test.go b/pkg/reconciler/ingress/config/istio_test.go index a6a464613b..6b46fa0d51 100644 --- a/pkg/reconciler/ingress/config/istio_test.go +++ b/pkg/reconciler/ingress/config/istio_test.go @@ -56,7 +56,7 @@ func TestGatewayConfiguration(t *testing.T) { gatewayConfigTests := []struct { name string wantErr bool - wantIstio interface{} + wantIstio *Istio config *corev1.ConfigMap }{{ name: "gateway configuration with no network input", @@ -71,9 +71,8 @@ func TestGatewayConfiguration(t *testing.T) { }, }, }, { - name: "gateway configuration with invalid url", - wantErr: true, - wantIstio: (*Istio)(nil), + name: "gateway configuration with invalid url", + wantErr: true, config: &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Namespace: system.Namespace(), @@ -84,8 +83,7 @@ func TestGatewayConfiguration(t *testing.T) { }, }, }, { - name: "gateway configuration with valid url", - wantErr: false, + name: "gateway configuration with valid url", wantIstio: &Istio{ IngressGateways: []Gateway{{ Namespace: "knative-testing", @@ -104,8 +102,7 @@ func TestGatewayConfiguration(t *testing.T) { }, }, }, { - name: "gateway configuration with valid url having a dot at the end", - wantErr: false, + name: "gateway configuration with valid url having a dot at the end", wantIstio: &Istio{ IngressGateways: []Gateway{{ Namespace: "knative-testing", @@ -124,8 +121,7 @@ func TestGatewayConfiguration(t *testing.T) { }, }, }, { - name: "gateway configuration in custom namespace with valid url", - wantErr: false, + name: "gateway configuration in custom namespace with valid url", wantIstio: &Istio{ IngressGateways: []Gateway{{ Namespace: "custom-namespace", @@ -144,9 +140,8 @@ func TestGatewayConfiguration(t *testing.T) { }, }, }, { - name: "gateway configuration in custom namespace with invalid url", - wantErr: true, - wantIstio: (*Istio)(nil), + name: "gateway configuration in custom namespace with invalid url", + wantErr: true, config: &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Namespace: system.Namespace(), @@ -157,8 +152,7 @@ func TestGatewayConfiguration(t *testing.T) { }, }, }, { - name: "local gateway configuration with valid url", - wantErr: false, + name: "local gateway configuration with valid url", wantIstio: &Istio{ IngressGateways: defaultIngressGateways(), LocalGateways: []Gateway{{ @@ -177,9 +171,8 @@ func TestGatewayConfiguration(t *testing.T) { }, }, }, { - name: "local gateway configuration with invalid url", - wantErr: true, - wantIstio: (*Istio)(nil), + name: "local gateway configuration with invalid url", + wantErr: true, config: &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Namespace: system.Namespace(), @@ -190,8 +183,7 @@ func TestGatewayConfiguration(t *testing.T) { }, }, }, { - name: "local gateway configuration in custom namespace with valid url", - wantErr: false, + name: "local gateway configuration in custom namespace with valid url", wantIstio: &Istio{ IngressGateways: defaultIngressGateways(), LocalGateways: []Gateway{{ @@ -210,9 +202,8 @@ func TestGatewayConfiguration(t *testing.T) { }, }, }, { - name: "local gateway configuration in custom namespace with invalid url", - wantErr: true, - wantIstio: (*Istio)(nil), + name: "local gateway configuration in custom namespace with invalid url", + wantErr: true, config: &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Namespace: system.Namespace(), @@ -222,6 +213,231 @@ func TestGatewayConfiguration(t *testing.T) { "local-gateway.custom-namespace.invalid": "_invalid", }, }, + }, { + name: "new format alone", + wantIstio: &Istio{ + IngressGateways: []Gateway{{ + Namespace: "namespace1", + Name: "gateway1", + ServiceURL: "istio-ingressbackroad.istio-system.svc.cluster.local", + }}, + LocalGateways: []Gateway{{ + Namespace: "namespace2", + Name: "gateway2", + ServiceURL: "istio-local-gateway.istio-system.svc.cluster.local", + }}, + }, + config: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: system.Namespace(), + Name: IstioConfigName, + }, + Data: map[string]string{ + "external-gateways": ` + - namespace: "namespace1" + name: "gateway1" + service: "istio-ingressbackroad.istio-system.svc.cluster.local"`, + "local-gateways": ` + - namespace: "namespace2" + name: "gateway2" + service: "istio-local-gateway.istio-system.svc.cluster.local"`, + }, + }, + }, { + name: "new & old format", + wantErr: true, + config: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: system.Namespace(), + Name: IstioConfigName, + }, + Data: map[string]string{ + "external-gateways": ` + - namespace: "namespace1" + name: "gateway1" + service: "istio-ingressbackroad.istio-system.svc.cluster.local"`, + "local-gateways": ` + - namespace: "namespace2" + name: "gateway2" + service: "istio-local-gateway.istio-system.svc.cluster.local"`, + "local-gateway.custom-namespace.custom-local-gateway": "istio-ingressbackroad.istio-system.svc.cluster.local", + "gateway.custom-namespace.custom-gateway": "istio-ingressfreeway.istio-system.svc.cluster.local", + }, + }, + }, { + name: "new format - double local gateway", + wantErr: true, + config: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: system.Namespace(), + Name: IstioConfigName, + }, + Data: map[string]string{ + "external-gateways": ` + - namespace: "namespace1" + name: "gateway1" + service: "istio-ingressbackroad.istio-system.svc.cluster.local"`, + "local-gateways": ` + - namespace: "namespace1" + name: "gateway1" + service: "istio-local-gateway1.istio-system.svc.cluster.local" + - namespace: "namespace2" + name: "gateway2" + service: "istio-local-gateway2.istio-system.svc.cluster.local"`, + }, + }, + }, { + name: "new format - invalid URL", + wantErr: true, + config: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: system.Namespace(), + Name: IstioConfigName, + }, + Data: map[string]string{ + "external-gateways": ` + - namespace: "namespace1" + name: "gateway1" + service: "istio-ingressbackroad.istio-system.svc.cluster.local"`, + "local-gateways": ` + - namespace: "namespace2" + name: "gateway2" + service: "_invalid"`, + }, + }, + }, { + name: "new format - missing service", + wantErr: true, + config: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: system.Namespace(), + Name: IstioConfigName, + }, + Data: map[string]string{ + "external-gateways": ` + - namespace: "namespace1" + name: "gateway1" + service: ""`, + "local-gateways": ` + - namespace: "namespace2" + name: "gateway2" + service: "istio-local-gateway.istio-system.svc.cluster.local"`, + }, + }, + }, { + name: "new format - missing name", + wantErr: true, + config: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: system.Namespace(), + Name: IstioConfigName, + }, + Data: map[string]string{ + "external-gateways": ` + - namespace: "namespace1" + name: "" + service: "istio-ingressbackroad.istio-system.svc.cluster.local"`, + "local-gateways": ` + - namespace: "namespace2" + name: "gateway2" + service: "istio-local-gateway.istio-system.svc.cluster.local"`, + }, + }, + }, { + name: "new format - missing namespace", + wantErr: true, + config: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: system.Namespace(), + Name: IstioConfigName, + }, + Data: map[string]string{ + "external-gateways": ` + - namespace: "namespace1" + name: "gateway1" + service: "istio-ingressbackroad.istio-system.svc.cluster.local"`, + "local-gateways": ` + - namespace: "" + name: "gateway2" + service: "istio-local-gateway.istio-system.svc.cluster.local"`, + }, + }, + }, { + name: "new format - invalid local yaml", + wantErr: true, + config: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: system.Namespace(), + Name: IstioConfigName, + }, + Data: map[string]string{ + "external-gateways": ` + - namespace: "namespace1" + name: "gateway1" + service: "istio-ingressbackroad.istio-system.svc.cluster.local"`, + "local-gateways": "notYAML", + }, + }, + }, { + name: "new format - invalid external yaml", + wantErr: true, + config: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: system.Namespace(), + Name: IstioConfigName, + }, + Data: map[string]string{ + "external-gateways": "notYAML", + "local-gateways": ` + - namespace: "namespace2" + name: "gateway2" + service: "istio-local-gateway.istio-system.svc.cluster.local"`, + }, + }, + }, { + name: "new format - missing external gateway configuration", + config: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: system.Namespace(), + Name: IstioConfigName, + }, + Data: map[string]string{ + "local-gateways": ` + - namespace: "namespace2" + name: "gateway2" + service: "istio-local-gateway.istio-system.svc.cluster.local"`, + }, + }, + wantIstio: &Istio{ + LocalGateways: []Gateway{{ + Namespace: "namespace2", + Name: "gateway2", + ServiceURL: "istio-local-gateway.istio-system.svc.cluster.local", + }}, + IngressGateways: defaultIngressGateways(), + }, + }, { + name: "new format - missing local gateway configuration", + config: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: system.Namespace(), + Name: IstioConfigName, + }, + Data: map[string]string{ + "external-gateways": ` + - namespace: "namespace1" + name: "gateway1" + service: "istio-ingressbackroad.istio-system.svc.cluster.local"`, + }, + }, + wantIstio: &Istio{ + IngressGateways: []Gateway{{ + Namespace: "namespace1", + Name: "gateway1", + ServiceURL: "istio-ingressbackroad.istio-system.svc.cluster.local", + }}, + LocalGateways: defaultLocalGateways(), + }, }} for _, tt := range gatewayConfigTests { @@ -232,7 +448,7 @@ func TestGatewayConfiguration(t *testing.T) { } if diff := cmp.Diff(actualIstio, tt.wantIstio); diff != "" { - t.Fatalf("Want %v, but got %v", tt.wantIstio, actualIstio) + t.Fatalf("Want %+v, but got %+v", tt.wantIstio, actualIstio) } }) }