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

Fix pattern for matching SRV service names #26332

Merged
merged 7 commits into from Mar 18, 2019

Conversation

Projects
None yet
7 participants
@renatocaval
Copy link
Contributor

commented Feb 4, 2019

The pattern that's checking for a valid Service Name is too restrictive.

Currently, the following SRV strings can be passed:

  • _http._tcp.my-service.local
  • _http._tcp.my-service.svc.cluster.local

However, _http._tcp.my-service will be considered invalid.
It turns out that my-service is a valid domain name as well.

The current pattern follows the spec defined at: https://tools.ietf.org/html/rfc1034

Basic rules are:

  • a node must have from 1 to 63 chars in the range [a-zA-Z0-9-] and should not start or end with a -.
  • nodes are separated by a . (dot)

In combination with #25987, one could build a Loopup object from _http._tcp.my-service and have the rest of the domain filled by the search domain functionality.

@akka-ci akka-ci added the validating label Feb 4, 2019

private def validDomainName(name: String): Boolean =
DomainName.pattern.asPredicate().test(name)
private def validServiceName(name: String): Boolean =
ServiceName.pattern.asPredicate().test(name)

This comment has been minimized.

Copy link
@renatocaval

renatocaval Feb 4, 2019

Author Contributor

Note: this is all private code and the only impact on potential users is that it's now less restrictive.

@akka-ci akka-ci added tested and removed validating labels Feb 4, 2019

@akka-ci

This comment has been minimized.

Copy link
Collaborator

commented Feb 4, 2019

Test PASSed.

@renatocaval

This comment has been minimized.

Copy link
Contributor Author

commented Feb 4, 2019

Strange enough, this build passed and it should have not. I realise that the new regex is wrong and it should break the build.

Correction, current PR will pass but pattern is wrong. The tests are not exhaustive. I'll fix it tomorrow.

@johanandren johanandren added the wip label Feb 5, 2019

@akka-ci

This comment has been minimized.

Copy link
Collaborator

commented Feb 5, 2019

Test PASSed.

@renatocaval

This comment has been minimized.

Copy link
Contributor Author

commented Feb 5, 2019

@johanandren, I updated the PR with a new regex. This is good for review and discussions.

The pattern still complies with domain name format (maybe we should rename it back to DomainName).

It accepts:

  • my-service
  • my-service.local
  • my-service.something.com

@johanandren johanandren removed the wip label Feb 5, 2019

@johanandren

This comment has been minimized.

Copy link
Member

commented Feb 8, 2019

How I interpret the SRV record spec it should be a domain name, and not just any arbitrary name:
https://tools.ietf.org/html/rfc2782

I may be reading that wrong though.

@renatocaval

This comment has been minimized.

Copy link
Contributor Author

commented Feb 8, 2019

You are right, and the definition of it is here:
https://tools.ietf.org/html/rfc1034

My initial explanation in the description is wrong. We should consider it a domain name, but the pattern we were using were not correct.

  • each node in the name must be maximum 63 chars (that was correct)
  • we should not have a restriction for at least one . (localhost without tld is valid domain)
  • last node should not be restricted to 2 and 6 chars. (scala.consulting is a valid domain for instance)

@renatocaval renatocaval changed the title Relax pattern for matching SRV service names [WIP] Relax pattern for matching SRV service names Feb 8, 2019

@renatocaval

This comment has been minimized.

Copy link
Contributor Author

commented Feb 8, 2019

I will update this PR with the correct pattern. It must have a restriction on the number of chars per node. I just realised that I removed it.

@patriknw

This comment has been minimized.

Copy link
Member

commented Feb 12, 2019

let us know when it's ready for final review

@chbatey chbatey added the wip label Feb 17, 2019

@renatocaval renatocaval force-pushed the renatocaval:service-name-pattern branch from 2febf99 to ecf433e Feb 25, 2019

@renatocaval renatocaval changed the title [WIP] Relax pattern for matching SRV service names Fix pattern for matching SRV service names Feb 25, 2019

@akka-ci akka-ci added validating and removed tested labels Feb 25, 2019

@renatocaval

This comment has been minimized.

Copy link
Contributor Author

commented Feb 25, 2019

This is ready for a new review. I updated the description and title.

(this PR consumed all my regex skills quota for 2019)

@akka-ci akka-ci added tested and removed validating labels Feb 25, 2019

@akka-ci

This comment has been minimized.

Copy link
Collaborator

commented Feb 25, 2019

Test PASSed.

@akka-ci

This comment has been minimized.

Copy link
Collaborator

commented Feb 26, 2019

Test PASSed.

@johanandren
Copy link
Member

left a comment

LGTM

@akka-ci

This comment has been minimized.

Copy link
Collaborator

commented Feb 26, 2019

Test PASSed.

@patriknw patriknw added 2 - pick next and removed wip labels Mar 6, 2019

@patriknw patriknw requested a review from raboof Mar 6, 2019

@marcospereira

This comment has been minimized.

Copy link
Contributor

commented Mar 15, 2019

@renatocaval just to let you know that this is approved, but has some conflicts. You may want to fix them so that we can have this in the next Akka release. :-)

@johanandren

This comment has been minimized.

Copy link
Member

commented Mar 18, 2019

I fixed it for you, minimal conflict.

@akka-ci akka-ci added validating and removed tested labels Mar 18, 2019

@renatocaval

This comment has been minimized.

Copy link
Contributor Author

commented Mar 18, 2019

Thanks!

@raboof

raboof approved these changes Mar 18, 2019

Copy link
Member

left a comment

Not entirely comfortable with how strict we are here wrt labels starting with numbers, but surely an improvement so 👍

@akka-ci akka-ci added tested and removed validating labels Mar 18, 2019

@akka-ci

This comment has been minimized.

Copy link
Collaborator

commented Mar 18, 2019

Test PASSed.

@johanandren johanandren merged commit 4d0e666 into akka:master Mar 18, 2019

3 checks passed

Jenkins PR Validation Test PASSed. 32 tests run, 0 skipped, 0 failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
typesafe-cla-validator All users have signed the CLA
Details

@johanandren johanandren added this to the 2.5.22 milestone Mar 18, 2019

@renatocaval renatocaval deleted the renatocaval:service-name-pattern branch Mar 19, 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.