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

Revert naming clientName -> clusterName #15772

Merged
merged 4 commits into from Oct 18, 2019

Conversation

@kwart
Copy link
Contributor

kwart commented Oct 15, 2019

The group name in client configuration was first renamed to cluster name and subsequently to the client-name (see #15651) as part of security config changes in Hazelcast 4.
After the discussion with the client team, we are reverting the name to the cluster name and a new field will be introduced into the client protocol authentication messages (hazelcast/hazelcast-client-protocol#248).
The cluster name sent by the client will be used by members to verify the connecting party when the security is not enabled on the members.

@kwart kwart added this to the 4.0 milestone Oct 15, 2019
@kwart kwart requested a review from sancar Oct 15, 2019
@kwart kwart requested a review from hazelcast/clients as a code owner Oct 15, 2019
@kwart kwart self-assigned this Oct 15, 2019
@kwart kwart changed the title Security cluster name on client Revert naming clientName -> clusterName Oct 15, 2019
@kwart kwart requested review from petrpleshachkov and vojtechtoman Oct 15, 2019
* credential object is necessary for the client's authentication, then identity configuration within
* {@link ClientSecurityConfig} class should be used. The default authentication method on client protocol just compares
* provided {@code clientName} against the {@code clusterName} defined in the Hazelcast member's configuration.
* Returns the configured cluster name. The name is send as part of client authentication message and may be verified on the

This comment has been minimized.

Copy link
@vojtechtoman

vojtechtoman Oct 15, 2019

Contributor

Minor: The name is sent...

@@ -58,13 +58,20 @@ private ClientAuthenticationCodec() {
@edu.umd.cs.findbugs.annotations.SuppressFBWarnings({"URF_UNREAD_PUBLIC_OR_PROTECTED_FIELD"})
public static class RequestParameters {

/**
* Cluster name that will client connect to.

This comment has been minimized.

Copy link
@vojtechtoman

vojtechtoman Oct 15, 2019

Contributor

Minor: "...that client will connect to"?

@@ -58,6 +58,11 @@ private ClientAuthenticationCustomCodec() {
@edu.umd.cs.findbugs.annotations.SuppressFBWarnings({"URF_UNREAD_PUBLIC_OR_PROTECTED_FIELD"})
public static class RequestParameters {

/**
* Cluster name that will client connect to.

This comment has been minimized.

Copy link
@vojtechtoman

vojtechtoman Oct 15, 2019

Contributor

Minor: "...that client will connect to"?

@@ -79,10 +79,10 @@
</config-replacers>

<!--
Specifies the client name. It's a name used for client authentication to the cluster and its value is
the same as cluster name by default.
Specifies the cluster name. It's send as part of the client authentication authentication message to Hazelcast

This comment has been minimized.

Copy link
@vojtechtoman

vojtechtoman Oct 15, 2019

Contributor

Minor: authentication authentication

@kwart kwart force-pushed the kwart:security-cluster-name-on-client branch 2 times, most recently from d30be65 to f44de29 Oct 15, 2019
@sancar
sancar approved these changes Oct 16, 2019
@sancar sancar self-requested a review Oct 17, 2019
@sancar
sancar approved these changes Oct 17, 2019
@kwart kwart force-pushed the kwart:security-cluster-name-on-client branch 3 times, most recently from 8fe741d to 313045d Oct 17, 2019
@kwart kwart force-pushed the kwart:security-cluster-name-on-client branch from 313045d to e7ab648 Oct 17, 2019
@@ -176,7 +176,7 @@ private void connectToClusterInternal() {
while (discoveryService.hasNext() && client.getLifecycleService().isRunning()) {
CandidateClusterContext candidateClusterContext = discoveryService.next();
beforeClusterSwitch(candidateClusterContext);
logger.info("Trying to connect to next cluster with client name: " + candidateClusterContext.getClientName());
logger.info("Trying to connect to next cluster with client name: " + candidateClusterContext.getClusterName());

This comment has been minimized.

Copy link
@petrpleshachkov

petrpleshachkov Oct 18, 2019

Contributor

Minor: "with cluster name"

@@ -36,23 +36,27 @@

/**
* Creates member {@link LoginContext}.
*
* @param clusterName TODO

This comment has been minimized.

Copy link
@petrpleshachkov

petrpleshachkov Oct 18, 2019

Contributor

add Javadoc?

@@ -79,10 +79,10 @@
</config-replacers>

<!--
Specifies the client name. It's a name used for client authentication to the cluster and its value is
the same as cluster name by default.
Specifies the cluster name. It's send as part of the client authentication message to Hazelcast

This comment has been minimized.

Copy link
@petrpleshachkov

petrpleshachkov Oct 18, 2019

Contributor

Minor: sent?

(byte) 1, "abc", "xxx", new ArrayList<>(), -1, null);
private ClientMessage createAuthenticationMessage(HazelcastInstance hz, String clientName) {
return ClientAuthenticationCodec.encodeRequest(hz.getConfig().getClusterName(), null, null, UUID.randomUUID(), "FOO",
(byte) 1, clientName, "xxx", new ArrayList<>(), -1, null);

This comment has been minimized.

Copy link
@petrpleshachkov

petrpleshachkov Oct 18, 2019

Contributor

Use emptyList() instead new ArrayList<>()

Copy link
Contributor

petrpleshachkov left a comment

LGTM, minor things to fix.

@kwart

This comment has been minimized.

Copy link
Contributor Author

kwart commented Oct 18, 2019

Unrelated test failure. Merging.

@kwart kwart merged commit 572c748 into hazelcast:master Oct 18, 2019
1 check failed
1 check failed
default Test `FAIL`ed. http://jenkins.hazelcast.com/job/Hazelcast-pr-builder/3922/
Details
@kwart kwart deleted the kwart:security-cluster-name-on-client branch Oct 18, 2019
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.