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

Corrected port number validation range #304

Merged
merged 10 commits into from
May 21, 2020
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ public static Connector parse(String binding, boolean management) {
} else {
port = Integer.parseInt(listeningPort);
}
if (port >= Short.MAX_VALUE) {
if (port >= Short.MAX_VALUE * 2 + 1) {
throw new IllegalArgumentException("Invalid port number: " + binding);
}
return new Connector(port, false, host, String.valueOf(port), ssl, management);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import java.util.List;
import java.util.Properties;
import java.util.concurrent.CountDownLatch;
import org.apache.commons.io.FileUtils;
import org.apache.commons.io.IOUtils;
import org.pytorch.serve.http.DescribeModelResponse;
import org.pytorch.serve.http.ErrorResponse;
Expand Down Expand Up @@ -94,7 +95,8 @@ public void afterSuite() {
@Test
public void test()
throws InterruptedException, HttpPostRequestEncoder.ErrorDataEncoderException,
IOException, NoSuchFieldException, IllegalAccessException {
IOException, NoSuchFieldException, IllegalAccessException,
GeneralSecurityException, InvalidSnapshotException {
Channel channel = null;
Channel managementChannel = null;
for (int i = 0; i < 5; ++i) {
Expand Down Expand Up @@ -189,6 +191,8 @@ public void test()
testUnregisterModelFailure("noopversioned", "1.2.1");

testTS();
testTSValidPort();
testTSInvalidPort();
}

public void testTS()
Expand Down Expand Up @@ -1244,4 +1248,65 @@ private void testMetricManager() throws JsonParseException, InterruptedException
}
}
}

private void testTSValidPort()
throws InterruptedException, GeneralSecurityException, InvalidSnapshotException,
IOException {
// test case for verifying port range refer https://github.com/pytorch/serve/issues/291
server.stop();
System.setProperty("tsConfigFile", "src/test/resources/config_port.properties");
FileUtils.deleteQuietly(new File(System.getProperty("LOG_LOCATION"), "config"));
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am curious to know what this file is, that we are deleting.

Copy link
Contributor

Choose a reason for hiding this comment

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

Due to snapshot feature, the first preference will be given to last available snapshot file instead of the properties file set in the system properties. Thus cleaning up the generated snapshot before restart.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is snapshot given a preference over the runtime configuration? Whats the order of preference for this?

Copy link
Contributor

Choose a reason for hiding this comment

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

The preference order for properties file is as follows :

  • System environment
  • Command line argument
  • Last snapshot
  • Java System Property.

ConfigManager.init(new ConfigManager.Arguments());
configManager = ConfigManager.getInstance();
PluginsManager.getInstance().initialize();

InternalLoggerFactory.setDefaultFactory(Slf4JLoggerFactory.INSTANCE);
server = new ModelServer(configManager);
server.start();

Channel channel = null;
Channel managementChannel = null;
for (int i = 0; i < 5; ++i) {
channel = TestUtils.connect(false, configManager);
if (channel != null) {
break;
}
Thread.sleep(100);
}

for (int i = 0; i < 5; ++i) {
managementChannel = TestUtils.connect(true, configManager);
if (managementChannel != null) {
break;
}
Thread.sleep(100);
}

Assert.assertNotNull(channel, "Failed to connect to inference port.");
Assert.assertNotNull(managementChannel, "Failed to connect to management port.");

testPing(channel);
}

private void testTSInvalidPort()
throws InterruptedException, GeneralSecurityException, InvalidSnapshotException,
IOException {
// test case for verifying port range refer https://github.com/pytorch/serve/issues/291
// invalid port test
server.stop();
System.setProperty("tsConfigFile", "src/test/resources/config_invalid_port.properties");
Copy link
Collaborator

Choose a reason for hiding this comment

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

I was thinking about this test a little more. Since this test is only testing hte JAVA code path, couldn't we simply modify the TS_INFERENCE_ADDRESS property in ConfigManager and restart the server? Do we need this additional config.properties file?

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

This is a much simpler approach. Thanks.

FileUtils.deleteQuietly(new File(System.getProperty("LOG_LOCATION"), "config"));
ConfigManager.init(new ConfigManager.Arguments());
configManager = ConfigManager.getInstance();
PluginsManager.getInstance().initialize();

InternalLoggerFactory.setDefaultFactory(Slf4JLoggerFactory.INSTANCE);
server = new ModelServer(configManager);
try {
server.start();
} catch (Exception e) {
Assert.assertEquals(e.getClass(), IllegalArgumentException.class);
Assert.assertEquals(e.getMessage(), "Invalid port number: https://127.0.0.1:65536");
}
}
}
1 change: 0 additions & 1 deletion frontend/server/src/test/resources/config.properties
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ certificate_file=src/test/resources/certs.pem
# max_response_size=6553500
max_request_size=10485760
# blacklist_env_vars=.*USERNAME.*|.*PASSWORD.*
# enable_env_vars_config=false
maaquib marked this conversation as resolved.
Show resolved Hide resolved
# decode_input_request=true
enable_envvars_config=true
# default_service_handler=/path/to/service.py:handle
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
inference_address=https://127.0.0.1:65536
management_address=unix:/tmp/management.sock
# model_server_home=../..
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can remove the commented properties in test configurations.

model_store=../modelarchive/src/test/resources/models
enable_envvars_config=true
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need inference_address to be true? The configuration seems to be set in the properties file itself.

4 changes: 4 additions & 0 deletions frontend/server/src/test/resources/config_port.properties
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
inference_address=https://127.0.0.1:42523
management_address=unix:/tmp/management.sock
model_store=../modelarchive/src/test/resources/models
enable_envvars_config=true
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,5 @@ certificate_file=src/test/resources/certs.pem
# max_response_size=6553500
max_request_size=10485760
# blacklist_env_vars=.*USERNAME.*|.*PASSWORD.*
# enable_env_vars_config=false
# decode_input_request=true
# enable_envvars_config=false
# decode_input_request=true