-
Notifications
You must be signed in to change notification settings - Fork 863
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
Api change #2888
Api change #2888
Conversation
… to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initial review. Added a few questions about the implementation.
return true; | ||
} | ||
|
||
public List<String> parseFile(File tsTokenFile) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
parseFile
is invoked by checkTokenAuthorization
which in turn will be called during all inference and management API calls. Reading from file during each of these calls is going to be expensive. Could we potentially maintain the required data, i.e the tokens and expiration time in memory so that we can readily check against them. Also, we can flush this data to the file using say updateKeyFile
when it is initially generated and when any of the data changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to maintain the data in memory and no longer need to parse the key file.
if (tokenBearer == null) { | ||
throw new InvalidKeyException("NO TOKEN PROVIDED"); | ||
} | ||
String[] arrOfStr = tokenBearer.split(" ", 2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could make parsing the token from the header more robust using a regex pattern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No longer need to parse the key file.
Map<String, ModelServerEndpoint> plugins = | ||
PluginsManager.getInstance().getInferenceEndpoints(); | ||
if (plugins.containsKey("token")) { | ||
configManager.generateKeyFile(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should a new key be generated as long as model server is started?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, when model server is started the key file is generated with the three keys.
|
||
@Endpoint( | ||
urlPattern = "token", | ||
endpointType = EndpointTypes.INFERENCE, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should belong to management endpoint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed
private String managementToken; | ||
|
||
@Override | ||
public void doGet(Request req, Response rsp, Context ctx) throws IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it seems everyone has permission to get/set the server's token by accessing this endpoint. Then, what is the security protection from this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The api can only be accessed with the api key which is held by the owner thus maintaining the security of the api.
} | ||
|
||
public String findManagementKey() { | ||
String userDirectory = System.getProperty("user.dir"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does "user.dir" work for linux, mac, and windows?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it does, "user.dir" returns the current working directory regardless of the operating system.
|
||
public boolean keyFile(String managementToken) throws IOException { | ||
String fileData = " "; | ||
String userDirectory = System.getProperty("user.dir"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
ConfigManager configManager = ConfigManager.getInstance(); | ||
if (isInferenceReq(segments)) { | ||
if (endpointMap.getOrDefault(segments[1], null) != null) { | ||
configManager.checkTokenAuthorization(req, false); | ||
handleCustomEndpoint(ctx, req, segments, decoder); | ||
} else { | ||
configManager.checkTokenAuthorization(req, true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the implementation token authentication should not be spreaded in each endpoint. It should be handled by InvalidRequestHandler.
ConfigManager configManager = ConfigManager.getInstance(); | ||
configManager.checkTokenAuthorization(req, false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
ConfigManager configManager = ConfigManager.getInstance(); | ||
configManager.checkTokenAuthorization(req, true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
|
||
ConfigManager configManager = ConfigManager.getInstance(); | ||
configManager.checkTokenAuthorization(req, true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
|
||
ConfigManager configManager = ConfigManager.getInstance(); | ||
configManager.checkTokenAuthorization(req, false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall great work. Left some comments.
docs/token_authorization_api.md
Outdated
2. Torchserve will enable token authorization if the plugin is provided. In the model server home folder a file `key_file.txt` will be generated. | ||
1. Example key file: | ||
|
||
`Management Key: aadJv_R6 --- Expiration time: 2024-01-16T22:23:32.952499Z` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to make this a json to make is easier machine readable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the key file example still accurate now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also why use an array to store the keys in the json file? Better to use a more structured approach like a dict:
{"management":{"key":"some_key", "expiration_time":"some_timestamp"},"inference":{}, ...etc}
docs/token_authorization_api.md
Outdated
@@ -0,0 +1,39 @@ | |||
# TorchServe token authorization API | |||
|
|||
## Customer Use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about "Basic Configuration" or "Basics"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
noted
docs/token_authorization_api.md
Outdated
2. Inference key: Used for inference apis. Example: | ||
`curl http://127.0.0.1:8080/predictions/densenet161 -T examples/image_classifier/kitten.jpg -H "Authorization: Bearer poZXAlqe"` | ||
3. API key: Used for the token authorization api. Check section 4 for api use. | ||
4. 3 tokens allow the owner with the most flexibility in use and enables them to adapt the tokens to their use. Owners of the server can provide users with the inference token if users should not mess with models. The owner can also provide owners with the management key if owners want users to add and remove models. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need to sell it here. Its also pretty self-explanatory in my opinion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is better to have the full explanation, does not change anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case it should be part of the leading description as it does not fit into the list of keys.
docs/token_authorization_api.md
Outdated
`curl http://localhost:8081/models/densenet161 -H "Authorization: Bearer aadJv_R6"` | ||
2. Inference key: Used for inference apis. Example: | ||
`curl http://127.0.0.1:8080/predictions/densenet161 -T examples/image_classifier/kitten.jpg -H "Authorization: Bearer poZXAlqe"` | ||
3. API key: Used for the token authorization api. Check section 4 for api use. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: forall: api->API
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
@@ -0,0 +1,32 @@ | |||
package org.pytorch.serve.archive.model; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this file in model/s3?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to model folder
} else { | ||
test = "{\n\t\"Error\": " + queryResponse + "\n}\n"; | ||
} | ||
rsp.getOutputStream().write(test.getBytes(StandardCharsets.UTF_8)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we ensure https is enforced and send the new key through this secured channel? That way the caller would already poses the new key and can distribute to users.
} | ||
|
||
public String generateKey() { | ||
byte[] randomBytes = new byte[6]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why only 6 bytes here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To keep the key length short
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And we keep the key length short for saving on bandwidth?
// 2: generates inference key and keeps other 2 the same | ||
public boolean generateKeyFile(Integer keyCase) throws IOException { | ||
String fileData = " "; | ||
String userDirectory = System.getProperty("user.dir") + "/key_file.txt"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to restrict rights for this file to 600 like ssh key files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noted
switch (keyCase) { | ||
case 1: | ||
managementKey = generateKey(); | ||
managementExpirationTime = generateTokenExpiration(timeToExpiration); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
timeToExpiration is class variable, why give it as a parameter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
+ "\nAPI Key: " | ||
+ apiKey | ||
+ "\n"; | ||
Files.write(Paths.get("key_file.txt"), fileData.getBytes()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment about machine readable format
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add test in modelservertest
} | ||
|
||
// checks the token provided in the http with the saved keys depening on parameters | ||
public boolean checkTokenAuthorization(FullHttpRequest req, Integer keyCase) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's better to set keyCase as enum.
ConfigManager configManager = ConfigManager.getInstance(); | ||
configManager.checkTokenAuthorization(req, 2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you create a TokenAuthenticationHandler and add it into the handlerchain so that such call will not be spread everywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noted
if (req.toString().contains("/token")) { | ||
configManager.checkTokenAuthorization(req, 0); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
handleCustomEndpoint(ctx, req, segments, decoder); | ||
} else { | ||
configManager.checkTokenAuthorization(req, 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
docs/token_authorization_api.md
Outdated
|
||
## Customer Use | ||
1. Enable token authorization by adding the provided plugin at start using the `--plugin-path` command. | ||
2. Torchserve will enable token authorization if the plugin is provided. In the model server home folder a file `key_file.txt` will be generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Token.jar set the folder is "user.dir" which can be not model server home folder.
tokenAuthorizationEnabled = true; | ||
} | ||
|
||
public void dumpKeyLogs() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the doc, it seems that dumping key in log file will trigger security concern. Please get security engineer's feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reached out. As of now was told not to include log dumps for the key but will update if any changes.
// 0: token api | ||
// 1: management api | ||
// 2: inference api | ||
public void checkTokenAuthorization(FullHttpRequest req, Integer requestType) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's better to move this func to TokenAuthenticationHandler
} catch (ClassNotFoundException e) { | ||
e.printStackTrace(); | ||
} catch (NoSuchMethodException | ||
| IllegalAccessException | ||
| InstantiationException | ||
| InvocationTargetException e) { | ||
e.printStackTrace(); | ||
} | ||
tokenAuthorizationEnabled = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add log to explicitly notify user that token is not enabled successfully.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noted
private boolean tokenAuthorizationEnabled; | ||
private Class<?> tokenClass; | ||
private Object tokenObject; | ||
private Integer timeToExpiration = 60; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please explicitly add unit in var name. eg. timeToExpirationSec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noted
Map<String, ModelServerEndpoint> plugins = | ||
PluginsManager.getInstance().getManagementEndpoints(); | ||
if (plugins.containsKey("token")) { | ||
configManager.setupTokenClass(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you move this plugin check in PluginManager?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noted
docs/token_authorization_api.md
Outdated
|
||
## Customization | ||
Torchserve offers various ways to customize the token authorization to allow owners to reach the desired result. | ||
1. Time to expiration is set to default at 60 minutes but can be changed in the config.properties by adding `token_expiration`. Ex:`token_expiration=30` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please specify the unit is min in variable (eg, token_expiration_min). It's better to give an example of setting expiration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noted will change the variable to 'token_expiration_min'. The example on how to set the time is in the docs.
String[] segments) | ||
throws ModelException, DownloadArchiveException, WorkflowException, | ||
WorkerInitializationException { | ||
ConfigManager configManager = ConfigManager.getInstance(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"if (tokenEnabled" should happen at the beginning to skip immediately if it is disabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
noted
|
||
|
||
# Set up token plugin | ||
def get_plugin_jar(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can create a jar file in test/resource, and then remove this function to reduce the time for regression test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left the setup in the tests in case future changes to the plugin are made, makes it easier for new tests and changes to be made in the future.
ts/model_server.py
Outdated
try: | ||
os.remove(os.getcwd() + "/key_file.json") | ||
except FileNotFoundError: | ||
print("Token authorization not enabled") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can print delete the file if the file exists.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
noted
if plugin_folder: | ||
cmd.extend(["--plugins-path", plugin_folder]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did you extend torchserve cmd's option?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added plugin option to be passed in as a parameter to the torchserve start command in the test util.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some comments. Especially, the silently failing of the token authentication should be changed into a fatal error.
} else { | ||
checkTokenAuthorization(req, "management"); | ||
if (tokenEnabled) { | ||
ConfigManager configManager = ConfigManager.getInstance(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you clarify what this line does?
If we're leveraging a side effect of this call we should make it either explicit or at least leave a comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No use so removed it.
ts/model_server.py
Outdated
@@ -51,7 +51,7 @@ def start() -> None: | |||
try: | |||
os.remove(os.getcwd() + "/key_file.json") | |||
except FileNotFoundError: | |||
print("Token authorization not enabled") | |||
print("Delete key file if it exists") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For a user this message will be very cryptic as he has no context on this. Also the file should not be there on FileNotFoundError. Better to check silently for the file and delete if present.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The server should delete the file if present but that is assuming that the owner doesnt change directories from where the file was created in the first place. Can change the print output to "If token authorization was enabled, delete key file if still present"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the user moved the files this should not be our concern. In that case we should silently move on. Not seeing the value in the print here. If you leave it its better to use logging.debug instead of print.
docs/token_authorization_api.md
Outdated
2. Inference key: Used for inference apis. Example: | ||
`curl http://127.0.0.1:8080/predictions/densenet161 -T examples/image_classifier/kitten.jpg -H "Authorization: Bearer poZXAlqe"` | ||
3. API key: Used for the token authorization api. Check section 4 for api use. | ||
4. 3 tokens allow the owner with the most flexibility in use and enables them to adapt the tokens to their use. Owners of the server can provide users with the inference token if users should not mess with models. The owner can also provide owners with the management key if owners want users to add and remove models. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case it should be part of the leading description as it does not fit into the list of keys.
} | ||
|
||
public String generateKey() { | ||
byte[] randomBytes = new byte[6]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And we keep the key length short for saving on bandwidth?
docs/token_authorization_api.md
Outdated
2. Torchserve will enable token authorization if the plugin is provided. In the model server home folder a file `key_file.txt` will be generated. | ||
1. Example key file: | ||
|
||
`Management Key: aadJv_R6 --- Expiration time: 2024-01-16T22:23:32.952499Z` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the key file example still accurate now?
docs/token_authorization_api.md
Outdated
2. Torchserve will enable token authorization if the plugin is provided. In the model server home folder a file `key_file.txt` will be generated. | ||
1. Example key file: | ||
|
||
`Management Key: aadJv_R6 --- Expiration time: 2024-01-16T22:23:32.952499Z` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also why use an array to store the keys in the json file? Better to use a more structured approach like a dict:
{"management":{"key":"some_key", "expiration_time":"some_timestamp"},"inference":{}, ...etc}
|
||
if (!setFilePermissions()) { | ||
try { | ||
Files.delete(Paths.get("key_file.txt")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Filename has changed. Better to store the name in a constant somewhere to provide central edit access.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noted, but will still be needed to be changed in multiple places. Will make a variable for the plugin but the frontend will not use it.
} catch (IOException e) { | ||
return false; | ||
} | ||
return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we're silently (only some messages in the log) failing if something goes wrong with the token setup. We should not let torchserve start at all if user requests token auth and this happens as we leave the server (unknowingly) unprotected otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
noted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some comment especially regarding testing. Let me know if you have questions.
ts/model_server.py
Outdated
@@ -51,7 +51,7 @@ def start() -> None: | |||
try: | |||
os.remove(os.getcwd() + "/key_file.json") | |||
except FileNotFoundError: | |||
print("Token authorization not enabled") | |||
print("Delete key file if it exists") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the user moved the files this should not be our concern. In that case we should silently move on. Not seeing the value in the print here. If you leave it its better to use logging.debug instead of print.
docs/token_authorization_api.md
Outdated
|
||
```python | ||
[ | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer a dict instead of a list as the main structure. Otherwise you have to look into the object to see what key you have. (see test where you check if "managemen_key in iterm" etc). Why not {"management": {"key":"..."} etc}? In that case you can easily access the values with
json_content.get("management", {}).get("key", "NOT_PRESENT")
) | ||
time.sleep(5) | ||
print("register reponse") | ||
print(response.text) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to remove the print statements or use logging here.
print("register reponse") | ||
print(response.text) | ||
|
||
result = subprocess.run( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we use cat + stdout here instead of Path(file).read_text() or similar?
PLUGIN_STORE = os.path.join(ROOT_DIR, "plugins-path") | ||
|
||
test_utils.start_torchserve(no_config_snapshots=True, plugin_folder=PLUGIN_STORE) | ||
time.sleep(10) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to wait 10 seconds as start_torchserve already waits for torchserve to start. These fixed waits just slows down our tests.
response = requests.post( | ||
"http://localhost:8081/models", params=params, headers=header | ||
) | ||
time.sleep(5) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this 5 seconds wait for?
# Test describe model API with token enabled | ||
def test_managament_api_with_token(): | ||
test_utils.stop_torchserve() | ||
setup_torchserve() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better to make this a module wide fixture that starts on module load and stops torchserve on cleanup. You can not rely on this test always being run.
header = {"Authorization": "Bearer abcd1234"} | ||
|
||
response = requests.get(f"http://localhost:8081/models/resnet18", headers=header) | ||
time.sleep(5) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to wait here. requests already does this for us.
key = read_key_file("management") | ||
header = {"Authorization": f"Bearer {key}"} | ||
print(key) | ||
response = requests.get("http://localhost:8081/models/resnet18", headers=header) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be part of the fixture as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See this example for module scope fixtures:
def register_model(mar_file_path, model_store, torchserve): |
You should still test management api but this should be independent from setup for the other tests.
|
||
assert management_key != read_key_file("management"), "Key file not updated" | ||
assert response.status_code == 200, "Token check failed" | ||
test_utils.stop_torchserve() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment about module scope fixture for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be good to actually test token expiration as well. Just setup expiration to a low value and run two requests while waiting in-between for token to expire.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some comments
|
||
|
||
@pytest.fixture(scope="module") | ||
def setup_torchserve_expiration(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This potentially tries to start 2 torchserves at a time. Better to move the second one into its own module.
response = requests.get("http://localhost:8081/models/resnet18", headers=header) | ||
assert response.status_code == 200, "Token check failed" | ||
|
||
time.sleep(60) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This adds one minute to the total execution of our tests which is pretty inefficient. Better to wait for less. If we can not give fractional minutes to the frontend we should use seconds instead of minutes.
ts/model_server.py
Outdated
os.remove(os.getcwd() + "/key_file.json") | ||
except FileNotFoundError: | ||
print("Delete key file if it exists") | ||
os.remove(os.getcwd() + "/key_file.json") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fails now if user moved the file. Better use: pathlib.Path(filename).unlink(missing_ok=True)
) | ||
os.chdir(REPO_ROOT) | ||
result = subprocess.run( | ||
f"python ts_scripts/install_from_source", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't like to install_from_source in a test. Better to assume that we're testing the currently installed version (Otherwise each test would have to make sure that the newest version is installed etc.)
plugin_folder = os.path.join(REPO_ROOT, "plugins") | ||
os.makedirs(new_folder_path, exist_ok=True) | ||
os.chdir(plugin_folder) | ||
subprocess.run(["./gradlew", "formatJava"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the format necessary here?
os.makedirs(new_folder_path, exist_ok=True) | ||
os.chdir(plugin_folder) | ||
subprocess.run(["./gradlew", "formatJava"]) | ||
result = subprocess.run(["./gradlew", "build"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
building plugin is okay
jar_path = os.path.join(plugin_folder, "endpoints/build/libs") | ||
jar_file = [file for file in os.listdir(jar_path) if file.endswith(".jar")] | ||
if jar_file: | ||
shutil.move( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably better to just copy as otherwise users have to recompile plugin after they run a test.
MODEL_STORE = os.path.join(ROOT_DIR, "model_store/") | ||
PLUGIN_STORE = os.path.join(ROOT_DIR, "plugins-path") | ||
|
||
test_utils.start_torchserve(no_config_snapshots=True, plugin_folder=PLUGIN_STORE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to set gen_mar=False to avoid unnecessary recreation.
|
||
params = ( | ||
("model_name", "resnet18"), | ||
("url", "resnet-18.mar"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better use mnist model instead of resnet18 to keep lower test runtime on cpu
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM now
The requested changes were addressed.
* token authorization update * update token * token authorization plugin test * fix format * add key file generation at default * fix format * updated token plugin * fixed file delete * fixed imports * added custom expection * fix format * token handler * fix doc * fixed handler * added integration tests * Added integration tests * updating token auth * small changes to token auth * fixing changes * changed keyfile to dictionary and updated readme and tests * remove comments * changes to tests * added config file * reduce time for expiration test * change test to mnist * removing install from src * final test change * Fix spellcheck --------- Co-authored-by: Ubuntu <ubuntu@ip-172-31-11-32.us-west-2.compute.internal> Co-authored-by: Matthias Reso <13337103+mreso@users.noreply.github.com>
Adding plugin and associated code that generate three tokens (1 management 1 inference 1 api) that will then be mandated for all subsequent curl commands. Check the readme for complete information