Add check for tenant existence

[MaIT-HgA]

• Tenancy check is added in getting available IP and prefix
• Failed tenancy check doesn't imply reconcile error but create a k8s Event
• Test cases for the tenant check are added in unit tests
• Not-found Netbox error "type" is added in utils
• Error in some functions are updated with the new error "type"
• Manual system testing is done.

Close #39

[bruelea]

The check is only required if a new ip address is assigned. Instead of directly checking this in the reconcile loop we could check it in the GetAvailableIpAddressClaim (

netbox-operator/pkg/netbox/api/ip_address_claim.go

Line 59 in 508165e

func (r *NetboxClient) GetAvailableIpAddressByClaim(ipAddressClaim *models.IPAddressClaim) (*models.IPAddress, error) {

) function.

[bruelea]

It would be great to have this check also for the prefix claims.

[faebr]

I just did a manual check, in my opinion the operator should not log the stack trace when the tenant is not found. This makes it look like a reconciler error. Its ok when the loglevel is error but adding the stack trace is not needed.
Example:
2024-09-26T13:48:31+02:00 INFO reconcile loop started {"controller": "ipaddressclaim", "controllerGroup": "netbox.dev", "controllerKind": "IpAddressClaim", "IpAddressClaim": {"name":"ipaddressclaim-sample-2","namespace":"default"}, "namespace": "default", "name": "ipaddressclaim-sample-2", "reconcileID": "6d11f8c1-78bf-44ca-a2b7-01b42f3d7459"}

2024-09-26T13:48:31+02:00 ERROR Reconciler error {"controller": "ipaddressclaim", "controllerGroup": "netbox.dev", "controllerKind": "IpAddressClaim", "IpAddressClaim": {"name":"ipaddressclaim-sample-2","namespace":"default"}, "namespace": "default", "name": "ipaddressclaim-sample-2", "reconcileID": "6d11f8c1-78bf-44ca-a2b7-01b42f3d7459", "error": "tenant not found"}

sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller[...]).reconcileHandler /Users/taascfas/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:316 sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller[...]).processNextWorkItem /Users/taascfas/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:263 sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller[...]).Start.func2.2 /Users/taascfas/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:224

Also when a claim succeeds a status condition is set, would it make sense to also set a status condition when tenant is not found?

I see that this is the same when the parent-prefix does not exist, could we also change this? @bruelea @alexandernorth @jstudler
2024-09-26T13:54:05+02:00 ERROR Reconciler error {"controller": "ipaddressclaim", "controllerGroup": "netbox.dev", "controllerKind": "IpAddressClaim", "IpAddressClaim": {"name":"ipaddressclaim-sample","namespace":"default"}, "namespace": "default", "name": "ipaddressclaim-sample", "reconcileID": "7204d041-cc55-40d4-80d1-bc252fcbcdd3", "error": "parent prefix not found"}

sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller[...]).reconcileHandler /Users/taascfas/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:316 sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller[...]).processNextWorkItem /Users/taascfas/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:263 sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller[...]).Start.func2.2 /Users/taascfas/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:224

[bruelea]

Also when a claim succeeds a status condition is set, would it make sense to also set a status condition when tenant is not found?

Agree, the status condition should be updated too. In #74 this is implemented.

[MaIT-HgA]

the status condition and the events will be implemented in #74

[jstudler]

Maybe I'm getting it wrong but wouldn't the simplest solution be to just add a function like e.g. "TenantExists" which takes a tenant string and returns a bool and then check this in e.g. https://github.com/netbox-community/netbox-operator/blob/main/internal/controller/ipaddressclaim_controller.go#L79 and https://github.com/netbox-community/netbox-operator/blob/main/internal/controller/ipaddress_controller.go#L103 (and the respective location in the prefix controllers)? It's kind of a config validation and should happen early in the process. If it doesn't exist, we write a log entry and exit.

With the proposed solution it will create a lease and add other complexity that we don't need. wdyt @MaIT-HgA @faebr @bruelea ?

[bruelea]

Maybe I'm getting it wrong but wouldn't the simplest solution be to just add a function like e.g. "TenantExists" which takes a tenant string and returns a bool and then check this in e.g. https://github.com/netbox-community/netbox-operator/blob/main/internal/controller/ipaddressclaim_controller.go#L79 and https://github.com/netbox-community/netbox-operator/blob/main/internal/controller/ipaddress_controller.go#L103 (and the respective location in the prefix controllers)? It's kind of a config validation and should happen early in the process. If it doesn't exist, we write a log entry and exit.

With the proposed solution it will create a lease and add other complexity that we don't need. wdyt @MaIT-HgA @faebr @bruelea ?

That would work too. The motivation to do the check if the tenant exists was that the check is only required if the IP is not yet assigned (#72 (review)). We could check the "IPAssigned" status condition in the reconcile loop and check if the tenant exists if the status condition is "false".

[jstudler]

Maybe I'm getting it wrong but wouldn't the simplest solution be to just add a function like e.g. "TenantExists" which takes a tenant string and returns a bool and then check this in e.g. https://github.com/netbox-community/netbox-operator/blob/main/internal/controller/ipaddressclaim_controller.go#L79 and https://github.com/netbox-community/netbox-operator/blob/main/internal/controller/ipaddress_controller.go#L103 (and the respective location in the prefix controllers)? It's kind of a config validation and should happen early in the process. If it doesn't exist, we write a log entry and exit.
With the proposed solution it will create a lease and add other complexity that we don't need. wdyt @MaIT-HgA @faebr @bruelea ?

That would work too. The motivation to do the check if the tenant exists was that the check is only required if the IP is not yet assigned (#72 (review)). We could check the "IPAssigned" status condition in the reconcile loop and check if the tenant exists if the status condition is "false".

True, this is a valid concern for the (rare?) case that the tenant is removed from NetBox after the ip/ipc/px/pxc resource(s) have been sucessfully created. So maybe we keep it as is.

[MaIT-HgA]

Maybe I'm getting it wrong but wouldn't the simplest solution be to just add a function like e.g. "TenantExists" which takes a tenant string and returns a bool and then check this in e.g. https://github.com/netbox-community/netbox-operator/blob/main/internal/controller/ipaddressclaim_controller.go#L79 and https://github.com/netbox-community/netbox-operator/blob/main/internal/controller/ipaddress_controller.go#L103 (and the respective location in the prefix controllers)? It's kind of a config validation and should happen early in the process. If it doesn't exist, we write a log entry and exit.

With the proposed solution it will create a lease and add other complexity that we don't need. wdyt @MaIT-HgA @faebr @bruelea ?

This is what what I did at the very beginning but then @bruelea suggested to do it in the function getting available IP/prefix since we only need to do the check for new ip assignment

[MaIT-HgA]

Finalizer changes is removed from this MR and will be in another MR

[bruelea]

Looks good!