Proposal: strict port handling and enable flags for Skaffold API (deprecate --enable-rpc)

[ahmetb]

Problem

Right now our GRPC/HTTP servers try to choose a port that's different than the provided port number if that's busy. This is problematic for a number of reasons:

1. we query the system to find usedPorts, and then make a decision based on that → this is not atomic (something might've bind()ed to that port by then, which makes the tests flaky)
2. potential callers of skaffold who provide a preferred port would not^{[citation needed]} expect skaffold to choose a different port (or they will be told to parse starting gRPC server on port %d) → either case bad UX fo the caller?
3. it's already causing test flakes: --rpc-port should either succeed or fail (TestRunPortForward flake) #6375 [Flake] TestEventsRPC is flaky #6417

Context: I believe this port choosing logic was added in response to #4253.

Proposed Solution

1. Do not start API server by default unless the caller specifies --rpc-port or both --rpc-http-port. (Current behavior: by default dev/run commands start grpc/http ports 50051/50052, and find adjacent available ports if they are not available).

2. Usage of either --rpc-port or --rpc-http-port now imply --enable-rpc=true.

   ⚠️ This means the --enable-rpc flag will be deprecated. All notable customers such as Cloud Code for VSC+IntelliJ are impacted from this.

   ⚠️ This flag will be marked as deprecated in the v1.32 release, and will be removed in v1.33 or in 3 months (whichever is longer, per deprecation policy) since the API is a beta feature.

3. If specified port numbers are busy, Skaffold now fails instead of trying to find an adjacent available port. (Let caller find a free port, they can call random() if they like). We might

Impact on Existing Users

Taken only Cloud Code into consideration:

This is a breaking change for those (1) relying on API servers starting by default and (2) those relying on skaffold's "find an adjacent open port if given port is unavailable" (and parse the actual port from logs). Thankfully Cloud Code is unimpacted:

• CC for VSCode uses only --rpc-http-port and they use a library to find an available port (which uses :0 mechanism, more prone to collisions). They also use --enable-rpc=true which will be deprecated with this proposal.
• CC for IntelliJ uses only --rpc-port and they also scan local ports to find an open port (chooses rand port from an open range, less prone to collisions.) They also use --enable-rpc=true which will be deprecated with this proposal.

In both cases, the port finding is susceptible to a race condition where the caller thinks the found port number is available, and they pass it to Skaffold, but some other process might've started using it. I don't think we can fix this problem (short of using Unix domain sockets and Windows named pipes).

/cc @nkubala
/area cleanup
/kind design discussion

[gsquared94]

My upvote for:

Do not listen on any ports for dev/run commands by default. Bind to ports only if they are specified (that way we can retain 50051/50052) by a caller intending to use these APIs.

AFAIK the API server should only be relevant to skaffold dev and skaffold debug. Having explicit flags only defined for these two commands, and not launching the server for all command execution makes a lot of sense IMO.

[tejal29]

AFAIK the API server should only be relevant to skaffold dev and skaffold debug. Having explicit flags only defined for these two commands, and not launching the server for all command execution makes a lot of sense IMO.

I think the IDEs rely on these flags and use them. You will have to co-ordinate this change with them.

[ahmetb]

Ack. I'll find out who are the 1st-party callers of these servers and coordinate changes.

If they are not specifying --http-port/--rpc-port already, this is a breaking change as it won't start the server by default.

Based on recommendations I am updating the proposal to:

Do not listen on any ports during dev/run commands by default. Ports will start only if the caller specifies --rpc-port (or both --rpc-port and --http-port).

[ahmetb]

Added an Impact on Existing Users section in the proposal.

I think this change is safe to do with the assumption that Cloud Code is the only notable user of the APIs.

[ahmetb]

@gsquared94 @briandealwis If we were to start servers via specifying --rpc-port or --rpc-http-port (or both), should we deprecate the --enable-rpc flag?

It seems like now that the enablement would be implied by setting those other flags, and we no longer would be doing per-command enabling of this flag (for dev,debug), so this --enable-rpc now sounds redundant.

Guidance is appreciated.

[gsquared94]

I think specifying --enable-rpc flag without the --rpc-port or --rpc-http-port could mean skaffold selecting any random port. If we don't want that behavior, we can probably do away with that flag according to the deprecation policy

[ahmetb]

--enable-rpc flag without the --rpc-port or --rpc-http-port could mean skaffold selecting any random port.

@gsquared94 I am thinking this behavior can be acceptable. But we can add strict failures to specifying --enable-rpc without any port flags.

What's worse is specifying ports without --enable-rpc (flag with intention should not be silently ignored). I actually wanted to do this in the PR but I could not figure out where this validation would go, yet.

[ahmetb]

, we can probably do away with that flag according to the deprecation policy

That's the consensus we now reached in our offline meeting!
I'm updating the proposal above accordingly to note the deprecation.