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

Add Ports field to app spec #47706

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
16 changes: 16 additions & 0 deletions api/proto/teleport/legacy/types/types.proto
Original file line number Diff line number Diff line change
Expand Up @@ -993,6 +993,11 @@ message AppSpecV3 {
repeated string RequiredAppNames = 10 [(gogoproto.jsontag) = "required_app_names,omitempty"];
// CORSPolicy defines the Cross-Origin Resource Sharing settings for the app.
CORSPolicy CORS = 11 [(gogoproto.jsontag) = "cors,omitempty"];
// Ports 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 Ports = 12 [(gogoproto.jsontag) = "ports,omitempty"];
ravicious marked this conversation as resolved.
Show resolved Hide resolved
}

// AppServerOrSAMLIdPServiceProviderV1 holds either an AppServerV3 or a SAMLIdPServiceProviderV1 resource (never both).
Expand Down Expand Up @@ -1034,6 +1039,17 @@ 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-65535.
uint32 Port = 1 [(gogoproto.jsontag) = "port"];
// EndPort describes the end of the range, inclusive. It must be between 2-65535 and be greater
// than Port when describing a port range. When describing a single port, it must be set to 0.
uint32 EndPort = 2 [(gogoproto.jsontag) = "end_port,omitempty"];
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe transforming this field into optional field?

I wonder if we shouldn't have

  • Port
  • StartPort
  • EndPort

We validate so you can't specify Port and any of StartPort/Endport as it reads far easier or transform this into a oneof Port vs Range.

Copy link
Member Author

Choose a reason for hiding this comment

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

maybe transforming this field into optional field?

How exactly? My impression was that this field is already optional, as when describing a single port you can omit it. I should change the comment though to spell that out loud explicitly.


Regarding StartPort, I don't think Port & EndPort reads that terribly to warrant using one of the other two options. With three fields in a single struct, there's more invalid states that can be represented. With a oneof, as far as I understand, we'd lose some simplicity in lib/config & lib/service/servicecfg mirroring the structures defined in api/types. oneof in protos necessitates a wrapper struct which we'd have to replicate in those two packages.

Or I guess it could be something like this:

type PortSpec struct {
  Port Port
  PortRange PortRange
} 

…or some variation of it. But then again we'd have more stuff to validate and those three packages would no longer mirror their data structures in this specific case. Which I'm not sure how worthy of a goal it is to strive for, but it seems to me like a trait worth having.

Copy link
Contributor

@tigrato tigrato Oct 21, 2024

Choose a reason for hiding this comment

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

How exactly? My impression was that this field is already optional, as when describing a single port you can omit it. I should change the comment though to spell that out loud explicitly.

Proto3 recently introduced the optional keyword that transforms the field into *fieldType. It's just a simpler way to say it out loud that it's not mandatory.


My idea is to keep it as simple as possible and be able to evolve independently. I don't think it reads bad, I just wonder if we want to evolve them in the future separately but if we don't want to do it, that's fine.
A brief history, network policies already exist in Kubernetes for ~4y when they added support for ranges, so to keep the compatibility, they kept the port and introduced the endPort

}

// 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
68 changes: 65 additions & 3 deletions api/types/app.go
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,10 @@ type Application interface {
GetRequiredAppNames() []string
// GetCORS returns the CORS configuration for the app.
GetCORS() *CORSPolicy
// GetPorts returns port ranges supported by the app to which connections can be forwarded to.
GetPorts() []*PortRange
// SetPorts sets port ranges to which connections can be forwarded to.
SetPorts([]*PortRange)
}

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

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

// SetPorts sets port ranges to which connections can be forwarded to.
func (a *AppV3) SetPorts(ports []*PortRange) {
a.Spec.Ports = 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 +393,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 +416,54 @@ func (a *AppV3) CheckAndSetDefaults() error {
}
}

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

return nil
}

func (a *AppV3) checkPorts() 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("app URI %q must not include a port number when the app spec defines a list of ports", a.Spec.URI)
}

const minPort = 1
const maxPort = 65535
for _, portRange := range a.Spec.Ports {
if portRange.Port < minPort || portRange.Port > maxPort {
return trace.BadParameter("app port must be between %d and %d, but got %d", minPort, maxPort, portRange.Port)
}

if portRange.EndPort != 0 {
if portRange.EndPort < minPort+1 || portRange.EndPort > maxPort {
return trace.BadParameter("app end port must be between %d and %d, but got %d", minPort+1, maxPort, portRange.EndPort)
}

if portRange.EndPort <= portRange.Port {
return trace.BadParameter("app end port must be greater than port (%d vs %d)", portRange.EndPort, portRange.Port)
}
}
}

return nil
}

Expand Down
130 changes: 130 additions & 0 deletions api/types/app_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,136 @@ func TestAppPublicAddrValidation(t *testing.T) {
}
}

func TestAppPortsValidation(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))
}
}
hasErrTypeBadParameterAndContains := func(msg string) check {
return func(t *testing.T, err error) {
require.True(t, trace.IsBadParameter(err), "err should be trace.BadParameter")
require.ErrorContains(t, err, msg)
}
}
hasErrAndContains := func(msg string) check {
return func(t *testing.T, err error) {
require.ErrorContains(t, err, msg)
}
}

tests := []struct {
name string
ports []*PortRange
uri string
check check
}{
{
name: "valid ranges and single ports",
ports: []*PortRange{
&PortRange{Port: 22, EndPort: 25},
&PortRange{Port: 26},
&PortRange{Port: 65535},
},
check: hasNoErr(),
},
{
name: "valid overlapping ranges",
ports: []*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",
ports: []*PortRange{
&PortRange{Port: 123456789},
&PortRange{Port: 10, EndPort: 2},
},
check: hasNoErr(),
},
// Test cases for invalid ports.
{
name: "port smaller than 1",
ports: []*PortRange{
&PortRange{Port: 0},
},
check: hasErrTypeBadParameter(),
},
{
name: "port bigger than 65535",
ports: []*PortRange{
&PortRange{Port: 78787},
},
check: hasErrTypeBadParameter(),
},
{
name: "end port smaller than 2",
ports: []*PortRange{
&PortRange{Port: 5, EndPort: 1},
},
check: hasErrTypeBadParameterAndContains("end port must be between"),
},
{
name: "end port bigger than 65535",
ports: []*PortRange{
&PortRange{Port: 1, EndPort: 78787},
},
check: hasErrTypeBadParameter(),
},
{
name: "end port smaller than port",
ports: []*PortRange{
&PortRange{Port: 10, EndPort: 5},
},
check: hasErrTypeBadParameterAndContains("end port must be greater than port"),
},
{
name: "uri specifies port",
uri: "tcp://localhost:1234",
ports: []*PortRange{
&PortRange{Port: 1000, EndPort: 1500},
},
check: hasErrTypeBadParameterAndContains("must not include a port number"),
},
{
name: "invalid uri",
uri: "%",
ports: []*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",
Ports: tc.ports,
}
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
Loading
Loading