[v17] Add multi-port support for TCP apps #49711
Open
+8,869
−6,829
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ravicious
force-pushed
the
r7s/v17/backport-multi-port-backend
branch
from
29c4a88 to
ba7e743
Compare
github-actions
bot
added
application-access
backport
documentation
rfd
github-actions
bot
requested review from
espadolini,
marcoandredinis,
nklaassen,
rosstimothy and
tigrato
marcoandredinis
approved these changes
Dec 3, 2024
nklaassen
approved these changes
Dec 3, 2024
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
* 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
…ti-port TCP apps * Add TargetPort to RouteToApp and AppMetadata proto messages * Pass TargetPort during cert generation * Refactor Pack.makeTLSConfig to accept struct This will make it easier to add targetPort to it. * Add labels to UUIDs used by appaccess test pack app servers This makes them easier to distinguish when routing doesn't work as expected. * Refactor Pack.CreateAppSession to accept a struct * TestTCP: Create app session within test If we kept the old code, we'd need to manually create a session for each target port, which would create a lot of duplication. * Prepare integration test fixtures for multi-port tests * Add api/utils/net.IsPortInRange * Use TargetPort when routing TCP connections * Inline dialMultiPortTCPApp, centralize logic * Check target port when connecting to single-port app * Reorder check in IsPortInRange * Use int instead of uint16 * Extract picking dialTarget to separate function * Improve err msg for single-port apps when targetPort != uriPort * Fix unnecessary conversion to int
* Prepare app specs in tests for specifying TCP ports * Refactor logging in lib/vnet/app_resolver.go Use libutils.NewPackageLogger, call it log instead of slog which makes it harder to use the imported default slog logger instead of the one from a struct. Move creation of logger within TCPAppResolver.resolveTCPHandlerForCluster * Pass port from VNet to local proxy * Don't create another package logger * Don't pass logger to newTCPAppHandler * Fix typo in comment * Explicitly pass port to dialHost * Convert multi-line definitions of simple appSpecs to single-line * Add TODO comment about validating local port * Empty commit to trigger CI
ravicious
force-pushed
the
r7s/v17/backport-multi-port-backend
branch
from
0680d31 to
785004c
Compare
This comment was marked as off-topic.
This comment was marked as off-topic.
|
@ravicious - this PR will require admin approval to merge due to its size. Consider breaking it up into a series smaller changes. |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Backports:
TargetPorttoRouteToApp& use it to route connections to multi-port TCP apps #49047This is the "backend" part of multi-port support. The only conflicts were in protos, since #49300 adds
AppIdentityCentertoAppSpecV3under the index of 12 but it hasn't been merged yet (I guess it waits for v17.1 too).changelog: Added support for multiple ports to TCP applications