-
Notifications
You must be signed in to change notification settings - Fork 2
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
Implement dynamic selection of parent prefix from a set of custom fields #90
base: main
Are you sure you want to change the base?
Conversation
335e04b
to
97c512d
Compare
We agree that the The |
Improved CRD validation: According to [2], the Reference: |
787fecd
to
1ad0f81
Compare
For some reason, when we are querying for custom fields, for example using this URL Reproduction step:
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 |
To maintain the backward compatibility of the restoration hash and extend it to support parentPrefixSelection, we have made the following changes to the hash:
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. |
1ad0f81
to
f65550b
Compare
@@ -132,9 +132,19 @@ func (r *PrefixReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr | |||
return ctrl.Result{}, err | |||
} | |||
|
|||
if prefixClaim.Status.ParentPrefix == "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this check required? I would expect the prefix CR not to be created if the ParentPrefix wasn't calculated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done |
Please add me as reviewer. |
2181370
to
5c97a94
Compare
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]>
5c97a94
to
592375a
Compare
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]>
592375a
to
cac0ce6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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 typestring
. For exampleWe now have 3 cases:
parentPrefix
is set, we continue on as-isparentPrefixSelector
is set, we would take all Prefixes that exactly match all custom field's key-value specified inparentPrefixSelector
. We would then pick the first prefix that is able to satisfy theprefixLength
requirement as the parent prefix.parentPrefix
andparentPrefixSelector
present in the CR, the CRD validation will reject this yaml fileThe parentPrefix will be set in the
status
fieldParentPrefix
, and this will be used as the source of truth for the rest of the reconcile function.Known issue
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 thatsite
is a mutable field #100 before we can properly fix this bugTODO
status
field to store the computedParentPrefix
value is desiredparentPrefixSelector
would require some documentation, as there might be the case there the picked parent prefix is different for the sameparentPrefixSelector
(e.g. due to prefix space exhaustion), and thus, the restoration might not work as user anticipated.Notes