[TF] Cleanup echo Port (istio#37808)

We currently have Port and PortName and the former is a pointer. This leads to a lot of complexity to the logic. This PR removes PortName and makes Port a struct rather than a pointer.

Author: nmittler

pkg/config/protocol/instance.go

@@ -19,6 +19,10 @@ import "strings"
 // Instance defines network protocols for ports
 type Instance string
 
+func (i Instance) String() string {
+	return string(i)
+}
+
 const (
 	// GRPC declares that the port carries gRPC traffic.
 	GRPC Instance = "GRPC"

pkg/test/framework/components/echo/calloptions.go

@@ -92,18 +92,17 @@ type CallOptions struct {
 	// To is the Target to be called. Required.
 	To Instance
 
-	// Port on the target Instance. Either Port or PortName must be specified.
-	Port *Port
+	// Port to be used for the call. Ignored if Scheme == DNS. If the Port.ServicePort is set,
+	// either Port.Protocol or Scheme must also be set. If Port.ServicePort is not set,
+	// the port is looked up in To by either Port.Name or Port.Protocol.
+	Port Port
 
-	// PortName of the port on the target Instance. Either Port or PortName must be specified.
-	PortName string
-
-	// Scheme to be used when making the call. If not provided, an appropriate default for the
-	// port will be used (if feasible).
+	// Scheme to be used when making the call. If not provided, the Scheme will be selected
+	// based on the Port.Protocol.
 	Scheme scheme.Instance
 
 	// Address specifies the host name or IP address to be used on the request. If not provided,
-	// an appropriate default is chosen for the target Instance.
+	// an appropriate default is chosen for To.
 	Address string
 
 	// Count indicates the number of exchanges that should be made with the service endpoint.
@@ -161,10 +160,6 @@ func (o CallOptions) GetHost() string {
 
 func (o CallOptions) DeepCopy() CallOptions {
 	clone := o
-	if o.Port != nil {
-		dc := *o.Port
-		clone.Port = &dc
-	}
 	if o.TLS.Alpn != nil {
 		clone.TLS.Alpn = make([]string, len(o.TLS.Alpn))
 		copy(clone.TLS.Alpn, o.TLS.Alpn)
@@ -174,50 +169,112 @@ func (o CallOptions) DeepCopy() CallOptions {
 
 // FillDefaults fills out any defaults that haven't been explicitly specified.
 func (o *CallOptions) FillDefaults() error {
+	// Fill in the address if not set.
+	if err := o.fillAddress(); err != nil {
+		return err
+	}
+
+	// Fill in the port if not set or the service port is missing.
+	if err := o.fillPort(); err != nil {
+		return err
+	}
+
+	// Fill in the scheme if not set, using the port information.
+	if err := o.fillScheme(); err != nil {
+		return err
+	}
+
+	// Fill in HTTP headers
+	o.fillHeaders()
+
+	if o.Timeout <= 0 {
+		o.Timeout = common.DefaultRequestTimeout
+	}
+
+	if o.Count <= 0 {
+		o.Count = common.DefaultCount
+	}
+
+	// Add any user-specified options after the default options (last option wins for each type of option).
+	o.Retry.Options = append(append([]retry.Option{}, DefaultCallRetryOptions()...), o.Retry.Options...)
+
+	// If no Check was specified, assume no error.
+	if o.Check == nil {
+		o.Check = check.None()
+	}
+	return nil
+}
+
+func (o *CallOptions) fillAddress() error {
+	if o.Address == "" {
+		if o.To == nil {
+			return errors.New("if address is not set, then To must be set")
+		}
+
+		// No host specified, use the fully qualified domain name for the service.
+		o.Address = o.To.Config().ClusterLocalFQDN()
+	}
+	return nil
+}
+
+func (o *CallOptions) fillPort() error {
+	if o.Scheme == scheme.DNS {
+		// Port is not used for DNS.
+		return nil
+	}
+
+	if o.Port.ServicePort > 0 {
+		if o.Port.Protocol == "" && o.Scheme == "" {
+			return errors.New("callOptions: servicePort specified, but no protocol or scheme was set")
+		}
+
+		// The service port was set explicitly. Nothing else to do.
+		return nil
+	}
+
 	if o.To != nil {
 		servicePorts := o.To.Config().Ports.GetServicePorts()
-		if o.PortName == "" {
-			// Validate the Port value.
 
-			if o.Port == nil {
-				return errors.New("callOptions: PortName or Port must be provided")
+		if o.Port.Name != "" {
+			// Look up the port by name.
+			p, found := servicePorts.ForName(o.Port.Name)
+			if !found {
+				return fmt.Errorf("callOptions: no port named %s available in To Instance", o.Port.Name)
 			}
+			o.Port = p
+			return nil
+		}
 
-			// Check the specified port for a match against the To Instance
-			if !servicePorts.Contains(*o.Port) {
-				return fmt.Errorf("callOptions: Port does not match any To port")
-			}
-		} else {
-			// Look up the port.
-			p, found := servicePorts.ForName(o.PortName)
+		if o.Port.Protocol != "" {
+			// Look up the port by protocol.
+			p, found := servicePorts.ForProtocol(o.Port.Protocol)
 			if !found {
-				return fmt.Errorf("callOptions: no port named %s available in To Instance", o.PortName)
+				return fmt.Errorf("callOptions: no port for protocol %s available in To Instance", o.Port.Protocol)
 			}
-			o.Port = &p
-		}
-	} else if o.Scheme == scheme.DNS {
-		// Just need address
-		if o.Address == "" {
-			return fmt.Errorf("for DNS, address must be set")
+			o.Port = p
+			return nil
 		}
-		o.Port = &Port{}
-	} else if o.Port == nil || o.Port.ServicePort <= 0 || (o.Port.Protocol == "" && o.Scheme == "") || o.Address == "" {
+	}
+
+	if o.Port.ServicePort <= 0 || (o.Port.Protocol == "" && o.Scheme == "") || o.Address == "" {
 		return fmt.Errorf("if target is not set, then port.servicePort, port.protocol or schema, and address must be set")
 	}
 
+	return nil
+}
+
+func (o *CallOptions) fillScheme() error {
 	if o.Scheme == "" {
 		// No protocol, fill it in.
 		var err error
 		if o.Scheme, err = o.Port.Scheme(); err != nil {
 			return err
 		}
 	}
+	return nil
+}
 
-	if o.Address == "" {
-		// No host specified, use the fully qualified domain name for the service.
-		o.Address = o.To.Config().ClusterLocalFQDN()
-	}
-
+func (o *CallOptions) fillHeaders() {
 	// Initialize the headers and add a default Host header if none provided.
 	if o.HTTP.Headers == nil {
 		o.HTTP.Headers = make(http.Header)
@@ -229,21 +286,4 @@ func (o *CallOptions) FillDefaults() error {
 	if h := o.GetHost(); len(h) > 0 {
 		o.HTTP.Headers.Set(headers.Host, h)
 	}
-
-	if o.Timeout <= 0 {
-		o.Timeout = common.DefaultRequestTimeout
-	}
-
-	if o.Count <= 0 {
-		o.Count = common.DefaultCount
-	}
-
-	// Add any user-specified options after the default options (last option wins for each type of option).
-	o.Retry.Options = append(append([]retry.Option{}, DefaultCallRetryOptions()...), o.Retry.Options...)
-
-	// If no Check was specified, assume no error.
-	if o.Check == nil {
-		o.Check = check.None()
-	}
-	return nil
 }

pkg/test/framework/components/echo/common/call.go

@@ -156,16 +156,13 @@ func ForwardEcho(srcName string, clientProvider EchoClientProvider, opts *echo.C
 		return c.ForwardEcho(context.Background(), req)
 	})
 	if err != nil {
-		if opts.Port != nil {
-			err = fmt.Errorf("failed calling %s->'%s://%s:%d/%s': %v",
-				srcName,
-				strings.ToLower(string(opts.Port.Protocol)),
-				opts.Address,
-				opts.Port.ServicePort,
-				opts.HTTP.Path,
-				err)
-		}
-		return nil, err
+		return nil, fmt.Errorf("failed calling %s->'%s://%s:%d/%s': %v",
+			srcName,
+			strings.ToLower(string(opts.Port.Protocol)),
+			opts.Address,
+			opts.Port.ServicePort,
+			opts.HTTP.Path,
+			err)
 	}
 	return res, nil
 }

pkg/test/framework/components/echo/kube/instance.go

@@ -193,7 +193,7 @@ func (c *instance) Restart() error {
 // aggregateResponses forwards an echo request from all workloads belonging to this echo instance and aggregates the results.
 func (c *instance) aggregateResponses(opts echo.CallOptions) (echoClient.Responses, error) {
 	// TODO put this somewhere else, or require users explicitly set the protocol - quite hacky
-	if c.Config().IsProxylessGRPC() && (opts.Scheme == scheme.GRPC || opts.PortName == "grpc" || opts.Port != nil && opts.Port.Protocol == protocol.GRPC) {
+	if c.Config().IsProxylessGRPC() && (opts.Scheme == scheme.GRPC || opts.Port.Name == "grpc" || opts.Port.Protocol == protocol.GRPC) {
 		// for gRPC calls, use XDS resolver
 		opts.Scheme = scheme.XDS
 	}

pkg/test/framework/components/istio/ingress.go

@@ -22,10 +22,10 @@ import (
 	"strconv"
 	"time"
 
-	"istio.io/istio/pkg/config/protocol"
 	"istio.io/istio/pkg/http/headers"
 	"istio.io/istio/pkg/test"
 	echoClient "istio.io/istio/pkg/test/echo"
+	"istio.io/istio/pkg/test/echo/common/scheme"
 	"istio.io/istio/pkg/test/framework/components/cluster"
 	"istio.io/istio/pkg/test/framework/components/echo"
 	"istio.io/istio/pkg/test/framework/components/echo/common"
@@ -182,51 +182,67 @@ func (c *ingressImpl) CallOrFail(t test.Failer, options echo.CallOptions) echoCl
 	return resp
 }
 
-func (c *ingressImpl) callEcho(options echo.CallOptions) (echoClient.Responses, error) {
-	if options.Port == nil || options.Port.Protocol == "" {
-		return nil, fmt.Errorf("must provide protocol")
-	}
+func (c *ingressImpl) callEcho(opts echo.CallOptions) (echoClient.Responses, error) {
 	var (
 		addr string
 		port int
 	)
-	options = options.DeepCopy()
-	if options.Port.ServicePort == 0 {
+	opts = opts.DeepCopy()
+
+	if opts.Port.ServicePort == 0 {
+		s, err := c.schemeFor(opts)
+		if err != nil {
+			return nil, err
+		}
+		opts.Scheme = s
+
 		// Default port based on protocol
-		switch options.Port.Protocol {
-		case protocol.HTTP:
+		switch s {
+		case scheme.HTTP:
 			addr, port = c.HTTPAddress()
-		case protocol.HTTPS:
+		case scheme.HTTPS:
 			addr, port = c.HTTPSAddress()
-		case protocol.TCP:
+		case scheme.TCP:
 			addr, port = c.TCPAddress()
 		default:
-			return nil, fmt.Errorf("protocol %v not supported, provide explicit port", options.Port.Protocol)
+			return nil, fmt.Errorf("ingress: scheme %v not supported. Options: %v+", s, opts)
 		}
 	} else {
-		addr, port = c.AddressForPort(options.Port.ServicePort)
+		addr, port = c.AddressForPort(opts.Port.ServicePort)
 	}
 
 	if addr == "" || port == 0 {
-		scopes.Framework.Warnf("failed to get host and port for %s/%d", options.Port.Protocol, options.Port.ServicePort)
+		scopes.Framework.Warnf("failed to get host and port for %s/%d", opts.Port.Protocol, opts.Port.ServicePort)
 	}
 
 	// Even if they set ServicePort, when load balancer is disabled, we may need to switch to NodePort, so replace it.
-	options.Port.ServicePort = port
-	if len(options.Address) == 0 {
+	opts.Port.ServicePort = port
+	if len(opts.Address) == 0 {
 		// Default address based on port
-		options.Address = addr
+		opts.Address = addr
 	}
-	if options.HTTP.Headers == nil {
-		options.HTTP.Headers = map[string][]string{}
+	if opts.HTTP.Headers == nil {
+		opts.HTTP.Headers = map[string][]string{}
 	}
-	if host := options.GetHost(); len(host) > 0 {
-		options.HTTP.Headers.Set(headers.Host, host)
+	if host := opts.GetHost(); len(host) > 0 {
+		opts.HTTP.Headers.Set(headers.Host, host)
 	}
 	if len(c.cluster.HTTPProxy()) > 0 {
-		options.HTTP.HTTPProxy = c.cluster.HTTPProxy()
+		opts.HTTP.HTTPProxy = c.cluster.HTTPProxy()
 	}
-	return common.CallEcho(&options)
+	return common.CallEcho(&opts)
+}
+
+func (c *ingressImpl) schemeFor(opts echo.CallOptions) (scheme.Instance, error) {
+	if opts.Scheme == "" && opts.Port.Protocol == "" {
+		return "", fmt.Errorf("must provide either protocol or scheme")
+	}
+
+	if opts.Scheme != "" {
+		return opts.Scheme, nil
+	}
+
+	return opts.Port.Scheme()
 }
 
 func (c *ingressImpl) ProxyStats() (map[string]int, error) {