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

changes in websocket connection logic to pass on headers #275

Merged
merged 20 commits into from May 17, 2019

Conversation

@reetchow
Copy link
Contributor

commented Mar 29, 2019

Prior to this change websockets were not working with zlux - in particular the TN3270 app which utilizes websockets to communicate with the backend. What I've done here is pass on the handshake headers which are given from the client to the api-layer onto the backend server with which we're communicating.

The other problem that I fixed here was the service paths. The hard coded service.getServiceUrl() + "/" was yielding a url that looked like ws://localhost:8544//ZLUX/plugins/... The two slashes after the port were causing issues because the service.getServiceUrl() yielded a single slash. In my change I test to see if the last character of the service.getServiceUrl() is a slash and if not append one. Otherwise I just use the serviceUrl as is.

This fixed the issue with the TN3270 app in zlux and is the last piece of the puzzle in the api-layer and zlux integration for now.

Signed-off-by: Reet Chowdhary rchowdhary@rocketsoftware.com

changes in websocket connection logic to pass on headers
Signed-off-by: Reet Chowdhary <rchowdhary@rocketsoftware.com>

@ghost ghost assigned reetchow Mar 29, 2019

@ghost ghost added the review label Mar 29, 2019

@codecov

This comment has been minimized.

Copy link

commented Mar 29, 2019

Codecov Report

Merging #275 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #275   +/-   ##
=========================================
  Coverage     68.47%   68.47%           
  Complexity       12       12           
=========================================
  Files           231      231           
  Lines          4466     4466           
  Branches        561      561           
=========================================
  Hits           3058     3058           
  Misses         1259     1259           
  Partials        149      149

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4a765a9...e570690. Read the comment docs.

@plavjanik

This comment has been minimized.

Copy link
Collaborator

commented Mar 29, 2019

Hello Reet,

thank you for making the change and fixing the problem with the invalid path too.

The Codecov.io plugin complains that the new code is not covered by unit tests. Since we do not take coverage for integrations tests that are used for WebSockets, we do not need to follow it strictly.

I think that you have tested manually using other WebSocket clients.

It would be good to have the test for headers automated and included in the integration tests - https://github.com/zowe/api-layer/blob/master/integration-tests/src/test/java/com/ca/mfaas/ws/WebSocketProxyTest.java

@ilkinabdullayev
Copy link
Collaborator

left a comment

Hello Reet,

Thanks for fixing the problem. Good job! The solution looks good. My comments are about more clean code stuff.

@@ -98,8 +98,10 @@ public void afterConnectionEstablished(WebSocketSession webSocketSession) throws

private void openWebSocketConnection(RoutedService service, ServiceInstance serviceInstance, Object uri,
String path, WebSocketSession webSocketSession) throws IOException {
String serviceUrl = service.getServiceUrl();
String servicePath = serviceUrl.charAt(serviceUrl.length() - 1) == '/' ? serviceUrl : serviceUrl + "/";

This comment has been minimized.

Copy link
@ilkinabdullayev

ilkinabdullayev Mar 29, 2019

Collaborator
Suggested change
String servicePath = serviceUrl.charAt(serviceUrl.length() - 1) == '/' ? serviceUrl : serviceUrl + "/";
String servicePath = serviceUrl.endsWith("/") ? serviceUrl : serviceUrl + "/";

This comment has been minimized.

Copy link
@ilkinabdullayev

ilkinabdullayev Mar 29, 2019

Collaborator

Firstly, thank you for making the change. I would suggest extract target URL generation piece to the new method by clean code perspective :)

private String getTargetUrl(String serviceUrl, ServiceInstance serviceInstance, String path) {
        String servicePath = serviceUrl.endsWith("/") ? serviceUrl : serviceUrl + "/";
        return (serviceInstance.isSecure() ? "wss" : "ws") + "://" + serviceInstance.getHost() + ":"
                + serviceInstance.getPort() + servicePath + "/" + path;
    }

reetchow added some commits Apr 2, 2019

base integration test for header forwarding
Signed-off-by: Reet Chowdhary <rchowdhary@rocketsoftware.com>
removed incomplete unit test for headers
Signed-off-by: Reet Chowdhary <rchowdhary@rocketsoftware.com>
Remove old not working fix
Signed-off-by: Andrea Tabone <andrea.tabone@ca.com>
@ilkinabdullayev
Copy link
Collaborator

left a comment

Thanks Reet!

@taban03 taban03 merged commit b4bf185 into zowe:master May 17, 2019

3 checks passed

DCO DCO
Details
WIP Ready for review
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details

@ghost ghost removed the review label May 17, 2019

cZikos pushed a commit that referenced this pull request May 23, 2019

changes in websocket connection logic to pass on headers (#275)
* changes in websocket connection logic to pass on headers

Signed-off-by: Reet Chowdhary <rchowdhary@rocketsoftware.com>

* base integration test for header forwarding

Signed-off-by: Reet Chowdhary <rchowdhary@rocketsoftware.com>

* removed incomplete unit test for headers

Signed-off-by: Reet Chowdhary <rchowdhary@rocketsoftware.com>

* first test version

Signed-off-by: Reet Chowdhary <rchowdhary@rocketsoftware.com>

* Remove unused headers

* Check custom header in the WebSocket routing

* updates to test and refactoring ws code

Signed-off-by: Reet Chowdhary <rchowdhary@rocketsoftware.com>

* added access control header as proposed by andrea and ilken

Signed-off-by: Reet Chowdhary <rchowdhary@rocketsoftware.com>

* reformating for squid errors

Signed-off-by: Reet Chowdhary <rchowdhary@rocketsoftware.com>

* Minor refactor

Signed-off-by: JirkaAichler <jiri.aichler@broadcom.com>

* Add check for uri

Signed-off-by: Andrea Tabone <andrea.tabone@ca.com>

* Allow all origin header values

Signed-off-by: Andrea Tabone <andrea.tabone@ca.com>

* Remove old not working fix

Signed-off-by: Andrea Tabone <andrea.tabone@ca.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.