From 65ac94120443115e2ed73dd9669cf8646d77b705 Mon Sep 17 00:00:00 2001 From: pastequo Date: Wed, 7 Feb 2024 14:21:01 +0100 Subject: [PATCH 1/5] feat: Gateway filtering based on labels (#1248) --- go.mod | 4 +- pkg/reconciler/ingress/config/istio.go | 40 +- pkg/reconciler/ingress/config/istio_test.go | 122 +++++-- .../ingress/config/zz_generated.deepcopy.go | 17 +- pkg/reconciler/ingress/ingress.go | 65 ++-- pkg/reconciler/ingress/lister.go | 7 +- pkg/reconciler/ingress/lister_test.go | 123 +++++++ pkg/reconciler/ingress/resources/gateway.go | 160 +++++++- .../ingress/resources/gateway_test.go | 343 +++++++++++++++--- pkg/reconciler/ingress/resources/secret.go | 10 +- .../ingress/resources/secret_test.go | 2 +- test/e2e/exposition_test.go | 260 +++++++++++++ 12 files changed, 1007 insertions(+), 146 deletions(-) create mode 100644 test/e2e/exposition_test.go diff --git a/go.mod b/go.mod index 5fa82cfecf..00b03282bc 100644 --- a/go.mod +++ b/go.mod @@ -7,7 +7,6 @@ require ( go.uber.org/zap v1.27.0 golang.org/x/sync v0.6.0 google.golang.org/protobuf v1.33.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.29.2 @@ -16,6 +15,7 @@ require ( knative.dev/hack v0.0.0-20240318013248-424e75ed769a knative.dev/networking v0.0.0-20240326200906-419ebf9a1d87 knative.dev/pkg v0.0.0-20240325103648-fd7cc2153e6a + sigs.k8s.io/yaml v1.4.0 ) require ( @@ -83,6 +83,7 @@ require ( google.golang.org/grpc v1.62.1 // 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.29.2 // indirect k8s.io/code-generator v0.29.2 // indirect k8s.io/gengo v0.0.0-20240129211411-f967bbeff4b4 // indirect @@ -91,5 +92,4 @@ require ( k8s.io/utils v0.0.0-20240102154912-e7106e64919e // indirect sigs.k8s.io/json v0.0.0-20221116044647-bc3834ca7abd // indirect sigs.k8s.io/structured-merge-diff/v4 v4.4.1 // indirect - sigs.k8s.io/yaml v1.4.0 // indirect ) diff --git a/pkg/reconciler/ingress/config/istio.go b/pkg/reconciler/ingress/config/istio.go index 6641eeafbd..26a7c5303f 100644 --- a/pkg/reconciler/ingress/config/istio.go +++ b/pkg/reconciler/ingress/config/istio.go @@ -21,11 +21,12 @@ import ( "sort" "strings" - "gopkg.in/yaml.v3" corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/validation" "knative.dev/pkg/network" "knative.dev/pkg/system" + "sigs.k8s.io/yaml" ) const ( @@ -76,9 +77,10 @@ func defaultLocalGateways() []Gateway { // Gateway specifies the name of the Gateway and the K8s Service backing it. type Gateway struct { - Namespace string - Name string - ServiceURL string `yaml:"service"` + Namespace string + Name string + ServiceURL string `json:"service"` + LabelSelector *metav1.LabelSelector `json:"labelSelector,omitempty"` } // QualifiedName returns gateway name in '{namespace}/{name}' format. @@ -109,10 +111,10 @@ func (g Gateway) Validate() error { // Istio contains istio related configuration defined in the // istio config map. type Istio struct { - // IngressGateway specifies the gateway urls for public Ingress. + // IngressGateways specifies the gateway urls for public Ingress. IngressGateways []Gateway - // LocalGateway specifies the gateway urls for public & private Ingress. + // LocalGateways specifies the gateway urls for public & private Ingress. LocalGateways []Gateway } @@ -132,6 +134,28 @@ func (i Istio) Validate() error { return nil } +func (i Istio) DefaultExternalGateways() []Gateway { + return defaultGateways(i.IngressGateways) +} + +func (i Istio) DefaultLocalGateways() []Gateway { + return defaultGateways(i.LocalGateways) +} + +func defaultGateways(gtws []Gateway) []Gateway { + ret := make([]Gateway, 0) + + for _, gtw := range gtws { + gateway := gtw + + if gtw.LabelSelector == nil { + ret = append(ret, gateway) + } + } + + return ret +} + // NewIstioFromConfigMap creates an Istio config from the supplied ConfigMap func NewIstioFromConfigMap(configMap *corev1.ConfigMap) (*Istio, error) { ret := &Istio{} @@ -207,10 +231,6 @@ func parseNewFormat(configMap *corev1.ConfigMap) (*Istio, error) { 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 } diff --git a/pkg/reconciler/ingress/config/istio_test.go b/pkg/reconciler/ingress/config/istio_test.go index 6b46fa0d51..ebef1b59e2 100644 --- a/pkg/reconciler/ingress/config/istio_test.go +++ b/pkg/reconciler/ingress/config/istio_test.go @@ -264,28 +264,6 @@ func TestGatewayConfiguration(t *testing.T) { "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, @@ -438,6 +416,106 @@ func TestGatewayConfiguration(t *testing.T) { }}, LocalGateways: defaultLocalGateways(), }, + }, { + name: "new format - label selector", + config: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: system.Namespace(), + Name: IstioConfigName, + }, + Data: map[string]string{ + "external-gateways": ` + - namespace: "namespace" + name: "gateway" + service: "istio-gateway.istio-system.svc.cluster.local" + labelSelector: + matchExpressions: + - key: "key" + operator: "In" + values: ["value"]`, + + "local-gateways": ` + - namespace: "namespace2" + name: "gateway2" + service: "istio-local-gateway.istio-system.svc.cluster.local"`, + }, + }, + wantIstio: &Istio{ + IngressGateways: []Gateway{{ + Namespace: "namespace", + Name: "gateway", + ServiceURL: "istio-gateway.istio-system.svc.cluster.local", + LabelSelector: &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: "key", + Operator: metav1.LabelSelectorOpIn, + Values: []string{"value"}, + }, + }, + }, + }}, + LocalGateways: []Gateway{{ + Namespace: "namespace2", + Name: "gateway2", + ServiceURL: "istio-local-gateway.istio-system.svc.cluster.local", + }}, + }, + }, { + name: "local gateway with selector", + config: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: system.Namespace(), + Name: IstioConfigName, + }, + Data: map[string]string{ + "local-gateways": ` + - namespace: "namespace" + name: "gateway" + service: "istio-local.istio-system.svc.cluster.local" + labelSelector: + matchExpressions: + - key: "key" + operator: "In" + values: ["value"] + - namespace: "namespace1" + name: "gateway1" + service: "istio-local-gateway.istio-system.svc.cluster.local"`, + + "external-gateways": ` + - namespace: "namespace2" + name: "gateway2" + service: "istio-gateway.istio-system.svc.cluster.local"`, + }, + }, + wantIstio: &Istio{ + IngressGateways: []Gateway{{ + Namespace: "namespace2", + Name: "gateway2", + ServiceURL: "istio-gateway.istio-system.svc.cluster.local", + }}, + LocalGateways: []Gateway{ + { + Namespace: "namespace", + Name: "gateway", + ServiceURL: "istio-local.istio-system.svc.cluster.local", + LabelSelector: &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: "key", + Operator: metav1.LabelSelectorOpIn, + Values: []string{"value"}, + }, + }, + }, + }, + { + Namespace: "namespace1", + Name: "gateway1", + ServiceURL: "istio-local-gateway.istio-system.svc.cluster.local", + }, + }, + }, }} for _, tt := range gatewayConfigTests { diff --git a/pkg/reconciler/ingress/config/zz_generated.deepcopy.go b/pkg/reconciler/ingress/config/zz_generated.deepcopy.go index a1ccb447c4..3a56fddf84 100644 --- a/pkg/reconciler/ingress/config/zz_generated.deepcopy.go +++ b/pkg/reconciler/ingress/config/zz_generated.deepcopy.go @@ -21,9 +21,18 @@ limitations under the License. package config +import ( + v1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *Gateway) DeepCopyInto(out *Gateway) { *out = *in + if in.LabelSelector != nil { + in, out := &in.LabelSelector, &out.LabelSelector + *out = new(v1.LabelSelector) + (*in).DeepCopyInto(*out) + } return } @@ -43,12 +52,16 @@ func (in *Istio) DeepCopyInto(out *Istio) { if in.IngressGateways != nil { in, out := &in.IngressGateways, &out.IngressGateways *out = make([]Gateway, len(*in)) - copy(*out, *in) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } } if in.LocalGateways != nil { in, out := &in.LocalGateways, &out.LocalGateways *out = make([]Gateway, len(*in)) - copy(*out, *in) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } } return } diff --git a/pkg/reconciler/ingress/ingress.go b/pkg/reconciler/ingress/ingress.go index 2289f8278b..d8370dfcc3 100644 --- a/pkg/reconciler/ingress/ingress.go +++ b/pkg/reconciler/ingress/ingress.go @@ -113,8 +113,13 @@ func (r *Reconciler) reconcileIngress(ctx context.Context, ing *v1alpha1.Ingress ing.Status.InitializeConditions() logger.Infof("Reconciling ingress: %#v", ing) + defaultGateways, err := resources.QualifiedGatewayNamesFromContext(ctx, ing) + if err != nil { + return err + } + gatewayNames := map[v1alpha1.IngressVisibility]sets.Set[string]{} - gatewayNames[v1alpha1.IngressVisibilityClusterLocal] = qualifiedGatewayNamesFromContext(ctx)[v1alpha1.IngressVisibilityClusterLocal] + gatewayNames[v1alpha1.IngressVisibilityClusterLocal] = defaultGateways[v1alpha1.IngressVisibilityClusterLocal] gatewayNames[v1alpha1.IngressVisibilityExternalIP] = sets.New[string]() externalIngressGateways := []*v1beta1.Gateway{} @@ -131,7 +136,7 @@ func (r *Reconciler) reconcileIngress(ctx context.Context, ing *v1alpha1.Ingress if err != nil { return err } - targetWildcardSecrets, err := resources.MakeWildcardSecrets(ctx, wildcardSecrets) + targetWildcardSecrets, err := resources.MakeWildcardSecrets(ctx, wildcardSecrets, ing) if err != nil { return err } @@ -154,7 +159,7 @@ func (r *Reconciler) reconcileIngress(ctx context.Context, ing *v1alpha1.Ingress // same wildcard host. We need to handle wildcard certificate specially because Istio does // not fully support multiple TLS Servers (or Gateways) share the same certificate. // https://istio.io/docs/ops/common-problems/network-issues/ - desiredWildcardGateways, err := resources.MakeWildcardTLSGateways(ctx, wildcardSecrets, r.svcLister) + desiredWildcardGateways, err := resources.MakeWildcardTLSGateways(ctx, ing, wildcardSecrets, r.svcLister) if err != nil { return err } @@ -201,8 +206,7 @@ func (r *Reconciler) reconcileIngress(ctx context.Context, ing *v1alpha1.Ingress } else { // Otherwise, we fall back to the default global Gateways for HTTP behavior. // We need this for the backward compatibility. - defaultGlobalHTTPGateways := qualifiedGatewayNamesFromContext(ctx)[v1alpha1.IngressVisibilityExternalIP] - gatewayNames[v1alpha1.IngressVisibilityExternalIP].Insert(sets.List(defaultGlobalHTTPGateways)...) + gatewayNames[v1alpha1.IngressVisibilityExternalIP].Insert(sets.List(defaultGateways[v1alpha1.IngressVisibilityExternalIP])...) } if err := r.reconcileIngressGateways(ctx, externalIngressGateways); err != nil { @@ -258,8 +262,18 @@ func (r *Reconciler) reconcileIngress(ctx context.Context, ing *v1alpha1.Ingress } if ready { - publicLbs := getLBStatus(publicGatewayServiceURLFromContext(ctx)) - privateLbs := getLBStatus(privateGatewayServiceURLFromContext(ctx)) + publicGateways, err := resources.PublicGatewayServiceURLFromContext(ctx, ing) + if err != nil { + return fmt.Errorf("failed to get public gateways: %w", err) + } + publicLbs := getLBStatus(publicGateways) + + privateGateways, err := resources.PrivateGatewayServiceURLFromContext(ctx, ing) + if err != nil { + return fmt.Errorf("failed to get private gateways: %w", err) + } + privateLbs := getLBStatus(privateGateways) + ing.Status.MarkLoadBalancerReady(publicLbs, privateLbs) } else { ing.Status.MarkLoadBalancerNotReady() @@ -446,7 +460,7 @@ func (r *Reconciler) cleanupCertificateSecrets(ctx context.Context, ing *v1alpha errs := []error{} for _, tls := range ing.Spec.TLS { - nameNamespaces, err := resources.GetIngressGatewaySvcNameNamespaces(ctx) + nameNamespaces, err := resources.GetIngressGatewaySvcNameNamespaces(ctx, ing) if err != nil { errs = append(errs, err) continue @@ -518,41 +532,6 @@ func (r *Reconciler) GetDestinationRuleLister() istiolisters.DestinationRuleList return r.destinationRuleLister } -// qualifiedGatewayNamesFromContext get gateway names from context -func qualifiedGatewayNamesFromContext(ctx context.Context) map[v1alpha1.IngressVisibility]sets.Set[string] { - ci := config.FromContext(ctx).Istio - publicGateways := sets.New[string]() - for _, gw := range ci.IngressGateways { - publicGateways.Insert(gw.QualifiedName()) - } - - privateGateways := sets.New[string]() - for _, gw := range ci.LocalGateways { - privateGateways.Insert(gw.QualifiedName()) - } - - return map[v1alpha1.IngressVisibility]sets.Set[string]{ - v1alpha1.IngressVisibilityExternalIP: publicGateways, - v1alpha1.IngressVisibilityClusterLocal: privateGateways, - } -} - -func publicGatewayServiceURLFromContext(ctx context.Context) string { - cfg := config.FromContext(ctx).Istio - if len(cfg.IngressGateways) > 0 { - return cfg.IngressGateways[0].ServiceURL - } - return "" -} - -func privateGatewayServiceURLFromContext(ctx context.Context) string { - cfg := config.FromContext(ctx).Istio - if len(cfg.LocalGateways) > 0 { - return cfg.LocalGateways[0].ServiceURL - } - return "" -} - // getLBStatus gets the LB Status. func getLBStatus(gatewayServiceURL string) []v1alpha1.LoadBalancerIngressStatus { // The Ingress isn't load-balanced by any particular diff --git a/pkg/reconciler/ingress/lister.go b/pkg/reconciler/ingress/lister.go index 11e4778a9e..5f0449d0a3 100644 --- a/pkg/reconciler/ingress/lister.go +++ b/pkg/reconciler/ingress/lister.go @@ -31,6 +31,7 @@ import ( corev1listers "k8s.io/client-go/listers/core/v1" "k8s.io/client-go/tools/cache" istiolisters "knative.dev/net-istio/pkg/client/istio/listers/networking/v1beta1" + "knative.dev/net-istio/pkg/reconciler/ingress/resources" "knative.dev/networking/pkg/apis/networking/v1alpha1" "knative.dev/networking/pkg/ingress" "knative.dev/networking/pkg/k8s" @@ -60,7 +61,11 @@ type gatewayPodTargetLister struct { func (l *gatewayPodTargetLister) ListProbeTargets(ctx context.Context, ing *v1alpha1.Ingress) ([]status.ProbeTarget, error) { results := []status.ProbeTarget{} - hostsByGateway := ingress.HostsPerVisibility(ing, qualifiedGatewayNamesFromContext(ctx)) + gatewayQualifiedNames, err := resources.QualifiedGatewayNamesFromContext(ctx, ing) + if err != nil { + return nil, fmt.Errorf("failed to get gateways for ingress: %w", err) + } + hostsByGateway := ingress.HostsPerVisibility(ing, gatewayQualifiedNames) gatewayNames := make([]string, 0, len(hostsByGateway)) for gatewayName := range hostsByGateway { gatewayNames = append(gatewayNames, gatewayName) diff --git a/pkg/reconciler/ingress/lister_test.go b/pkg/reconciler/ingress/lister_test.go index 48f9ca1367..f8c616d249 100644 --- a/pkg/reconciler/ingress/lister_test.go +++ b/pkg/reconciler/ingress/lister_test.go @@ -1491,6 +1491,129 @@ func TestListProbeTargets(t *testing.T) { Port: "80", URLs: []*url.URL{{Scheme: "http", Host: "foo.bar.com:80"}}, }}, + }, { + name: "one gateway matched", + ingressGateways: []config.Gateway{ + { + Name: "gateway-not-matching", + Namespace: "default", + }, + { + Name: "gateway-matching", + Namespace: "default", + LabelSelector: &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: "exposition", + Operator: metav1.LabelSelectorOpIn, + Values: []string{"special"}, + }, + }, + }, + }}, + gatewayLister: &fakeGatewayLister{ + gateways: []*v1beta1.Gateway{{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "default", + Name: "gateway-not-matching", + }, + Spec: istiov1beta1.Gateway{ + Servers: []*istiov1beta1.Server{{ + Hosts: []string{"*"}, + Port: &istiov1beta1.Port{ + Name: "http", + Number: 8888, + Protocol: "HTTP", + }, + }}, + Selector: map[string]string{ + "gwt": "istio-not-matching", + }, + }, + }, + { + ObjectMeta: metav1.ObjectMeta{ + Namespace: "default", + Name: "gateway-matching", + }, + Spec: istiov1beta1.Gateway{ + Servers: []*istiov1beta1.Server{{ + Hosts: []string{"*"}, + Port: &istiov1beta1.Port{ + Name: "http", + Number: 8080, + Protocol: "HTTP", + }, + }}, + Selector: map[string]string{ + "gwt": "istio", + }, + }, + }}, + }, + endpointsLister: &fakeEndpointsLister{ + endpointses: []*v1.Endpoints{{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "default", + Name: "gateway-matching", + }, + Subsets: []v1.EndpointSubset{{ + Ports: []v1.EndpointPort{{ + Name: "bogus", + Port: 8081, + }, { + Name: "real", + Port: 8080, + }}, + Addresses: []v1.EndpointAddress{{ + IP: "1.1.1.1", + }}, + }}, + }}, + }, + serviceLister: &fakeServiceLister{ + services: []*v1.Service{{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "default", + Name: "gateway-matching", + Labels: map[string]string{ + "gwt": "istio", + }, + }, + Spec: v1.ServiceSpec{ + Ports: []v1.ServicePort{{ + Name: "bogus", + Port: 8081, + }, { + Name: "real", + Port: 8080, + }}, + }, + }}, + }, + ingress: &v1alpha1.Ingress{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "default", + Name: "whatever", + Labels: map[string]string{ + "exposition": "special", + }, + }, + Spec: v1alpha1.IngressSpec{ + Rules: []v1alpha1.IngressRule{{ + Hosts: []string{ + "foo.bar.com", + }, + Visibility: v1alpha1.IngressVisibilityExternalIP, + }}, + }, + }, + results: []status.ProbeTarget{{ + PodIPs: sets.New("1.1.1.1"), + PodPort: "8080", + Port: "8080", + URLs: []*url.URL{{Scheme: "http", Host: "foo.bar.com:8080"}}, + }}, }} for _, test := range tests { diff --git a/pkg/reconciler/ingress/resources/gateway.go b/pkg/reconciler/ingress/resources/gateway.go index 8e9eae59bd..e8db7182c1 100644 --- a/pkg/reconciler/ingress/resources/gateway.go +++ b/pkg/reconciler/ingress/resources/gateway.go @@ -30,6 +30,7 @@ import ( "istio.io/client-go/pkg/apis/networking/v1beta1" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/fields" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/util/sets" corev1listers "k8s.io/client-go/listers/core/v1" @@ -113,7 +114,7 @@ func MakeIngressTLSGateways(ctx context.Context, ing *v1alpha1.Ingress, visibili if len(ingressTLS) == 0 { return []*v1beta1.Gateway{}, nil } - gatewayServices, err := getGatewayServices(ctx, svcLister) + gatewayServices, err := getGatewayServices(ctx, ing, svcLister) if err != nil { return nil, err } @@ -130,7 +131,7 @@ func MakeIngressTLSGateways(ctx context.Context, ing *v1alpha1.Ingress, visibili // MakeExternalIngressGateways creates Gateways with given Servers for a given Ingress. func MakeExternalIngressGateways(ctx context.Context, ing *v1alpha1.Ingress, servers []*istiov1beta1.Server, svcLister corev1listers.ServiceLister) ([]*v1beta1.Gateway, error) { - gatewayServices, err := getGatewayServices(ctx, svcLister) + gatewayServices, err := getGatewayServices(ctx, ing, svcLister) if err != nil { return nil, err } @@ -146,12 +147,12 @@ func MakeExternalIngressGateways(ctx context.Context, ing *v1alpha1.Ingress, ser // MakeWildcardTLSGateways creates gateways that only contain TLS server with wildcard hosts based on the wildcard secret information. // For each public ingress service, we will create a list of Gateways. Each Gateway of the list corresponds to a wildcard cert secret. -func MakeWildcardTLSGateways(ctx context.Context, originWildcardSecrets map[string]*corev1.Secret, +func MakeWildcardTLSGateways(ctx context.Context, ing *v1alpha1.Ingress, originWildcardSecrets map[string]*corev1.Secret, svcLister corev1listers.ServiceLister) ([]*v1beta1.Gateway, error) { if len(originWildcardSecrets) == 0 { return []*v1beta1.Gateway{}, nil } - gatewayServices, err := getGatewayServices(ctx, svcLister) + gatewayServices, err := getGatewayServices(ctx, ing, svcLister) if err != nil { return nil, err } @@ -265,8 +266,8 @@ func makeIngressGateway(ing *v1alpha1.Ingress, visibility v1alpha1.IngressVisibi } } -func getGatewayServices(ctx context.Context, svcLister corev1listers.ServiceLister) ([]*corev1.Service, error) { - ingressSvcMetas, err := GetIngressGatewaySvcNameNamespaces(ctx) +func getGatewayServices(ctx context.Context, ing *v1alpha1.Ingress, svcLister corev1listers.ServiceLister) ([]*corev1.Service, error) { + ingressSvcMetas, err := GetIngressGatewaySvcNameNamespaces(ctx, ing) if err != nil { return nil, err } @@ -395,24 +396,47 @@ func GetNonWildcardIngressTLS(ingressTLS []v1alpha1.IngressTLS, nonWildcardSecre return result } -// GetIngressGatewaySvcNameNamespaces gets the Istio ingress namespaces from ConfigMap. -// TODO(nghia): Remove this by parsing at config parsing time. -func GetIngressGatewaySvcNameNamespaces(ctx context.Context) ([]metav1.ObjectMeta, error) { - cfg := config.FromContext(ctx).Istio - nameNamespaces := make([]metav1.ObjectMeta, len(cfg.IngressGateways)) - for i, ingressgateway := range cfg.IngressGateways { - parts := strings.SplitN(ingressgateway.ServiceURL, ".", 3) - if len(parts) != 3 { - return nil, fmt.Errorf("unexpected service URL form: %s", ingressgateway.ServiceURL) - } - nameNamespaces[i] = metav1.ObjectMeta{ - Name: parts[0], - Namespace: parts[1], +// GetIngressGatewaySvcNameNamespaces gets the Istio ingress namespaces from ConfigMap for gateways that should expose the service. +func GetIngressGatewaySvcNameNamespaces(ctx context.Context, ing *v1alpha1.Ingress) ([]metav1.ObjectMeta, error) { + nameNamespaces := make([]metav1.ObjectMeta, 0) + + serviceGateways, err := gatewaysFromContext(ctx, ing) + if err != nil { + return nil, fmt.Errorf("failed to get gateway from configuration: %w", err) + } + + servicePublicGateways, ok := serviceGateways[v1alpha1.IngressVisibilityExternalIP] + if !ok { + return nameNamespaces, nil + } + + for _, ingressgateway := range servicePublicGateways { + meta, err := parseIngressGatewayConfig(ingressgateway) + if err != nil { + return nil, err } + + nameNamespaces = append(nameNamespaces, meta) } + return nameNamespaces, nil } +// TODO(nghia): Remove this by parsing at config parsing time. +func parseIngressGatewayConfig(ingressgateway config.Gateway) (metav1.ObjectMeta, error) { + ret := metav1.ObjectMeta{} + + parts := strings.SplitN(ingressgateway.ServiceURL, ".", 3) + if len(parts) != 3 { + return ret, fmt.Errorf("unexpected service URL form: %s", ingressgateway.ServiceURL) + } + + ret.Name = parts[0] + ret.Namespace = parts[1] + + return ret, nil +} + // UpdateGateway replaces the existing servers with the wanted servers. func UpdateGateway(gateway *v1beta1.Gateway, want []*istiov1beta1.Server, existing []*istiov1beta1.Server) *v1beta1.Gateway { existingServers := sets.New[string]() @@ -445,3 +469,101 @@ func UpdateGateway(gateway *v1beta1.Gateway, want []*istiov1beta1.Server, existi func isPlaceHolderServer(server *istiov1beta1.Server) bool { return cmp.Equal(server, &placeholderServer, protocmp.Transform()) } + +// QualifiedGatewayNamesFromContext get gateway names from context. +func QualifiedGatewayNamesFromContext(ctx context.Context, ing *v1alpha1.Ingress) (map[v1alpha1.IngressVisibility]sets.Set[string], error) { + ret := make(map[v1alpha1.IngressVisibility]sets.Set[string]) + + gateways, err := gatewaysFromContext(ctx, ing) + if err != nil { + return ret, fmt.Errorf("failed to get gateways from configuration: %w", err) + } + + for _, visibility := range []v1alpha1.IngressVisibility{v1alpha1.IngressVisibilityClusterLocal, v1alpha1.IngressVisibilityExternalIP} { + ret[visibility] = sets.New[string]() + + for _, gtw := range gateways[visibility] { + ret[visibility] = ret[visibility].Insert(gtw.QualifiedName()) + } + } + + return ret, nil +} + +// gatewaysFromContext get gateways relevant to this ingress from context. +func gatewaysFromContext(ctx context.Context, ing *v1alpha1.Ingress) (map[v1alpha1.IngressVisibility][]config.Gateway, error) { + ret := make(map[v1alpha1.IngressVisibility][]config.Gateway) + + istioConfig := config.FromContext(ctx).Istio + + // External gateways selection + externalGateways, err := filterGateway(istioConfig.IngressGateways, ing.ObjectMeta.Labels) + if err != nil { + return ret, fmt.Errorf("failed to filter external gateways: %w", err) + } + + if len(externalGateways) == 0 { + externalGateways = istioConfig.DefaultExternalGateways() + } + + ret[v1alpha1.IngressVisibilityExternalIP] = externalGateways + + // Local gateways selection + localGateways, err := filterGateway(istioConfig.LocalGateways, ing.ObjectMeta.Labels) + if err != nil { + return ret, fmt.Errorf("failed to filter local gateways: %w", err) + } + + if len(localGateways) == 0 { + localGateways = istioConfig.DefaultLocalGateways() + } + + ret[v1alpha1.IngressVisibilityClusterLocal] = localGateways + + return ret, nil +} + +func filterGateway(gtws []config.Gateway, ingressLabels map[string]string) ([]config.Gateway, error) { + ret := make([]config.Gateway, 0, 1) + + for _, gtw := range gtws { + if gtw.LabelSelector == nil { // default value + continue + } + + selector, err := metav1.LabelSelectorAsSelector(gtw.LabelSelector) + if err != nil { + return ret, fmt.Errorf("failed to create selector from gateway (%s) label selector: %w", gtw.QualifiedName(), err) + } + + if !selector.Matches(fields.Set(ingressLabels)) { + continue + } + + ret = append(ret, gtw) + } + + return ret, nil +} + +func PublicGatewayServiceURLFromContext(ctx context.Context, ing *v1alpha1.Ingress) (string, error) { + return getGatewayServiceURLFromContext(ctx, ing, v1alpha1.IngressVisibilityExternalIP) +} + +func PrivateGatewayServiceURLFromContext(ctx context.Context, ing *v1alpha1.Ingress) (string, error) { + return getGatewayServiceURLFromContext(ctx, ing, v1alpha1.IngressVisibilityClusterLocal) +} + +func getGatewayServiceURLFromContext(ctx context.Context, ing *v1alpha1.Ingress, visibility v1alpha1.IngressVisibility) (string, error) { + allGateways, err := gatewaysFromContext(ctx, ing) + if err != nil { + return "", fmt.Errorf("failed to get gateways from configuration: %w", err) + } + + gateways, ok := allGateways[visibility] + if ok && len(gateways) > 0 { + return gateways[0].ServiceURL, nil + } + + return "", nil +} diff --git a/pkg/reconciler/ingress/resources/gateway_test.go b/pkg/reconciler/ingress/resources/gateway_test.go index b4d01ddf68..888ebe8b91 100644 --- a/pkg/reconciler/ingress/resources/gateway_test.go +++ b/pkg/reconciler/ingress/resources/gateway_test.go @@ -29,6 +29,7 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/util/sets" corev1listers "k8s.io/client-go/listers/core/v1" istiov1beta1 "istio.io/api/networking/v1beta1" @@ -157,6 +158,17 @@ var ingressResource = v1alpha1.Ingress{ Spec: ingressSpec, } +var ingressResourceWithPublicGatewayLabel = v1alpha1.Ingress{ + ObjectMeta: metav1.ObjectMeta{ + Name: "ingress", + Namespace: "test-ns", + Labels: map[string]string{ + "exposition": "special", + }, + }, + Spec: ingressSpec, +} + var ingressResourceWithDotName = v1alpha1.Ingress{ ObjectMeta: metav1.ObjectMeta{ Name: "ingress.com", @@ -682,7 +694,7 @@ func TestMakeWildcardGateways(t *testing.T) { }, }) t.Run(tc.name, func(t *testing.T) { - got, err := MakeWildcardTLSGateways(ctx, tc.wildcardSecrets, svcLister) + got, err := MakeWildcardTLSGateways(ctx, &ingressResource, tc.wildcardSecrets, svcLister) if (err != nil) != tc.wantErr { t.Fatalf("Test: %s; MakeWildcardGateways error = %v, WantErr %v", tc.name, err, tc.wantErr) } @@ -726,64 +738,137 @@ func TestGetQualifiedGatewayNames(t *testing.T) { } func TestMakeExternalIngressGateways(t *testing.T) { + createGateway := func(qualifiedName string, sel map[string]string, serv *istiov1beta1.Server) *v1beta1.Gateway { + return &v1beta1.Gateway{ + ObjectMeta: metav1.ObjectMeta{ + Name: fmt.Sprintf("ingress-%d", adler32.Checksum([]byte(qualifiedName))), + Namespace: "test-ns", + OwnerReferences: []metav1.OwnerReference{*kmeta.NewControllerRef(&ingressResource)}, + Labels: map[string]string{ + networking.IngressLabelKey: "ingress", + }, + }, + Spec: istiov1beta1.Gateway{ + Selector: sel, + Servers: []*istiov1beta1.Server{serv}, + }, + } + } + + configDefaultGateway := &config.Config{ + Istio: &config.Istio{ + IngressGateways: []config.Gateway{{ + Name: config.KnativeIngressGateway, + ServiceURL: fmt.Sprintf("%s.%s.svc.cluster.local", defaultGatewayService.Name, defaultGatewayService.Namespace), + }}, + }, + Network: &netconfig.Config{ + HTTPProtocol: netconfig.HTTPEnabled, + }, + } + + var gateway1Service = corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "gateway1", + Namespace: "aNamespace", + }, + Spec: corev1.ServiceSpec{ + Selector: map[string]string{ + "istio": "ingressgateway1", + }, + }, + } + + var gateway2Service = corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "gateway2", + Namespace: "aNamespace", + }, + Spec: corev1.ServiceSpec{ + Selector: map[string]string{ + "istio": "ingressgateway2", + }, + }, + } + + configDoubleGateway := &config.Config{ + Istio: &config.Istio{ + IngressGateways: []config.Gateway{ + { + Namespace: "knative-serving", + Name: "gateway1", + ServiceURL: "gateway1.aNamespace.svc.cluster.local", + LabelSelector: &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: "exposition", + Operator: metav1.LabelSelectorOpIn, + Values: []string{"special"}, + }, + }, + }, + }, + { + Namespace: "knative-serving", + Name: "gateway2", + ServiceURL: "gateway2.aNamespace.svc.cluster.local", + }, + }, + }, + Network: &netconfig.Config{ + HTTPProtocol: netconfig.HTTPEnabled, + }, + } + cases := []struct { name string ia *v1alpha1.Ingress + conf *config.Config servers []*istiov1beta1.Server want []*v1beta1.Gateway wantErr bool }{{ name: "HTTP server", ia: &ingressResource, + conf: configDefaultGateway, servers: []*istiov1beta1.Server{&httpServer}, - want: []*v1beta1.Gateway{{ - ObjectMeta: metav1.ObjectMeta{ - Name: fmt.Sprintf("ingress-%d", adler32.Checksum([]byte("istio-system/istio-ingressgateway"))), - Namespace: "test-ns", - OwnerReferences: []metav1.OwnerReference{*kmeta.NewControllerRef(&ingressResource)}, - Labels: map[string]string{ - networking.IngressLabelKey: "ingress", - }, - }, - Spec: istiov1beta1.Gateway{ - Selector: selector, - Servers: []*istiov1beta1.Server{&httpServer}, - }, - }}, + want: []*v1beta1.Gateway{createGateway("istio-system/istio-ingressgateway", selector, &httpServer)}, }, { name: "HTTPS server", ia: &ingressResource, + conf: configDefaultGateway, servers: []*istiov1beta1.Server{&modifiedDefaultTLSServer}, - want: []*v1beta1.Gateway{{ - ObjectMeta: metav1.ObjectMeta{ - Name: fmt.Sprintf("ingress-%d", adler32.Checksum([]byte("istio-system/istio-ingressgateway"))), - Namespace: "test-ns", - OwnerReferences: []metav1.OwnerReference{*kmeta.NewControllerRef(&ingressResource)}, - Labels: map[string]string{ - networking.IngressLabelKey: "ingress", - }, - }, - Spec: istiov1beta1.Gateway{ - Selector: selector, - Servers: []*istiov1beta1.Server{&modifiedDefaultTLSServer}, - }, - }}, + want: []*v1beta1.Gateway{createGateway("istio-system/istio-ingressgateway", selector, &modifiedDefaultTLSServer)}, + }, { + name: "HTTP Server Gateways filtered", + ia: &ingressResourceWithPublicGatewayLabel, + conf: configDoubleGateway, + servers: []*istiov1beta1.Server{&httpServer}, + want: []*v1beta1.Gateway{createGateway("aNamespace/gateway1", map[string]string{ + "istio": "ingressgateway1", + }, &httpServer)}, + }, { + name: "HTTPS Server Gateways filtered", + ia: &ingressResourceWithPublicGatewayLabel, + conf: configDoubleGateway, + servers: []*istiov1beta1.Server{&modifiedDefaultTLSServer}, + want: []*v1beta1.Gateway{createGateway("aNamespace/gateway1", map[string]string{ + "istio": "ingressgateway1", + }, &modifiedDefaultTLSServer)}, + }, { + name: "No gateway matched", + ia: &ingressResourceWithPublicGatewayLabel, + conf: configDefaultGateway, // default config have a default gateway + servers: []*istiov1beta1.Server{&httpServer}, + want: []*v1beta1.Gateway{createGateway("istio-system/istio-ingressgateway", selector, &httpServer)}, }} for _, c := range cases { ctx, cancel, _ := rtesting.SetupFakeContextWithCancel(t) defer cancel() - svcLister := serviceLister(ctx, &defaultGatewayService) - ctx = config.ToContext(context.Background(), &config.Config{ - Istio: &config.Istio{ - IngressGateways: []config.Gateway{{ - Name: config.KnativeIngressGateway, - ServiceURL: fmt.Sprintf("%s.%s.svc.cluster.local", defaultGatewayService.Name, defaultGatewayService.Namespace), - }}, - }, - Network: &netconfig.Config{ - HTTPProtocol: netconfig.HTTPEnabled, - }, - }) + + svcLister := serviceLister(ctx, &defaultGatewayService, &gateway1Service, &gateway2Service) + ctx = config.ToContext(context.Background(), c.conf) + t.Run(c.name, func(t *testing.T) { got, err := MakeExternalIngressGateways(ctx, c.ia, c.servers, svcLister) if (err != nil) != c.wantErr { @@ -1082,3 +1167,179 @@ func TestGatewayNameLongIngressName(t *testing.T) { t.Errorf("Unexpected gateway name. want %q, got %q", want, got) } } + +func TestParseIngressGatewayConfig(t *testing.T) { + type Expectation struct { + Error bool + Value metav1.ObjectMeta + } + + cases := []struct { + name string + gateway config.Gateway + want Expectation + }{ + { + name: "Happy path svc.cluster.local", + gateway: config.Gateway{ServiceURL: "service.namespace.svc.cluster.local"}, + want: Expectation{Value: metav1.ObjectMeta{Name: "service", Namespace: "namespace"}}, + }, + { + name: "Happy path custom suffix", + gateway: config.Gateway{ServiceURL: "service.namespace.customsuffix"}, + want: Expectation{Value: metav1.ObjectMeta{Name: "service", Namespace: "namespace"}}, + }, + { + name: "Invalid service URL, no suffix", + gateway: config.Gateway{ServiceURL: "service.namespace"}, + want: Expectation{Error: true}, + }, + { + name: "Invalid service URL, no namespace, no suffix", + gateway: config.Gateway{ServiceURL: "service"}, + want: Expectation{Error: true}, + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + gotMeta, err := parseIngressGatewayConfig(c.gateway) + if (err != nil) != c.want.Error { + t.Errorf("Expecting error: %v, got error: %v", c.want.Error, err) + } + + if !c.want.Error { + if diff := cmp.Diff(c.want.Value, gotMeta); diff != "" { + t.Error("Unexpected meta (-want, +got):", diff) + } + } + }) + } +} + +func TestQualifiedGatewayNamesFromContext(t *testing.T) { + cases := []struct { + name string + cfg *config.Istio + ingress *v1alpha1.Ingress + want map[v1alpha1.IngressVisibility]sets.Set[string] + shouldFail bool + }{ + { + name: "All match", + cfg: &config.Istio{ + IngressGateways: []config.Gateway{ + {Namespace: "ns1", Name: "gtw1", LabelSelector: &metav1.LabelSelector{MatchLabels: map[string]string{"expo": "my-value"}}}, + }, + LocalGateways: []config.Gateway{ + {Namespace: "ns1", Name: "gtw2"}, + }, + }, + ingress: &v1alpha1.Ingress{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{ + "expo": "my-value", + }}}, + want: map[v1alpha1.IngressVisibility]sets.Set[string]{ + v1alpha1.IngressVisibilityExternalIP: sets.New[string]("ns1/gtw1"), + v1alpha1.IngressVisibilityClusterLocal: sets.New[string]("ns1/gtw2"), + }, + }, + { + name: "Different expo", + cfg: &config.Istio{ + IngressGateways: []config.Gateway{ + {Namespace: "ns1", Name: "gtw1", LabelSelector: &metav1.LabelSelector{MatchLabels: map[string]string{"expo1": "my-value-1"}}}, + {Namespace: "ns1", Name: "gtw2", LabelSelector: &metav1.LabelSelector{MatchLabels: map[string]string{"expo2": "my-value-2"}}}, + }, + LocalGateways: []config.Gateway{ + {Namespace: "ns2", Name: "gtw3"}, + }, + }, + ingress: &v1alpha1.Ingress{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{ + "expo1": "my-value-1", + }}}, + want: map[v1alpha1.IngressVisibility]sets.Set[string]{ + v1alpha1.IngressVisibilityExternalIP: sets.New[string]("ns1/gtw1"), + v1alpha1.IngressVisibilityClusterLocal: sets.New[string]("ns2/gtw3"), + }, + }, + { + name: "Partial match", + cfg: &config.Istio{ + IngressGateways: []config.Gateway{ + {Namespace: "ns1", Name: "gtw1"}, + {Namespace: "wontmatch", Name: "wontmatch1", LabelSelector: &metav1.LabelSelector{MatchLabels: map[string]string{"good-key": "bad-value"}}}, + {Namespace: "wontmatch", Name: "wontmatch2", LabelSelector: &metav1.LabelSelector{MatchLabels: map[string]string{"bad-key": "good-value"}}}, + {Namespace: "matchingns", Name: "matching", LabelSelector: &metav1.LabelSelector{MatchLabels: map[string]string{"good-key": "good-value"}}}, + }, + LocalGateways: []config.Gateway{ + {Namespace: "ns1", Name: "gtw2"}, + }, + }, + ingress: &v1alpha1.Ingress{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{ + "good-key": "good-value", + }}}, + want: map[v1alpha1.IngressVisibility]sets.Set[string]{ + v1alpha1.IngressVisibilityExternalIP: sets.New[string]("matchingns/matching"), + v1alpha1.IngressVisibilityClusterLocal: sets.New[string]("ns1/gtw2"), + }, + }, + { + name: "Matching default", + cfg: &config.Istio{ + IngressGateways: []config.Gateway{ + {Namespace: "ns1", Name: "gtw1"}, + {Namespace: "wontmatch", Name: "wontmatch", LabelSelector: &metav1.LabelSelector{MatchLabels: map[string]string{"foo": "bar"}}}, + }, + LocalGateways: []config.Gateway{ + {Namespace: "ns1", Name: "gtw2"}, + }, + }, + ingress: &v1alpha1.Ingress{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{ + "not": "relevant", + }}}, + want: map[v1alpha1.IngressVisibility]sets.Set[string]{ + v1alpha1.IngressVisibilityExternalIP: sets.New[string]("ns1/gtw1"), + v1alpha1.IngressVisibilityClusterLocal: sets.New[string]("ns1/gtw2"), + }, + }, + { + name: "No annotation", + cfg: &config.Istio{ + IngressGateways: []config.Gateway{ + {Namespace: "ns1", Name: "gtw1"}, + }, + LocalGateways: []config.Gateway{ + {Namespace: "ns1", Name: "gtw2"}, + }, + }, + ingress: &v1alpha1.Ingress{}, + want: map[v1alpha1.IngressVisibility]sets.Set[string]{ + v1alpha1.IngressVisibilityExternalIP: sets.New[string]("ns1/gtw1"), + v1alpha1.IngressVisibilityClusterLocal: sets.New[string]("ns1/gtw2"), + }, + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + ctx := config.ToContext(context.Background(), &config.Config{Istio: c.cfg}) + + got, err := QualifiedGatewayNamesFromContext(ctx, c.ingress) + if c.shouldFail && (err == nil) { + t.Fatal("test is supposed to fail and it's not") + } + + if !c.shouldFail && (err != nil) { + t.Fatal("test is supposed to succeed and it's not") + } + + if c.shouldFail { + return + } + + if diff := cmp.Diff(c.want, got); diff != "" { + t.Error("Unexpected Gateways (-want, +got):", diff) + } + }) + } +} diff --git a/pkg/reconciler/ingress/resources/secret.go b/pkg/reconciler/ingress/resources/secret.go index 1ebcb9c368..7781211f50 100644 --- a/pkg/reconciler/ingress/resources/secret.go +++ b/pkg/reconciler/ingress/resources/secret.go @@ -52,8 +52,8 @@ func GetSecrets(ing *v1alpha1.Ingress, visibility v1alpha1.IngressVisibility, se } // MakeSecrets makes copies of the origin Secrets under the namespace of Istio gateway service. -func MakeSecrets(ctx context.Context, originSecrets map[string]*corev1.Secret, accessor kmeta.OwnerRefableAccessor) ([]*corev1.Secret, error) { - nameNamespaces, err := GetIngressGatewaySvcNameNamespaces(ctx) +func MakeSecrets(ctx context.Context, originSecrets map[string]*corev1.Secret, ing *v1alpha1.Ingress) ([]*corev1.Secret, error) { + nameNamespaces, err := GetIngressGatewaySvcNameNamespaces(ctx, ing) if err != nil { return nil, err } @@ -65,7 +65,7 @@ func MakeSecrets(ctx context.Context, originSecrets map[string]*corev1.Secret, a // as the origin namespace continue } - secrets = append(secrets, makeSecret(originSecret, targetSecret(originSecret, accessor), meta.Namespace, + secrets = append(secrets, makeSecret(originSecret, targetSecret(originSecret, ing), meta.Namespace, MakeTargetSecretLabels(originSecret.Name, originSecret.Namespace), MakeTargetSecretAnnotations(originSecret.Name))) } } @@ -74,8 +74,8 @@ func MakeSecrets(ctx context.Context, originSecrets map[string]*corev1.Secret, a // MakeWildcardSecrets copies wildcard certificates from origin namespace to the namespace of gateway services, so they can be // consumed by Istio ingress. -func MakeWildcardSecrets(ctx context.Context, originWildcardCerts map[string]*corev1.Secret) ([]*corev1.Secret, error) { - nameNamespaces, err := GetIngressGatewaySvcNameNamespaces(ctx) +func MakeWildcardSecrets(ctx context.Context, originWildcardCerts map[string]*corev1.Secret, ing *v1alpha1.Ingress) ([]*corev1.Secret, error) { + nameNamespaces, err := GetIngressGatewaySvcNameNamespaces(ctx, ing) if err != nil { return nil, err } diff --git a/pkg/reconciler/ingress/resources/secret_test.go b/pkg/reconciler/ingress/resources/secret_test.go index 59d0db443c..f5947c11ce 100644 --- a/pkg/reconciler/ingress/resources/secret_test.go +++ b/pkg/reconciler/ingress/resources/secret_test.go @@ -292,7 +292,7 @@ func TestMakeWildcardSecrets(t *testing.T) { originSecrets := map[string]*corev1.Secret{ fmt.Sprintf("%s/%s", c.originSecret.Namespace, c.originSecret.Name): c.originSecret, } - secrets, err := MakeWildcardSecrets(ctx, originSecrets) + secrets, err := MakeWildcardSecrets(ctx, originSecrets, &ci) if (err != nil) != c.wantErr { t.Fatalf("Test: %q; MakeWildcardSecrets() error = %v, WantErr %v", c.name, err, c.wantErr) } diff --git a/test/e2e/exposition_test.go b/test/e2e/exposition_test.go new file mode 100644 index 0000000000..7a0f1c3447 --- /dev/null +++ b/test/e2e/exposition_test.go @@ -0,0 +1,260 @@ +//go:build e2e +// +build e2e + +/* +Copyright 2020 The Knative Authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package e2e + +import ( + "context" + "testing" + + "github.com/google/go-cmp/cmp" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/intstr" + "knative.dev/net-istio/pkg/reconciler/ingress/config" + "knative.dev/networking/pkg/apis/networking" + "knative.dev/networking/pkg/apis/networking/v1alpha1" + "knative.dev/networking/test" + "knative.dev/networking/test/conformance/ingress" + "knative.dev/pkg/system" +) + +func TestExposition(t *testing.T) { + clients := Setup(t) + namespace := system.Namespace() + + // Save the current config to restore it at the end of the test + oldConfig, err := clients.KubeClient.CoreV1().ConfigMaps(namespace).Get(context.Background(), config.IstioConfigName, metav1.GetOptions{}) + if err != nil { + t.Fatalf("Failed to get configmap %s/%s", namespace, config.IstioConfigName) + } + + // After the test ends, restore the old gateway + test.EnsureCleanup(t, func() { + curConfig, err := clients.KubeClient.CoreV1().ConfigMaps(namespace).Get(context.Background(), config.IstioConfigName, metav1.GetOptions{}) + if err != nil { + t.Fatalf("Failed to get configmap %s/%s", namespace, config.IstioConfigName) + } + + curConfig.Data = oldConfig.Data + if _, err := clients.KubeClient.CoreV1().ConfigMaps(namespace).Update(context.Background(), curConfig, metav1.UpdateOptions{}); err != nil { + t.Fatalf("Failed to restore configmap %s/%s: %v", namespace, config.IstioConfigName, err) + } + }) + + cases := []struct { + name string + configMapData map[string]string + ingressLabels map[string]string + expectedPrivateIngresses []v1alpha1.LoadBalancerIngressStatus + expectedPublicIngresses []v1alpha1.LoadBalancerIngressStatus + }{ + { + name: "no label", + configMapData: map[string]string{ + "local-gateways": ` + - namespace: "knative-serving" + name: "knative-local-gateway" + service: "istio-ingressgateway.istio-system.svc.cluster.local"`, + + "external-gateways": ` + - namespace: "knative-serving" + name: "knative-ingress-gateway" + service: "istio-ingressgateway.istio-system.svc.cluster.local"`, + }, + expectedPrivateIngresses: []v1alpha1.LoadBalancerIngressStatus{ + {MeshOnly: false, DomainInternal: "istio-ingressgateway.istio-system.svc.cluster.local"}, + }, + expectedPublicIngresses: []v1alpha1.LoadBalancerIngressStatus{ + {MeshOnly: false, DomainInternal: "istio-ingressgateway.istio-system.svc.cluster.local"}, + }, + }, + { + name: "match all", + configMapData: map[string]string{ + "local-gateways": ` + - namespace: "knative-serving" + name: "knative-local-gateway" + service: "istio-ingressgateway.istio-system.svc.cluster.local"`, + + "external-gateways": ` + - namespace: "knative-serving" + name: "knative-ingress-gateway" + service: "istio-ingressgateway.istio-system.svc.cluster.local" + labelSelector: + matchExpressions: + - key: "foo" + operator: "In" + values: ["bar"]`, + }, + ingressLabels: map[string]string{"foo": "bar"}, + expectedPrivateIngresses: []v1alpha1.LoadBalancerIngressStatus{ + {MeshOnly: false, DomainInternal: "istio-ingressgateway.istio-system.svc.cluster.local"}, + }, + expectedPublicIngresses: []v1alpha1.LoadBalancerIngressStatus{ + {MeshOnly: false, DomainInternal: "istio-ingressgateway.istio-system.svc.cluster.local"}, + }, + }, + { + name: "match only one", + configMapData: map[string]string{ + "local-gateways": ` + - namespace: "knative-serving" + name: "knative-local-gateway" + service: "istio-ingressgateway.istio-system.svc.cluster.local"`, + + "external-gateways": ` + - namespace: "knative-serving" + name: "knative-ingress-gateway" + service: "istio-ingressgateway.istio-system.svc.cluster.local" + labelSelector: + matchExpressions: + - key: "foo" + operator: "In" + values: ["bar"] + - namespace: "knative-serving" + name: "knative-local-gateway" + service: "istio-ingressgateway.istio-system.svc.cluster.local" + labelSelector: + matchExpressions: + - key: "wont" + operator: "In" + values: ["match"]`, + }, + ingressLabels: map[string]string{"foo": "bar"}, + expectedPrivateIngresses: []v1alpha1.LoadBalancerIngressStatus{ + {MeshOnly: false, DomainInternal: "istio-ingressgateway.istio-system.svc.cluster.local"}, + }, + expectedPublicIngresses: []v1alpha1.LoadBalancerIngressStatus{ + {MeshOnly: false, DomainInternal: "istio-ingressgateway.istio-system.svc.cluster.local"}, + }, + }, + { + name: "match nothing", + configMapData: map[string]string{ + "local-gateways": ` + - namespace: "knative-serving" + name: "knative-local-gateway" + service: "istio-ingressgateway.istio-system.svc.cluster.local"`, + + "external-gateways": ` + - namespace: "knative-serving" + name: "knative-ingress-gateway" + service: "istio-ingressgateway.istio-system.svc.cluster.local" + labelSelector: + matchExpressions: + - key: "wont" + operator: "In" + values: ["match"]`, + }, + ingressLabels: map[string]string{"not": "relevant"}, + expectedPrivateIngresses: []v1alpha1.LoadBalancerIngressStatus{ + {MeshOnly: false, DomainInternal: "istio-ingressgateway.istio-system.svc.cluster.local"}, + }, + expectedPublicIngresses: []v1alpha1.LoadBalancerIngressStatus{ + {MeshOnly: true}, + }, + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + // Update configmap + cm, err := clients.KubeClient.CoreV1().ConfigMaps(namespace).Get(context.Background(), config.IstioConfigName, metav1.GetOptions{}) + if err != nil { + t.Fatalf("Failed to get configmap %s/%s", namespace, config.IstioConfigName) + } + + cm.Data = c.configMapData + + if _, err := clients.KubeClient.CoreV1().ConfigMaps(namespace).Update(context.Background(), cm, metav1.UpdateOptions{}); err != nil { + t.Fatalf("Failed to update configmap %s/%s: %v", namespace, config.IstioConfigName, err) + } + + // Create ingress + name, port, cancel := ingress.CreateRuntimeService(context.Background(), t, clients.NetworkingClient, networking.ServicePortNameHTTP1) + hosts := []string{name + ".example.com"} + + spec := v1alpha1.IngressSpec{ + Rules: []v1alpha1.IngressRule{{ + Hosts: hosts, + Visibility: v1alpha1.IngressVisibilityExternalIP, + HTTP: &v1alpha1.HTTPIngressRuleValue{ + Paths: []v1alpha1.HTTPIngressPath{{ + Splits: []v1alpha1.IngressBackendSplit{{ + IngressBackend: v1alpha1.IngressBackend{ + ServiceName: name, + ServiceNamespace: test.ServingNamespace, + ServicePort: intstr.FromInt(port), + }, + }}, + }}, + }, + }}, + } + + ing, cancel2 := ingress.CreateIngress(context.Background(), t, clients.NetworkingClient, spec, func(target *v1alpha1.Ingress) { + if target.ObjectMeta.Labels == nil { + target.ObjectMeta.Labels = make(map[string]string) + } + + for k, v := range c.ingressLabels { + target.ObjectMeta.Labels[k] = v + } + }) + + // Wait ingress to be ready and get its status + for { + ing, err = clients.NetworkingClient.NetworkingClient.Ingresses.Get(context.Background(), ing.Name, metav1.GetOptions{}) + if err != nil { + t.Fatalf("Failed to get ingress %s", ing.Name) + } + + if ing.IsReady() { + break + } + } + + // Check + diff := cmp.Diff(c.expectedPrivateIngresses, ing.Status.PrivateLoadBalancer.Ingress) + if diff != "" { + t.Error("Unexpected private ingresses (-want, +got):", diff) + } + + diff = cmp.Diff(c.expectedPublicIngresses, ing.Status.PublicLoadBalancer.Ingress) + if diff != "" { + t.Error("Unexpected public ingresses (-want, +got):", diff) + } + + cancel2() + cancel() + + // Restore configmap + cm, err = clients.KubeClient.CoreV1().ConfigMaps(namespace).Get(context.Background(), config.IstioConfigName, metav1.GetOptions{}) + if err != nil { + t.Fatalf("Failed to get configmap %s/%s", namespace, config.IstioConfigName) + } + + cm.Data = oldConfig.Data + + if _, err := clients.KubeClient.CoreV1().ConfigMaps(namespace).Update(context.Background(), cm, metav1.UpdateOptions{}); err != nil { + t.Fatalf("Failed to restore configmap %s/%s: %v", namespace, config.IstioConfigName, err) + } + }) + } +} From 19213204b370f6eb27702789c2f02b8ca7f29b4f Mon Sep 17 00:00:00 2001 From: pastequo Date: Thu, 29 Feb 2024 17:48:17 +0100 Subject: [PATCH 2/5] chore: Fix various MR comments --- pkg/reconciler/ingress/config/istio.go | 8 + pkg/reconciler/ingress/config/istio_test.go | 191 +++++++++--------- pkg/reconciler/ingress/ingress.go | 41 ++-- pkg/reconciler/ingress/resources/gateway.go | 42 +--- .../ingress/resources/gateway_test.go | 28 +-- 5 files changed, 156 insertions(+), 154 deletions(-) diff --git a/pkg/reconciler/ingress/config/istio.go b/pkg/reconciler/ingress/config/istio.go index 26a7c5303f..e3c43084ca 100644 --- a/pkg/reconciler/ingress/config/istio.go +++ b/pkg/reconciler/ingress/config/istio.go @@ -231,6 +231,14 @@ func parseNewFormat(configMap *corev1.ConfigMap) (*Istio, error) { ret.LocalGateways = localGateways } + if len(ret.DefaultExternalGateways()) > 1 { + return ret, fmt.Errorf("only one external gateway with no selector can be defined, here: %v", ret.DefaultExternalGateways()) + } + + if len(ret.DefaultLocalGateways()) > 1 { + return ret, fmt.Errorf("only one local gateway with no selector can be defined, here: %v", ret.DefaultLocalGateways()) + } + return ret, nil } diff --git a/pkg/reconciler/ingress/config/istio_test.go b/pkg/reconciler/ingress/config/istio_test.go index ebef1b59e2..8e3865ce22 100644 --- a/pkg/reconciler/ingress/config/istio_test.go +++ b/pkg/reconciler/ingress/config/istio_test.go @@ -17,6 +17,7 @@ limitations under the License. package config import ( + "strings" "testing" "github.com/google/go-cmp/cmp" @@ -233,14 +234,14 @@ func TestGatewayConfiguration(t *testing.T) { 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"`, + "external-gateways": replaceTabs(` + - namespace: "namespace1" + name: "gateway1" + service: "istio-ingressbackroad.istio-system.svc.cluster.local"`), + "local-gateways": replaceTabs(` + - namespace: "namespace2" + name: "gateway2" + service: "istio-local-gateway.istio-system.svc.cluster.local"`), }, }, }, { @@ -252,14 +253,14 @@ func TestGatewayConfiguration(t *testing.T) { 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"`, + "external-gateways": replaceTabs(` + - namespace: "namespace1" + name: "gateway1" + service: "istio-ingressbackroad.istio-system.svc.cluster.local"`), + "local-gateways": replaceTabs(` + - 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", }, @@ -273,14 +274,14 @@ func TestGatewayConfiguration(t *testing.T) { 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"`, + "external-gateways": replaceTabs(` + - namespace: "namespace1" + name: "gateway1" + service: "istio-ingressbackroad.istio-system.svc.cluster.local"`), + "local-gateways": replaceTabs(` + - namespace: "namespace2" + name: "gateway2" + service: "_invalid"`), }, }, }, { @@ -292,14 +293,14 @@ func TestGatewayConfiguration(t *testing.T) { 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"`, + "external-gateways": replaceTabs(` + - namespace: "namespace1" + name: "gateway1" + service: ""`), + "local-gateways": replaceTabs(` + - namespace: "namespace2" + name: "gateway2" + service: "istio-local-gateway.istio-system.svc.cluster.local"`), }, }, }, { @@ -311,14 +312,14 @@ func TestGatewayConfiguration(t *testing.T) { 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"`, + "external-gateways": replaceTabs(` + - namespace: "namespace1" + name: "" + service: "istio-ingressbackroad.istio-system.svc.cluster.local"`), + "local-gateways": replaceTabs(` + - namespace: "namespace2" + name: "gateway2" + service: "istio-local-gateway.istio-system.svc.cluster.local"`), }, }, }, { @@ -330,14 +331,14 @@ func TestGatewayConfiguration(t *testing.T) { 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"`, + "external-gateways": replaceTabs(` + - namespace: "namespace1" + name: "gateway1" + service: "istio-ingressbackroad.istio-system.svc.cluster.local"`), + "local-gateways": replaceTabs(` + - namespace: "" + name: "gateway2" + service: "istio-local-gateway.istio-system.svc.cluster.local"`), }, }, }, { @@ -349,10 +350,10 @@ func TestGatewayConfiguration(t *testing.T) { Name: IstioConfigName, }, Data: map[string]string{ - "external-gateways": ` - - namespace: "namespace1" - name: "gateway1" - service: "istio-ingressbackroad.istio-system.svc.cluster.local"`, + "external-gateways": replaceTabs(` + - namespace: "namespace1" + name: "gateway1" + service: "istio-ingressbackroad.istio-system.svc.cluster.local"`), "local-gateways": "notYAML", }, }, @@ -366,10 +367,10 @@ func TestGatewayConfiguration(t *testing.T) { }, Data: map[string]string{ "external-gateways": "notYAML", - "local-gateways": ` - - namespace: "namespace2" - name: "gateway2" - service: "istio-local-gateway.istio-system.svc.cluster.local"`, + "local-gateways": replaceTabs(` + - namespace: "namespace2" + name: "gateway2" + service: "istio-local-gateway.istio-system.svc.cluster.local"`), }, }, }, { @@ -380,10 +381,10 @@ func TestGatewayConfiguration(t *testing.T) { Name: IstioConfigName, }, Data: map[string]string{ - "local-gateways": ` - - namespace: "namespace2" - name: "gateway2" - service: "istio-local-gateway.istio-system.svc.cluster.local"`, + "local-gateways": replaceTabs(` + - namespace: "namespace2" + name: "gateway2" + service: "istio-local-gateway.istio-system.svc.cluster.local"`), }, }, wantIstio: &Istio{ @@ -402,10 +403,10 @@ func TestGatewayConfiguration(t *testing.T) { Name: IstioConfigName, }, Data: map[string]string{ - "external-gateways": ` - - namespace: "namespace1" - name: "gateway1" - service: "istio-ingressbackroad.istio-system.svc.cluster.local"`, + "external-gateways": replaceTabs(` + - namespace: "namespace1" + name: "gateway1" + service: "istio-ingressbackroad.istio-system.svc.cluster.local"`), }, }, wantIstio: &Istio{ @@ -424,20 +425,20 @@ func TestGatewayConfiguration(t *testing.T) { Name: IstioConfigName, }, Data: map[string]string{ - "external-gateways": ` - - namespace: "namespace" - name: "gateway" - service: "istio-gateway.istio-system.svc.cluster.local" - labelSelector: - matchExpressions: - - key: "key" - operator: "In" - values: ["value"]`, + "external-gateways": replaceTabs(` + - namespace: "namespace" + name: "gateway" + service: "istio-gateway.istio-system.svc.cluster.local" + labelSelector: + matchExpressions: + - key: "key" + operator: "In" + values: ["value"]`), - "local-gateways": ` - - namespace: "namespace2" - name: "gateway2" - service: "istio-local-gateway.istio-system.svc.cluster.local"`, + "local-gateways": replaceTabs(` + - namespace: "namespace2" + name: "gateway2" + service: "istio-local-gateway.istio-system.svc.cluster.local"`), }, }, wantIstio: &Istio{ @@ -469,23 +470,23 @@ func TestGatewayConfiguration(t *testing.T) { Name: IstioConfigName, }, Data: map[string]string{ - "local-gateways": ` - - namespace: "namespace" - name: "gateway" - service: "istio-local.istio-system.svc.cluster.local" - labelSelector: - matchExpressions: - - key: "key" - operator: "In" - values: ["value"] - - namespace: "namespace1" - name: "gateway1" - service: "istio-local-gateway.istio-system.svc.cluster.local"`, + "local-gateways": replaceTabs(` + - namespace: "namespace" + name: "gateway" + service: "istio-local.istio-system.svc.cluster.local" + labelSelector: + matchExpressions: + - key: "key" + operator: "In" + values: ["value"] + - namespace: "namespace1" + name: "gateway1" + service: "istio-local-gateway.istio-system.svc.cluster.local"`), - "external-gateways": ` - - namespace: "namespace2" - name: "gateway2" - service: "istio-gateway.istio-system.svc.cluster.local"`, + "external-gateways": replaceTabs(` + - namespace: "namespace2" + name: "gateway2" + service: "istio-gateway.istio-system.svc.cluster.local"`), }, }, wantIstio: &Istio{ @@ -531,3 +532,7 @@ func TestGatewayConfiguration(t *testing.T) { }) } } + +func replaceTabs(s string) string { + return strings.ReplaceAll(s, "\t", " ") +} diff --git a/pkg/reconciler/ingress/ingress.go b/pkg/reconciler/ingress/ingress.go index d8370dfcc3..6a87c13751 100644 --- a/pkg/reconciler/ingress/ingress.go +++ b/pkg/reconciler/ingress/ingress.go @@ -113,14 +113,19 @@ func (r *Reconciler) reconcileIngress(ctx context.Context, ing *v1alpha1.Ingress ing.Status.InitializeConditions() logger.Infof("Reconciling ingress: %#v", ing) - defaultGateways, err := resources.QualifiedGatewayNamesFromContext(ctx, ing) + defaultGateways, err := resources.GatewaysFromContext(ctx, ing) if err != nil { return err } - gatewayNames := map[v1alpha1.IngressVisibility]sets.Set[string]{} - gatewayNames[v1alpha1.IngressVisibilityClusterLocal] = defaultGateways[v1alpha1.IngressVisibilityClusterLocal] - gatewayNames[v1alpha1.IngressVisibilityExternalIP] = sets.New[string]() + gatewayNames := map[v1alpha1.IngressVisibility]sets.Set[string]{ + v1alpha1.IngressVisibilityClusterLocal: sets.New[string](), + v1alpha1.IngressVisibilityExternalIP: sets.New[string](), + } + + for _, gateway := range defaultGateways[v1alpha1.IngressVisibilityClusterLocal] { + gatewayNames[v1alpha1.IngressVisibilityClusterLocal].Insert(gateway.QualifiedName()) + } externalIngressGateways := []*v1beta1.Gateway{} if shouldReconcileExternalDomainTLS(ing) { @@ -206,7 +211,11 @@ func (r *Reconciler) reconcileIngress(ctx context.Context, ing *v1alpha1.Ingress } else { // Otherwise, we fall back to the default global Gateways for HTTP behavior. // We need this for the backward compatibility. - gatewayNames[v1alpha1.IngressVisibilityExternalIP].Insert(sets.List(defaultGateways[v1alpha1.IngressVisibilityExternalIP])...) + defaultGlobalHTTPGateways := defaultGateways[v1alpha1.IngressVisibilityExternalIP] + + for _, gateway := range defaultGlobalHTTPGateways { + gatewayNames[v1alpha1.IngressVisibilityExternalIP].Insert(gateway.QualifiedName()) + } } if err := r.reconcileIngressGateways(ctx, externalIngressGateways); err != nil { @@ -262,17 +271,11 @@ func (r *Reconciler) reconcileIngress(ctx context.Context, ing *v1alpha1.Ingress } if ready { - publicGateways, err := resources.PublicGatewayServiceURLFromContext(ctx, ing) - if err != nil { - return fmt.Errorf("failed to get public gateways: %w", err) - } - publicLbs := getLBStatus(publicGateways) + publicGatewayURL := gatewayServiceURL(defaultGateways[v1alpha1.IngressVisibilityExternalIP]) + publicLbs := getLBStatus(publicGatewayURL) - privateGateways, err := resources.PrivateGatewayServiceURLFromContext(ctx, ing) - if err != nil { - return fmt.Errorf("failed to get private gateways: %w", err) - } - privateLbs := getLBStatus(privateGateways) + privateGatewayURL := gatewayServiceURL(defaultGateways[v1alpha1.IngressVisibilityClusterLocal]) + privateLbs := getLBStatus(privateGatewayURL) ing.Status.MarkLoadBalancerReady(publicLbs, privateLbs) } else { @@ -532,6 +535,14 @@ func (r *Reconciler) GetDestinationRuleLister() istiolisters.DestinationRuleList return r.destinationRuleLister } +func gatewayServiceURL(gateways []config.Gateway) string { + if len(gateways) == 0 { + return "" + } + + return gateways[0].ServiceURL +} + // getLBStatus gets the LB Status. func getLBStatus(gatewayServiceURL string) []v1alpha1.LoadBalancerIngressStatus { // The Ingress isn't load-balanced by any particular diff --git a/pkg/reconciler/ingress/resources/gateway.go b/pkg/reconciler/ingress/resources/gateway.go index e8db7182c1..72e0efb34e 100644 --- a/pkg/reconciler/ingress/resources/gateway.go +++ b/pkg/reconciler/ingress/resources/gateway.go @@ -266,8 +266,8 @@ func makeIngressGateway(ing *v1alpha1.Ingress, visibility v1alpha1.IngressVisibi } } -func getGatewayServices(ctx context.Context, ing *v1alpha1.Ingress, svcLister corev1listers.ServiceLister) ([]*corev1.Service, error) { - ingressSvcMetas, err := GetIngressGatewaySvcNameNamespaces(ctx, ing) +func getGatewayServices(ctx context.Context, obj kmeta.Accessor, svcLister corev1listers.ServiceLister) ([]*corev1.Service, error) { + ingressSvcMetas, err := GetIngressGatewaySvcNameNamespaces(ctx, obj) if err != nil { return nil, err } @@ -397,10 +397,10 @@ func GetNonWildcardIngressTLS(ingressTLS []v1alpha1.IngressTLS, nonWildcardSecre } // GetIngressGatewaySvcNameNamespaces gets the Istio ingress namespaces from ConfigMap for gateways that should expose the service. -func GetIngressGatewaySvcNameNamespaces(ctx context.Context, ing *v1alpha1.Ingress) ([]metav1.ObjectMeta, error) { +func GetIngressGatewaySvcNameNamespaces(ctx context.Context, obj kmeta.Accessor) ([]metav1.ObjectMeta, error) { nameNamespaces := make([]metav1.ObjectMeta, 0) - serviceGateways, err := gatewaysFromContext(ctx, ing) + serviceGateways, err := GatewaysFromContext(ctx, obj) if err != nil { return nil, fmt.Errorf("failed to get gateway from configuration: %w", err) } @@ -471,10 +471,10 @@ func isPlaceHolderServer(server *istiov1beta1.Server) bool { } // QualifiedGatewayNamesFromContext get gateway names from context. -func QualifiedGatewayNamesFromContext(ctx context.Context, ing *v1alpha1.Ingress) (map[v1alpha1.IngressVisibility]sets.Set[string], error) { +func QualifiedGatewayNamesFromContext(ctx context.Context, obj kmeta.Accessor) (map[v1alpha1.IngressVisibility]sets.Set[string], error) { ret := make(map[v1alpha1.IngressVisibility]sets.Set[string]) - gateways, err := gatewaysFromContext(ctx, ing) + gateways, err := GatewaysFromContext(ctx, obj) if err != nil { return ret, fmt.Errorf("failed to get gateways from configuration: %w", err) } @@ -490,14 +490,14 @@ func QualifiedGatewayNamesFromContext(ctx context.Context, ing *v1alpha1.Ingress return ret, nil } -// gatewaysFromContext get gateways relevant to this ingress from context. -func gatewaysFromContext(ctx context.Context, ing *v1alpha1.Ingress) (map[v1alpha1.IngressVisibility][]config.Gateway, error) { +// GatewaysFromContext get gateways relevant to this ingress from context. +func GatewaysFromContext(ctx context.Context, obj kmeta.Accessor) (map[v1alpha1.IngressVisibility][]config.Gateway, error) { ret := make(map[v1alpha1.IngressVisibility][]config.Gateway) istioConfig := config.FromContext(ctx).Istio // External gateways selection - externalGateways, err := filterGateway(istioConfig.IngressGateways, ing.ObjectMeta.Labels) + externalGateways, err := filterGateway(istioConfig.IngressGateways, obj.GetLabels()) if err != nil { return ret, fmt.Errorf("failed to filter external gateways: %w", err) } @@ -509,7 +509,7 @@ func gatewaysFromContext(ctx context.Context, ing *v1alpha1.Ingress) (map[v1alph ret[v1alpha1.IngressVisibilityExternalIP] = externalGateways // Local gateways selection - localGateways, err := filterGateway(istioConfig.LocalGateways, ing.ObjectMeta.Labels) + localGateways, err := filterGateway(istioConfig.LocalGateways, obj.GetLabels()) if err != nil { return ret, fmt.Errorf("failed to filter local gateways: %w", err) } @@ -545,25 +545,3 @@ func filterGateway(gtws []config.Gateway, ingressLabels map[string]string) ([]co return ret, nil } - -func PublicGatewayServiceURLFromContext(ctx context.Context, ing *v1alpha1.Ingress) (string, error) { - return getGatewayServiceURLFromContext(ctx, ing, v1alpha1.IngressVisibilityExternalIP) -} - -func PrivateGatewayServiceURLFromContext(ctx context.Context, ing *v1alpha1.Ingress) (string, error) { - return getGatewayServiceURLFromContext(ctx, ing, v1alpha1.IngressVisibilityClusterLocal) -} - -func getGatewayServiceURLFromContext(ctx context.Context, ing *v1alpha1.Ingress, visibility v1alpha1.IngressVisibility) (string, error) { - allGateways, err := gatewaysFromContext(ctx, ing) - if err != nil { - return "", fmt.Errorf("failed to get gateways from configuration: %w", err) - } - - gateways, ok := allGateways[visibility] - if ok && len(gateways) > 0 { - return gateways[0].ServiceURL, nil - } - - return "", nil -} diff --git a/pkg/reconciler/ingress/resources/gateway_test.go b/pkg/reconciler/ingress/resources/gateway_test.go index 888ebe8b91..b3b63a519d 100644 --- a/pkg/reconciler/ingress/resources/gateway_test.go +++ b/pkg/reconciler/ingress/resources/gateway_test.go @@ -187,6 +187,18 @@ var defaultGatewayService = corev1.Service{ }, } +var configDefaultGateway = &config.Config{ + Istio: &config.Istio{ + IngressGateways: []config.Gateway{{ + Name: config.KnativeIngressGateway, + ServiceURL: fmt.Sprintf("%s.%s.svc.cluster.local", defaultGatewayService.Name, defaultGatewayService.Namespace), + }}, + }, + Network: &netconfig.Config{ + HTTPProtocol: netconfig.HTTPEnabled, + }, +} + var defaultGatewayCmpOpts = protocmp.Transform() func TestGetServers(t *testing.T) { @@ -755,19 +767,7 @@ func TestMakeExternalIngressGateways(t *testing.T) { } } - configDefaultGateway := &config.Config{ - Istio: &config.Istio{ - IngressGateways: []config.Gateway{{ - Name: config.KnativeIngressGateway, - ServiceURL: fmt.Sprintf("%s.%s.svc.cluster.local", defaultGatewayService.Name, defaultGatewayService.Namespace), - }}, - }, - Network: &netconfig.Config{ - HTTPProtocol: netconfig.HTTPEnabled, - }, - } - - var gateway1Service = corev1.Service{ + gateway1Service := corev1.Service{ ObjectMeta: metav1.ObjectMeta{ Name: "gateway1", Namespace: "aNamespace", @@ -779,7 +779,7 @@ func TestMakeExternalIngressGateways(t *testing.T) { }, } - var gateway2Service = corev1.Service{ + gateway2Service := corev1.Service{ ObjectMeta: metav1.ObjectMeta{ Name: "gateway2", Namespace: "aNamespace", From ec0d6d8468e2e43ca42ffed489a66d9b8419bb66 Mon Sep 17 00:00:00 2001 From: "Bernardin, Matthieu" Date: Mon, 11 Mar 2024 11:29:54 +0100 Subject: [PATCH 3/5] feat: Ensure exactly one default gateway is set --- pkg/reconciler/ingress/config/istio.go | 29 +++-- pkg/reconciler/ingress/config/istio_test.go | 94 ++++++++++++-- test/e2e/exposition_test.go | 135 +++++++++----------- 3 files changed, 166 insertions(+), 92 deletions(-) diff --git a/pkg/reconciler/ingress/config/istio.go b/pkg/reconciler/ingress/config/istio.go index e3c43084ca..cac18703ba 100644 --- a/pkg/reconciler/ingress/config/istio.go +++ b/pkg/reconciler/ingress/config/istio.go @@ -105,6 +105,11 @@ func (g Gateway) Validate() error { return fmt.Errorf("invalid gateway service format: %v", errs) } + _, err := metav1.LabelSelectorAsSelector(g.LabelSelector) + if err != nil { + return fmt.Errorf("failed to create selector from label selector: %w", err) + } + return nil } @@ -121,13 +126,13 @@ type Istio struct { func (i Istio) Validate() error { for _, gtw := range i.IngressGateways { if err := gtw.Validate(); err != nil { - return fmt.Errorf("invalid gateway: %w", err) + return fmt.Errorf("invalid gateway %s: %w", gtw.QualifiedName(), err) } } for _, gtw := range i.LocalGateways { if err := gtw.Validate(); err != nil { - return fmt.Errorf("invalid local gateway: %w", err) + return fmt.Errorf("invalid local gateway %s: %w", gtw.QualifiedName(), err) } } @@ -177,10 +182,10 @@ func NewIstioFromConfigMap(configMap *corev1.ConfigMap) (*Istio, error) { } case oldFormatDefined: ret = parseOldFormat(configMap) + default: + defaultValues(ret) } - defaultValues(ret) - err = ret.Validate() if err != nil { return nil, fmt.Errorf("invalid configuration: %w", err) @@ -231,12 +236,14 @@ func parseNewFormat(configMap *corev1.ConfigMap) (*Istio, error) { ret.LocalGateways = localGateways } - if len(ret.DefaultExternalGateways()) > 1 { - return ret, fmt.Errorf("only one external gateway with no selector can be defined, here: %v", ret.DefaultExternalGateways()) + defaultValues(ret) + + if len(ret.DefaultExternalGateways()) != 1 { + return ret, fmt.Errorf("exactly one external gateway with no selector can be defined, here: %v", ret.DefaultExternalGateways()) } - if len(ret.DefaultLocalGateways()) > 1 { - return ret, fmt.Errorf("only one local gateway with no selector can be defined, here: %v", ret.DefaultLocalGateways()) + if len(ret.DefaultLocalGateways()) != 1 { + return ret, fmt.Errorf("exactly one local gateway with no selector can be defined, here: %v", ret.DefaultLocalGateways()) } return ret, nil @@ -254,10 +261,14 @@ func parseNewFormatGateways(data string) ([]Gateway, error) { } func parseOldFormat(configMap *corev1.ConfigMap) *Istio { - return &Istio{ + ret := &Istio{ IngressGateways: parseOldFormatGateways(configMap, gatewayKeyPrefix), LocalGateways: parseOldFormatGateways(configMap, localGatewayKeyPrefix), } + + defaultValues(ret) + + return ret } func parseOldFormatGateways(configMap *corev1.ConfigMap, prefix string) []Gateway { diff --git a/pkg/reconciler/ingress/config/istio_test.go b/pkg/reconciler/ingress/config/istio_test.go index 8e3865ce22..396c7f790e 100644 --- a/pkg/reconciler/ingress/config/istio_test.go +++ b/pkg/reconciler/ingress/config/istio_test.go @@ -417,6 +417,46 @@ func TestGatewayConfiguration(t *testing.T) { }}, LocalGateways: defaultLocalGateways(), }, + }, { + name: "new format - missing default gateway", + config: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: system.Namespace(), + Name: IstioConfigName, + }, + Data: map[string]string{ + "external-gateways": replaceTabs(` + - namespace: "namespace" + name: "gateway" + service: "istio-gateway.istio-system.svc.cluster.local" + labelSelector: + matchExpressions: + - key: "key" + operator: "In" + values: ["value"]`), + }, + }, + wantErr: true, + }, { + name: "new format - missing default local gateway", + config: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: system.Namespace(), + Name: IstioConfigName, + }, + Data: map[string]string{ + "local-gateways": replaceTabs(` + - namespace: "namespace" + name: "gateway" + service: "istio-gateway.istio-system.svc.cluster.local" + labelSelector: + matchExpressions: + - key: "key" + operator: "In" + values: ["value"]`), + }, + }, + wantErr: true, }, { name: "new format - label selector", config: &corev1.ConfigMap{ @@ -426,6 +466,9 @@ func TestGatewayConfiguration(t *testing.T) { }, Data: map[string]string{ "external-gateways": replaceTabs(` + - namespace: "unused" + name: "unused" + service: "default.default.svc.cluster.local" - namespace: "namespace" name: "gateway" service: "istio-gateway.istio-system.svc.cluster.local" @@ -442,20 +485,27 @@ func TestGatewayConfiguration(t *testing.T) { }, }, wantIstio: &Istio{ - IngressGateways: []Gateway{{ - Namespace: "namespace", - Name: "gateway", - ServiceURL: "istio-gateway.istio-system.svc.cluster.local", - LabelSelector: &metav1.LabelSelector{ - MatchExpressions: []metav1.LabelSelectorRequirement{ - { - Key: "key", - Operator: metav1.LabelSelectorOpIn, - Values: []string{"value"}, + IngressGateways: []Gateway{ + { + Namespace: "unused", + Name: "unused", + ServiceURL: "default.default.svc.cluster.local", + }, + { + Namespace: "namespace", + Name: "gateway", + ServiceURL: "istio-gateway.istio-system.svc.cluster.local", + LabelSelector: &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: "key", + Operator: metav1.LabelSelectorOpIn, + Values: []string{"value"}, + }, }, }, }, - }}, + }, LocalGateways: []Gateway{{ Namespace: "namespace2", Name: "gateway2", @@ -517,6 +567,28 @@ func TestGatewayConfiguration(t *testing.T) { }, }, }, + }, { + name: "new format - invalid label selector", + config: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: system.Namespace(), + Name: IstioConfigName, + }, + Data: map[string]string{ + "external-gateways": replaceTabs(` + - namespace: "default" + name: "default" + service: "default.default.svc.cluster.local" + - namespace: "namespace" + name: "gateway" + service: "istio-gateway.istio-system.svc.cluster.local" + labelSelector: + matchExpressions: + - key: "key" + operator: "In"`), + }, + }, + wantErr: true, }} for _, tt := range gatewayConfigTests { diff --git a/test/e2e/exposition_test.go b/test/e2e/exposition_test.go index 7a0f1c3447..1aef866cfd 100644 --- a/test/e2e/exposition_test.go +++ b/test/e2e/exposition_test.go @@ -21,6 +21,8 @@ package e2e import ( "context" + "os" + "strings" "testing" "github.com/google/go-cmp/cmp" @@ -35,6 +37,11 @@ import ( ) func TestExposition(t *testing.T) { + mesh, meshSet := os.LookupEnv("MESH") + if meshSet && mesh == "1" { + return + } + clients := Setup(t) namespace := system.Namespace() @@ -67,15 +74,15 @@ func TestExposition(t *testing.T) { { name: "no label", configMapData: map[string]string{ - "local-gateways": ` - - namespace: "knative-serving" - name: "knative-local-gateway" - service: "istio-ingressgateway.istio-system.svc.cluster.local"`, - - "external-gateways": ` - - namespace: "knative-serving" - name: "knative-ingress-gateway" - service: "istio-ingressgateway.istio-system.svc.cluster.local"`, + "local-gateways": replaceTabs(` + - namespace: "knative-serving" + name: "knative-local-gateway" + service: "istio-ingressgateway.istio-system.svc.cluster.local"`), + + "external-gateways": replaceTabs(` + - namespace: "knative-serving" + name: "knative-ingress-gateway" + service: "istio-ingressgateway.istio-system.svc.cluster.local"`), }, expectedPrivateIngresses: []v1alpha1.LoadBalancerIngressStatus{ {MeshOnly: false, DomainInternal: "istio-ingressgateway.istio-system.svc.cluster.local"}, @@ -87,20 +94,23 @@ func TestExposition(t *testing.T) { { name: "match all", configMapData: map[string]string{ - "local-gateways": ` - - namespace: "knative-serving" - name: "knative-local-gateway" - service: "istio-ingressgateway.istio-system.svc.cluster.local"`, - - "external-gateways": ` - - namespace: "knative-serving" - name: "knative-ingress-gateway" - service: "istio-ingressgateway.istio-system.svc.cluster.local" - labelSelector: - matchExpressions: - - key: "foo" - operator: "In" - values: ["bar"]`, + "local-gateways": replaceTabs(` + - namespace: "knative-serving" + name: "knative-local-gateway" + service: "istio-ingressgateway.istio-system.svc.cluster.local"`), + + "external-gateways": replaceTabs(` + - namespace: "unused" + name: "unused" + service: "default.default.svc.cluster.local" + - namespace: "knative-serving" + name: "knative-ingress-gateway" + service: "istio-ingressgateway.istio-system.svc.cluster.local" + labelSelector: + matchExpressions: + - key: "foo" + operator: "In" + values: ["bar"]`), }, ingressLabels: map[string]string{"foo": "bar"}, expectedPrivateIngresses: []v1alpha1.LoadBalancerIngressStatus{ @@ -113,61 +123,38 @@ func TestExposition(t *testing.T) { { name: "match only one", configMapData: map[string]string{ - "local-gateways": ` - - namespace: "knative-serving" - name: "knative-local-gateway" - service: "istio-ingressgateway.istio-system.svc.cluster.local"`, - - "external-gateways": ` - - namespace: "knative-serving" - name: "knative-ingress-gateway" - service: "istio-ingressgateway.istio-system.svc.cluster.local" - labelSelector: - matchExpressions: - - key: "foo" - operator: "In" - values: ["bar"] - - namespace: "knative-serving" - name: "knative-local-gateway" - service: "istio-ingressgateway.istio-system.svc.cluster.local" - labelSelector: - matchExpressions: - - key: "wont" - operator: "In" - values: ["match"]`, + "local-gateways": replaceTabs(` + - namespace: "knative-serving" + name: "knative-local-gateway" + service: "istio-ingressgateway.istio-system.svc.cluster.local"`), + + "external-gateways": replaceTabs(` + - namespace: "unused" + name: "unused" + service: "default.default.svc.cluster.local" + - namespace: "knative-serving" + name: "knative-ingress-gateway" + service: "istio-ingressgateway-1.istio-system.svc.cluster.local" + labelSelector: + matchExpressions: + - key: "foo" + operator: "In" + values: ["bar"] + - namespace: "knative-serving" + name: "knative-local-gateway" + service: "istio-ingressgateway-2.istio-system.svc.cluster.local" + labelSelector: + matchExpressions: + - key: "wont" + operator: "In" + values: ["match"]`), }, ingressLabels: map[string]string{"foo": "bar"}, expectedPrivateIngresses: []v1alpha1.LoadBalancerIngressStatus{ {MeshOnly: false, DomainInternal: "istio-ingressgateway.istio-system.svc.cluster.local"}, }, expectedPublicIngresses: []v1alpha1.LoadBalancerIngressStatus{ - {MeshOnly: false, DomainInternal: "istio-ingressgateway.istio-system.svc.cluster.local"}, - }, - }, - { - name: "match nothing", - configMapData: map[string]string{ - "local-gateways": ` - - namespace: "knative-serving" - name: "knative-local-gateway" - service: "istio-ingressgateway.istio-system.svc.cluster.local"`, - - "external-gateways": ` - - namespace: "knative-serving" - name: "knative-ingress-gateway" - service: "istio-ingressgateway.istio-system.svc.cluster.local" - labelSelector: - matchExpressions: - - key: "wont" - operator: "In" - values: ["match"]`, - }, - ingressLabels: map[string]string{"not": "relevant"}, - expectedPrivateIngresses: []v1alpha1.LoadBalancerIngressStatus{ - {MeshOnly: false, DomainInternal: "istio-ingressgateway.istio-system.svc.cluster.local"}, - }, - expectedPublicIngresses: []v1alpha1.LoadBalancerIngressStatus{ - {MeshOnly: true}, + {MeshOnly: false, DomainInternal: "istio-ingressgateway-1.istio-system.svc.cluster.local"}, }, }, } @@ -258,3 +245,7 @@ func TestExposition(t *testing.T) { }) } } + +func replaceTabs(s string) string { + return strings.ReplaceAll(s, "\t", " ") +} From a857cb69f11915a97fa153bef713d5f3e4809f61 Mon Sep 17 00:00:00 2001 From: "Bernardin, Matthieu" Date: Tue, 12 Mar 2024 16:56:27 +0100 Subject: [PATCH 4/5] chore: MR comments resolution --- pkg/reconciler/ingress/config/istio.go | 5 +++-- pkg/reconciler/ingress/resources/gateway.go | 1 + test/e2e/exposition_test.go | 2 +- 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/pkg/reconciler/ingress/config/istio.go b/pkg/reconciler/ingress/config/istio.go index cac18703ba..553699b4db 100644 --- a/pkg/reconciler/ingress/config/istio.go +++ b/pkg/reconciler/ingress/config/istio.go @@ -105,8 +105,7 @@ func (g Gateway) Validate() error { return fmt.Errorf("invalid gateway service format: %v", errs) } - _, err := metav1.LabelSelectorAsSelector(g.LabelSelector) - if err != nil { + if _, err := metav1.LabelSelectorAsSelector(g.LabelSelector); err != nil { return fmt.Errorf("failed to create selector from label selector: %w", err) } @@ -139,10 +138,12 @@ func (i Istio) Validate() error { return nil } +// DefaultExternalGateways returns the external gateway without any label selector func (i Istio) DefaultExternalGateways() []Gateway { return defaultGateways(i.IngressGateways) } +// DefaultLocalGateways returns the local gateway without any label selector func (i Istio) DefaultLocalGateways() []Gateway { return defaultGateways(i.LocalGateways) } diff --git a/pkg/reconciler/ingress/resources/gateway.go b/pkg/reconciler/ingress/resources/gateway.go index 72e0efb34e..d16ae7d1d2 100644 --- a/pkg/reconciler/ingress/resources/gateway.go +++ b/pkg/reconciler/ingress/resources/gateway.go @@ -146,6 +146,7 @@ func MakeExternalIngressGateways(ctx context.Context, ing *v1alpha1.Ingress, ser } // MakeWildcardTLSGateways creates gateways that only contain TLS server with wildcard hosts based on the wildcard secret information. +// The configuration gateways that applies are determined with the ingress parameter. // For each public ingress service, we will create a list of Gateways. Each Gateway of the list corresponds to a wildcard cert secret. func MakeWildcardTLSGateways(ctx context.Context, ing *v1alpha1.Ingress, originWildcardSecrets map[string]*corev1.Secret, svcLister corev1listers.ServiceLister) ([]*v1beta1.Gateway, error) { diff --git a/test/e2e/exposition_test.go b/test/e2e/exposition_test.go index 1aef866cfd..e3e888536f 100644 --- a/test/e2e/exposition_test.go +++ b/test/e2e/exposition_test.go @@ -2,7 +2,7 @@ // +build e2e /* -Copyright 2020 The Knative Authors +Copyright 2024 The Knative Authors Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. From 1f02be88b93ab195a5edbf9ffa58377ea811d08c Mon Sep 17 00:00:00 2001 From: "Bernardin, Matthieu" Date: Thu, 14 Mar 2024 09:32:13 +0100 Subject: [PATCH 5/5] chore: Update comments --- pkg/reconciler/ingress/resources/gateway.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/reconciler/ingress/resources/gateway.go b/pkg/reconciler/ingress/resources/gateway.go index d16ae7d1d2..cd7b320fe1 100644 --- a/pkg/reconciler/ingress/resources/gateway.go +++ b/pkg/reconciler/ingress/resources/gateway.go @@ -146,7 +146,7 @@ func MakeExternalIngressGateways(ctx context.Context, ing *v1alpha1.Ingress, ser } // MakeWildcardTLSGateways creates gateways that only contain TLS server with wildcard hosts based on the wildcard secret information. -// The configuration gateways that applies are determined with the ingress parameter. +// Gateways generated are based on the related ingress being reconciled. // For each public ingress service, we will create a list of Gateways. Each Gateway of the list corresponds to a wildcard cert secret. func MakeWildcardTLSGateways(ctx context.Context, ing *v1alpha1.Ingress, originWildcardSecrets map[string]*corev1.Secret, svcLister corev1listers.ServiceLister) ([]*v1beta1.Gateway, error) {