-
Notifications
You must be signed in to change notification settings - Fork 25
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 support for setting VLAN in network devices through config #329
base: 0.7
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
anything missing? |
@@ -91,6 +91,7 @@ GATEWAY: "10.10.10.1" # The gateway for | |||
IP_PREFIX: "25" # Subnet Mask in CIDR notation for your node IP ranges | |||
DNS_SERVERS: "[8.8.8.8,8.8.4.4]" # The dns nameservers for the machines network-config. | |||
BRIDGE: "vmbr1" # The network bridge device for Proxmox VE VMs | |||
VLAN: "2" # (optional) The VLAN to put the bridge device into |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
VLAN: "2" # (optional) The VLAN to put the bridge device into | |
VLAN: "2" # The VLAN to put the bridge device into |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you want, you can also set the default value in the template. so it's considered optional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think setting it mandatory would break the previous flow as existing configs used to generate the templates will complain. Need to have a look which default works here. From *uint16
it should be null
… 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the templates to have a null
default value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the null will not work for most cases,
and you don't need to add VLAN to all cluster templates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why will the null
not work? The default of an unset pointer is null
and the type is a *uint16
… 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "VLAN" field doesn't actually select or add a vlan, it only adds a tag. It's basically broken, and we should fix documentation on this (or remove). This is also why it's not in the templates.
Edit: I'll word it more clearly, this tells the bridge in proxmox (on the hypervisor) to tag with a vlan id, which only works in certain hypervisor configurations. In general you're expected to create a bridge per vlan id you want.
So if it is basically broken, we can close this PR and the related issue? |
It may or may not work depending on how you set up your proxmox servers, it's like tagging a port in a switch, rather than creating a vlan in the deployed machine. |
Issue #, if available: #303
Description of changes:
VLAN
,SECONDARY_VLAN
andEXT_SERVICE_VLAN
variables to the template to configure VLAN on the network devicesTesting performed:
VLAN
set: Expected behavior, without ENV-var akubectl describe
shows no configuredvlan
, withVLAN
set to2
it showsvlan: 2
.