Skip to content

Conversation

@rrajendran17
Copy link
Contributor

Problem:

New CRD required for introducing host network configs on harvester hosts

Solution:

new crd for creating host network configs on harvester host and its related permissions

Related Issue(s):

harvester/harvester#8101

Test plan:

https://github.com/rrajendran17/harvester-harvester/blob/HEP-ipconfig/enhancements/20251015-support-ipconfig-on-clusternetworks.md

Additional documentation or context

Copilot AI review requested due to automatic review settings January 2, 2026 22:07
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adds a new HostNetworkConfig Custom Resource Definition (CRD) to manage node-level network configurations on Harvester hosts, along with the necessary RBAC permissions to access this resource.

Key Changes:

  • Introduces the HostNetworkConfig CRD supporting both static and DHCP IP assignment modes with VLAN configuration capabilities
  • Updates RBAC permissions for harvester-network-controller and harvester-network-webhook to include access to the new hostnetworkconfigs resource

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 8 comments.

File Description
charts/harvester-network-controller/templates/crds/network.harvesterhci.io_hostnetworkconfigs.yaml New CRD defining the HostNetworkConfig resource with spec for network configuration and status for tracking per-node application state
charts/harvester-network-controller/templates/rbac.yaml Adds "hostnetworkconfigs" to the resources list in both the main controller ClusterRole and webhook ClusterRole for appropriate access permissions

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
name: hostnetworkconfigs.network.harvesterhci.io
Copy link

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

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

The metadata section is missing standard annotations that are present in other CRDs in this repository. For consistency with vlanconfigs, vlanstatuses, linkmonitors, and clusternetworks, this should include an empty annotations object and creationTimestamp set to null.

Suggested change
name: hostnetworkconfigs.network.harvesterhci.io
name: hostnetworkconfigs.network.harvesterhci.io
annotations: {}
creationTimestamp: null

Copilot uses AI. Check for mistakes.
names:
plural: hostnetworkconfigs
singular: hostnetworkconfig
kind: HostNetworkConfig
Copy link

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

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

The names section is missing the 'listKind' field which is present in all other CRDs in this repository. For consistency with vlanconfigs, vlanstatuses, linkmonitors, and clusternetworks, this should include 'listKind: HostNetworkConfigList'.

Suggested change
kind: HostNetworkConfig
kind: HostNetworkConfig
listKind: HostNetworkConfigList

Copilot uses AI. Check for mistakes.
Comment on lines 27 to 29
type: string
kind:
type: string
Copy link

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

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

The apiVersion and kind fields lack the detailed descriptions that are present in other CRDs in this repository. For consistency with vlanconfigs, vlanstatuses, and linkmonitors, these fields should include standard Kubernetes descriptions explaining their purpose.

Suggested change
type: string
kind:
type: string
type: string
description: |
APIVersion defines the versioned schema of this representation of an object.
Servers should convert recognized schemas to the latest internal value, and
may reject unrecognized values. More info:
https://kubernetes.io/docs/reference/using-api/api-overview/
kind:
type: string
description: |
Kind is a string value representing the REST resource this object represents.
Servers may infer this from the endpoint the client submits requests to.
Cannot be updated. In CamelCase. More info:
https://kubernetes.io/docs/reference/using-api/api-overview/

Copilot uses AI. Check for mistakes.
versions:
- name: v1beta1
served: true
storage: true
Copy link

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

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

The CRD is missing additionalPrinterColumns which are present in similar CRDs like vlanconfigs and vlanstatuses. Adding printer columns for key fields such as clusterNetwork, mode, and AGE would improve the user experience when listing resources with kubectl.

Suggested change
storage: true
storage: true
additionalPrinterColumns:
- name: ClusterNetwork
type: string
jsonPath: .spec.clusterNetwork
- name: Mode
type: string
jsonPath: .spec.mode
- name: Age
type: date
jsonPath: .metadata.creationTimestamp

Copilot uses AI. Check for mistakes.
Comment on lines 20 to 96
schema:
openAPIV3Schema:
type: object
description: |
HostNetworkConfig defines node-level network configuration for Harvester.
properties:
apiVersion:
type: string
kind:
type: string
metadata:
type: object

