Harmony 1980 - Support for external validation services#707
Conversation
chris-durbin
left a comment
There was a problem hiding this comment.
Looks great - tested successfully and really liked the bash server that printed out the messages received. My only comments are logging related.
| - image/gif | ||
| reprojection: true # The service supports reprojection | ||
| validate_variables: true # Whether to validate the requested variables exist in the CMR. Defaults to true. | ||
| external_validation_url: http://example.com # Optional endpoint to be called to validate the user making a request |
There was a problem hiding this comment.
I'm wondering if we should try to future proof it to allow for multiple validations and different types of validations. e.g.
validations:
- url: !Env ${FOO_ENDPOINT}
- docker: !Env ${FOO_IMAGE}
- code: /app/validations/foo.ts
It's probably fine to keep it just a single URL for now and change it if needed in the future.
There was a problem hiding this comment.
Yeah, I think we will keep it simple for now and support those use cases as needed.
| if (e.response) { | ||
| return next(new ExternalValidationError(e.response.data, e.response.status)); | ||
| } else { | ||
| req.context.logger.error('THROWING 500 ERROR'); |
There was a problem hiding this comment.
Change this to log something like Error calling validation endpoint: ${url}.
| return next(new ExternalValidationError(e.response.data, e.response.status)); | ||
| } else { | ||
| req.context.logger.error('THROWING 500 ERROR'); | ||
| req.context.logger.error(JSON.stringify(e, null, 2)); |
There was a problem hiding this comment.
Remove this log line (it's displaying things we do not want in logs).
| } catch (e) { | ||
| req.context.logger.error('External validation failed'); | ||
| if (e.response) { | ||
| return next(new ExternalValidationError(e.response.data, e.response.status)); |
There was a problem hiding this comment.
We should log the response body and status code the validation URL returned - I didn't see either in the logs.
| if (!url) return next(); | ||
|
|
||
| try { | ||
| await axios.post( |
There was a problem hiding this comment.
I'd like to add an info level timing message for calling the validation URL (see the other places we log durationMs in the code).
| } catch (e) { | ||
| req.context.logger.error('External validation failed'); | ||
| if (e.response) { | ||
| req.context.logger.error(`Validation response: ${JSON.stringify(e.response.data, null, 2)}`); |
There was a problem hiding this comment.
nit: log the response status here as well.
Jira Issue ID
HARMONY-1980
Description
Adds support for external validation for services.
NOTE: When we add an actual external validation for a service we will need to add an environment variable for it and refer to this in services.yml. Since we don't have any yet I have not done this.
Local Test Steps
umm_sentry:This should succeed.
To test in hiab change the line from step 1 to
Then rebuild the harmony image to pick up latest code changes. Then after running hiab repeat steps 2-6.
PR Acceptance Checklist