Skip to content

secondary networks for servers - #185

Open
hown3d wants to merge 4 commits into
mainfrom
secondary-networks
Open

secondary networks for servers#185
hown3d wants to merge 4 commits into
mainfrom
secondary-networks

Conversation

@hown3d

@hown3d hown3d commented Jul 6, 2026

Copy link
Copy Markdown
Member

How to categorize this PR?

/kind enhancement

What this PR does / why we need it:
Support multiple additional networks for a server.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Breaking changes:

@ske-prow ske-prow Bot added the kind/enhancement Enhancement, improvement, extension label Jul 6, 2026
@ske-prow

ske-prow Bot commented Jul 6, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign xoxys for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ske-prow ske-prow Bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 6, 2026
@hown3d
secondary networks for servers
d849c35 
Signed-off-by: Lukas Hoehl <lukas.hoehl@stackit.cloud>
@hown3d
hown3d force-pushed the secondary-networks branch from 3913699 to d849c35 Compare July 6, 2026 14:55
@mstueer

mstueer commented Jul 7, 2026

Copy link
Copy Markdown

Looks fine, but tests are missing?

@hown3d
tests
1a8da19 
Signed-off-by: Lukas Hoehl <lukas.hoehl@stackit.cloud>
@ske-prow ske-prow Bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 7, 2026
@hown3d

hown3d commented Jul 7, 2026

Copy link
Copy Markdown
Member Author

I added some tests

breuerfelix
breuerfelix reviewed Jul 8, 2026
View reviewed changes
Comment thread pkg/provider/apis/validation/validation.go
breuerfelix
breuerfelix reviewed Jul 15, 2026
View reviewed changes
Comment thread docs/machine-class.md Outdated
Comment thread pkg/provider/apis/validation/validation.go
Comment thread pkg/provider/create.go
return nil, status.Error(codes.Unavailable, fmt.Sprintf("failed to patch NICs for server: %v", err))
}

if providerSpec.Networking != nil && len(providerSpec.Networking.SecondaryNetworkIDs) > 0 {

@breuerfelix breuerfelix Jul 15, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

doesn't this has to happen before patching the networkinterfaces?

if this happens after patchNetworkInterface, the address from the new interface is not reflected in the CreateMachineResponse.Addresses and also it does not get the allowedAddresses patched.

This also means in the ensureAdditionalNetworks we have to attach the network and then wait until all networks are attached since this operation is async.
If we do not wait until all networks are attached, the patch function could skip some nics.

All this "waiting" feels a little cumbersome but its just how the create interface is designed. One call to rule em all.

@hown3d hown3d Aug 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We should think about how we want to implement secondary networks in general.
In my idea, the primary network would be the one designed to route pod traffic. So this should be the only one where allowedAddresses are set to the pod CIDR.

Returning the address of the secondary network in the addresses might be something we want to do but also not want to use. These IPs are used by gardener for example to create SelfHostedShoot exposure resources.
Would a user want to use only the primary network for this or all networks where a server is attached to.

/cc @dergeberl

@stackit-ske

stackit-ske commented Jul 30, 2026

Copy link
Copy Markdown

Our project currently lacks enough active contributors to adequately respond to all PRs.
This bot triages PRs according to the following rules:

  • After 15d of inactivity, lifecycle/stale is applied
  • After 15d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 7d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Mark this PR as fresh with /remove-lifecycle stale
  • Mark this PR as rotten with /lifecycle rotten
  • Close this PR with /close

/lifecycle stale

@ske-prow ske-prow Bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 30, 2026
@dergeberl

dergeberl commented Jul 31, 2026

Copy link
Copy Markdown
Member

/remove-lifecycle stale

@ske-prow ske-prow Bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 31, 2026
@ske-prow
ske-prow Bot requested a review from dergeberl August 3, 2026 11:01
hown3d added 2 commits August 3, 2026 13:05
@hown3d
docs: rephrase networkingSpec
2685e2d 
Signed-off-by: Lukas Hoehl <lukas.hoehl@stackit.cloud>
@hown3d
validate secondary network != networkId
fa72f5a 
Signed-off-by: Lukas Hoehl <lukas.hoehl@stackit.cloud>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@breuerfelix breuerfelix breuerfelix left review comments
@dergeberl dergeberl Awaiting requested review from dergeberl

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

kind/enhancement Enhancement, improvement, extension size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@hown3d @mstueer @stackit-ske @dergeberl @breuerfelix