spec:
type: object
required:
- clusterNetwork
- mode
properties:
underlay:
type: boolean
description: |
Indicates whether this configuration represents the underlay network.
Default is false. Only one HostNetworkConfig per node may set this to true.
default: false
clusterNetwork:
type: string
description: Name of the cluster network
vlanID:
type: integer
format: int32
minimum: 2
maximum: 4094
description: VLAN ID associated with the host network
mode:
type: string
description: IP assignment mode
enum:
- static
- dhcp
ips:
type: object
description: Static IP assignments per node (used when mode is static)
additionalProperties:
type: string
description: CIDR-formatted IP address assigned to the node

status:
type: object
description: Observed state of the HostNetworkConfig
properties:
result:
type: string
description: Final result of applying the configuration
enum:
- Success
- Failure
reason:
type: string
description: Reason for failure when result is Failure
nodeStatuses:
type: object
description: Per-node application status
additionalProperties:
type: object
properties:
applied:
type: boolean
description: Whether configuration was successfully applied on the node
ip:
type: string
description: IP address applied to the node
error:
type: string
description: Error message if configuration failed on this node

Copy link

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

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

The schema is missing a top-level 'required' field for the spec property. Other CRDs in this repository (vlanconfigs, linkmonitors, vlanstatuses) include a 'required' array at the root level of the schema to enforce that spec is present. This should be added for consistency and to ensure proper validation.

Copilot uses AI. Check for mistakes.
Comment on lines 60 to 64
ips:
type: object
description: Static IP assignments per node (used when mode is static)
additionalProperties:
type: string
description: CIDR-formatted IP address assigned to the node
Copy link

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

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

The validation for the 'ips' field may need additional constraints. When mode is 'static', the ips field should likely be required, but when mode is 'dhcp', it should not be used. Consider adding validation rules or documenting this relationship more clearly to prevent invalid configurations.

Copilot uses AI. Check for mistakes.
format: int32
minimum: 2
maximum: 4094
description: VLAN ID associated with the host network
Copy link

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

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

The vlanID field has validation constraints (minimum: 2, maximum: 4094) but is not marked as required. The documentation should clarify whether vlanID is optional and, if so, what the behavior is when it's not specified. Alternatively, if vlanID is always required for host network configurations, it should be added to the required array.

Suggested change
description: VLAN ID associated with the host network
description: |
Optional VLAN ID associated with the host network. If omitted, the
host network configuration uses the controller's default VLAN behavior
(for example, it may be treated as an untagged network).

Copilot uses AI. Check for mistakes.
@@ -0,0 +1,95 @@
apiVersion: apiextensions.k8s.io/v1
Copy link

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

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

The CRD file is missing the YAML document separator (---) at the beginning. All other CRD files in this directory (vlanconfigs, vlanstatuses, linkmonitors, clusternetworks) start with "---" on line 1. This should be added for consistency.

Copilot uses AI. Check for mistakes.
@rrajendran17 rrajendran17 force-pushed the hostnetworkconfig branch 4 times, most recently from 5d9d85d to b496234 Compare January 6, 2026 02:48
@innobead
Copy link
Contributor

innobead commented Jan 6, 2026

@rrajendran17
Copy link
Contributor Author

Did you add this new CRD first to https://github.com/harvester/network-controller-harvester/tree/master/pkg/apis/network.harvesterhci.io/v1beta1 and https://github.com/harvester/network-controller-harvester/tree/master/manifests/crds? I assume this PR should come after updating the component repo first.

@innobead yes the corresponding network controller changes are here rrajendran17/network-controller-harvester@7824dcd (work in progress), I will post it for review once complete, Thanks

Comment on lines 37 to 40
additionalProperties:
description: CIDR-formatted IP address assigned to the node
type: string
Copy link
Contributor

Choose a reason for hiding this comment

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

One thing you can consider doing is to use the built-in CEL validation to validate the CIDR format. It makes the API more expressive and ensures K8s rejects any invalid format immediately, without the request ever reaching our webhook.

For example,

ips:
    additionalProperties:
        description: |-
               CIDR is an IP address range in CIDR notation (for example, "10.0.0.0/8" or "fd00::/8").
        maxLength: 18 # required
        type: string
        x-kubernetes-validations:
            - message: Invalid CIDR format provided
              rule: isCIDR(self)

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