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: issue dhcp request on windows secondary interfaces #3047

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

QxBytes
Copy link
Contributor

@QxBytes QxBytes commented Oct 3, 2024

Reason for Change:

We need to issue a dhcp request to the host in order to create a mapping for the secondary interface. The mapping allows for us to query the azure dns server on the secondary interface. The dhcp discover request occurs before the interface has the intended ip or is moved into the container namespace, but it does have an apipa autoconfigured ip. The auto assigned ip allows us to set the source ip of the packet so that the dhcp packet goes out on the interface that we want. We assume the interface always gets an auto configured ip. Then, once cni completes, we expect that secondary interface to be moved into the container.

Adds validation for nwCfg.Master since we issue exec commands with the field, so ensure this validation does not break anything else.

Reorganizes code into dhcp linux, windows and common.

Reorganizes retry logic into separate package

Confirm that we only run this code if we are on swiftv2 windows (is the nic type condition correct?). Confirmed yes.

Tested:

Limitations:

  • Assumes auto configuration ip will always be assigned to the interface

Issue Fixed:

Requirements:

Notes:
Related to #2989

@QxBytes QxBytes self-assigned this Oct 3, 2024
@QxBytes QxBytes added cni Related to CNI. windows swift-v2 fix Fixes something. labels Oct 3, 2024
dhcp/dhcp.go Outdated Show resolved Hide resolved
@QxBytes QxBytes marked this pull request as ready for review October 4, 2024 17:56
@QxBytes QxBytes requested review from a team as code owners October 4, 2024 17:56
@QxBytes QxBytes requested a review from rbtr October 4, 2024 17:56
dhcp/dhcp_windows.go Outdated Show resolved Hide resolved
dhcp/dhcp_windows.go Outdated Show resolved Hide resolved
dhcp/dhcp_windows.go Outdated Show resolved Hide resolved
dhcp/dhcp_windows.go Outdated Show resolved Hide resolved
network/network_windows.go Outdated Show resolved Hide resolved
retry/retry.go Outdated Show resolved Hide resolved
…t existing cni code

include last error cause when cooldown function errors (discuss) for compatability
use durations instead of millis
correct usage of number retries vs number of runs
create helper function for retrying in transparent vlan mode
create wrapper method that makes all passed in errors temporary (retriable) errors
@QxBytes QxBytes force-pushed the alew/dhcp-win branch 2 times, most recently from 59225e4 to a10f0df Compare October 8, 2024 21:05
@QxBytes QxBytes requested a review from timraymond October 9, 2024 16:38
Copy link
Member

@timraymond timraymond left a comment

Choose a reason for hiding this comment

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

LGTM on @estebancams 's approval

Copy link

This pull request is stale because it has been open for 2 weeks with no activity. Remove stale label or comment or this will be closed in 7 days

@github-actions github-actions bot added the stale Stale due to inactivity. label Oct 29, 2024
@QxBytes QxBytes removed the stale Stale due to inactivity. label Oct 29, 2024
Copy link

This pull request is stale because it has been open for 2 weeks with no activity. Remove stale label or comment or this will be closed in 7 days

@github-actions github-actions bot added the stale Stale due to inactivity. label Nov 13, 2024
@timraymond
Copy link
Member

@QxBytes stalebot is targeting this PR again. Anything else needed here?

@QxBytes QxBytes removed the stale Stale due to inactivity. label Nov 13, 2024
@QxBytes
Copy link
Contributor Author

QxBytes commented Nov 13, 2024

there is no repro to test this fix on at the moment

Copy link

This pull request is stale because it has been open for 2 weeks with no activity. Remove stale label or comment or this will be closed in 7 days

@github-actions github-actions bot added the stale Stale due to inactivity. label Nov 28, 2024
@QxBytes QxBytes removed the stale Stale due to inactivity. label Dec 2, 2024
@QxBytes QxBytes added the exempt-stale Keep this fresh label Dec 13, 2024
Copy link

This pull request is stale because it has been open for 2 weeks with no activity. Remove stale label or comment or this will be closed in 7 days

@github-actions github-actions bot added the stale Stale due to inactivity. label Dec 28, 2024
Copy link

github-actions bot commented Jan 5, 2025

Pull request closed due to inactivity.

@github-actions github-actions bot closed this Jan 5, 2025
@github-actions github-actions bot deleted the alew/dhcp-win branch January 5, 2025 00:01
@QxBytes QxBytes restored the alew/dhcp-win branch January 6, 2025 17:43
@QxBytes QxBytes reopened this Jan 6, 2025
@QxBytes QxBytes removed stale Stale due to inactivity. exempt-stale Keep this fresh labels Jan 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cni Related to CNI. fix Fixes something. swift-v2 windows
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants