Skip to content

Commit

Permalink
Add Ports field to app spec (#47706)
Browse files Browse the repository at this point in the history 
* Add Ports to AppSpecV3

* Validate ports of api/types.AppV3

* Add Ports to lib/config and lib/service/servicecfg

* lib/config TestApps: Improve error messages

* lib/service: Convert servicecfg.PortRange to types.PortRange

* Add multi-port TCP apps to config and tctl tests

* Rename Ports to TCPPorts

* Change port fields to uint16 where possible

* Update comments for Port and EndPort

* Extract port range validation to api/utils/net

* Replace custom check type with require.ErrorAssertionFunc

* Simplify validation of end port

* Fix expected message in api/types test

I forgot to update that one.
  • Loading branch information
@ravicious
ravicious authored Dec 3, 2024
1 parent ea58a68 commit 87320df
Show file tree
Hide file tree
Showing 17 changed files with 3,299 additions and 2,240 deletions.
17 changes: 17 additions & 0 deletions api/proto/teleport/legacy/types/types.proto
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1054,6 +1054,11 @@ message AppSpecV3 {
// IdentityCenter encasulates AWS identity-center specific information. Only
// valid for Identity Center account apps.
AppIdentityCenter IdentityCenter = 12 [(gogoproto.jsontag) = "identity_center,omitempty"];
// TCPPorts is a list of ports and port ranges that an app agent can forward connections to.
// Only applicable to TCP App Access.
// If this field is not empty, URI is expected to contain no port number and start with the tcp
// protocol.
repeated PortRange TCPPorts = 13 [(gogoproto.jsontag) = "tcp_ports,omitempty"];
}

// AppServerOrSAMLIdPServiceProviderV1 holds either an AppServerV3 or a SAMLIdPServiceProviderV1 resource (never both).
Expand Down Expand Up @@ -1095,6 +1100,18 @@ message Header {
string Value = 2 [(gogoproto.jsontag) = "value"];
}

// PortRange describes a port range for TCP apps. The range starts with Port and ends with EndPort.
// PortRange can be used to describe a single port in which case the Port field is the port and the
// EndPort field is 0.
message PortRange {
// Port describes the start of the range. It must be between 1 and 65535.
uint32 Port = 1 [(gogoproto.jsontag) = "port"];
// EndPort describes the end of the range, inclusive. If set, it must be between 2 and 65535 and
// be greater than Port when describing a port range. When omitted or set to zero, it signifies
// that the port range defines a single port.
uint32 EndPort = 2 [(gogoproto.jsontag) = "end_port,omitempty"];
}

// CommandLabelV2 is a label that has a value as a result of the
// output generated by running command, e.g. hostname
message CommandLabelV2 {
Expand Down
57 changes: 54 additions & 3 deletions api/types/app.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import (
"github.com/gravitational/teleport/api/constants"
"github.com/gravitational/teleport/api/types/compare"
"github.com/gravitational/teleport/api/utils"
netutils "github.com/gravitational/teleport/api/utils/net"
)

var _ compare.IsEqual[Application] = (*AppV3)(nil)
Expand Down Expand Up @@ -86,6 +87,10 @@ type Application interface {
GetRequiredAppNames() []string
// GetCORS returns the CORS configuration for the app.
GetCORS() *CORSPolicy
// GetTCPPorts returns port ranges supported by the app to which connections can be forwarded to.
GetTCPPorts() []*PortRange
// SetTCPPorts sets port ranges to which connections can be forwarded to.
SetTCPPorts([]*PortRange)
}

// NewAppV3 creates a new app resource.
Expand Down Expand Up @@ -306,6 +311,16 @@ func (a *AppV3) SetUserGroups(userGroups []string) {
a.Spec.UserGroups = userGroups
}

// GetTCPPorts returns port ranges supported by the app to which connections can be forwarded to.
func (a *AppV3) GetTCPPorts() []*PortRange {
return a.Spec.TCPPorts
}

// SetTCPPorts sets port ranges to which connections can be forwarded to.
func (a *AppV3) SetTCPPorts(ports []*PortRange) {
a.Spec.TCPPorts = ports
}

// GetIntegration will return the Integration.
// If present, the Application must use the Integration's credentials instead of ambient credentials to access Cloud APIs.
func (a *AppV3) GetIntegration() string {
Expand Down Expand Up @@ -379,13 +394,13 @@ func (a *AppV3) CheckAndSetDefaults() error {
if !strings.Contains(publicAddr, "//") && strings.Contains(publicAddr, ":") {
publicAddr = "//" + publicAddr
}
url, err := url.Parse(publicAddr)
publicAddrURL, err := url.Parse(publicAddr)
if err != nil {
return trace.BadParameter("invalid PublicAddr format: %v", err)
}
host := a.Spec.PublicAddr
if url.Host != "" {
host = url.Host
if publicAddrURL.Host != "" {
host = publicAddrURL.Host
}

if strings.HasPrefix(host, constants.KubeTeleportProxyALPNPrefix) {
Expand All @@ -402,6 +417,42 @@ func (a *AppV3) CheckAndSetDefaults() error {
}
}

if len(a.Spec.TCPPorts) != 0 {
if err := a.checkTCPPorts(); err != nil {
return trace.Wrap(err)
}
}

return nil
}

func (a *AppV3) checkTCPPorts() error {
// Parsing the URI here does not break compatibility. The URI is parsed only if Ports are present.
// This means that old apps that do have invalid URIs but don't use Ports can continue existing.
uri, err := url.Parse(a.Spec.URI)
if err != nil {
return trace.BadParameter("invalid app URI format: %v", err)
}

// The scheme of URI is enforced to be "tcp" on purpose. This way in the future we can add
// multi-port support to web apps without throwing hard errors when a cluster with a multi-port
// web app gets downgraded to a version which supports multi-port only for TCP apps.
//
// For now, we simply ignore the Ports field set on non-TCP apps.
if uri.Scheme != "tcp" {
return nil
}

if uri.Port() != "" {
return trace.BadParameter("TCP app URI %q must not include a port number when the app spec defines a list of ports", a.Spec.URI)
}

for _, portRange := range a.Spec.TCPPorts {
if err := netutils.ValidatePortRange(int(portRange.Port), int(portRange.EndPort)); err != nil {
return trace.Wrap(err, "validating a port range of a TCP app")
}
}

return nil
}

Expand Down
156 changes: 135 additions & 21 deletions api/types/app_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,58 +29,45 @@ import (
// TestAppPublicAddrValidation tests PublicAddr field validation to make sure that
// an app with internal "kube-teleport-proxy-alpn." ServerName prefix won't be created.
func TestAppPublicAddrValidation(t *testing.T) {
type check func(t *testing.T, err error)

hasNoErr := func() check {
return func(t *testing.T, err error) {
require.NoError(t, err)
}
}
hasErrTypeBadParameter := func() check {
return func(t *testing.T, err error) {
require.True(t, trace.IsBadParameter(err))
}
}

tests := []struct {
name string
publicAddr string
check check
check require.ErrorAssertionFunc
}{
{
name: "kubernetes app",
publicAddr: "kubernetes.example.com:3080",
check: hasNoErr(),
check: hasNoErr,
},
{
name: "kubernetes app public addr without port",
publicAddr: "kubernetes.example.com",
check: hasNoErr(),
check: hasNoErr,
},
{
name: "kubernetes app http",
publicAddr: "http://kubernetes.example.com:3080",
check: hasNoErr(),
check: hasNoErr,
},
{
name: "kubernetes app https",
publicAddr: "https://kubernetes.example.com:3080",
check: hasNoErr(),
check: hasNoErr,
},
{
name: "public address with internal kube ServerName prefix",
publicAddr: constants.KubeTeleportProxyALPNPrefix + "example.com:3080",
check: hasErrTypeBadParameter(),
check: hasErrTypeBadParameter,
},
{
name: "https public address with internal kube ServerName prefix",
publicAddr: "https://" + constants.KubeTeleportProxyALPNPrefix + "example.com:3080",
check: hasErrTypeBadParameter(),
check: hasErrTypeBadParameter,
},
{
name: "addr with numbers in the host",
publicAddr: "123456789012.teleport.example.com:3080",
check: hasNoErr(),
check: hasNoErr,
},
}

Expand All @@ -97,6 +84,112 @@ func TestAppPublicAddrValidation(t *testing.T) {
}
}

func TestAppPortsValidation(t *testing.T) {
tests := []struct {
name string
tcpPorts []*PortRange
uri string
check require.ErrorAssertionFunc
}{
{
name: "valid ranges and single ports",
tcpPorts: []*PortRange{
&PortRange{Port: 22, EndPort: 25},
&PortRange{Port: 26},
&PortRange{Port: 65535},
},
check: hasNoErr,
},
{
name: "valid overlapping ranges",
tcpPorts: []*PortRange{
&PortRange{Port: 100, EndPort: 200},
&PortRange{Port: 150, EndPort: 175},
&PortRange{Port: 111},
&PortRange{Port: 150, EndPort: 210},
&PortRange{Port: 1, EndPort: 65535},
},
check: hasNoErr,
},
{
name: "valid non-TCP app with ports ignored",
uri: "http://localhost:8000",
tcpPorts: []*PortRange{
&PortRange{Port: 123456789},
&PortRange{Port: 10, EndPort: 2},
},
check: hasNoErr,
},
// Test cases for invalid ports.
{
name: "port smaller than 1",
tcpPorts: []*PortRange{
&PortRange{Port: 0},
},
check: hasErrTypeBadParameter,
},
{
name: "port bigger than 65535",
tcpPorts: []*PortRange{
&PortRange{Port: 78787},
},
check: hasErrTypeBadParameter,
},
{
name: "end port smaller than 2",
tcpPorts: []*PortRange{
&PortRange{Port: 5, EndPort: 1},
},
check: hasErrTypeBadParameterAndContains("end port must be between 6 and 65535"),
},
{
name: "end port bigger than 65535",
tcpPorts: []*PortRange{
&PortRange{Port: 1, EndPort: 78787},
},
check: hasErrTypeBadParameter,
},
{
name: "end port smaller than port",
tcpPorts: []*PortRange{
&PortRange{Port: 10, EndPort: 5},
},
check: hasErrTypeBadParameterAndContains("end port must be between 11 and 65535"),
},
{
name: "uri specifies port",
uri: "tcp://localhost:1234",
tcpPorts: []*PortRange{
&PortRange{Port: 1000, EndPort: 1500},
},
check: hasErrTypeBadParameterAndContains("must not include a port number"),
},
{
name: "invalid uri",
uri: "%",
tcpPorts: []*PortRange{
&PortRange{Port: 1000, EndPort: 1500},
},
check: hasErrAndContains("invalid URL escape"),
},
}

for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
spec := AppSpecV3{
URI: "tcp://localhost",
TCPPorts: tc.tcpPorts,
}
if tc.uri != "" {
spec.URI = tc.uri
}

_, err := NewAppV3(Metadata{Name: "TestApp"}, spec)
tc.check(t, err)
})
}
}

func TestAppServerSorter(t *testing.T) {
t.Parallel()

Expand Down Expand Up @@ -469,3 +562,24 @@ func TestNewAppV3(t *testing.T) {
})
}
}

func hasNoErr(t require.TestingT, err error, msgAndArgs ...interface{}) {
require.NoError(t, err, msgAndArgs...)
}

func hasErrTypeBadParameter(t require.TestingT, err error, msgAndArgs ...interface{}) {
require.True(t, trace.IsBadParameter(err), "expected bad parameter error, got %+v", err)
}

func hasErrTypeBadParameterAndContains(msg string) require.ErrorAssertionFunc {
return func(t require.TestingT, err error, msgAndArgs ...interface{}) {
require.True(t, trace.IsBadParameter(err), "err should be trace.BadParameter")
require.ErrorContains(t, err, msg, msgAndArgs...)
}
}

func hasErrAndContains(msg string) require.ErrorAssertionFunc {
return func(t require.TestingT, err error, msgAndArgs ...interface{}) {
require.ErrorContains(t, err, msg, msgAndArgs...)
}
}
Loading

0 comments on commit 87320df

Please sign in to comment.