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 shared-network configuration #237

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Valantin
Copy link

@Valantin Valantin commented Jul 9, 2024

Add shared_network option to use different subnet on the same layer2 like multi subnet network or relay dhcp redirection

fixes #236

@@ -46,6 +46,7 @@
Hash[String, Hash] $hosts = {},
Variant[Array[String], Optional[String]] $includes = undef,
String $config_comment = 'dhcpd.conf',
Optional[String] $shared_network = undef,
Copy link
Member

Choose a reason for hiding this comment

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

Is it fine to allow empty strings here, or should we update it?

Suggested change
Optional[String] $shared_network = undef,
Optional[String[1]] $shared_network = undef,

Copy link
Member

Choose a reason for hiding this comment

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

I'd also like to document every new parameter. Ideally we'd have all parameters documented, but if we start with documenting new ones we at least move in that direction.

$interface = $facts['networking']['interfaces']['#{interface}']

class { 'dhcp':
interfaces => ['#{interface}'],
Copy link
Member

Choose a reason for hiding this comment

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

this variable interpolation looks a bit complicated. can't we just do:

Suggested change
interfaces => ['#{interface}'],
interfaces => [$facts['networking']['interfaces']['eth0']],

Then you can drop line 10

Copy link
Author

Choose a reason for hiding this comment

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

I've copied the test from the repo, it use the variable interface during the test
I can replace it in all the manifest with the long facts

Copy link
Member

Choose a reason for hiding this comment

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

No, the current code is correct.

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Shouldn't this be part of subnet or its own define, possibly with its own parameter subnets? The current implementation only allows one definition and will add all subnets to that single shared network but that doesn't have to be true.

On the other hand, that may be too complex to handle.

@Valantin
Copy link
Author

@ekohl I need to digest your comment and think a way to implement it.
My initial idea was to continue to use the current resources an minimize the impact to the smart_proxy module to add this implementation.

@Valantin Valantin requested a review from ekohl July 18, 2024 12:43
concat::fragment { "dhcp.conf+${order}-end_${name}.dhcp":
target => "${dhcp::dhcp_dir}/dhcpd.conf",
content => "}\n",
order => "${order}-9end",
Copy link
Author

Choose a reason for hiding this comment

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

Probably I need to change 9end in Zend to ensure it is at the end os the of the dhcp::subnet resource

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.

shared_network support
3 participants