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

Add logic to remove duplicates from the bound port list to prevent internal services from being bound to the server twice. #5022

Merged
merged 10 commits into from
Aug 4, 2023

Conversation

tomatophobia
Copy link
Contributor

@tomatophobia tomatophobia commented Jul 11, 2023

Motivation:

  • When the internal-services.port and management.server.port are set to the same, the internal services are bound twice.

Modifications:

  • Add code to remove duplicates from the list of bound ports to prevent duplicate binding of internal services.

Result:

…al service port and the management server port to be the same.
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 for fixing the bug, @tomatophobia! ❤️

@ikhoon ikhoon added this to the 1.25.0 milestone Jul 17, 2023
@ikhoon ikhoon added the defect label Jul 17, 2023
@ikhoon
Copy link
Contributor

ikhoon commented Jul 17, 2023

Could you check the failures of lint and flaky-tests?

@ikhoon ikhoon self-requested a review July 17, 2023 13:39
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 @tomatophobia ! Happy with the changes once @ikhoon 's comment is addressed 🙇 👍 🙇

@tomatophobia
Copy link
Contributor Author

Test code permalink.

@DynamicPropertySource
static void registerPortProperties(DynamicPropertyRegistry registry) {
    final int port = PortUtil.unusedTcpPort();
    registry.add("armeria.internal-services.port", () -> port);
    registry.add("management.server.port", () -> port);
}

@Test
void exposeActuatorServiceHasSingleCustomPrefixAtInternalPort() throws Exception {
    ...
}

Thank you for review! 🙇‍♂️ I have found the cause of the flaky test failure, but it seems to contradict the assumptions of this PR. As mentioned in the comment of issue #5022, I proceeded with the assumption that it was a mistake for the user to set armeria.internal-services.port and management.server.port to the same value.

However, the above test intentionally tests the scenario where they are the same. So, I'm wondering if I can simply delete it or if I need to reconsider a solution. (I should have noticed it right after submitting the PR, and I apologize for the delay. 😭)

@minwoox
Copy link
Member

minwoox commented Jul 20, 2023

I'm wondering if I can simply delete it

Yes, I think we can simply remove the test. 😉

Could you also fix this line? 😉

@minwoox
Copy link
Member

minwoox commented Jul 20, 2023

Had a chat with @jrhee17 and we realized that this will break the current behavior if a user uses the same ports with WebFlux.
(Currently, users cannot use the same port when they use embedded tomcat so we should fix it though.)

So how about just deduping instead of raising an exception as the original issue suggested?

Copy link
Contributor Author

@tomatophobia tomatophobia left a comment

Choose a reason for hiding this comment

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

I have removed the previously written code. Following the revised direction, I added conditions to prevent the use of duplicate ports.

Copy link
Contributor Author

@tomatophobia tomatophobia left a comment

Choose a reason for hiding this comment

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

I have modified the code using the dedupPorts method to remove duplicates.

@tomatophobia tomatophobia force-pushed the raise-exception-port-collision branch from 050ef23 to 4115556 Compare July 21, 2023 16:08
@tomatophobia tomatophobia force-pushed the raise-exception-port-collision branch from 4115556 to 3ecce56 Compare July 21, 2023 16:24
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.

Awesome! Thanks a lot! 👍 👍 👍

@ikhoon
Copy link
Contributor

ikhoon commented Jul 31, 2023

@tomatophobia Would you update the PR title and description to reflect the final change?

@tomatophobia tomatophobia changed the title Raise an exception when user mistakenly sets the internal service port and the management server port to be the same. Add logic to remove duplicates from the bound port list to prevent internal services from being bound to the server twice. Jul 31, 2023
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, @tomatophobia! 🙇

Copy link
Member

@trustin trustin left a comment

Choose a reason for hiding this comment

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

Thanks, @tomatophobia! 👍

@codecov
Copy link

codecov bot commented Aug 4, 2023

Codecov Report

Patch coverage: 90.47% and project coverage change: -0.01% ⚠️

Comparison is base (3d169b6) 74.18% compared to head (6d763f9) 74.18%.
Report is 4 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #5022      +/-   ##
============================================
- Coverage     74.18%   74.18%   -0.01%     
- Complexity    19464    19469       +5     
============================================
  Files          1672     1672              
  Lines         71749    71768      +19     
  Branches       9189     9197       +8     
============================================
+ Hits          53230    53243      +13     
- Misses        14191    14196       +5     
- Partials       4328     4329       +1     
Files Changed Coverage Δ
...eria/internal/spring/ArmeriaConfigurationUtil.java 77.99% <90.47%> (+1.14%) ⬆️

... and 11 files with indirect coverage changes

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

@trustin trustin merged commit f2fe087 into line:main Aug 4, 2023
14 checks passed
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.

Internal services are bound twice to the server
5 participants