Skip to content

Conversation

@vaishutin
Copy link

This PR introduces support for managing tags on IPAM/Prefix resources in the Netbox Operator.
• Extended the CRD to include a tags field in the PrefixSpec.
• Implemented tag handling logic in the Netbox client and controller.
• Ensures that tags are created/updated/removed accordingly when syncing Prefix objects with NetBox.

Why:
Tags are widely used in NetBox to group and organize objects. Adding tag support to the operator makes Prefix management more consistent with how NetBox is typically used, and allows users to fully automate tagging via Kubernetes manifests.

Notes:
• Backwards-compatible change (resources without tags continue to work as before).
• Includes CRD schema updates and regeneration of api/v1.

Copy link
Collaborator

@jstudler jstudler left a comment

Choose a reason for hiding this comment

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

Hey @vaishutin

Thanks a lot for your proposal and this MR. I think we can generally allow tags to be added to NetBox Operator resources since it will add value to some users and as long as it will be non-breaking.

After an initial review of this MR I have a few comments and questions:

  • Generally, I think it would make sense that this aligns with other usages of NetBox slug/name combinations in the Operator. With site and tenant we use the "name" value and not the "slug" in CRs. Example: https://github.com/netbox-community/netbox-operator/blob/main/api/v1/prefix_types.go#L34-L43
  • If we stick to supporting both slug and name, we should verify on the kube-api that the tags either use a slug or a name. This can be done using XValidation rules. Example: // +kubebuilder:validation:XValidation:rule="(!has(self.parentPrefix) && has(self.parentPrefixSelector)) || (has(self.parentPrefix) && !has(self.parentPrefixSelector))" https://github.com/netbox-community/netbox-operator/blob/main/api/v1/prefixclaim_types.go#L26
  • If we support tags in the Prefix CRs, we should add it as well in the PrefixClaim.
  • NetBox IP Ranges and IP Addresses also support tags, I propose to add it as well in IpAddress, IpAddressClaim, IpRange, IpRangeClaim.
  • Please add new e2e or integration tests that test the creation of resources with tags.
  • If tags are mutable in the Kubernetes CR, we should also test mutating the tags in the e2e or integration tests. If tags are immutable, please add a validation rule. Example: //+kubebuilder:validation:XValidation:rule="self == oldSelf",message="Field 'parentPrefixSelector' is immutable" https://github.com/netbox-community/netbox-operator/blob/main/api/v1/prefixclaim_types.go#L40

What do you think?

Tenant string `json:"tenant,omitempty"`

// A list of tags that will be assigned to the resource in NetBox.
// Each tag may contain either the `name` or `slug` field (one of them is required).
Copy link
Collaborator

Choose a reason for hiding this comment

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

If it's either name or slug we should add a XValidation rule for this. The rule could probably be on the Tag struct.

request = extras.NewExtrasTagsListParams().WithSlug(&slug)
}

if name == "" && slug == "" {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This case should be prevented during creation on kube-api using XValidation rule in the api/CRD.

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.

2 participants