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.

Sign up for GitHub

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

Jump to bottom

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

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

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
94a42a4
feat: allow ip detector to exclude certain ip addresses
junfuchen99 Nov 27, 2023
1278984
fix: pr comment
junfuchen99 Nov 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,13 +45,13 @@
<dependency>
<groupId>com.aws.greengrass</groupId>
<artifactId>nucleus</artifactId>
<version>2.2.0-SNAPSHOT</version>
<version>2.12.0-SNAPSHOT</version>
<scope>provided</scope>
</dependency>
<dependency>
<groupId>com.aws.greengrass</groupId>
<artifactId>nucleus</artifactId>
<version>2.2.0-SNAPSHOT</version>
<version>2.12.0-SNAPSHOT</version>
<type>test-jar</type>
<scope>test</scope>
</dependency>
Expand Down
19 changes: 19 additions & 0 deletions src/main/java/com/aws/greengrass/detector/config/Config.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,15 @@
package com.aws.greengrass.detector.config;

import com.aws.greengrass.componentmanager.KernelConfigResolver;
import com.aws.greengrass.config.Topic;
import com.aws.greengrass.config.Topics;
import com.aws.greengrass.logging.api.Logger;
import com.aws.greengrass.logging.impl.LogManager;
import com.aws.greengrass.util.Coerce;
import lombok.Getter;

import java.util.ArrayList;
import java.util.List;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.concurrent.atomic.AtomicInteger;

