Bill commented on a change in pull request #6714:
URL: https://github.com/apache/geode/pull/6714#discussion_r708752232
##########
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.
Review comment:
Second sentence confuses me. What do you think of this:
In older versions of Geode {@link MemberData#getHostName()} can return an IP
address if network partition detection is enabled. If SSL endpoint
identification is enabled, those product versions supply the result of a
reverse lookup to the TLS handshake API. Endpoint identification will fail if
e.g. the lookup returned a fully-qualified name but the certificate had just a
(non-fully-qualified) hostname in a Subject Alternate Name field.
In version 1.15.0 member identifiers were changed so that if a bind address
is specified, that exact string will be carried as the host name. That gives
the administrator better control over endpoint identification. When upgrading
from earlier versions we convert any IP numbers to hostnames via reverse lookup
here.
##########
File path:
geode-core/src/integrationTest/resources/org/apache/geode/codeAnalysis/sanctionedDataSerializables.txt
##########
@@ -1749,18 +1751,10 @@
org/apache/geode/internal/cache/tier/sockets/ObjectPartList,2
fromData,148
toData,201
-org/apache/geode/internal/cache/tier/sockets/ObjectPartList651,2
-fromData,165
-toData,223
Review comment:
ah ok 👍 `ObjectPartList651` isn't in the project any more. Ditto
`SerializedObjectPartList` below.
##########
File path:
geode-core/src/main/java/org/apache/geode/distributed/internal/membership/InternalDistributedMember.java
##########
@@ -455,6 +455,12 @@ public void readExternal(ObjectInput in) throws
IOException, ClassNotFoundExcept
durableClientAttributes = null;
}
+ @Override
+ public void toDataPre_GEODE_1_15_0_0(DataOutput out, SerializationContext
context)
+ throws IOException {
+ memberIdentifier.toDataPre_GEODE_1_15_0_0(out, context);
+ }
+
Review comment:
sorry to be nitpicky but would you mind putting `toData()` before each
of the version-specific to-data methods. And then order the to-data methods
from newest to oldest. I believe that's the pattern established in other DSFIDs.
Ordering them consistently helps maintainers.
##########
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:
should we be guarding also on (SSL endpoint identification is enabled)?
--
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]
