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

feat: allow ip detector to exclude certain ip addresses #28

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

junfuchen99
Copy link
Member

Description of changes:

  • Add a configuration excludedIPAddresses to exclude some ip addresses from ip detector.
  • Bump nucleus snapshot.

Why is this change necessary:
Give customer granular controls over which IP addresses to detect and upload to cloud.

How was this change tested:
Unit test and manual test.

Any additional information or context required to review the change:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Copy link

github-actions bot commented Nov 28, 2023

Unit Tests Coverage Report

File Coverage Lines Branches
All files 76% 80% 73%
com.aws.greengrass.detector.IpDetectorManager 82% 64% 100%
com.aws.greengrass.detector.IpDetectorService 0% 0% 0%
com.aws.greengrass.detector.detector.IpDetector 98% 96% 100%
com.aws.greengrass.detector.uploader.ConnectivityUpdater 74% 90% 58%
com.aws.greengrass.detector.config.Config 92% 100% 83%

Minimum allowed coverage is 60%

Generated by 🐒 cobertura-action against 1278984

@@ -44,6 +44,9 @@ List<InetAddress> getIpAddresses(Enumeration<NetworkInterface> interfaces, Confi

for (InterfaceAddress interfaceAddress : networkInterface.getInterfaceAddresses()) {
InetAddress address = interfaceAddress.getAddress();
if (config.getExcludedIPAddresses().contains(address.getHostAddress())) {
Copy link
Member

@jcosentino11 jcosentino11 Nov 28, 2023

Choose a reason for hiding this comment

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

just a heads up this is running in a different thread than config changes, so there's technically some interleaving here when new config is cleared/set

Copy link
Member

Choose a reason for hiding this comment

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

and by heads up i mean, lets address it :) the worst case (although unlikely) means that connectivity info will be published with excluded ips for bit

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not too concerned about this issue, because when IP Detector is deployed, the config merge happens before its startup lifecycle. When IP detector goes into startup, it should already have the deployed excluded-ip config.

We do not have any use case for its config update outside deployments as of today, since component itself does not update it and other components are not allowed to update it.

Copy link
Member

Choose a reason for hiding this comment

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

but what about deployments that are just config updates? these exclusions wouldn't change? even if not, there's still the mental overhead of having to justify why the race condition isn't an issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants