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: Do not include router option if no nat #5727

Open
wants to merge 13 commits into
base: develop
Choose a base branch
from

Conversation

MMaiero
Copy link
Contributor

@MMaiero MMaiero commented Feb 15, 2025

Note: We are using the Conventional Commits convention for our pull request titles. Please take a look at the PR title format document for the supported types and scopes.

Brief description of the PR. [e.g. Added null check on object to avoid NullPointerException]

Related Issue: This PR fixes/closes {issue number}

Description of the solution adopted: A more detailed description of the changes made to solve/close one or more issues. If the PR is simple and easy to understand this section can be skipped

Screenshots: If applicable, add screenshots to help explain your solution

Manual Tests: Optional description of the tests performed to check correct functioning of changes, useful for an efficient review

Any side note on the changes made: Description of any other change that has been made, which is not directly linked to the issue resolution [e.g. Code clean up/Sonar issue resolution]

@MMaiero MMaiero marked this pull request as ready for review February 17, 2025 08:09
@pierantoniomerlino
Copy link
Contributor

pierantoniomerlino commented Feb 20, 2025

@MMaiero I verified the issue on my device and that this PR fixes it. I tested the solution with dhcpd and dnsmasq. I noticed that dhcpd had been already configured to not add the router address if in DHCP Only.

However, it seems that if the framework is configured in DHCP Only and the PassDNS is set to true, the DNS are indeed passed to the client:

DHCPonlyPassDNS

I don't think that this is a correct configuration. I know that it's bit out of scope...

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

PR Overview

This pull request ensures that the router option is omitted in the DHCP configuration when NAT is disabled. Key changes include updates to test cases to cover NAT enabled/disabled scenarios, modifications in the DHCP server configuration writer to conditionally set the router, and adjustments in the DHCP config converters (Dhcpd, Dnsmasq, and Udhcpd) to properly handle the absence of a router address.

Reviewed Changes

File Description
kura/test/org.eclipse.kura.nm.test/src/test/java/org/eclipse/kura/nm/configuration/writer/DhcpServerConfigWriterTest.java Added test cases to cover scenarios with NAT disabled.
kura/org.eclipse.kura.nm/src/main/java/org/eclipse/kura/nm/configuration/writer/DhcpServerConfigWriter.java Updated to conditionally set the router field based on the NAT flag.
kura/org.eclipse.kura.linux.net/src/main/java/org/eclipse/kura/linux/net/dhcp/server/DhcpdConfigConverter.java Refactored configuration appending by splitting DNS, interface, router, lease times, and pool range into dedicated methods.
kura/test/org.eclipse.kura.linux.net.test/src/test/java/org/eclipse/kura/linux/net/dhcp/server/DhcpdConfigConverterTest.java Updated expected configurations for NAT disabled scenarios.
kura/org.eclipse.kura.linux.net/src/main/java/org/eclipse/kura/linux/net/dhcp/server/DnsmasqConfigConverter.java Adjusted the router option handling to conditionally include the router value.
kura/org.eclipse.kura.linux.net/src/main/java/org/eclipse/kura/linux/net/dhcp/server/UdhcpdConfigConverter.java Modified to conditionally output the router line based on its availability.

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Comment on lines +38 to 40
String routerAddress = isNull(config.getRouterAddress()) ? "" : "," + config.getRouterAddress().toString();
sb.append(DHCP_OPTION_KEY).append(config.getInterfaceName()).append(",3").append(routerAddress).append("\n");

Copy link
Preview

Copilot AI Feb 27, 2025

Choose a reason for hiding this comment

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

When 'nat.enabled' is false and config.getRouterAddress() is null, this line produces a router option output like 'dhcp-option=eth0,3' without a value. To adhere to the PR intent of omitting the router option entirely, consider conditionally skipping the entire router option line when routerAddress is null.

Suggested change
String routerAddress = isNull(config.getRouterAddress()) ? "" : "," + config.getRouterAddress().toString();
sb.append(DHCP_OPTION_KEY).append(config.getInterfaceName()).append(",3").append(routerAddress).append("\n");
if (!isNull(config.getRouterAddress())) {
String routerAddress = "," + config.getRouterAddress().toString();
sb.append(DHCP_OPTION_KEY).append(config.getInterfaceName()).append(",3").append(routerAddress).append("\n");
}

Copilot is powered by AI, so mistakes are possible. Review output carefully before use.

Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
@MMaiero MMaiero force-pushed the fix_dhcp-server-no-nat branch from 11d437e to b0cbbcb Compare February 28, 2025 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

2 participants