Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: Migrate to sets.Set[string] when possible #1142

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 10 additions & 10 deletions pkg/reconciler/ingress/ingress.go
Original file line number Diff line number Diff line change
Expand Up @@ -111,9 +111,9 @@ func (r *Reconciler) reconcileIngress(ctx context.Context, ing *v1alpha1.Ingress
ing.Status.InitializeConditions()
logger.Infof("Reconciling ingress: %#v", ing)

gatewayNames := map[v1alpha1.IngressVisibility]sets.String{}
gatewayNames := map[v1alpha1.IngressVisibility]sets.Set[string]{}
gatewayNames[v1alpha1.IngressVisibilityClusterLocal] = qualifiedGatewayNamesFromContext(ctx)[v1alpha1.IngressVisibilityClusterLocal]
gatewayNames[v1alpha1.IngressVisibilityExternalIP] = sets.String{}
gatewayNames[v1alpha1.IngressVisibilityExternalIP] = sets.New[string]()

ingressGateways := []*v1beta1.Gateway{}
if shouldReconcileTLS(ing) {
Expand Down Expand Up @@ -178,7 +178,7 @@ func (r *Reconciler) reconcileIngress(ctx context.Context, ing *v1alpha1.Ingress
// 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(defaultGlobalHTTPGateways.List()...)
gatewayNames[v1alpha1.IngressVisibilityExternalIP].Insert(sets.List(defaultGlobalHTTPGateways)...)
}

if err := r.reconcileIngressGateways(ctx, ingressGateways); err != nil {
Expand Down Expand Up @@ -235,13 +235,13 @@ func (r *Reconciler) reconcileIngress(ctx context.Context, ing *v1alpha1.Ingress
}

func getPublicHosts(ing *v1alpha1.Ingress) []string {
hosts := sets.String{}
hosts := sets.New[string]()
for _, rule := range ing.Spec.Rules {
if rule.Visibility == v1alpha1.IngressVisibilityExternalIP {
hosts.Insert(rule.Hosts...)
}
}
return hosts.List()
return sets.List(hosts)
}

func (r *Reconciler) reconcileCertSecrets(ctx context.Context, ing *v1alpha1.Ingress, desiredSecrets []*corev1.Secret) error {
Expand Down Expand Up @@ -297,7 +297,7 @@ func (r *Reconciler) reconcileSystemGeneratedGateway(ctx context.Context, desire
func (r *Reconciler) reconcileVirtualServices(ctx context.Context, ing *v1alpha1.Ingress,
desired []*v1beta1.VirtualService) error {
// First, create all needed VirtualServices.
kept := sets.NewString()
kept := sets.New[string]()
for _, d := range desired {
if d.GetAnnotations()[networking.IngressClassAnnotationKey] != netconfig.IstioIngressClassName {
// We do not create resources that do not have istio ingress class annotation.
Expand Down Expand Up @@ -438,19 +438,19 @@ func (r *Reconciler) GetVirtualServiceLister() istiolisters.VirtualServiceLister
}

// qualifiedGatewayNamesFromContext get gateway names from context
func qualifiedGatewayNamesFromContext(ctx context.Context) map[v1alpha1.IngressVisibility]sets.String {
func qualifiedGatewayNamesFromContext(ctx context.Context) map[v1alpha1.IngressVisibility]sets.Set[string] {
ci := config.FromContext(ctx).Istio
publicGateways := make(sets.String, len(ci.IngressGateways))
publicGateways := sets.New[string]()
for _, gw := range ci.IngressGateways {
publicGateways.Insert(gw.QualifiedName())
}

privateGateways := make(sets.String, len(ci.LocalGateways))
privateGateways := sets.New[string]()
for _, gw := range ci.LocalGateways {
privateGateways.Insert(gw.QualifiedName())
}

return map[v1alpha1.IngressVisibility]sets.String{
return map[v1alpha1.IngressVisibility]sets.Set[string]{
v1alpha1.IngressVisibilityExternalIP: publicGateways,
v1alpha1.IngressVisibilityClusterLocal: privateGateways,
}
Expand Down
35 changes: 8 additions & 27 deletions pkg/reconciler/ingress/ingress_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,6 @@ import (
"google.golang.org/protobuf/proto"
"google.golang.org/protobuf/testing/protocmp"

istiov1alpha1 "istio.io/api/meta/v1alpha1"
istiov1beta1 "istio.io/api/networking/v1beta1"
"istio.io/client-go/pkg/apis/networking/v1beta1"

Expand Down Expand Up @@ -140,11 +139,11 @@ var (
"gateway." + config.KnativeIngressGateway: newDomainInternal,
"gateway.knative-test-gateway": originDomainInternal,
}
ingressGateway = map[v1alpha1.IngressVisibility]sets.String{
v1alpha1.IngressVisibilityExternalIP: sets.NewString(config.KnativeIngressGateway),
ingressGateway = map[v1alpha1.IngressVisibility]sets.Set[string]{
v1alpha1.IngressVisibilityExternalIP: sets.New(config.KnativeIngressGateway),
}
gateways = map[v1alpha1.IngressVisibility]sets.String{
v1alpha1.IngressVisibilityExternalIP: sets.NewString("knative-test-gateway", config.KnativeIngressGateway),
gateways = map[v1alpha1.IngressVisibility]sets.Set[string]{
v1alpha1.IngressVisibilityExternalIP: sets.New("knative-test-gateway", config.KnativeIngressGateway),
}
perIngressGatewayName = resources.GatewayName(ingressWithTLS("reconciling-ingress", ingressTLS), ingressService)
)
Expand Down Expand Up @@ -1256,24 +1255,6 @@ func ingressWithTLSAndStatusClusterLocal(name string, tls []v1alpha1.IngressTLS,
return ci
}

func meshVirtualServiceWithStatus(ing *v1alpha1.Ingress, gateways map[v1alpha1.IngressVisibility]sets.String, status *istiov1alpha1.IstioStatus, generation int64, observedGeneration int64) *v1beta1.VirtualService {
vs := resources.MakeMeshVirtualService(ing, gateways)
vs.Status = *status.DeepCopy()
vs.ObjectMeta.Generation = generation
vs.Status.ObservedGeneration = observedGeneration

return vs
}

func ingressVirtualServiceWithStatus(ing *v1alpha1.Ingress, gateways map[v1alpha1.IngressVisibility]sets.String, status *istiov1alpha1.IstioStatus, generation int64, observedGeneration int64) *v1beta1.VirtualService {
vs := resources.MakeIngressVirtualService(ing, gateways)
vs.Status = *status.DeepCopy()
vs.ObjectMeta.Generation = generation
vs.Status.ObservedGeneration = observedGeneration

return vs
}

func newTestSetup(t *testing.T, configs ...*corev1.ConfigMap) (
context.Context,
context.CancelFunc,
Expand Down Expand Up @@ -1536,10 +1517,10 @@ func TestGlobalResyncOnUpdateNetwork(t *testing.T) {
}
}

func makeGatewayMap(publicGateways []string, privateGateways []string) map[v1alpha1.IngressVisibility]sets.String {
return map[v1alpha1.IngressVisibility]sets.String{
v1alpha1.IngressVisibilityExternalIP: sets.NewString(publicGateways...),
v1alpha1.IngressVisibilityClusterLocal: sets.NewString(privateGateways...),
func makeGatewayMap(publicGateways []string, privateGateways []string) map[v1alpha1.IngressVisibility]sets.Set[string] {
return map[v1alpha1.IngressVisibility]sets.Set[string]{
v1alpha1.IngressVisibilityExternalIP: sets.New(publicGateways...),
v1alpha1.IngressVisibilityClusterLocal: sets.New(privateGateways...),
}
}

Expand Down
13 changes: 11 additions & 2 deletions pkg/reconciler/ingress/lister.go
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ 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))
hostsByGateway := ingress.HostsPerVisibility(ing, convertVisibilityMap(qualifiedGatewayNamesFromContext(ctx)))
gatewayNames := make([]string, 0, len(hostsByGateway))
for gatewayName := range hostsByGateway {
gatewayNames = append(gatewayNames, gatewayName)
Expand Down Expand Up @@ -129,7 +129,7 @@ func (l *gatewayPodTargetLister) listGatewayTargets(gateway *v1beta1.Gateway) ([
return nil, fmt.Errorf("failed to get Endpoints: %w", err)
}

seen := sets.NewString()
seen := sets.New[string]()
targets := []status.ProbeTarget{}
for _, server := range gateway.Spec.Servers {
tURL := &url.URL{}
Expand Down Expand Up @@ -187,3 +187,12 @@ func (l *gatewayPodTargetLister) listGatewayTargets(gateway *v1beta1.Gateway) ([
}
return targets, nil
}

// Remove me when ingress.HostsPerVisibility uses sets.Set[string]
// nolint: staticcheck
func convertVisibilityMap(input map[v1alpha1.IngressVisibility]sets.Set[string]) map[v1alpha1.IngressVisibility]sets.String {
return map[v1alpha1.IngressVisibility]sets.String{
v1alpha1.IngressVisibilityExternalIP: sets.NewString(sets.List(input[v1alpha1.IngressVisibilityExternalIP])...),
v1alpha1.IngressVisibilityClusterLocal: sets.NewString(sets.List(input[v1alpha1.IngressVisibilityClusterLocal])...),
}
}
2 changes: 1 addition & 1 deletion pkg/reconciler/ingress/resources/gateway.go
Original file line number Diff line number Diff line change
Expand Up @@ -398,7 +398,7 @@ func GetIngressGatewaySvcNameNamespaces(ctx context.Context) ([]metav1.ObjectMet

// UpdateGateway replaces the existing servers with the wanted servers.
func UpdateGateway(gateway *v1beta1.Gateway, want []*istiov1beta1.Server, existing []*istiov1beta1.Server) *v1beta1.Gateway {
existingServers := sets.String{}
existingServers := sets.New[string]()
for i := range existing {
existingServers.Insert(existing[i].Port.Name)
}
Expand Down
61 changes: 35 additions & 26 deletions pkg/reconciler/ingress/resources/virtual_service.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,15 +45,15 @@ func VirtualServiceNamespace(ing *v1alpha1.Ingress) string {

// MakeIngressVirtualService creates Istio VirtualService as network
// programming for Istio Gateways other than 'mesh'.
func MakeIngressVirtualService(ing *v1alpha1.Ingress, gateways map[v1alpha1.IngressVisibility]sets.String) *v1beta1.VirtualService {
func MakeIngressVirtualService(ing *v1alpha1.Ingress, gateways map[v1alpha1.IngressVisibility]sets.Set[string]) *v1beta1.VirtualService {
vs := &v1beta1.VirtualService{
ObjectMeta: metav1.ObjectMeta{
Name: names.IngressVirtualService(ing),
Namespace: VirtualServiceNamespace(ing),
OwnerReferences: []metav1.OwnerReference{*kmeta.NewControllerRef(ing)},
Annotations: ing.GetAnnotations(),
},
Spec: *makeVirtualServiceSpec(ing, gateways, ingress.ExpandedHosts(getHosts(ing))),
Spec: *makeVirtualServiceSpec(ing, gateways, expandedHosts(getHosts(ing))),
}

// Populate the Ingress labels.
Expand All @@ -65,12 +65,12 @@ func MakeIngressVirtualService(ing *v1alpha1.Ingress, gateways map[v1alpha1.Ingr
}

// MakeMeshVirtualService creates a mesh Virtual Service
func MakeMeshVirtualService(ing *v1alpha1.Ingress, gateways map[v1alpha1.IngressVisibility]sets.String) *v1beta1.VirtualService {
func MakeMeshVirtualService(ing *v1alpha1.Ingress, gateways map[v1alpha1.IngressVisibility]sets.Set[string]) *v1beta1.VirtualService {
hosts := keepLocalHostnames(getHosts(ing))
// If cluster local gateway is configured, we need to expand hosts because of
// https://github.com/knative/serving/issues/6488#issuecomment-573513768.
if len(gateways[v1alpha1.IngressVisibilityClusterLocal]) != 0 {
hosts = ingress.ExpandedHosts(hosts)
hosts = expandedHosts(hosts)
}
if len(hosts) == 0 {
return nil
Expand All @@ -82,9 +82,9 @@ func MakeMeshVirtualService(ing *v1alpha1.Ingress, gateways map[v1alpha1.Ingress
OwnerReferences: []metav1.OwnerReference{*kmeta.NewControllerRef(ing)},
Annotations: ing.GetAnnotations(),
},
Spec: *makeVirtualServiceSpec(ing, map[v1alpha1.IngressVisibility]sets.String{
v1alpha1.IngressVisibilityExternalIP: sets.NewString("mesh"),
v1alpha1.IngressVisibilityClusterLocal: sets.NewString("mesh"),
Spec: *makeVirtualServiceSpec(ing, map[v1alpha1.IngressVisibility]sets.Set[string]{
v1alpha1.IngressVisibilityExternalIP: sets.New("mesh"),
v1alpha1.IngressVisibilityClusterLocal: sets.New("mesh"),
}, hosts),
}
// Populate the Ingress labels.
Expand All @@ -96,7 +96,7 @@ func MakeMeshVirtualService(ing *v1alpha1.Ingress, gateways map[v1alpha1.Ingress
}

// MakeVirtualServices creates a mesh VirtualService and a virtual service for each gateway
func MakeVirtualServices(ing *v1alpha1.Ingress, gateways map[v1alpha1.IngressVisibility]sets.String) ([]*v1beta1.VirtualService, error) {
func MakeVirtualServices(ing *v1alpha1.Ingress, gateways map[v1alpha1.IngressVisibility]sets.Set[string]) ([]*v1beta1.VirtualService, error) {
// Insert probe header
ing = ing.DeepCopy()
if _, err := ingress.InsertProbe(ing); err != nil {
Expand All @@ -122,39 +122,39 @@ func MakeVirtualServices(ing *v1alpha1.Ingress, gateways map[v1alpha1.IngressVis
return vss, nil
}

func makeVirtualServiceSpec(ing *v1alpha1.Ingress, gateways map[v1alpha1.IngressVisibility]sets.String, hosts sets.String) *istiov1beta1.VirtualService {
func makeVirtualServiceSpec(ing *v1alpha1.Ingress, gateways map[v1alpha1.IngressVisibility]sets.Set[string], hosts sets.Set[string]) *istiov1beta1.VirtualService {
spec := istiov1beta1.VirtualService{
Hosts: hosts.List(),
Hosts: sets.List(hosts),
}

gw := sets.String{}
gw := sets.New[string]()
for _, rule := range ing.Spec.Rules {
for i := range rule.HTTP.Paths {
p := rule.HTTP.Paths[i]
hosts := hosts.Intersection(sets.NewString(rule.Hosts...))
hosts := hosts.Intersection(sets.New(rule.Hosts...))
if hosts.Len() != 0 {
http := makeVirtualServiceRoute(hosts, &p, gateways, rule.Visibility)
// Add all the Gateways that exist inside the http.match section of
// the VirtualService.
// This ensures that we are only using the Gateways that actually appear
// in VirtualService routes.
for _, m := range http.Match {
gw = gw.Union(sets.NewString(m.Gateways...))
gw = gw.Union(sets.New(m.Gateways...))
}
spec.Http = append(spec.Http, http)
}
}
}
spec.Gateways = gw.List()
spec.Gateways = sets.List(gw)
return &spec
}

func makeVirtualServiceRoute(hosts sets.String, http *v1alpha1.HTTPIngressPath, gateways map[v1alpha1.IngressVisibility]sets.String, visibility v1alpha1.IngressVisibility) *istiov1beta1.HTTPRoute {
func makeVirtualServiceRoute(hosts sets.Set[string], http *v1alpha1.HTTPIngressPath, gateways map[v1alpha1.IngressVisibility]sets.Set[string], visibility v1alpha1.IngressVisibility) *istiov1beta1.HTTPRoute {
matches := []*istiov1beta1.HTTPMatchRequest{}
// Deduplicate hosts to avoid excessive matches, which cause a combinatorial expansion in Istio
distinctHosts := getDistinctHostPrefixes(hosts)

for _, host := range distinctHosts.List() {
for _, host := range sets.List(distinctHosts) {
matches = append(matches, makeMatch(host, http.Path, http.Headers, gateways[visibility]))
}

Expand Down Expand Up @@ -210,11 +210,11 @@ func makeVirtualServiceRoute(hosts sets.String, http *v1alpha1.HTTPIngressPath,

// getDistinctHostPrefixes deduplicate a set of prefix matches. For example, the set {a, aabb} can be
// reduced to {a}, as a prefix match on {a} accepts all the same inputs as {a, aabb}.
func getDistinctHostPrefixes(hosts sets.String) sets.String {
func getDistinctHostPrefixes(hosts sets.Set[string]) sets.Set[string] {
// First we sort the list. This ensures that we always process the smallest elements (which match against
// the most patterns, as they are less specific) first.
all := hosts.List()
ns := sets.NewString()
all := sets.List(hosts)
ns := sets.New[string]()
for _, h := range all {
prefixExists := false
h = hostPrefix(h)
Expand All @@ -233,20 +233,20 @@ func getDistinctHostPrefixes(hosts sets.String) sets.String {
return ns
}

func keepLocalHostnames(hosts sets.String) sets.String {
func keepLocalHostnames(hosts sets.Set[string]) sets.Set[string] {
localSvcSuffix := ".svc." + network.GetClusterDomainName()
retained := sets.NewString()
for _, h := range hosts.List() {
retained := sets.New[string]()
for _, h := range sets.List(hosts) {
if strings.HasSuffix(h, localSvcSuffix) {
retained.Insert(h)
}
}
return retained
}

func makeMatch(host, path string, headers map[string]v1alpha1.HeaderMatch, gateways sets.String) *istiov1beta1.HTTPMatchRequest {
func makeMatch(host, path string, headers map[string]v1alpha1.HeaderMatch, gateways sets.Set[string]) *istiov1beta1.HTTPMatchRequest {
match := &istiov1beta1.HTTPMatchRequest{
Gateways: gateways.List(),
Gateways: sets.List(gateways),
Authority: &istiov1beta1.StringMatch{
// Do not use Regex as Istio 1.4 or later has 100 bytes limitation.
MatchType: &istiov1beta1.StringMatch_Prefix{Prefix: host},
Expand Down Expand Up @@ -283,8 +283,8 @@ func hostPrefix(host string) string {
return strings.TrimSuffix(host, localDomainSuffix)
}

func getHosts(ing *v1alpha1.Ingress) sets.String {
hosts := sets.NewString()
func getHosts(ing *v1alpha1.Ingress) sets.Set[string] {
hosts := sets.New[string]()
for _, rule := range ing.Spec.Rules {
hosts.Insert(rule.Hosts...)
}
Expand Down Expand Up @@ -312,3 +312,12 @@ func getPublicIngressRules(i *v1alpha1.Ingress) []v1alpha1.IngressRule {

return result
}

// Keep me until ingress.ExpandedHosts uses sets.Set[string]
func expandedHosts(hosts sets.Set[string]) sets.Set[string] {
tmp := sets.NewString(sets.List(hosts)...)

ret := ingress.ExpandedHosts(tmp)

return sets.New(ret.List()...)
}
Loading
Loading