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

Merged
merged 15 commits into from
Dec 3, 2024
Merged

Add Ports field to app spec #47706

merged 15 commits into from
Dec 3, 2024

Conversation

ravicious
Copy link
Member

@ravicious ravicious commented Oct 18, 2024

This PR makes it possible to configure multi-port TCP apps. This is in accordance with RFD 182. The goal is to be able to define an app with multiple ports like this:

kind: app
version: v3
metadata:
  name: multi-port-app
  labels:
    env: test
spec:
  uri: tcp://localhost
  ports:
    - port: 22
    - port: 443
    - port: 555
      end_port: 679

This PR adds Ports and relevant validation to both api/types and lib/config. The new field is validated no matter if someone attempts to add a dynamic app or include an app in an instance config.

This specific format was chosen after a brief discussion (#46169 (comment)) base on prior art of network policies in Kubernetes.

I'll wait with merging this PR until I have the backend implementation ready.

@ravicious ravicious added do-not-merge no-changelog Indicates that a PR does not require a changelog entry labels Oct 18, 2024
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

api/proto/teleport/legacy/types/types.proto Outdated Show resolved Hide resolved
Copy link

github-actions bot commented Nov 7, 2024

🤖 Vercel preview here: https://docs-84qfg2ra5-goteleport.vercel.app/docs/ver/preview

Copy link
Contributor

@marcoandredinis marcoandredinis left a comment

Choose a reason for hiding this comment

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

I would just simplify the endPort validation, but up to you 👍

api/utils/net/ports.go Outdated Show resolved Hide resolved
api/utils/net/ports.go Outdated Show resolved Hide resolved
@ravicious
Copy link
Member Author

Now that the auth/app service part and the VNet part are both approved, I'll start merging these PRs to master and then open backports that are going to wait for v17.1.

Conflicts:
	api/proto/teleport/legacy/types/types.proto
	api/types/types.pb.go
	integrations/terraform/tfschema/types_terraform.go
Copy link

github-actions bot commented Dec 3, 2024

🤖 Vercel preview here: https://docs-qj6iq7o8e-goteleport.vercel.app/docs

I forgot to update that one.
Copy link

github-actions bot commented Dec 3, 2024

🤖 Vercel preview here: https://docs-m3rv910ui-goteleport.vercel.app/docs

@ravicious ravicious added this pull request to the merge queue Dec 3, 2024
Merged via the queue into master with commit 87320df Dec 3, 2024
44 checks passed
@ravicious ravicious deleted the r7s/ports-app-spec branch December 3, 2024 12:59
@public-teleport-github-review-bot

@ravicious See the table below for backport results.

Branch Result
branch/v17 Failed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/branch/v17 documentation no-changelog Indicates that a PR does not require a changelog entry size/lg
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants