From 1db1a5c7719648beec0e2580ed6c8989f3a50019 Mon Sep 17 00:00:00 2001 From: John Howard Date: Mon, 13 May 2024 11:17:47 -0700 Subject: [PATCH] Fix usage of infrastructure labels/annotations on gateway Fixes https://github.com/istio/istio/issues/50983, I think Basically, we have logic to use `infra.Labels || meta.Labels`. We modify meta.Labels. This is wrong, we should modify AFTER we pick which set of labels to use. Also, we need to deepcopy anything we mutate since its accessing a shared object cache --- .../kube/gateway/deploymentcontroller.go | 115 +++++++++--------- .../kube/gateway/deploymentcontroller_test.go | 6 +- .../infrastructure-labels-annotations.yaml | 4 + 3 files changed, 68 insertions(+), 57 deletions(-) diff --git a/pilot/pkg/config/kube/gateway/deploymentcontroller.go b/pilot/pkg/config/kube/gateway/deploymentcontroller.go index d971fde2a106..4ae740122b92 100644 --- a/pilot/pkg/config/kube/gateway/deploymentcontroller.go +++ b/pilot/pkg/config/kube/gateway/deploymentcontroller.go @@ -18,6 +18,7 @@ import ( "context" "encoding/json" "fmt" + "istio.io/istio/pkg/maps" "strconv" "strings" @@ -28,6 +29,7 @@ import ( klabels "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" + gatewayv1 "sigs.k8s.io/gateway-api/apis/v1" gateway "sigs.k8s.io/gateway-api/apis/v1beta1" "sigs.k8s.io/yaml" @@ -346,36 +348,7 @@ func (d *DeploymentController) configureIstioGateway(log *istiolog.Scope, gw gat serviceType = corev1.ServiceType(o) } - // TODO: Codify this API (i.e how to know if a specific gateway is an Istio waypoint gateway) - isWaypointGateway := strings.Contains(string(gw.Spec.GatewayClassName), "waypoint") - // Default the network label for waypoints if not explicitly set in gateway's labels - network := d.injectConfig().Values.Struct().GetGlobal().GetNetwork() - if _, ok := gw.GetLabels()[label.TopologyNetwork.Name]; !ok && network != "" && isWaypointGateway { - if gw.Labels == nil { - gw.Labels = make(map[string]string) - } - gw.Labels[label.TopologyNetwork.Name] = d.injectConfig().Values.Struct().GetGlobal().GetNetwork() - } - - // TODO this sprays ambient annotations/labels all over EVER gateway resource (serviceaccts, services, etc) - // where they have no meaning/are not used/are ignored. We really only need them on the deployment podspec. - var hasAmbientLabel bool - if _, ok := gw.Labels[constants.DataplaneModeLabel]; ok { - hasAmbientLabel = true - } - if gw.Spec.Infrastructure != nil { - if _, ok := gw.Spec.Infrastructure.Labels[constants.DataplaneModeLabel]; ok { - hasAmbientLabel = true - } - } - // If no ambient redirection label is set explicitly, explicitly disable. - if features.EnableAmbientWaypoints && !isWaypointGateway && !hasAmbientLabel { - if gw.Labels == nil { - gw.Labels = make(map[string]string) - } - gw.Labels[constants.DataplaneModeLabel] = constants.DataplaneModeNone - } input := TemplateInput{ Gateway: &gw, @@ -395,37 +368,15 @@ func (d *DeploymentController) configureIstioGateway(log *istiolog.Scope, gw gat } d.setGatewayNameLabel(&input) + // Default to the gateway labels/annotations and overwrite if infrastructure labels/annotations are set gwInfra := gw.Spec.Infrastructure - if gwInfra != nil && gwInfra.Labels != nil { - infraLabels := make(map[string]string, len(gwInfra.Labels)) - for k, v := range gw.Spec.Infrastructure.Labels { - if strings.HasPrefix(string(k), "gateway.networking.k8s.io/") { - continue // ignore this prefix to avoid conflicts - } - infraLabels[string(k)] = string(v) - } + input.InfrastructureLabels = extractInfrastructureMetadata(gwInfra, true, gw) + input.InfrastructureAnnotations = extractInfrastructureMetadata(gwInfra, false, gw) - // Default the network label for waypoints if not explicitly set in infra labels - // We do this a second time here for correctness since if infra labels are set (according to the gwapi spec), - // the gateway's labels are ignored. - if _, ok := infraLabels[label.TopologyNetwork.Name]; !ok && network != "" && isWaypointGateway { - infraLabels[label.TopologyNetwork.Name] = network - } - - input.InfrastructureLabels = infraLabels - } + // Set label overrides - if gwInfra != nil && gwInfra.Annotations != nil { - infraAnnotations := make(map[string]string, len(gwInfra.Annotations)) - for k, v := range gw.Spec.Infrastructure.Annotations { - if strings.HasPrefix(string(k), "gateway.networking.k8s.io/") { - continue // ignore this prefix to avoid conflicts - } - infraAnnotations[string(k)] = string(v) - } - input.InfrastructureAnnotations = infraAnnotations - } + d.setLabelOverrides(gw, input) if overwriteControllerVersion { log.Debugf("write controller version, existing=%v", existingControllerVersion) @@ -450,6 +401,58 @@ func (d *DeploymentController) configureIstioGateway(log *istiolog.Scope, gw gat return nil } +func (d *DeploymentController) setLabelOverrides(gw gateway.Gateway, input TemplateInput) { + // TODO: Codify this API (i.e how to know if a specific gateway is an Istio waypoint gateway) + isWaypointGateway := strings.Contains(string(gw.Spec.GatewayClassName), "waypoint") + + var hasAmbientLabel bool + if _, ok := gw.Labels[constants.DataplaneModeLabel]; ok { + hasAmbientLabel = true + } + if _, ok := input.InfrastructureLabels[constants.DataplaneModeLabel]; ok { + hasAmbientLabel = true + } + // If no ambient redirection label is set explicitly, explicitly disable. + // TODO this sprays ambient annotations/labels all over EVER gateway resource (serviceaccts, services, etc) + if features.EnableAmbientWaypoints && !isWaypointGateway && !hasAmbientLabel { + input.InfrastructureLabels[constants.DataplaneModeLabel] = constants.DataplaneModeNone + } + + // Default the network label for waypoints if not explicitly set in gateway's labels + network := d.injectConfig().Values.Struct().GetGlobal().GetNetwork() + if _, ok := gw.GetLabels()[label.TopologyNetwork.Name]; !ok && network != "" && isWaypointGateway { + input.InfrastructureLabels[label.TopologyNetwork.Name] = d.injectConfig().Values.Struct().GetGlobal().GetNetwork() + } +} + +func extractInfrastructureMetadata(gwInfra *gatewayv1.GatewayInfrastructure, isLabel bool, gw gateway.Gateway) map[string]string { + var field map[gatewayv1.AnnotationKey]gatewayv1.AnnotationValue + if gwInfra != nil && isLabel && gwInfra.Labels != nil { + field = gwInfra.Labels + } else if gwInfra != nil && !isLabel && gwInfra.Annotations != nil { + field = gwInfra.Annotations + } + if field != nil { + infra := make(map[string]string, len(field)) + for k, v := range field { + if strings.HasPrefix(string(k), "gateway.networking.k8s.io/") { + continue // ignore this prefix to avoid conflicts + } + infra[string(k)] = string(v) + } + return infra + } else if isLabel { + if gw.GetLabels() == nil { + return make(map[string]string) + } + return maps.Clone(gw.GetLabels()) + } + if gw.GetAnnotations() == nil { + return make(map[string]string) + } + return maps.Clone(gw.GetAnnotations()) +} + const ( // ControllerVersionAnnotation is an annotation added to the Gateway by the controller specifying // the "controller version". The original intent of this was to work around diff --git a/pilot/pkg/config/kube/gateway/deploymentcontroller_test.go b/pilot/pkg/config/kube/gateway/deploymentcontroller_test.go index 66aa018b9d5f..61f85303f018 100644 --- a/pilot/pkg/config/kube/gateway/deploymentcontroller_test.go +++ b/pilot/pkg/config/kube/gateway/deploymentcontroller_test.go @@ -346,7 +346,7 @@ func TestConfigureIstioGateway(t *testing.T) { kube.SetObjectFilter(client, tt.discoveryNamespaceFilter) client.Kube().Discovery().(*fakediscovery.FakeDiscovery).FakedServerVersion = &kubeVersion.Info{Major: "1", Minor: "28"} kclient.NewWriteClient[*k8sbeta.GatewayClass](client).Create(customClass) - kclient.NewWriteClient[*k8sbeta.Gateway](client).Create(&tt.gw) + kclient.NewWriteClient[*k8sbeta.Gateway](client).Create(tt.gw.DeepCopy()) stop := test.NewStop(t) env := model.NewEnvironment() env.PushContext().ProxyConfigs = tt.pcs @@ -374,6 +374,10 @@ func TestConfigureIstioGateway(t *testing.T) { resp := timestampRegex.ReplaceAll(buf.Bytes(), []byte("lastTransitionTime: fake")) util.CompareContent(t, resp, filepath.Join("testdata", "deployment", tt.name+".yaml")) } + // ensure we didn't mutate the object + if !tt.ignore { + assert.Equal(t, d.gateways.Get(tt.gw.Name, tt.gw.Namespace), &tt.gw) + } }) } } diff --git a/pilot/pkg/config/kube/gateway/testdata/deployment/infrastructure-labels-annotations.yaml b/pilot/pkg/config/kube/gateway/testdata/deployment/infrastructure-labels-annotations.yaml index d94dfa007e75..6659c44a15f4 100644 --- a/pilot/pkg/config/kube/gateway/testdata/deployment/infrastructure-labels-annotations.yaml +++ b/pilot/pkg/config/kube/gateway/testdata/deployment/infrastructure-labels-annotations.yaml @@ -13,6 +13,7 @@ metadata: foo: bar gateway.istio.io/managed: istio.io-gateway-controller gateway.networking.k8s.io/gateway-name: default + istio.io/dataplane-mode: none istio.io/gateway-name: default name: default-istio namespace: default @@ -31,6 +32,7 @@ metadata: foo: bar gateway.istio.io/managed: istio.io-gateway-controller gateway.networking.k8s.io/gateway-name: default + istio.io/dataplane-mode: none istio.io/gateway-name: default name: default-istio namespace: default @@ -54,6 +56,7 @@ spec: labels: foo: bar gateway.networking.k8s.io/gateway-name: default + istio.io/dataplane-mode: none istio.io/gateway-name: default service.istio.io/canonical-name: default-istio service.istio.io/canonical-revision: latest @@ -228,6 +231,7 @@ metadata: foo: bar gateway.istio.io/managed: istio.io-gateway-controller gateway.networking.k8s.io/gateway-name: default + istio.io/dataplane-mode: none istio.io/gateway-name: default name: default-istio namespace: default