Fix GRPC address assignment to localhost by default

[namannandan]

Description

In the current implementation, the configured GRPC port is by default associated with the wildcard address

serve/frontend/server/src/main/java/org/pytorch/serve/ModelServer.java

Line 450 in cdba0fd

NettyServerBuilder.forPort(configManager.getGRPCPort(connectorType))

$ torchserve --ncs --start --model-store ./model-store

$ netstat -an | grep 7070
Proto Recv-Q Send-Q  Local Address          Foreign Address        (state)
tcp46      0      0  *.7070                 *.*                    LISTEN

$ netstat -an | grep 7071
Proto Recv-Q Send-Q  Local Address          Foreign Address        (state)
tcp46      0      0  *.7071                 *.*                    LISTEN

Note that the grpc ports 7070 and 7071 are associated with the wildcard address *.

With the fix in this PR, the grpc ports are associated with localhost(127.0.0.1) by default.

$ torchserve --ncs --start --model-store ./model-store

$ netstat -an | grep 7070
Proto Recv-Q Send-Q  Local Address          Foreign Address        (state)
tcp46      0      0  127.0.0.1.7070                 *.*                    LISTEN

$ netstat -an | grep 7071
Proto Recv-Q Send-Q  Local Address          Foreign Address        (state)
tcp46      0      0  127.0.0.1.7071                 *.*                    LISTEN

In case of docker, the grpc address will need to be configured to 0.0.0.0 similar to the http management and inference addresses because otherwise, the grpc endpoint will not be accessible from outside the container:

serve/docker/config.properties

Lines 1 to 2 in cdba0fd

	inference_address=http://0.0.0.0:8080
	management_address=http://0.0.0.0:8081

Documentation already includes best practice to bind localhost ports to the ports exposed by docker to ensure access only to the host on which the container is running.
https://github.com/pytorch/serve/tree/master/docker#security-guideline

Type of change

Please delete options that are not relevant.

• Bug fix (non-breaking change which fixes an issue)

Feature/Issue validation/testing

• CI

• Manual Test

$ torchserve --ncs --start --model-store ./model-store

$ python ts_scripts/torchserve_grpc_client.py register densenet161
## Check densenet161.mar in mar_set : set()
## Register marfile: https://torchserve.s3.amazonaws.com/mar_files/densenet161.mar

Model densenet161 registered successfully

$ python ts_scripts/torchserve_grpc_client.py infer densenet161 examples/image_classifier/kitten.jpg
{
  "tabby": 0.46661901473999023,
  "tiger_cat": 0.46449053287506104,
  "Egyptian_cat": 0.06614045053720474,
  "lynx": 0.0012924427865073085,
  "plastic_bag": 0.00022909742256160825
}

$ python ts_scripts/torchserve_grpc_client.py unregister densenet161
Model densenet161 unregistered successfully

[msaroufim]

should we be more explicit in the if else that the second clause is only for inference?

[msaroufim]

how is the port set now?

[namannandan]

It uses the default values from here:

serve/frontend/server/src/main/java/org/pytorch/serve/util/ConfigManager.java

Lines 360 to 368 in 89c5389

	public int getGRPCPort(ConnectorType connectorType) {
	String port;
	if (connectorType == ConnectorType.MANAGEMENT_CONNECTOR) {
	port = prop.getProperty(TS_GRPC_MANAGEMENT_PORT, "7071");
	} else {
	port = prop.getProperty(TS_GRPC_INFERENCE_PORT, "7070");
	}
	return Integer.parseInt(port);
	}

And is called here: https://github.com/pytorch/serve/pull/3083/files#diff-45f9e03a9d3046ef0e2c18e18b0bb9c1889f9e3ff9918568870539cc926e3895R454

[namannandan]

Added configuration option to set grpc address separately from port to maintain backwards compatibility to retain the grpc port configuration:

serve/frontend/server/src/main/java/org/pytorch/serve/util/ConfigManager.java

Lines 104 to 105 in 89c5389

	private static final String TS_GRPC_INFERENCE_PORT = "grpc_inference_port";
	private static final String TS_GRPC_MANAGEMENT_PORT = "grpc_management_port";

[agunapal]

can we keep this consistent?
grpc_inference_address=http://0.0.0.0:7070
grpc_management_address=http://0.0.0.0:7071

[namannandan]

It is possible to support configuration format as grpc_inference_address=http://0.0.0.0:7070 but we've had support to configure the GRPC ports alone since v0.3.0:

serve/frontend/server/src/main/java/org/pytorch/serve/util/ConfigManager.java

Lines 104 to 105 in 89c5389

	private static final String TS_GRPC_INFERENCE_PORT = "grpc_inference_port";
	private static final String TS_GRPC_MANAGEMENT_PORT = "grpc_management_port";

To maintain backwards compatibility, I believe we'll need to retain the grpc port configuration.

If we were to allow specifying the port along with the address as grpc_inference_address=http://0.0.0.0:7070, I believe we'll have two options:

1. Ignore the grpc port configuration ex grpc_inference_port if grpc_inference_address includes the port already.
2. Override the port specified in grpc_inference_address with the port specified in grpc_inference_port.

Another option is to potentially rename grpc_inference_address to grpc_inference_ip_address to the make the configuration name more explicit to convey what the value is intended to be.

Let me know your thoughts or suggestions on better ways to handle it.