Implement dynamic selection of parent prefix from a set of custom fields #90

henrybear327 · 2024-10-08T11:07:59Z

Design

We introduce a new parentPrefixSelector field in the CR. It's a list of custom field key-value mapping, where both the key and value are of the type string. For example

parentPrefixSelector:
    environment: "Production"
    ipVersion: "4"

We now have 3 cases:

If only parentPrefix is set, we continue on as-is
If only parentPrefixSelector is set, we would take all Prefixes that exactly match all custom field's key-value specified in parentPrefixSelector. We would then pick the first prefix that is able to satisfy the prefixLength requirement as the parent prefix.
If we have both parentPrefix and parentPrefixSelector present in the CR, the CRD validation will reject this yaml file

The parentPrefix will be set in the status field ParentPrefix, and this will be used as the source of truth for the rest of the reconcile function.

Known issue

Implement better selection algorithm for the dynamic prefix selection #99
the site value is currently mutable, which we aren't handling properly yet during the allocation and restoration flow -> we need to wait for the conclusion of Document (or revisit) the reason that site is a mutable field #100 before we can properly fix this bug

TODO

Discuss if using status field to store the computed ParentPrefix value is desired
The restoration hash mechanism for parentPrefixSelector would require some documentation, as there might be the case there the picked parent prefix is different for the same parentPrefixSelector (e.g. due to prefix space exhaustion), and thus, the restoration might not work as user anticipated.

Notes

Closes Implement dynamic selection of parent prefix from a set of custom fields #79
Tests will be in a follow-up PR Implement tests for dynamic selection of parent prefix from a set of custom fields #91

internal/controller/prefix_controller.go

internal/controller/prefixclaim_controller.go

henrybear327 · 2024-10-09T07:37:31Z

Discuss if using the status field to store the computed ParentPrefix value is desired

We agree that the spec field is read-only, so we wouldn't put any computed ParentPrefix there.

The status field is what we have write access to and where we reflect the relevant internal states to the external user. Thus, we have decided to use it to store the computed ParentPrefix and use it as the source of truth.

henrybear327 · 2024-10-09T11:54:19Z

Improved CRD validation: According to [2], the oneOf validation will only come in the next version.

cc: @alexandernorth @bruelea

Reference:
[1] https://kubernetes.io/blog/2022/09/29/enforce-immutability-using-cel/
[2] https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/#validation-ratcheting

henrybear327 · 2024-10-10T21:31:12Z

For some reason, when we are querying for custom fields, for example using this URL http://localhost:8080/ipam/prefixes/?q=&cf_poolName=Pool+2&cf_environment=production, the value of the custom field cf_environment will be capitalized automatically. This has an impact on us as we can only enter the value in custom fields in capitalized form.

Reproduction step:

type in http://localhost:8080/ipam/prefixes/?q=&cf_poolName=Pool+2&cf_environment=production in your browser
you can see that the search that went through NetBox is actually http://localhost:8080/ipam/prefixes/?q=&cf_poolName=Pool+2&cf_environment=Production

The demo SQL files have adhered to this "observation", but I am not able to wrap my head around this, since for the restoration hash, we have no issue with it

cc: @alexandernorth @bruelea

henrybear327 · 2024-10-10T21:55:40Z

To maintain the backward compatibility of the restoration hash and extend it to support parentPrefixSelection, we have made the following changes to the hash:

append the parentPrefixSelection key-value pairs to the end of the hash during hash calculation. The exact string that is appended is done by sorting the keys first before appending, so in the case that only the key order of the parentPrefixSelection changes, the hash value won't be affected

With the aforementioned change, the hash can by design distinguish the prefix claimed by ParentPrefix or parentPrefixSelection, thus, making the restoration process relying on the hash achievable.

bruelea · 2024-10-11T08:12:11Z

internal/controller/prefix_controller.go

@@ -132,9 +132,19 @@ func (r *PrefixReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr
 return ctrl.Result{}, err
 }

