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

Label Templates: add IP addresses to the Network variables #885

Merged
merged 8 commits into from
Nov 26, 2024

Conversation

fgiudici
Copy link
Member

@fgiudici fgiudici commented Nov 12, 2024

Add the IPv4 and IPv6 addresses to the NICs template variables.
In particular:

  • ${Network/NICs//IPv4Addresses/}
  • ${Network/NICs//IPv6Addresses/}
  • ${Network/NICs//IPv4Address} # first address from IPv4Addresses
  • ${Network/NICs//IPv6Address} # first address from IPv6Addresses
  • ${Network/NICs//IPv4Addresses/}
  • ${Network/NICs//IPv6Addresses/}
  • ${Network/NICs//IPv4Address}
  • ${Network/NICs//IPv6Address}

Example:

apiVersion: elemental.cattle.io/v1beta1
kind: MachineRegistration
[...]
  machineInventoryLabels:
    element: fire
    eth1-IPv4: ${Network/NICs/eth1/IPv4Address}               # 10.100.103.144
    eth1-IPv6: ${Network/NICs/eth1/IPv6Address}               # fe80-922a-e5ea-fc77-e503 
    net0-name: ${Network/NICs/0/Name}                              # eth0
    net0-ipv4: ${Network/NICs/0/IPv4Address}                     # 172.21.100.153
    eth0-IPv4-0: ${Network/NICs/eth0/IPv4Addresses/0}    # 10.100.103.144
    eth0-IPv4-1: ${Network/NICs/eth0/IPv4Addresses/1}
    eth0-IPv6-0: ${Network/NICs/eth0/IPv6Addresses/0}    # fe80-922a-e5ea-fc77-e503 
    eth0-IPv6-1: ${Network/NICs/eth0/IPv6Addresses/1}

Part of #855

@github-actions github-actions bot added the area/operator operator related changes label Nov 12, 2024
Copy link
Contributor

@kkaempf kkaempf left a comment

Choose a reason for hiding this comment

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

Please add respective tests

@fgiudici fgiudici force-pushed the nic_ipaddress_annotations branch 2 times, most recently from 1c5c958 to 37b376d Compare November 15, 2024 18:29
@github-actions github-actions bot added area/tests test related changes area/build build related changes labels Nov 15, 2024
@fgiudici fgiudici force-pushed the nic_ipaddress_annotations branch from 37b376d to dffaec3 Compare November 15, 2024 18:30
@fgiudici fgiudici marked this pull request as ready for review November 15, 2024 18:30
@fgiudici fgiudici requested a review from a team as a code owner November 15, 2024 18:30
@fgiudici fgiudici self-assigned this Nov 15, 2024
@fgiudici fgiudici requested a review from kkaempf November 15, 2024 21:43
Copy link
Contributor

@anmazzotti anmazzotti left a comment

Choose a reason for hiding this comment

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

Looks good to me, just a few nitpicks.

However I'm worried about the implicit behavior of showing IPv4 before IPv6. Why not both?

My proposal is to explicitly implement *.ipv4 and *.ipv6 annotations.
Makes it also easier to parse the value, so that you don't have to implement a isIPv4? filter for example.

Last but not least, this PR enables this behavior by default, I wonder if this is the desired behavior for everyone or maybe this is worth a new toggle, like a new flag in the MachineRegistration to enable or not reporting of IPs.
If we have the toggle I'd still enable it by default, as this is very nice to have.

pkg/util/util.go Show resolved Hide resolved
pkg/util/net.go Outdated Show resolved Hide resolved
pkg/util/net.go Outdated Show resolved Hide resolved
Copy link
Contributor

@anmazzotti anmazzotti left a comment

Choose a reason for hiding this comment

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

Sorry I didn't mean to approve it, pressed the wrong button. :(

@fgiudici
Copy link
Member Author

However I'm worried about the implicit behavior of showing IPv4 before IPv6. Why not both?

My proposal is to explicitly implement *.ipv4 and *.ipv6 annotations. Makes it also easier to parse the value, so that you don't have to implement a isIPv4? filter for example.

Well, have to confess I was mainly looking at the final goal to have an IP address for k3s/RKE2 internal/external address values, and so ended up looking just for one IP address.. but good point, doesn't makes much sense to do this kind of filtering at the annotation level, would make more sense to have ipv4 and ipv6 👍🏼

@fgiudici fgiudici force-pushed the nic_ipaddress_annotations branch from dffaec3 to a0d4639 Compare November 25, 2024 16:59
track all available network addresses

Signed-off-by: Francesco Giudici <[email protected]>
Signed-off-by: Francesco Giudici <[email protected]>
Signed-off-by: Francesco Giudici <[email protected]>
@fgiudici fgiudici force-pushed the nic_ipaddress_annotations branch from a0d4639 to 84d3b51 Compare November 25, 2024 17:39
@fgiudici fgiudici changed the title Make NICs IP addresses available to annotations and as Label Template Vars Label Templates: add IP addresses to the Network variables Nov 25, 2024
@fgiudici
Copy link
Member Author

Changed PR to just expose IP addresses as Network variables (on demand).
No more new default annotations.
We now expose IPv4 and IPv6 addresses, updated the PR description accordingly.

Copy link
Contributor

@anmazzotti anmazzotti left a comment

Choose a reason for hiding this comment

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

Looks good, thank you for the changes!

@fgiudici fgiudici dismissed kkaempf’s stale review November 26, 2024 08:15

done, tests added

@fgiudici fgiudici merged commit 5e14e96 into rancher:main Nov 26, 2024
8 checks passed
@fgiudici fgiudici deleted the nic_ipaddress_annotations branch November 26, 2024 10:26
fgiudici added a commit to fgiudici/elemental-operator that referenced this pull request Dec 10, 2024
* util: add network util functions

Signed-off-by: Francesco Giudici <[email protected]>

* Label Templates: add IPAddress to the Network vars

Signed-off-by: Francesco Giudici <[email protected]>

* util/net: rework to allow mocking system ip address retrieval

Signed-off-by: Francesco Giudici <[email protected]>

* util/net: return all the available network addresses

Signed-off-by: Francesco Giudici <[email protected]>

* Label Templates: add IPAddresses.{num} to the Network vars

track all available network addresses

Signed-off-by: Francesco Giudici <[email protected]>

* make generate-mocks

Signed-off-by: Francesco Giudici <[email protected]>

* tests: add util/net coverage

Signed-off-by: Francesco Giudici <[email protected]>

* error strings should not be capitalized

Signed-off-by: Francesco Giudici <[email protected]>

---------

Signed-off-by: Francesco Giudici <[email protected]>
(cherry picked from commit 5e14e96)
fgiudici added a commit that referenced this pull request Dec 10, 2024
* util: add network util functions

Signed-off-by: Francesco Giudici <[email protected]>

* Label Templates: add IPAddress to the Network vars

Signed-off-by: Francesco Giudici <[email protected]>

* util/net: rework to allow mocking system ip address retrieval

Signed-off-by: Francesco Giudici <[email protected]>

* util/net: return all the available network addresses

Signed-off-by: Francesco Giudici <[email protected]>

* Label Templates: add IPAddresses.{num} to the Network vars

track all available network addresses

Signed-off-by: Francesco Giudici <[email protected]>

* make generate-mocks

Signed-off-by: Francesco Giudici <[email protected]>

* tests: add util/net coverage

Signed-off-by: Francesco Giudici <[email protected]>

* error strings should not be capitalized

Signed-off-by: Francesco Giudici <[email protected]>

---------

Signed-off-by: Francesco Giudici <[email protected]>
(cherry picked from commit 5e14e96)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/build build related changes area/operator operator related changes area/tests test related changes
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants