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.

Sign up for GitHub

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

Jump to bottom

Corrected port number validation range #304

Merged
merged 10 commits into from
May 21, 2020
Merged

Corrected port number validation range #304

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
368910c
Corrected port number validation range
shivamshriwas May 6, 2020
da29522
added UT for issue 291
shivamshriwas May 6, 2020
f1f1fb1
added ut dependency file formatting
shivamshriwas May 6, 2020
2eeea60
correct param in config_test_env.properties
shivamshriwas May 6, 2020
18d6715
removed incorrect flags from test case config files
shivamshriwas May 6, 2020
fef3127
Merge branch 'staging_0_1_1' into issue_291
harshbafna May 7, 2020
ce70699
added negative test cases
shivamshriwas May 7, 2020
ecfd86c
Merge branch 'issue_291' of https://github.com/pytorch/serve into iss…
shivamshriwas May 7, 2020
1c09831
removed properties file and changes in test case
shivamshriwas May 8, 2020
3a9fa73
resolved merge conflicts
shivamshriwas May 21, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion frontend/server/src/main/java/org/pytorch/serve/util/Connector.java
View file
Open in desktop
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
56 changes: 56 additions & 0 deletions frontend/server/src/test/java/org/pytorch/serve/ModelServerTest.java
View file
Open in desktop
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 @@ -1366,6 +1367,61 @@ public void testUnregisterModelFailure() throws InterruptedException {
TestUtils.unregisterModel(channel, "noopversioned", "1.2.1", false);
}

@Test(
alwaysRun = true,
dependsOnMethods = {"testUnregisterModelFailure"})
private void testTSValidPort()
throws InterruptedException, InvalidSnapshotException, GeneralSecurityException,
IOException {
// test case for verifying port range refer https://github.com/pytorch/serve/issues/291
server.stop();
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.setProperty("inference_address", "https://127.0.0.1:42523");
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();
}

@Test(
alwaysRun = true,
dependsOnMethods = {"testTSValidPort"})
private void testTSInvalidPort() {
// test case for verifying port range refer https://github.com/pytorch/serve/issues/291
// invalid port test
server.stop();
FileUtils.deleteQuietly(new File(System.getProperty("LOG_LOCATION"), "config"));
configManager.setProperty("inference_address", "https://127.0.0.1:65536");
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");
}
}

private void testLoadModel(String url, String modelName) throws InterruptedException {
Channel channel = TestUtils.getManagementChannel(configManager);
TestUtils.setResult(null);
Expand Down
1 change: 0 additions & 1 deletion frontend/server/src/test/resources/config.properties
View file
Open in desktop
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
3 changes: 1 addition & 2 deletions frontend/server/src/test/resources/config_test_env.properties
View file
Open in desktop
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