Skip to content

Conversation

@skade
Copy link
Contributor

@skade skade commented Jul 20, 2012

This concerns #664 . This pull request enables a plugin to implement any additional functionality on the transport handlers by itself by providing a different PipelineFactories.

A proof-of-concept plugin can be found here:
https://github.com/Asquera/elasticsearch-ssl-transport-plugin

The plugin is much lighter on the side of configurability, where #2105 does a much better job.

As a side-effect of the change, I also had to un-minify netty, because it was a huge pain to compile the plugin properly with SslHandler etc. living in a different (non-relocated) package.

Implementing SSL as a plugin would allow the plugin to be iterated independently of elasticsearch master, which, after thinking alot about what you can do after enabling SSL (different authentication strategies, etc).

skade added 2 commits July 20, 2012 12:25
This patch allows plugins to replace NettyTransports pipeline
factories. This allows things like SSL support to be implemented
in plugins instead of core elasticsearch.

This patch assumes that PipelineFactories will be replaced by
a subclass.
This makes life a lot harder for plugins wanting to use parts
of the vendored api. E.g. all SSL classes would be stripped from
netty, but at the same time be relocated into a new package,
making it hard to use those parts from the "real netty" later
on as the package differs.

This requires version 1.6 of the shade plugin to work.
@kimchy
Copy link
Member

kimchy commented Jul 21, 2012

See the comment I made on #2105. I don't really see a reason why extensions points should be written as netty pipeline handlers. SSL is a feature that should be in ES, not as a plugin, and on top of that, being able to "plug" custom logic should go on the transport layer, not on the netty layer, so it will be cross transport implementation.

@skade
Copy link
Contributor Author

skade commented Jul 22, 2012

This sounds reasonable, as introducing your own handler is really an odd edge case. Thank you for clarifying.

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.

2 participants