api: Strict port number handling and --enable-rpc deprecation (#6459)

* Strict port validation and deprecate --enable-rpc This path implements the proposal in issue 6425 and contains various breaking changes to most users of Skaffold APIs regarding enabling the API. * API is no longer enabled by default for "dev" and "debug" commands. This might be a breaking change for some users who were relying on the behavior. * APIs are only enabled if --rpc-port or --rpc-http-port (or both) are set. These flags no longer have defaults 50051 and 50052. * --enable-rpc flag is now marked as "deprecated" for 1.31 release. It will be removed in 3 months or 1 release (whichever is longer) since the Skaffold API is a beta feature. * If given port is busy, skaffold now fails with an error instead of trying to find an available port and printing it to logs. This patch also fixes the test flakes for the following integration tests (each ran 100 times) by behaving more strictly around busy ports. - TestEventsRPC - TestRunPortForward Signed-off-by: Ahmet Alp Balkan <ahmetb@google.com> * add warning if --enable-rpc but no ports are set Signed-off-by: Ahmet Alp Balkan <ahmetb@google.com> * replace deprecation warning with chosen rpc port Signed-off-by: Ahmet Alp Balkan <ahmetb@google.com> * show warning only if http port is also unspecified that way "-enable-rpc -rpc-http-port=X" users don't see a warning. Signed-off-by: Ahmet Alp Balkan <ahmetb@google.com>
GoogleContainerTools · Sep 14, 2021 · d64637c · d64637c
1 parent a0ea146
commit d64637c
Show file tree

Hide file tree

Showing 18 changed files with 228 additions and 136 deletions.
diff --git a/cmd/skaffold/app/cmd/debug_test.go b/cmd/skaffold/app/cmd/debug_test.go
@@ -40,7 +40,6 @@ func TestNewCmdDebug(t *testing.T) {
 
 		t.CheckDeepEqual(true, opts.Tail)
 		t.CheckDeepEqual(false, opts.Force)
-		t.CheckDeepEqual(true, opts.EnableRPC)
 	})
 }
 

diff --git a/cmd/skaffold/app/cmd/dev_test.go b/cmd/skaffold/app/cmd/dev_test.go
@@ -176,6 +176,5 @@ func TestNewCmdDev(t *testing.T) {
 
 		t.CheckDeepEqual(true, opts.Tail)
 		t.CheckDeepEqual(false, opts.Force)
-		t.CheckDeepEqual(true, opts.EnableRPC)
 	})
 }
diff --git a/cmd/skaffold/app/cmd/flags.go b/cmd/skaffold/app/cmd/flags.go
@@ -26,7 +26,6 @@ import (
 	"github.com/spf13/pflag"
 
 	"github.com/GoogleContainerTools/skaffold/cmd/skaffold/app/flags"
-	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/constants"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/instrumentation"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/output/log"
 )
