cluster.joinSeedNodes(...) does not throw warnings when seed hostname contains underscores #25287 #27222

helena · 2019-06-26T13:10:49Z

… contains underscores akka#25287

helena · 2019-06-26T13:18:47Z

akka-actor/src/main/scala/akka/actor/Address.scala

+   * are any unusual characters in the host string.
+   */
+  @InternalApi
+  private[akka] def hasInvalidHostCharacters: Boolean = host.exists(_.contains("_"))


Could expand of course but not all cases would need all char checks.
Could in future allow a user to provide a regex but also sounds over-engineery.
Best option is have a base here, and in a given module/function add the additional applicable checks.

helena · 2019-06-26T13:19:51Z

akka-cluster/src/main/scala/akka/cluster/Cluster.scala

+  private def checkHostCharacters(address: Address): Unit =
+    require(
+      !address.hasInvalidHostCharacters,
+      s"Joining the cluster using invalid host characters '${address.host}' in the Address is not allowed.")


Not saying I like a function that has a Unit signature which can throw.... but here we want to fail fast vs return something to handle.

akka-ci · 2019-06-26T14:05:09Z

Test PASSed.

patriknw

some more but otherwise looking good to add

patriknw · 2019-07-02T11:27:43Z

akka-cluster/src/main/scala/akka/cluster/Cluster.scala

@@ -300,8 +300,10 @@ class Cluster(val system: ExtendedActorSystem) extends Extension {
   * The name of the [[akka.actor.ActorSystem]] must be the same for all members of a
   * cluster.
   */
-  def join(address: Address): Unit =
+  def join(address: Address): Unit = {
+    checkHostCharacters(address)


should also be added to akka.cluster.typed.Join message (constructor check)

@patriknw I wonder if this is needed (because it isn't so simple to call this new existing in the Join constructor from where it is in Cluster and in typed it is immediately checked in the call to untyped. If we let it get checked in untyped it is cleaner though I agree with you.

case Join(address) => adaptedCluster.join(address) // executes checkHostCharacters case JoinSeedNodes(addresses) => adaptedCluster.joinSeedNodes(addresses) // executes checkHostCharacters

since it's a message it will not be fail fast (at the sender side)

You can move checkHostCharacters to Address, or some other place that is accessible from akka-cluster-typed and akka-cluster. It doesn't have to live inside the Cluster class since it's just stateless validation logic.

Sounds good, I originally thought it belonged in Address but didn't know if it was too leaky.

yes, not good in Address since the error message talks about cluster joining. Put it in Cluster companion object.

I removed the cluster part of the message and put it in Address for more reusability. If that makes sense.

akka-cluster/src/main/scala/akka/cluster/Cluster.scala

helena · 2019-07-02T13:06:15Z

@patriknw all review suggestions pushed.

akka-ci · 2019-07-02T13:14:12Z

Test FAILed.

patriknw

LGTM

patriknw · 2019-07-02T14:14:04Z

akka-actor/src/main/scala/akka/actor/Address.scala

@@ -65,6 +68,19 @@ final case class Address private (protocol: String, system: String, host: Option
   * `system@host:port`
   */
  def hostPort: String = toString.substring(protocol.length + 3)
+
+      /** INTERNAL API


weird formatting here

Fixed, needed to run fmt

Isn't "format on save" in IntelliJ working for you? That must be annoying. I never run sbt scalafmt

That became too annoying while adding something new. I may try it again in future.

akka-ci · 2019-07-02T15:10:45Z

Test PASSed.

akka-actor-tests/src/test/scala/akka/actor/ActorPathSpec.scala

akka-ci · 2019-07-05T19:32:49Z

Test PASSed.

akka-cluster/src/test/scala/akka/cluster/ClusterSpec.scala

cluster.joinSeedNodes(...) does not throw warnings when seed hostname…

262a905

… contains underscores akka#25287

akka-ci added the validating PR is currently being validated by Jenkins label Jun 26, 2019

helena commented Jun 26, 2019

View reviewed changes

akka-ci added tested PR that was successfully built and tested by Jenkins and removed validating PR is currently being validated by Jenkins labels Jun 26, 2019

patriknw reviewed Jul 2, 2019

View reviewed changes

akka-ci added validating PR is currently being validated by Jenkins and removed tested PR that was successfully built and tested by Jenkins labels Jul 2, 2019

akka-ci added needs-attention Indicates a PR validation failure (set by CI infrastructure) and removed validating PR is currently being validated by Jenkins labels Jul 2, 2019

PR review suggestions.

e8ca314

patriknw approved these changes Jul 2, 2019

View reviewed changes

helena force-pushed the cluster-join-address-check branch from cd59db2 to e8ca314 Compare July 2, 2019 14:14

akka-ci added validating PR is currently being validated by Jenkins and removed needs-attention Indicates a PR validation failure (set by CI infrastructure) labels Jul 2, 2019

helena removed the 1 - triaged Tickets that are safe to pick up for contributing in terms of likeliness of being accepted label Jul 2, 2019

akka-ci added tested PR that was successfully built and tested by Jenkins and removed validating PR is currently being validated by Jenkins labels Jul 2, 2019

johanandren reviewed Jul 4, 2019

View reviewed changes

akka-actor-tests/src/test/scala/akka/actor/ActorPathSpec.scala Show resolved Hide resolved

helena mentioned this pull request Jul 4, 2019

Cluster Sharding: configurable shard query timeout for askAllShards #27273

Closed

Added regex for broader host acceptance and validation.

123a737

akka-ci added validating PR is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed tested PR that was successfully built and tested by Jenkins validating PR is currently being validated by Jenkins labels Jul 5, 2019

johanandren reviewed Jul 8, 2019

View reviewed changes

akka-cluster/src/test/scala/akka/cluster/ClusterSpec.scala Show resolved Hide resolved

johanandren approved these changes Jul 8, 2019

View reviewed changes

helena merged commit 501848d into akka:master Jul 8, 2019

helena deleted the cluster-join-address-check branch July 15, 2019 11:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cluster.joinSeedNodes(...) does not throw warnings when seed hostname contains underscores #25287 #27222

cluster.joinSeedNodes(...) does not throw warnings when seed hostname contains underscores #25287 #27222

helena commented Jun 26, 2019

helena Jun 26, 2019

helena Jun 26, 2019

akka-ci commented Jun 26, 2019

patriknw left a comment

patriknw Jul 2, 2019

helena Jul 2, 2019

patriknw Jul 2, 2019

helena Jul 2, 2019

patriknw Jul 2, 2019

helena Jul 2, 2019

helena commented Jul 2, 2019

akka-ci commented Jul 2, 2019

patriknw left a comment

patriknw Jul 2, 2019

helena Jul 2, 2019

patriknw Jul 2, 2019 •

edited

helena Jul 2, 2019

akka-ci commented Jul 2, 2019

akka-ci commented Jul 5, 2019

cluster.joinSeedNodes(...) does not throw warnings when seed hostname contains underscores #25287 #27222

cluster.joinSeedNodes(...) does not throw warnings when seed hostname contains underscores #25287 #27222

Conversation

helena commented Jun 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akka-ci commented Jun 26, 2019

patriknw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

helena commented Jul 2, 2019

akka-ci commented Jul 2, 2019

patriknw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patriknw Jul 2, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akka-ci commented Jul 2, 2019

akka-ci commented Jul 5, 2019

patriknw Jul 2, 2019 •

edited