+ if prefixClaim.Status.ParentPrefix == "" {


Is this check required? I would expect the prefix CR not to be created if the ParentPrefix wasn't calculated.

I am not sure if this is required, but I would like to log this in case we change more code in the future and this condition gets tripped somehow. What do you think?

#90 (comment)

internal/controller/prefixclaim_controller.go

pkg/netbox/api/ip_address_claim.go

api/v1/prefixclaim_types.go

henrybear327 · 2024-10-11T14:24:14Z

Comment from @bruelea: add the newly created sample to the kustomization: config/samples/kustomization.yaml

Done

jitendrs · 2024-10-14T08:56:39Z

Please add me as reviewer.

Introduce new conditions for ParentPrefixSelector Add new custom fields and prefixes for testing ParentPrefixSelector

Signed-off-by: Hoanganh.Mai <[email protected]> Signed-off-by: Chun-Hung Tseng <[email protected]>

henrybear327 · 2024-10-15T08:55:55Z

For some reason, when we are querying for custom fields, for example using this URL http://localhost:8080/ipam/prefixes/?q=&cf_poolName=Pool+2&cf_environment=production, the value of the custom field cf_environment will be capitalized automatically. This has an impact on us as we can only enter the value in custom fields in capitalized form.

Reproduction step:

type in http://localhost:8080/ipam/prefixes/?q=&cf_poolName=Pool+2&cf_environment=production in your browser

you can see that the search that went through NetBox is actually http://localhost:8080/ipam/prefixes/?q=&cf_poolName=Pool+2&cf_environment=Production

The demo SQL files have adhered to this "observation", but I am not able to wrap my head around this, since for the restoration hash, we have no issue with it

cc: @alexandernorth @bruelea

So as @alexandernorth explained, on browsers, they might modify the URLs. But for the API endpoints what we send is directly being processed.

So in the case, this is not an issue for us when querying using API endpoints! But when we are doing debugging using URLs, we might run into issues.

Add unit test for restoration hash backward compatibility Support multiple custom field in a query Improve CRD validation to reject having both ParentPrefix and ParentPrefixSelector Reorder column sequence for PrefixClaim Update kustomization.yaml Notes: According to [2], the `oneOf` validation will only come in the next version Reference: [1] https://kubernetes.io/blog/2022/09/29/enforce-immutability-using-cel/ [2] https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/#validation-ratcheting Signed-off-by: Hoanganh.Mai <[email protected]> Signed-off-by: Chun-Hung Tseng <[email protected]>

jitendrs

Great work! The changes look solid, and everything is well-structured. I believe it aligns with our expectations overall. However, the section concerning site could be improved. I understand there is already open issue #100 for this expectation.

henrybear327 requested a review from bruelea October 8, 2024 11:07

henrybear327 self-assigned this Oct 8, 2024

henrybear327 marked this pull request as draft October 8, 2024 11:08

henrybear327 force-pushed the feat/issue_79 branch 7 times, most recently from 335e04b to 97c512d Compare October 8, 2024 11:25

bruelea reviewed Oct 9, 2024

View reviewed changes

internal/controller/prefix_controller.go Outdated Show resolved Hide resolved

internal/controller/prefixclaim_controller.go Outdated Show resolved Hide resolved

henrybear327 force-pushed the feat/issue_79 branch 2 times, most recently from 787fecd to 1ad0f81 Compare October 10, 2024 21:25

henrybear327 requested a review from bruelea October 10, 2024 21:27

henrybear327 marked this pull request as ready for review October 10, 2024 21:27

henrybear327 force-pushed the feat/issue_79 branch from 1ad0f81 to f65550b Compare October 10, 2024 22:00

bruelea reviewed Oct 11, 2024

View reviewed changes

api/v1/prefixclaim_types.go Outdated Show resolved Hide resolved

henrybear327 force-pushed the feat/issue_79 branch 2 times, most recently from 2181370 to 5c97a94 Compare October 15, 2024 08:45

henrybear327 added 2 commits October 15, 2024 10:46

Introduce ParentPrefixSelector in spec, status

954ed51

Introduce new conditions for ParentPrefixSelector Add new custom fields and prefixes for testing ParentPrefixSelector

Log clean up

d5a3cac

Signed-off-by: Hoanganh.Mai <[email protected]> Signed-off-by: Chun-Hung Tseng <[email protected]>

henrybear327 force-pushed the feat/issue_79 branch from 5c97a94 to 592375a Compare October 15, 2024 08:46

henrybear327 added 2 commits October 15, 2024 11:22

Update the sample yaml

cac0ce6

henrybear327 force-pushed the feat/issue_79 branch from 592375a to cac0ce6 Compare October 15, 2024 09:23

henrybear327 mentioned this pull request Oct 15, 2024

Document (or revisit) the reason that site is a mutable field #100

Open

henrybear327 requested review from jitendrs and CapiSSS October 15, 2024 14:51

jitendrs approved these changes Oct 16, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement dynamic selection of parent prefix from a set of custom fields #90

Implement dynamic selection of parent prefix from a set of custom fields #90

henrybear327 commented Oct 8, 2024 •

edited

Loading

henrybear327 commented Oct 9, 2024

henrybear327 commented Oct 9, 2024

henrybear327 commented Oct 10, 2024 •

edited

Loading

henrybear327 commented Oct 10, 2024

bruelea Oct 11, 2024

henrybear327 Oct 11, 2024

henrybear327 Oct 11, 2024

henrybear327 commented Oct 11, 2024

jitendrs commented Oct 14, 2024

henrybear327 commented Oct 15, 2024 •

edited

Loading

jitendrs left a comment

Implement dynamic selection of parent prefix from a set of custom fields #90

Are you sure you want to change the base?

Implement dynamic selection of parent prefix from a set of custom fields #90

Conversation

henrybear327 commented Oct 8, 2024 • edited Loading

Design

Known issue

TODO

Notes

henrybear327 commented Oct 9, 2024

henrybear327 commented Oct 9, 2024

henrybear327 commented Oct 10, 2024 • edited Loading

henrybear327 commented Oct 10, 2024

bruelea Oct 11, 2024

Choose a reason for hiding this comment

henrybear327 Oct 11, 2024

Choose a reason for hiding this comment

henrybear327 Oct 11, 2024

Choose a reason for hiding this comment

henrybear327 commented Oct 11, 2024

jitendrs commented Oct 14, 2024

henrybear327 commented Oct 15, 2024 • edited Loading

jitendrs left a comment

Choose a reason for hiding this comment

henrybear327 commented Oct 8, 2024 •

edited

Loading

henrybear327 commented Oct 10, 2024 •

edited

Loading

henrybear327 commented Oct 15, 2024 •

edited

Loading