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

Eureka: Remove encoded port from VIP address defaults #5451

Merged
merged 2 commits into from
Feb 19, 2024

Conversation

codefromthecrypt
Copy link
Contributor

@codefromthecrypt codefromthecrypt commented Feb 7, 2024

Motivation:

Currently, when armeria registers a server, it appends :${port} to Eureka VIP addresses to. While it makes sense to encode the port into the Eureka serviceId, the VIP address fields are meant to be hostnames or IPs. Encoding a port into the VIP address makes these unusable for certain clients like spring-cloud-sleuth who heuristically look for a port and only if it is absent trigger Eureka lookups.

Modifications:

  • I removed the logic that appended :${port} to both the vipAddress and secureVipAddress
  • I changed the test which actually proved the bug was present!

Result:

  • Manual integration with zipkin, sleuth is now able to look up its endpoint in Eureka.

Signed-off-by: Adrian Cole <adrian@tetrate.io>
@@ -266,7 +283,7 @@ private static String vipAddress(@Nullable String vipAddress, String hostName, P
if (!portWrapper.isEnabled()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this check is still helpful as it avoids adding secureVipAddress when there's no secure port

Copy link

codecov bot commented Feb 7, 2024

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (a117c25) 73.99% compared to head (6221885) 74.00%.
Report is 2 commits behind head on main.

❗ Current head 6221885 differs from pull request most recent head 4af19ac. Consider uploading reports for the commit 4af19ac to get more accurate results

Files Patch % Lines
.../armeria/server/eureka/EurekaUpdatingListener.java 62.50% 1 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##               main    #5451   +/-   ##
=========================================
  Coverage     73.99%   74.00%           
- Complexity    20740    20748    +8     
=========================================
  Files          1800     1800           
  Lines         76426    76431    +5     
  Branches       9728     9728           
=========================================
+ Hits          56552    56561    +9     
- Misses        15266    15267    +1     
+ Partials       4608     4603    -5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Adrian Cole <adrian@tetrate.io>
@codefromthecrypt
Copy link
Contributor Author

fixed the description

@codefromthecrypt
Copy link
Contributor Author

test flake is unrelated

@codefromthecrypt
Copy link
Contributor Author

ps the really really long story about this small bug https://github.com/openzipkin/zipkin/releases/tag/3.0.6

@minwoox minwoox added the defect label Feb 14, 2024
@minwoox minwoox added this to the 1.27.2 milestone Feb 14, 2024
Copy link
Member

@minwoox minwoox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I apologize for the lack of integration testing and greatly appreciate your effort!

@codefromthecrypt
Copy link
Contributor Author

there seems to be another glitch which I will look into (past this). More UX related? openzipkin/zipkin#3697 (comment)

Copy link
Contributor

@jrhee17 jrhee17 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @codefromthecrypt 🙇 👍 🙇

Copy link
Contributor

@ikhoon ikhoon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!! 🙇‍♂️

@jrhee17 jrhee17 merged commit 84751e9 into line:main Feb 19, 2024
14 of 15 checks passed
@codefromthecrypt codefromthecrypt deleted the eureka-vip-default branch February 19, 2024 02:55
jrhee17 pushed a commit to jrhee17/armeria that referenced this pull request Mar 4, 2024
Motivation:

Currently, when armeria registers a server, it appends `:${port}` to
Eureka VIP addresses to. While it makes sense to encode the port into
the Eureka `serviceId`, the VIP address fields are meant to be hostnames
or IPs. Encoding a port into the VIP address makes these unusable for
certain clients like spring-cloud-sleuth who heuristically look for a
port and only if it is absent trigger Eureka lookups.

Modifications:

- I removed the logic that appended `:${port}` to both the `vipAddress`
and `secureVipAddress`
- I changed the test which actually proved the bug was present!

Result:

- Manual integration with zipkin, sleuth is now able to look up its
endpoint in Eureka.

---------

Signed-off-by: Adrian Cole <adrian@tetrate.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants