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

Validate fields in IPPool objects upon creation and updates #18

Merged
merged 4 commits into from
Feb 16, 2024

Conversation

starbops
Copy link
Member

@starbops starbops commented Jan 31, 2024

IMPORTANT: Please do not create a Pull Request without creating an issue first.

Problem:

Solution:

If a user tries to configure the embedded DHCP server with a designated IP address, the IPPool validator will validate the input. The create/update requests will be rejected if:

  • The server IP is not within the defined subnet
  • The server IP is the same as the network IP or broadcast IP of the subnet
  • The server IP is the same as the router IP (if exists)
  • The server IP collides with any already-allocated IPs

Related Issue:

harvester/harvester#5065

Test plan:

  1. Install the vm-dhcp-controller chart with the version containing the fix

    • For reviewers: build a custom image based on the branch and install it with the chart directly
    • For QAs: the fix has already merged and released in v0.2.0 and could be activated by installing the harvester-vm-dhcp-controller add-on with the following value content and enable it
      apiVersion: harvesterhci.io/v1beta1
      kind: Addon
      metadata:
        name: harvester-vm-dhcp-controller
        namespace: harvester-system
        labels:
          addon.harvesterhci.io/experimental: "true"
      spec:
        enabled: false
        repo: https://charts.harvesterhci.io
        version: "v0.2.0"
        chart: harvester-vm-dhcp-controller
        valuesContent: |
          image:
            tag: "v0.2.0"
          agent:
            image:
              tag: "v0.2.0"
          webhook:
            image:
              tag: "v0.2.0"
      
  2. Create a VM Network (NAD) before creating any IPPool objects

  3. Create an IPPool object with the server IP outside of the subnet (should be rejected by the webhook)

    $ cat <<EOF | kubectl apply -f -
    apiVersion: network.harvesterhci.io/v1alpha1
    kind: IPPool
    metadata:
      namespace: default
      name: test-net
    spec:
      ipv4Config:
        serverIP: 172.18.100.100
        cidr: 172.19.0.0/16
        pool:
          start: 172.19.0.2
          end: 172.19.254.254
        router: 172.19.100.254
        leaseTime: 300
      networkName: default/test-net
    EOF
    
    Error from server (InternalError): error when creating "STDIN": admission webhook "validator.harvester-system.harvester-vm-dhcp-controller-webhook" denied the request: Internal error occurred: could not create IPPool default/test-net because server ip 172.18.100.100 is not within subnet
    
  4. Modify the .spec.serverIP to the same IP as the network IP, broadcast IP, and router IP, then try to create it with the manifest (all should be rejected by the webhook)

  5. Modify the .spec.serverIP to a sane IP address, i.e., within the subnet, but not collide with the network IP, broadcast IP, or router IP, then create it with the manifest (this should be successful)

  6. Create a VM attaching to the VM Network, which should have the vmnetcfg object created under the hood

  7. Check the IPPool object to see which IP address is allocated to the VM's interface (under .status.ipv4.allocated)

  8. Modify the .spec.serverIP of the IPPool object, updating it to the VM's IP (should be rejected by the webhook)

@starbops starbops marked this pull request as ready for review January 31, 2024 08:53
@starbops starbops marked this pull request as draft February 1, 2024 11:04
ibrokethecloud
ibrokethecloud previously approved these changes Feb 1, 2024
Copy link
Contributor

@ibrokethecloud ibrokethecloud left a comment

Choose a reason for hiding this comment

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

lgtm. thanks.

@starbops
Copy link
Member Author

starbops commented Feb 2, 2024

The recent commits address invalid user inputs not only limited to the serverIP field but also other mandatory fields, using CRD validation marks (thanks to @ibrokethecloud) and some custom logic implemented in the validating webhook.

@starbops starbops changed the title fix(webhook): add server ip validation Validate fields in IPPool objects upon creation and updates Feb 2, 2024
@starbops starbops marked this pull request as ready for review February 2, 2024 10:04
Copy link
Member

@w13915984028 w13915984028 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

If a user tries to configure the embedded DHCP server with a designated
IP address, the IPPool validator will validate the input. The create/
update request will be rejected if:

- The server IP is not within the defined subnet
- The server IP is the same as the network IP or broadcast IP of the
  subnet
- The server IP is the same as the router IP (if exists)
- The server IP collides with any already-allocated IPs

Signed-off-by: Zespre Chang <zespre.chang@suse.com>
Signed-off-by: Zespre Chang <zespre.chang@suse.com>
Adding validations against most of the fields. Also, change the way
loading CRDs at controller start-up. This is because CRD manifests are
now generated using controller-gen, not Wrangler codegen. The reason
behind it is that, currently only controller-gen supports CRD
validation using CEL.

Signed-off-by: Zespre Chang <zespre.chang@suse.com>
Signed-off-by: Zespre Chang <zespre.chang@suse.com>
@starbops
Copy link
Member Author

Merge conflict resolved.

Copy link
Contributor

@ibrokethecloud ibrokethecloud left a comment

Choose a reason for hiding this comment

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

lgtm. thanks.

@ibrokethecloud ibrokethecloud self-requested a review February 16, 2024 02:59
@starbops starbops merged commit 19f246c into harvester:main Feb 16, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants