Skip to content

Conversation

@maskit
Copy link
Member

@maskit maskit commented Feb 10, 2018

Motivation

  • Duplicate code makes maintenance hard

Modifications

  • Rename createNettySslContext to createNettySslContextForClient
  • Add createNettySslContextForServer
  • Use createNettySslContextForServer from PulsarChannelInitializer and ServiceChannelInitializer

Result

  • Less duplicate code

@maskit maskit added the type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages label Feb 10, 2018
@maskit maskit added this to the 1.22.0-incubating milestone Feb 10, 2018
@maskit maskit self-assigned this Feb 10, 2018
// allows insecure connection
builder.trustManager(InsecureTrustManagerFactory.INSTANCE);
SslContext sslCtx = builder.clientAuth(ClientAuth.OPTIONAL).build();
SslContext sslCtx = SecurityUtility.createNettySslContextForClient(true,
Copy link
Member Author

Choose a reason for hiding this comment

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

Note that the first argument forallowInsecureConnection is true because the original code always allows insecure connection regardless of configuration. If it wasn't intentional, it need to be fixed.

Copy link
Contributor

Choose a reason for hiding this comment

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

can we also add comment SecurityUtility.createNettySslContextForClient(true\* to allow InsecureConnection*\,..)

@maskit
Copy link
Member Author

maskit commented Feb 10, 2018

I know SecurityUtility class itself should be refactored too but I'll do that later to make this PR simple and easy to review.

Copy link
Contributor

@rdhabalia rdhabalia left a comment

Choose a reason for hiding this comment

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

LGTM..

// allows insecure connection
builder.trustManager(InsecureTrustManagerFactory.INSTANCE);
SslContext sslCtx = builder.clientAuth(ClientAuth.OPTIONAL).build();
SslContext sslCtx = SecurityUtility.createNettySslContextForClient(true,
Copy link
Contributor

Choose a reason for hiding this comment

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

can we also add comment SecurityUtility.createNettySslContextForClient(true\* to allow InsecureConnection*\,..)

Copy link
Contributor

@merlimat merlimat left a comment

Choose a reason for hiding this comment

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

👍

@maskit maskit merged commit 2c0616e into apache:master Feb 11, 2018
@maskit maskit deleted the use_security_utility branch October 23, 2018 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants