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

Drop support for Kuzzle Proxy #1401

Merged
merged 13 commits into from Aug 26, 2019

Conversation

@scottinet
Copy link
Member

commented Aug 23, 2019

Description

The Kuzzle Proxy was meant as a load balancer for Kuzzle cluster nodes. Since then, Kuzzle cluster has been re-architectured and is now masterless, rendering the Kuzzle Proxy project useless, and allowing users to use existing, more robust and more efficient proxies, such as nginx.

It was still supported throughout the entire v1 version of Kuzzle for backward-compatibility reasons, but it is too costly too maintain for too little a gain, so we decided to end its support.

Changes:

  • Dropped the dual entrypoint handling. The previous "embedded" entrypoint is now the only remaining one (and the "embedded" name has thus been removed)
  • HTTP objects emulating requests coming from the Proxy have been reworked to use a proper class (named HttpMessage), allowing for a better separation of concern, but also to remove superfluous JSON.stringify => JSON.parse processes when receiving pre-parsed JSON objects from HttpDataStream streams.
  • Dropped the internal broker service, as it was used only for communications between Kuzzle nodes and Kuzzle Proxy

Other changes

  • Dropped handling of whitelists and blacklists for service initializations (obsolete and unused for a long time now)
  • Bugfix: HTTP errors (not API errors) weren't correctly forwarded to users (this bugfix will also be implemented in Kuzzle v1)
  • Fix unit tests on HTTP requests that were always passing
@scottinet scottinet self-assigned this Aug 23, 2019
@Aschen Aschen added the 2.x label Aug 23, 2019
Copy link
Member

left a comment

👍 , you should also remove proxy from integration tests stacks and the PROXY environment variable from .travis.yml

@scottinet

This comment has been minimized.

Copy link
Member Author

commented Aug 23, 2019

@alexandrebouthinon > Adrien already did that. :)

@codecov

This comment has been minimized.

Copy link

commented Aug 23, 2019

Codecov Report

Merging #1401 into 2-dev will decrease coverage by 0.05%.
The diff coverage is 98.6%.

Impacted file tree graph

@@            Coverage Diff             @@
##            2-dev    #1401      +/-   ##
==========================================
- Coverage   93.83%   93.78%   -0.06%     
==========================================
  Files         106       99       -7     
  Lines        7386     7014     -372     
==========================================
- Hits         6931     6578     -353     
+ Misses        455      436      -19
Impacted Files Coverage Δ
lib/api/controllers/routerController.js 95% <ø> (ø) ⬆️
lib/config/index.js 100% <ø> (+10.52%) ⬆️
lib/api/kuzzle.js 89.14% <ø> (ø) ⬆️
lib/api/core/entrypoints/clientConnection.js 100% <ø> (ø)
lib/api/core/notifier.js 73.33% <ø> (ø) ⬆️
lib/api/core/entrypoints/protocols/protocol.js 92.85% <ø> (ø)
lib/util/shutdown.js 11.42% <0%> (ø) ⬆️
lib/api/core/entrypoints/protocols/websocket.js 96.95% <100%> (ø)
lib/api/core/janitor.js 89.79% <100%> (ø) ⬆️
lib/services/index.js 100% <100%> (+4.44%) ⬆️
... and 11 more

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 6110163...ea280e3. Read the comment docs.

@codecov

This comment has been minimized.

Copy link

commented Aug 23, 2019

Codecov Report

Merging #1401 into 2-dev will decrease coverage by 0.05%.
The diff coverage is 98.59%.

Impacted file tree graph

@@            Coverage Diff             @@
##            2-dev    #1401      +/-   ##
==========================================
- Coverage   93.83%   93.78%   -0.06%     
==========================================
  Files         106       99       -7     
  Lines        7386     7011     -375     
==========================================
- Hits         6931     6575     -356     
+ Misses        455      436      -19
Impacted Files Coverage Δ
lib/api/controllers/routerController.js 95% <ø> (ø) ⬆️
lib/config/index.js 100% <ø> (+10.52%) ⬆️
lib/api/kuzzle.js 89.14% <ø> (ø) ⬆️
lib/api/core/entrypoints/clientConnection.js 100% <ø> (ø)
lib/api/core/notifier.js 73.33% <ø> (ø) ⬆️
lib/api/core/entrypoints/protocols/protocol.js 92.85% <ø> (ø)
lib/util/shutdown.js 11.42% <0%> (ø) ⬆️
lib/api/core/entrypoints/protocols/websocket.js 96.95% <100%> (ø)
lib/api/core/janitor.js 89.79% <100%> (ø) ⬆️
lib/services/index.js 100% <100%> (+4.44%) ⬆️
... and 11 more

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 ce39d8d...c86eae5. Read the comment docs.

'network',
'entrypoint',
'unknown_event_received',
event

This comment has been minimized.

Copy link
@Aschen

Aschen Aug 23, 2019

Contributor

Nit: wrong wrapping