@@ -170,17 +169,14 @@ var flagRegistry = []Flag{
 		DefinedOn:     []string{"dev", "build", "run", "debug"},
 	},
 	{
-		Name:     "enable-rpc",
-		Usage:    "Enable gRPC for exposing Skaffold events",
-		Value:    &opts.EnableRPC,
-		DefValue: false,
-		DefValuePerCommand: map[string]interface{}{
-			"dev":   true,
-			"debug": true,
-		},
+		Name:          "enable-rpc",
+		Usage:         "Enable gRPC or HTTP APIs for exposing Skaffold events",
+		Value:         &opts.EnableRPC,
+		DefValue:      false,
 		FlagAddMethod: "BoolVar",
 		DefinedOn:     []string{"dev", "build", "run", "debug", "deploy", "render", "apply", "test"},
 		IsEnum:        true,
+		Deprecated:    "flags --rpc-port or --rpc-http-port now imply --enable-rpc=true, so please use only those instead",
 	},
 	{
 		Name:          "wait-for-connection",
@@ -193,7 +189,7 @@ var flagRegistry = []Flag{
 	},
 	{
 		Name:          "event-log-file",
-		Usage:         "Save Skaffold events to the provided file after skaffold has finished executing, requires --enable-rpc=true",
+		Usage:         "Save Skaffold events to the provided file after skaffold has finished executing, requires --rpc-port or --rpc-http-port",
 		Hidden:        true,
 		Value:         &opts.EventLogFile,
 		DefValue:      "",
@@ -202,18 +198,18 @@ var flagRegistry = []Flag{
 	},
 	{
 		Name:          "rpc-port",
-		Usage:         "tcp port to expose event API",
+		Usage:         "tcp port to expose the Skaffold API over gRPC",
 		Value:         &opts.RPCPort,
-		DefValue:      constants.DefaultRPCPort,
-		FlagAddMethod: "IntVar",
+		DefValue:      nil,
+		FlagAddMethod: "Var",
 		DefinedOn:     []string{"dev", "build", "run", "debug", "deploy", "test"},
 	},
 	{
 		Name:          "rpc-http-port",
-		Usage:         "tcp port to expose event REST API over HTTP",
+		Usage:         "tcp port to expose the Skaffold API over HTTP REST",
 		Value:         &opts.RPCHTTPPort,
-		DefValue:      constants.DefaultRPCHTTPPort,
-		FlagAddMethod: "IntVar",
+		DefValue:      nil,
+		FlagAddMethod: "Var",
 		DefinedOn:     []string{"dev", "build", "run", "debug", "deploy", "test"},
 	},
 	{
@@ -601,6 +597,8 @@ func methodNameByType(v reflect.Value) string {
 	switch t {
 	case reflect.Bool:
 		return "BoolVar"
+	case reflect.Int:
+		return "IntVar"
 	case reflect.String:
 		return "StringVar"
 	case reflect.Slice:

diff --git a/cmd/skaffold/app/cmd/init_test.go b/cmd/skaffold/app/cmd/init_test.go
@@ -157,7 +157,7 @@ func TestFlagsToConfigVersion(t *testing.T) {
 
 			// we ignore Skaffold options
 			test.expectedConfig.Opts = capturedConfig.Opts
-			t.CheckErrorAndDeepEqual(test.shouldErr, err, test.expectedConfig, capturedConfig, cmp.AllowUnexported(cfg.StringOrUndefined{}, cfg.BoolOrUndefined{}, cfg.SyncRemoteCacheOption{}))
+			t.CheckErrorAndDeepEqual(test.shouldErr, err, test.expectedConfig, capturedConfig, cmp.AllowUnexported(cfg.StringOrUndefined{}, cfg.BoolOrUndefined{}, cfg.IntOrUndefined{}, cfg.SyncRemoteCacheOption{}))
 		})
 	}
 }
diff --git a/docs/content/en/docs/design/api.md b/docs/content/en/docs/design/api.md
@@ -21,38 +21,36 @@ To retrieve information about the Skaffold pipeline, the Skaffold API provides t
 To control the individual phases of the Skaffold, the Skaffold API provides [fine-grained control]({{< relref "#control-api" >}})
 over the individual phases of the pipeline (build, deploy, and sync).
 
-
 ## Connecting to the Skaffold API
 The Skaffold API is `gRPC` based, and it is also exposed via the gRPC gateway as a JSON over HTTP service.
-The server is hosted locally on the same host where the skaffold process is running, and will serve by default on ports 50051 and 50052.
-These ports can be configured through the `--rpc-port` and `--rpc-http-port` flags.
 
-For reference, we generate the server's [gRPC service definitions and message protos]({{< relref "/docs/references/api/grpc" >}}) as well as the [Swagger based HTTP API Spec]({{< relref "/docs/references/api/swagger" >}}).
+The API can be enabled via setting the `--rpc-port` or `--rpc-http-port` flags (or both)
+depending on whether you want to enable the gRPC API or the HTTP REST API, respectively.
 
 
-### HTTP server
-The HTTP API is exposed on port `50052` by default. The default HTTP port can be overridden with the `--rpc-http-port` flag. 
-If the HTTP API port is taken, Skaffold will find the next available port.
-The final port can be found from Skaffold's startup logs.
+{{< alert title="Note">}}
+The `--enable-rpc` flag is now deprecated in favor of `--rpc-port` and `--rpc-http-port` flags.
+{{</alert>}}
 
-```code
-$ skaffold dev
-WARN[0000] port 50052 for gRPC HTTP server already in use: using 50055 instead
-```
 
-### gRPC Server
+For reference, we generate the server's [gRPC service definitions and message protos]({{< relref "/docs/references/api/grpc" >}}) as well as the [Swagger based HTTP API Spec]({{< relref "/docs/references/api/swagger" >}}).
+
+## gRPC Server
 
-The gRPC API is exposed on port `50051` by default and can be overridden with the `--rpc-port` flag.
-As with the HTTP API, if this port is taken, Skaffold will find the next available port.
-You can find this port from Skaffold's logs on startup.
+The gRPC API can be started by specifying the `--rpc-port` flag. If the specified port is not available, Skaffold will
+exit with failure.
 
-```code
-$ skaffold dev
-WARN[0000] port 50051 for gRPC server already in use: using 50053 instead
-```
+### HTTP server
+
+The HTTP REST API can be started by specifying the `--rpc-http-port` flag. If the specified port is not available,
+Skaffold will exit with failure.
+
+Starting the HTTP REST API will also start the gRPC API as it proxies the requests to the gRPC API. By default, Skaffold
+chooses a random available port for gRPC, but it can be customized (see below).
 
 #### Creating a gRPC Client
-To connect to the `gRPC` server at default port `50051`, create a client using the following code snippet.
+
+To connect to the `gRPC` server at the specified port, create a client using the following code snippet.
 
 {{< alert title="Note" >}}
 The skaffold gRPC server is not compatible with HTTPS, so connections need to be marked as insecure with `grpc.WithInsecure()`