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.

Sign up for GitHub

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

Jump to bottom

cli/compose: implement the ports validation method #5524

Merged
merged 1 commit into from
Oct 10, 2024
Merged

cli/compose: implement the ports validation method #5524

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
18 changes: 14 additions & 4 deletions cli/compose/schema/schema.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,11 @@ package schema
import (
"embed"
"fmt"
"math/big"
"strings"
"time"

"github.com/docker/go-connections/nat"
"github.com/pkg/errors"
"github.com/xeipuuv/gojsonschema"
)
Expand All @@ -20,9 +22,18 @@ const (

type portsFormatChecker struct{}

func (checker portsFormatChecker) IsFormat(_ any) bool {
// TODO: implement this
return true
func (checker portsFormatChecker) IsFormat(input any) bool {
var portSpec string

switch p := input.(type) {
case string:
portSpec = p
case *big.Rat:
portSpec = strings.Split(p.String(), "/")[0]
}

_, err := nat.ParsePortSpec(portSpec)
return err == nil
}

type durationFormatChecker struct{}
Expand All @@ -37,7 +48,6 @@ func (checker durationFormatChecker) IsFormat(input any) bool {
}

func init() {
gojsonschema.FormatCheckers.Add("expose", portsFormatChecker{})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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{}{}
}
}
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

gojsonschema.FormatCheckers.Add("ports", portsFormatChecker{})
gojsonschema.FormatCheckers.Add("duration", durationFormatChecker{})
}
Expand Down
105 changes: 105 additions & 0 deletions cli/compose/schema/schema_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,111 @@ func TestValidate(t *testing.T) {
assert.ErrorContains(t, Validate(config, "12345"), "unsupported Compose file version: 12345")
}

func TestValidatePorts(t *testing.T) {
testcases := []struct {
ports any
hasError bool
}{
{
ports: []int{8000},
hasError: false,
},
{
ports: []string{"8000:8000"},
hasError: false,
},
{
ports: []string{"8001-8005"},
hasError: false,
},
{
ports: []string{"8001-8005:8001-8005"},
hasError: false,
},
{
ports: []string{"8000"},
hasError: false,
},
{
ports: []string{"8000-9000:80"},
hasError: false,
},
{
ports: []string{"[::1]:8080:8000"},
hasError: false,
},
{
ports: []string{"[::1]:8080-8085:8000"},
hasError: false,
},
{
ports: []string{"127.0.0.1:8000:8000"},
hasError: false,
},
{
ports: []string{"127.0.0.1:8000-8005:8000-8005"},
hasError: false,
},
{
ports: []string{"127.0.0.1:8000:8000/udp"},
hasError: false,
},
{
ports: []string{"8000:8000/udp"},
hasError: false,
},
{
ports: []string{"8000:8000/http"},
hasError: true,
},
{
ports: []string{"-1"},
hasError: true,
},
{
ports: []string{"65536"},
hasError: true,
},
{
ports: []string{"-1:65536/http"},
hasError: true,
},
{
ports: []string{"invalid"},
hasError: true,
},
{
ports: []string{"12345678:8000:8000/tcp"},
hasError: true,
},
{
ports: []string{"8005-8000:8005-8000"},
hasError: true,
},
{
ports: []string{"8006-8000:8005-8000"},
hasError: true,
},
}

for _, tc := range testcases {
config := dict{
"version": "3.0",
"services": dict{
"foo": dict{
"image": "busybox",
"ports": tc.ports,
},
},
}
if tc.hasError {
assert.ErrorContains(t, Validate(config, "3"), "services.foo.ports.0 Does not match format 'ports'")
} else {
assert.NilError(t, Validate(config, "3"))
}
}
}

func TestValidateUndefinedTopLevelOption(t *testing.T) {
config := dict{
"version": "3.0",
Expand Down
Loading