Expand All @@ -19,6 +23,7 @@ public class Config {

static final String INCLUDE_IPV4_LOOPBACK_ADDRESSES_CONFIG_KEY = "includeIPv4LoopbackAddrs";
static final String INCLUDE_IPV4_LINK_LOCAL_ADDRESSES_CONFIG_KEY = "includeIPv4LinkLocalAddrs";
static final String EXCLUDED_IP_ADDRESSES_CONFIG_KEY = "excludedIPAddresses";
static final String DEFAULT_PORT_CONFIG_KEY = "defaultPort";
static final boolean DEFAULT_INCLUDE_IPV4_LOOPBACK_ADDRESSES = false;
static final boolean DEFAULT_INCLUDE_IPV4_LINK_LOCAL_ADDRESSES = false;
Expand All @@ -27,6 +32,8 @@ public class Config {
private AtomicInteger defaultPort = new AtomicInteger(DEFAULT_PORT);
private AtomicBoolean includeIPv4LoopbackAddrs = new AtomicBoolean(DEFAULT_INCLUDE_IPV4_LOOPBACK_ADDRESSES);
private AtomicBoolean includeIPv4LinkLocalAddrs = new AtomicBoolean(DEFAULT_INCLUDE_IPV4_LINK_LOCAL_ADDRESSES);
@Getter
private final List<String> excludedIPAddresses = new ArrayList<>();
jcosentino11 marked this conversation as resolved.
Show resolved Hide resolved

/**
* Config constructor.
Expand All @@ -39,6 +46,7 @@ public Config(Topics topics) {
if (configurationTopics.isEmpty()) {
this.includeIPv4LoopbackAddrs = new AtomicBoolean(DEFAULT_INCLUDE_IPV4_LOOPBACK_ADDRESSES);
this.includeIPv4LinkLocalAddrs = new AtomicBoolean(DEFAULT_INCLUDE_IPV4_LINK_LOCAL_ADDRESSES);
this.excludedIPAddresses.clear();
this.defaultPort = new AtomicInteger(DEFAULT_PORT);
return;
}
Expand All @@ -57,9 +65,20 @@ public Config(Topics topics) {
Coerce.toInt(
configurationTopics.findOrDefault(DEFAULT_PORT,
DEFAULT_PORT_CONFIG_KEY)));
Topic excludedIPTopic = configurationTopics.find(EXCLUDED_IP_ADDRESSES_CONFIG_KEY);
if (excludedIPTopic != null) {
if (excludedIPTopic.getOnce() instanceof List) {
this.excludedIPAddresses.clear();
this.excludedIPAddresses.addAll(Coerce.toStringList(excludedIPTopic.getOnce()));
} else {
logger.atWarn().kv("value", excludedIPTopic.getOnce()).log("Invalid config value for"
+ " excludedIPAddresses. The config must be input as a list");
}
}

logger.atInfo().kv("includeIPv4LoopbackAddrs", includeIPv4LoopbackAddrs.get())
.kv("includeIPv4LinkLocalAddrs", includeIPv4LinkLocalAddrs.get())
.kv("excludedIPAddresses", excludedIPAddresses)
.kv("defaultPort", defaultPort.get())
.log("Configuration updated");
});
Expand Down
3 changes: 3 additions & 0 deletions src/main/java/com/aws/greengrass/detector/detector/IpDetector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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

continue;
}
if (address instanceof Inet6Address) {
continue;
}
Expand Down
36 changes: 36 additions & 0 deletions src/test/java/com/aws/greengrass/detector/config/ConfigTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,20 @@


import com.aws.greengrass.config.ChildChanged;
import com.aws.greengrass.config.Topic;
import com.aws.greengrass.config.Topics;
import com.aws.greengrass.util.Coerce;
import com.aws.greengrass.utils.TestConstants;
import org.junit.jupiter.api.Test;
import org.mockito.Mockito;
import org.mockito.stubbing.Answer;

import java.util.Collections;
import java.util.List;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyBoolean;
import static org.mockito.ArgumentMatchers.anyInt;
Expand All @@ -30,8 +36,11 @@ class ConfigTest {
public void GIVEN_config_topics_WHEN_initialize_THEN_configuration_created() {
Topics topics = Mockito.mock(Topics.class);
Topics configTopics = Mockito.mock(Topics.class);
Topic excludedIpTopic = Mockito.mock(Topic.class);
String mockIncludeIPv4LoopbackAddrsConfig = "true";
String mockIncludeIPv4LinkLocalAddrsConfig = "true";
String mockExcludeIPsConfig = String.format("[%s]", TestConstants.IP_1);
List<String> mockList = Collections.singletonList(TestConstants.IP_1);
int mockPortValue = 9000;

// stub subscribe() to call just the callback method without adding watcher
Expand All @@ -49,13 +58,16 @@ public void GIVEN_config_topics_WHEN_initialize_THEN_configuration_created() {
Mockito.doReturn(mockPortValue)
.when(configTopics).findOrDefault(anyInt(), eq(Config.DEFAULT_PORT_CONFIG_KEY));

Mockito.doReturn(excludedIpTopic).when(configTopics).find(Config.EXCLUDED_IP_ADDRESSES_CONFIG_KEY);
Mockito.doReturn(mockList).when(excludedIpTopic).getOnce();
Mockito.doReturn(configTopics).when(topics).lookupTopics(anyString());
config = new Config(topics);

assertNotNull(config);
assertEquals(mockPortValue, config.getDefaultPort());
assertEquals(Coerce.toBoolean(mockIncludeIPv4LoopbackAddrsConfig), config.isIncludeIPv4LoopbackAddrs());
assertEquals(Coerce.toBoolean(mockIncludeIPv4LinkLocalAddrsConfig), config.isIncludeIPv4LinkLocalAddrs());
assertEquals(Coerce.toStringList(mockExcludeIPsConfig), config.getExcludedIPAddresses());
}

@Test
Expand All @@ -78,5 +90,29 @@ public void GIVEN_empty_config_topics_WHEN_initialize_THEN_default_configuration
assertEquals(Config.DEFAULT_INCLUDE_IPV4_LOOPBACK_ADDRESSES, config.isIncludeIPv4LoopbackAddrs());
assertEquals(Config.DEFAULT_INCLUDE_IPV4_LINK_LOCAL_ADDRESSES, config.isIncludeIPv4LinkLocalAddrs());
assertEquals(Config.DEFAULT_PORT, config.getDefaultPort());
assertTrue(config.getExcludedIPAddresses().isEmpty());
}

@Test
public void GIVEN_invalid_excluded_ips_list_WHEN_initialize_THEN_default_configuration_created() {
Topics topics = Mockito.mock(Topics.class);
Topics configTopics = Mockito.mock(Topics.class);

// stub subscribe() to call just the callback method without adding watcher
doAnswer((Answer<Void>) invocation -> {
ChildChanged childChanged = invocation.getArgument(0);
childChanged.childChanged(null, null);
return null;
}).when(configTopics).subscribe(any());

Mockito.doReturn(false).when(configTopics).isEmpty();
Mockito.doReturn(configTopics).when(topics).lookupTopics(anyString());
Topic excludedIpTopic = Mockito.mock(Topic.class);
Mockito.doReturn(excludedIpTopic).when(configTopics).find(Config.EXCLUDED_IP_ADDRESSES_CONFIG_KEY);
Mockito.doReturn("bad-config").when(excludedIpTopic).getOnce();
config = new Config(topics);

assertNotNull(config);
assertTrue(config.getExcludedIPAddresses().isEmpty());
}
}
30 changes: 30 additions & 0 deletions src/test/java/com/aws/greengrass/detector/detector/IpDetectorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,10 @@
import java.util.Collections;
import java.util.Enumeration;
import java.util.List;
import java.util.stream.Collectors;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;

@ExtendWith({MockitoExtension.class})
Expand Down Expand Up @@ -98,6 +100,34 @@ public void GIVEN_network_down_WHEN_get_ipAddresses_THEN_null_returned() throws
assertTrue(ipAddresses.isEmpty());
}

@Test
public void GIVEN_excludeIPAddresses_WHEN_get_ipAddresses_THEN_excludeIPs_filtered() throws SocketException {
// exclude IP_1 (0.61.124.18)
NetworkInterface networkInterface = Mockito.mock(NetworkInterface.class);
Config config = Mockito.mock(Config.class);

List<NetworkInterface> networkInterfaces = new ArrayList<>();
List<InterfaceAddress> interfaceAddresses = getAllAddresses();

Mockito.doReturn(interfaceAddresses).when(networkInterface).getInterfaceAddresses();
Mockito.doReturn(true).when(networkInterface).isUp();
// Exclude IPv4 Loopback addresses and Link-Local addresses
Mockito.doReturn(true).when(config).isIncludeIPv4LoopbackAddrs();
Mockito.doReturn(true).when(config).isIncludeIPv4LinkLocalAddrs();
Mockito.doReturn(Collections.singletonList(TestConstants.IP_1)).when(config).getExcludedIPAddresses();

networkInterfaces.add(networkInterface);
Enumeration<NetworkInterface> enumeration = Collections.enumeration(networkInterfaces);
ipDetector = new IpDetector();
List<InetAddress> ipAddresses = ipDetector.getIpAddresses(enumeration, config);

assertEquals(2, ipAddresses.size());
assertFalse(ipAddresses.stream().map(InetAddress::getHostAddress)
.collect(Collectors.joining()).contains(TestConstants.IP_1));
assertEquals(TestConstants.IPV4_LOOPBACK, ipAddresses.get(0).getHostAddress());
assertEquals(TestConstants.IPV4_LINK_LOCAL, ipAddresses.get(1).getHostAddress());
}

private List<InterfaceAddress> getAllAddresses() {
List<InterfaceAddress> interfaceAddresses = new ArrayList<>();
InterfaceAddress interfaceAddress1 = Mockito.mock(InterfaceAddress.class);
Expand Down
Loading