'network',
'entrypoint',
'invalid_network_port_number',
this.config.port

This comment has been minimized.

Copy link
@Aschen

Aschen Aug 23, 2019

Contributor

Nit: wrong wrapping

models: { RequestContext },
errors: { ServiceUnavailableError }
} = require('kuzzle-common-objects'),
MqttProtocol = require('./protocols/mqtt'),

This comment has been minimized.

Copy link
@Aschen

Aschen Aug 23, 2019

Contributor

Nit: we could have an index.js file exporting the protocols to reduce the number of imports

This comment has been minimized.

Copy link
@scottinet

scottinet Aug 23, 2019

Author Member

I don't see the point.

This comment has been minimized.

Copy link
@Aschen

Aschen Aug 26, 2019

Contributor

As for the Javascript SDK where all the protocols are "requirable" within the same file

      {
        MqttProtocol,
        HttpProtocol,
        SocketIOProtocol,
        WebSocketProtocol
      } = require('./protocols');

This comment has been minimized.

Copy link
@scottinet

scottinet Aug 26, 2019

Author Member

I would have found this useful if protocols were used in more than 1 place, but here we only need them in the entrypoint class.
I still don't see the point of creating a new intermediary JS file used only to regroup a few require calls that are used only by one other file: to me this adds an unneeded complexity.

debug(
'[server] client "%s" joining channel "%s"',
connectionId,
channel

This comment has been minimized.

Copy link
@Aschen

Aschen Aug 23, 2019

Contributor

Nit: Wrong wrapping rule

try {
this.protocols[client.protocol].joinChannel(
channel,
connectionId

This comment has been minimized.

Copy link
@Aschen

Aschen Aug 23, 2019

Contributor

Nit: Wrong wrapping rule

}

_isShuttingDownError (request, cb) {
request.setError(new ServiceUnavailableError('Kuzzle is shutting down'));

This comment has been minimized.

Copy link
@Aschen

Aschen Aug 23, 2019

Contributor

You should use the errorsManager instead

validateMessage (message) {
const contentType = message.headers['content-type'];

if (contentType && !contentType.startsWith('application/json')) {

This comment has been minimized.

Copy link
@Aschen

Aschen Aug 23, 2019

Contributor

Nit (don't know if it's worth it here): String.indexOf is faster than String.startsWith

This comment has been minimized.

Copy link
@scottinet

scottinet Aug 23, 2019

Author Member

It does.

this.data = {
requestId: this.httpRequest.requestId
requestId: message.requestId,
body:message.content,

This comment has been minimized.

Copy link
@Aschen

Aschen Aug 23, 2019

Contributor

Nit

Suggested change
body:message.content,
body: message.content,
};

for (const k of Object.keys(message.headers)) {
if ( k.toLowerCase() === 'authorization'

This comment has been minimized.

Copy link
@Aschen

Aschen Aug 23, 2019

Contributor

Nit

Suggested change
if ( k.toLowerCase() === 'authorization'
if (k.toLowerCase() === 'authorization'

This comment has been minimized.

Copy link
@scottinet

scottinet Aug 23, 2019

Author Member

Check our wrapping rules: we add an extra space when conditions are split to align them.


for (const k of Object.keys(message.headers)) {
if ( k.toLowerCase() === 'authorization'
&& message.headers[k].startsWith('Bearer ')

This comment has been minimized.

Copy link
@Aschen

Aschen Aug 23, 2019

Contributor

Nit: same remark about indexOf (Not sure it's worth it either here)

@scottinet scottinet requested a review from Aschen Aug 23, 2019
@scottinet

This comment has been minimized.

Copy link
Member Author

commented Aug 23, 2019

@Aschen > Applied all your reviews, and then some.

@Aschen
Aschen approved these changes Aug 26, 2019
@scottinet scottinet force-pushed the remove-deprecated-feature/proxy branch from 43e1804 to c86eae5 Aug 26, 2019
Yoann-Abbes added a commit that referenced this pull request Aug 26, 2019
The majority of unit tests on the HTTP router are written in such a way that they can never fail. This PR fixes those tests.

This is an adaptation of some of the changes made #1401
@Yoann-Abbes Yoann-Abbes merged commit 0fc0bd0 into 2-dev Aug 26, 2019
6 of 9 checks passed
6 of 9 checks passed
Header rules No header rules processed
Details
Pages changed 453 new files uploaded
Details
Redirect rules No redirect rules processed
Details
LGTM analysis: JavaScript No new or fixed alerts
Details
Mixed content No mixed content detected
Details
codecov/project Absolute coverage decreased by -0.05% but relative coverage increased by +4.75% compared to ce39d8d
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details
sonarqube SonarQube reported no issues
@Yoann-Abbes Yoann-Abbes deleted the remove-deprecated-feature/proxy branch Aug 26, 2019
Yoann-Abbes added a commit that referenced this pull request Aug 27, 2019
The majority of unit tests on the HTTP router are written in such a way that they can never fail. This PR fixes those tests.

This is an adaptation of some of the changes made #1401
Yoann-Abbes added a commit that referenced this pull request Aug 27, 2019
The majority of unit tests on the HTTP router are written in such a way that they can never fail. This PR fixes those tests.

This is an adaptation of some of the changes made #1401
@Aschen Aschen referenced this pull request Sep 19, 2019
alexandrebouthinon added a commit that referenced this pull request Sep 20, 2019
The Kuzzle Proxy was meant as a load balancer for Kuzzle cluster nodes. Since then, Kuzzle cluster has been re-architectured and is now masterless, rendering the Kuzzle Proxy project useless, and allowing users to use existing, more robust and more efficient proxies, such as nginx.

It was still supported throughout the entire v1 version of Kuzzle for backward-compatibility reasons, but it is too costly too maintain for too little a gain, so we decided to end its support.

Changes:

Dropped the dual entrypoint handling. The previous "embedded" entrypoint is now the only remaining one (and the "embedded" name has thus been removed)
HTTP objects emulating requests coming from the Proxy have been reworked to use a proper class (named HttpMessage), allowing for a better separation of concern, but also to remove superfluous JSON.stringify => JSON.parse processes when receiving pre-parsed JSON objects from HttpDataStream streams.
Dropped the internal broker service, as it was used only for communications between Kuzzle nodes and Kuzzle Proxy
Other changes
Dropped handling of whitelists and blacklists for service initializations (obsolete and unused for a long time now)
Bugfix: HTTP errors (not API errors) weren't correctly forwarded to users (this bugfix will also be implemented in Kuzzle v1)
Fix unit tests on HTTP requests that were always passing
Aschen added a commit that referenced this pull request Sep 20, 2019
# [2.0.0-rc1](https://github.com/kuzzleio/kuzzle/releases/tag/2.0.0-rc1) (2019-09-19)


#### Breaking changes

- [ [#1472](#1472) ] Remove CLI commands   ([Aschen](https://github.com/Aschen))
- [ [#1446](#1446) ] Refactor controllers   ([Aschen](https://github.com/Aschen))
- [ [#1447](#1447) ] Remove pending notifications   ([scottinet](https://github.com/scottinet))
- [ [#1445](#1445) ] Drop support for documents trashcan, garbage collector, and with the _meta tag   ([scottinet](https://github.com/scottinet))
- [ [#1448](#1448) ] Plugins and protocols manifest files are now required   ([scottinet](https://github.com/scottinet))
- [ [#1436](#1436) ] Refactor services and storage engines   ([Aschen](https://github.com/Aschen))
- [ [#1428](#1428) ] Drop support for permission closures   ([scottinet](https://github.com/scottinet))
- [ [#1397](#1397) ] Adapt Elasticsearch service for ES 7   ([Aschen](https://github.com/Aschen))
- [ [#1401](#1401) ] Drop support for Kuzzle Proxy   ([scottinet](https://github.com/scottinet))
- [ [#1385](#1385) ] Drop Node.js 6 & 8  and use 10 by default   ([Aschen](https://github.com/Aschen))

#### Bug fixes

- [ [#1470](#1470) ] Make the connection headers writable again   ([scottinet](https://github.com/scottinet))
- [ [#1471](#1471) ] Fix CLI start --enable-plugin command   ([alexandrebouthinon](https://github.com/alexandrebouthinon))
- [ [#1467](#1467) ] Support unicode payload in notifications   ([Aschen](https://github.com/Aschen))
- [ [#1456](#1456) ] Fix admin:resetSecurity result typo and documentation   ([alexandrebouthinon](https://github.com/alexandrebouthinon))
- [ [#1416](#1416) ] Fix building KuzzleErrors with non KuzzleError for errorsManager   ([Yoann-Abbes](https://github.com/Yoann-Abbes))
- [ [#1438](#1438) ] Fix hydrating a profile with unexisting role   ([Yoann-Abbes](https://github.com/Yoann-Abbes))
- [ [#1435](#1435) ] Fix cache roles and profiles   ([Yoann-Abbes](https://github.com/Yoann-Abbes))
- [ [#1408](#1408) ] Fix a crash occuring when trying to ping dead sockets   ([scottinet](https://github.com/scottinet))

#### Enhancements

- [ [#1432](#1432) ] Allow uppercased plugins   ([benoitvidis](https://github.com/benoitvidis))
- [ [#1414](#1414) ] Update Node.js version for Docker images   ([alexandrebouthinon](https://github.com/alexandrebouthinon))
- [ [#1410](#1410) ] Add '--enable-plugins' option to 'start' CLI command   ([alexandrebouthinon](https://github.com/alexandrebouthinon))
- [ [#1392](#1392) ] Adapt Bootstrap and PluginBootstrap to support ES7   ([Aschen](https://github.com/Aschen))

#### Others

- [ [#1424](#1424) ] Add cluster plugin to default plugins   ([alexandrebouthinon](https://github.com/alexandrebouthinon))
---
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.