cli/compose: implement the ports validation method

[Stavrospanakakis]

- What I did
This commit implements a validation method for the port mappings of compose. Also, it removes the ports validation method from the expose property since they do not always accept the same type of values. For example, ports property accepts the value 8000:8000 but the expose property not.

- How I did it
After reading the API Reference of Docker Compose syntax, I wrote a test that covers all the cases and then implemented the validation method until the tests pass.

- How to verify it
Run make test

- Description for the changelog

Implement the ports validation method for compose

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 60.63%. Comparing base (305985c) to head (0319795).
Report is 2 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5524      +/-   ##
==========================================
+ Coverage   60.58%   60.63%   +0.04%     
==========================================
  Files         345      345              
  Lines       23456    23462       +6     
==========================================
+ Hits        14211    14226      +15     
+ Misses       8273     8263      -10     
- Partials      972      973       +1     

[thaJeztah]

at my computer right now (commenting from my phone), but wondering if (most of this?) would be doable using the utilities that are used for the corresponding command-line options;

cli/vendor/github.com/docker/go-connections/nat/parse.go

Line 10 in 305985c

func ParsePortRange(ports string) (uint64, uint64, error) {

cli/vendor/github.com/docker/go-connections/nat/nat.go

Line 121 in 305985c

func ParsePortSpecs(ports []string) (map[Port]struct{}, map[Port][]PortBinding, error) {

cli/vendor/github.com/docker/go-connections/nat/nat.go

Line 171 in 305985c

func ParsePortSpec(rawPort string) ([]PortMapping, error) {

[Stavrospanakakis]

@thaJeztah Thank you for the review, I was not aware of this library. I implemented the changes, I did a git rebase with the master branch and pushed a new commit.

[laurazard]

LGTM!

[thaJeztah]

LGTM, thanks!

[thaJeztah]

Not for this PR, but we probably would be able to use the same utilities for this case as well, which could be either using ParsePortSpec and making sure the IP address is not set, or a combination of ParsePortRange and SplitProtoPort .

I see the latter is the approach taken for the --expose flag, but .. I'm actually wondering if the first approach would be better, because it also validates the given proto

cli/cli/command/container/opts.go

Lines 443 to 466 in 305985c

	// Merge in exposed ports to the map of published ports
	for _, e := range copts.expose.GetAll() {
	if strings.Contains(e, ":") {
	return nil, errors.Errorf("invalid port format for --expose: %s", e)
	}
	// support two formats for expose, original format <portnum>/[<proto>]
	// or <startport-endport>/[<proto>]
	proto, port := nat.SplitProtoPort(e)
	// parse the start and end port and create a sequence of ports to expose
	// if expose a port, the start and end port are the same
	start, end, err := nat.ParsePortRange(port)
	if err != nil {
	return nil, errors.Errorf("invalid range format for --expose: %s, error: %s", e, err)
	}
	for i := start; i <= end; i++ {
	p, err := nat.NewPort(proto, strconv.FormatUint(i, 10))
	if err != nil {
	return nil, err
	}
	if _, exists := ports[p]; !exists {
	ports[p] = struct{}{}
	}
	}
	}

[thaJeztah]

On second thought, MAYBE validating the proto shouldn't belong on the client-side, as the list of supported protocols could depend on what the daemon supports.

But given that we already do it elsewhere, maybe that's not too problematic (and if we change, we should do so across the board).

We could possibly convert the code linked above to a utility if we don't want to duplicate the logic.

[thaJeztah]

In any case, none of the above is needed for the current PR; it's mostly outlining thoughts for a follow-up 😅

[Stavrospanakakis]

@thaJeztah The ParsePortSpec method returns PortMapping while the expose propery is a PortBinding, so ParsePortSpec might not be the best for this case.

For this reason, I believe that the combination of ParsePortRange and SplitProtoPort will work better .

Regarding creating a utility, I am not sure that it will help to avoid duplicate code. For example:

Using a utility:

// utils.go

func validateExposePortOption(ctx context.Context, portSpec string) error {
  if strings.Contains(portSpec, ":") {
    return fmt.Errorf("invalid port format for expose %s", portSpec)
  }

  // support two formats for expose, original format <portnum>/[<proto>]
  // or <startport-endport>/[<proto>]
  proto, port := nat.SplitProtoPort(portSpec)
  
  // we could probably convert the validateProto method to public and validate the
  // proto to the client
  // https://github.com/docker/go-connections/blob/5df8d2b30ca886f2d94740ce3c54abd58a5bb2c9/nat/nat.go#L110
  // ......

  // parse the start and end port and create a sequence of ports to expose
  // if expose a port, the start and end port are the same
  _, _, err := nat.ParsePortRange(port) 
  if err != nil {
    return fmt.Errorf("invalid range format for expose: %s, error: %s", portSpec, err)
  }
  return nil
}

// opts.go

// Merge in exposed ports to the map of published ports
for _, e := range copts.expose.GetAll() {
        err := validateExposePortOption(ctx, e)
        if err != nil {
            return nil, errors.Errorf("invalid port format for --expose: %s, error: %s", e, err)
        }

        // DUPLICATE CODE WITH THE UTILITY
	start, end, err := nat.ParsePortRange(port)
	if err != nil {
		return nil, errors.Errorf("invalid range format for --expose: %s, error: %s", e, err)
	}
	for i := start; i <= end; i++ {
		p, err := nat.NewPort(proto, strconv.FormatUint(i, 10))
		if err != nil {
			return nil, err
		}
		if _, exists := ports[p]; !exists {
			ports[p] = struct{}{}
		}
	}
}

// schema.go

type exposeFormatChecker struct{}

func (checker exposeFormatChecker) IsFormat(input any) bool {
	var port string

	switch p := input.(type) {
	case string:
		port = p
	case *big.Rat:
		port = strings.Split(p.String(), "/")[0]
	}
    err := validateExposePortOption(context.Background(), port)
    return err == nil
}

Not using a utility:

// opts.go

// Merge in exposed ports to the map of published ports
for _, e := range copts.expose.GetAll() {
	if strings.Contains(e, ":") {
		return nil, errors.Errorf("invalid port format for --expose: %s", e)
	}
	
	// support two formats for expose, original format <portnum>/[<proto>]
	// or <startport-endport>/[<proto>]
	proto, port := nat.SplitProtoPort(e)
	
    // we could probably convert the validateProto method to public and validate the proto to the client
    // https://github.com/docker/go-connections/blob/5df8d2b30ca886f2d94740ce3c54abd58a5bb2c9/nat/nat.go#L110
    // ......
    
	// parse the start and end port and create a sequence of ports to expose
	// if expose a port, the start and end port are the same
	start, end, err := nat.ParsePortRange(port)
	if err != nil {
		return nil, errors.Errorf("invalid range format for --expose: %s, error: %s", e, err)
	}
	for i := start; i <= end; i++ {
		p, err := nat.NewPort(proto, strconv.FormatUint(i, 10))
		if err != nil {
			return nil, err
		}
		if _, exists := ports[p]; !exists {
			ports[p] = struct{}{}
		}
	}
}

// schema.go

type exposeFormatChecker struct{}

func (checker exposeFormatChecker) IsFormat(input any) bool {
    var port string
    
    switch p := input.(type) {
    case string:
    	port = p
    case *big.Rat:
    	port = strings.Split(p.String(), "/")[0]
    }

    if strings.Contains(portSpec, ":") {
        return false
    }
    
    // support two formats for expose, original format <portnum>/[<proto>]
    // or <startport-endport>/[<proto>]
    proto, port := nat.SplitProtoPort(portSpec)
    
    // we could probably convert the validateProto method to public and validate the
    // proto to the client
    // https://github.com/docker/go-connections/blob/5df8d2b30ca886f2d94740ce3c54abd58a5bb2c9/nat/nat.go#L110
    // ......
    
    // parse the start and end port and create a sequence of ports to expose
    // if expose a port, the start and end port are the same
    _, _, err := nat.ParsePortRange(port) 
    if err != nil {
        return false
    }
    
    return true
}

Both cases with or without a utility contain duplicate code.

What do you think?

I will be happy to open a new pull request for this.