kamilla1201 commented on a change in pull request #6714:
URL: https://github.com/apache/geode/pull/6714#discussion_r733848832
##########
File path:
geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/gms/MemberIdentifierImpl.java
##########
@@ -693,10 +701,43 @@ public void fromData(DataInput in,
// nope - it's from a pre-GEODE client or WAN site
}
}
+ convertIpToHostnameIfNeeded();
+ }
+
+ public void fromDataPre_GEODE_1_15_0_0(DataInput in, DeserializationContext
context)
+ throws IOException, ClassNotFoundException {
+ readMemberData(in);
+ memberData.readAdditionalData(in);
+ convertIpToHostnameIfNeeded();
}
public void fromDataPre_GFE_9_0_0_0(DataInput in, DeserializationContext
context)
throws IOException {
+ readMemberData(in);
+ convertIpToHostnameIfNeeded();
+ }
+
+ /**
+ * In older versions of Geode {@link MemberData#getHostName()} can return an
IP address
+ * if network partition is enabled. Also, older versions of Geode don't use
bind-address for
+ * SNI endpoint identification and do a reverse lookup to find the fully
qualified hostname.
+ * This can become a problem if TLS certificate doesn't have the fully
qualified name in it
+ * as a Subject Alternate Name, therefore this behavior was changed to
preserve the bind-address.
+ *
+ * During a rolling upgrade, we need to ensure that {@link
MemberData#getHostName()} returns
+ * a hostname because it might be later used for SNI endpoint identification.
+ */
+ private void convertIpToHostnameIfNeeded() {
+ if (memberData.isNetworkPartitionDetectionEnabled()) {
Review comment:
It would be good to check if SSL endpoint identification is enabled but
there's no easy way to do that in geode-membership. We could probably grab the
setting from a global and somehow make it accessible to the `fromData()
`through the `DeserializationContext`, but `DeserializationContext` doesn't
seem to be a good place to store this information. I think that this code
should work even without this check. If SSL endpoint identification is enabled,
users shouldn't use an IP address as a Subject Alternative Name in their
certificate. If it's disabled, it shouldn't matter if there's a hostname or an
IP address in the `hostname` field.
After running some tests I realized that we probably shouldn't check if
network partition is enabled either because this setting only matters when we
upgrade from a version without this fix to a version with this fix. Before
GEODE-9139, `memberData.hostname` contained an IP address if network partition
was enabled. But GEODE-9139 changed this behavior so that `hostname` contains
either bind-address (if specified) or an FQDN (regardless of network partition
setting). So if we keep this check, the behavior of
`convertIpToHostnameIfNeeded` will differ depending on the version we're
upgrading from, and I think we should avoid that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
