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

Remove need for loopback when using callService, callRootService #319

Closed

Conversation

1000TurquoisePogs
Copy link
Member

@1000TurquoisePogs 1000TurquoisePogs commented Jul 8, 2021

Signed-off-by: 1000TurquoisePogs [email protected]

Proposed changes

callService and callRootService currently do a loopback where there's a request from app-server to itself in order to reach the right part of the code to handle a request. This is overhead and causes ip config complications about how app-server reaches itself.
This PR fixes the problem in two ways.

  1. agent-bound routes no longer need loopback, as the call is direct. This may solve situations such as Problem login into zowe desktop zlux#676
  2. inter-server requests can be handled internally by mimicking a network request, providing the right url, method, headers and body to the router function attached to expressjs

Note 2, inter-server direct routing is disabled here for compatibility. It should be very compatible, but just dont want to disrupt behavior in v1. In zowe v2, we should switch this to enabled by default. The behavior can be enabled by setting the property node.internalRouting=true in the server config.

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)

PR Checklist

Please delete options that are not relevant.

  • If the changes in this PR are meant for the next release / mainline, this PR targets the "staging" branch.
  • My code follows the style guidelines of this project (see: Contributing guideline)
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • New and existing unit tests pass locally with my changes
  • video or image is included if visual changes are made
  • Relevant update to CHANGELOG.md
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works, or describe a test method below

Testing

Case 1)
To test, you should be attempting login to desktop with and without apiml.
Without apiml, the code should contact zss directly. With apiml, it should contact apiml to contact zss.
If login succeeds in both cases, then this is working.

Case 2)
Test inter-plugin service calls using this PR zowe/sample-angular-app#62
I sent a small message in postman and observed the following to prove it was working:

2021-08-02 13:00:58.826 <ZWED:2060> me DEBUG (_zsf.routing,webapp.js:1246) ZWED0198I - : Service called: org.zowe.zlux.sample.angular::callservice, POST /
2021-08-02 13:00:58.826 <ZWED:2060> me INFO (org.zowe.zlux.sample.angular:callservice,callService.js:19) Saw request, method=POST
2021-08-02 13:00:58.833 <ZWED:2060> me DEBUG (_zsf.routing,webapp.js:943) Call internally path=/ZLUX/plugins/org.zowe.zlux.sample.angular/services/hello/1.0.1
2021-08-02 13:00:58.834 <ZWED:2060> me DEBUG (_zsf.routing,webapp.js:1246) ZWED0198I - : Service called: org.zowe.zlux.sample.angular::hello, POST /
2021-08-02 13:00:58.834 <ZWED:2060> me INFO (org.zowe.zlux.sample.angular:hello,helloWorld.js:21) Saw request, method=POST
2021-08-02 13:00:58.835 <ZWED:2060> me INFO (org.zowe.zlux.sample.angular:callservice,callService.js:25) callService Returned: statusCode=200, statusMessage=OK, body length=288

image

Further comments

@ccc13481
Copy link

Hi
Is the a way for me to checkout this fix and try it out?
BR
Frank Allan Rasmussen
Danske Bank

@1000TurquoisePogs 1000TurquoisePogs changed the title Reduce loopback via direct agent contact Remove need for loopback when using callService, callRootService Aug 2, 2021
Copy link
Member

@DivergentEuropeans DivergentEuropeans left a comment

Choose a reason for hiding this comment

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

I'm approving because the code seems to make sense and matches what the description is saying. However, I did not yet test the Sample App & also did not test it with API ML

I did test with internal routing enabled & login still works

this.urlPrefix = urlPrefix;
if (!environment.loopbackConfig.port) {
installLog.severe(`ZWED0003E`, loopbackConfig); //installLog.severe(`loopback configuration not valid,`,loopbackConfig, `loopback calls will fail!`);
}
this.environment = environment;
this.isAgentService = isAgentService;
}
WebServiceHandle.prototype = {
constructor: WebServiceHandle,
Copy link
Member

Choose a reason for hiding this comment

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

The comment below can be updated to include turning off HTTP call to localhost by setting node.internalRouting=true in the server config.

Copy link
Member

Choose a reason for hiding this comment

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

Below line 856

requestOptions.path = this.environment.agentRequestOptions.apimlPrefix + requestOptions.path;
}
httpOrHttps = requestOptions.protocol == 'http:' ? http : https;
console.log(`call agent direct`, requestOptions.path, requestOptions.hostname);
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason why we're using console.log instead of utilLog ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thought I fixed every occurrence. Will fix now.

Copy link
Contributor

@lchudinov lchudinov left a comment

Choose a reason for hiding this comment

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

It works fine both with and without APIML. zowe/sample-angular-app#62 works fine, too. It looks safe to merge after removing(or replacing) console.log.

@1000TurquoisePogs
Copy link
Member Author

Sorry, this was actually the wrong branch to review. #321

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants