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.

Sign up for GitHub

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

Jump to bottom

Add Networks to InfrastructureConfig #88

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Add Networks to InfrastructureConfig #88

wants to merge 2 commits into from

Conversation

defo89
Copy link
Contributor

@defo89 defo89 commented Nov 28, 2024 •
edited
Loading

Proposed Changes

  • Infrastructure.spec.networks defines all networks and their entities in the infra.
  • We update Infrastructure.status.networking.nodes with Infrastructure.spec.networks.cidr which will propagate to Shoot.status.networking.nodes.
  • Later Infrastructure.spec.networks.name can be referenced by WorkerConfig to signal IPAM implementation which CIDR to use for IP assignment from pool.

Fixes #82

@defo89 defo89 added the enhancement New feature or request label Nov 28, 2024
@defo89 defo89 requested a review from a team as a code owner November 28, 2024 10:31
@github-actions github-actions bot added api-change controllers documentation Improvements or additions to documentation size/L labels Nov 28, 2024
afritzler
afritzler requested changes Nov 28, 2024
View reviewed changes
Copy link
Member

@afritzler afritzler left a comment

Choose a reason for hiding this comment

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

General question: What should happen with this Infrastructure configuration? Is somebody creating a Network with a given ID? How is the link between the machine configuration and the Workers CIDRs configured here? Would it make sense to have the Worker CIDR configured for each worker pool e.g. here: https://github.com/gardener/gardener/blob/master/example/90-shoot.yaml#L66

pkg/apis/metal/types_infrastructure.go Outdated Show resolved Hide resolved
pkg/apis/metal/types_infrastructure.go Outdated Show resolved Hide resolved
@afritzler
Copy link
Member

We should only put into the Infrastructure type only global configuration for a Shoot. The reference to AWS in #82 is somehow misleading as in the AWS provider you can configure on a VPC and subsequent on an AZ level specific subnet configuration - and then the infrastructure controller creates this VPC configuration. For us as I see it the configuration is always on a Worker Pool level as we need to compute out of that an effective Ignition to boot the machine with. Maybe we should start on a per Worker Pool configuration first until we have a better understanding how we want the general networking configuration to happen? Wdyt?

@defo89
Add Networks to InfrastructureConfig
32a37b3 
To update `Infrastructure.status.networking.nodes` which will propagate to `Shoot.status.networking.nodes`
@defo89 defo89 force-pushed the enh/infrastructure-networks branch from 9f3eb58 to 32a37b3 Compare November 28, 2024 14:14
Nuckal777
Nuckal777 reviewed Nov 28, 2024
View reviewed changes
Copy link
Contributor

@Nuckal777 Nuckal777 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I agree with @afritzler. My gut feeling sees the CIDR for workers on the worker pool as well. Does the provider extension contract have a strong opinion here? Edit: Seems so, the field we're interested in is part of the Infrastructure resource

Would it make sense to validate that the worker pool cidr is part of .spec.networking.nodes?

pkg/controller/infrastructure/actuator_reconcile.go Outdated Show resolved Hide resolved
@defo89
Copy link
Contributor Author

defo89 commented Nov 29, 2024 •
edited
Loading

@Nuckal777 Shoot .spec.networking.nodes has a limitation of single string. Using some larger summary is risky because it may overlap with 1) other non-relevant systems 2) Seed clusters which would break some checks on Gardener side.

Also this field is planned to be deprecated.

To solve this issue for us today, our extension would update Infrastructure status.networking.nodes which Gardener copies over to Shoot's status.networking.nodes.

Since networks and their details (CIDR, ID=vlan id) are already defined in Infrastructure, Worker config could just reference the network Name to choose cidr from:

    workers:
    - name: worker-bb123
      network:
        name: network-a
        ipFromPool: true  <-- to signal pool name annotation for ServerClaim

@Nuckal777
Copy link
Contributor

I had a short alignment with @afritzler and @ScheererJ. The network configuration can stay as is in the Infrastructure type, but we would like a optional field on the worker pools, which reference the network the pool is in. This we can pass to the MachineClass of the corresponding pool to make it available to IPAM.

@defo89
Copy link
Contributor Author

defo89 commented Dec 5, 2024

I had a short alignment with @afritzler and @ScheererJ. The network configuration can stay as is in the Infrastructure type, but we would like a optional field on the worker pools, which reference the network the pool is in. This we can pass to the MachineClass of the corresponding pool to make it available to IPAM.

Sure, I think this also matches the example 2 messages above. Should we leave the worker pool config to its own PR when we figure out how the IPAM will look like?

@Nuckal777
Copy link
Contributor

@afritzler I think you need to give this PR a green stamp.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Nuckal777 Nuckal777 Nuckal777 approved these changes

@afritzler afritzler Awaiting requested review from afritzler

Assignees
No one assigned
Labels
api-change controllers documentation Improvements or additions to documentation enhancement New feature or request size/L
Projects
Metal Automation
Status: In progress
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add node ranges config in InfrastructureConfig
3 participants
@defo89 @afritzler @Nuckal777