Skip to content
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 gateway class controller in Rust #360

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

EandrewJones
Copy link
Contributor

This PR implements the gateway class controller in Rust.

It does the following:

  • Creates a GatewayClass controller that marks managed GatewayClass resources as accepted
  • Updates the Gateway controller to verify that any GatewayClass is accepted before programming routes
  • Defines HasConditions trait to enable the creation and usage of a generic set_condition util function across both our existing controllers.

To Do:

  • Add integration tests for GatewayClass and Gateway which mirror our previous Golang-based tests

Closes #300.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 4, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: EandrewJones
Once this PR has been reviewed and has the lgtm label, please assign astoycos for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot requested a review from shaneutt February 4, 2025 06:17
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 4, 2025
@EandrewJones EandrewJones marked this pull request as draft February 4, 2025 06:18
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 4, 2025
@EandrewJones
Copy link
Contributor Author

@shaneutt @aryan9600 Looking for some light guidance to make sure this is on track before I go ahead with writing the integration tests.

Copy link
Member

@shaneutt shaneutt left a comment

Choose a reason for hiding this comment

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

Took a first pass, looking good! Mostly minor comments.

controlplane/src/gateway_controller.rs Show resolved Hide resolved
"GatewayClass {:?} not yet accepted",
gateway_class.name_any()
);
return Ok(Action::await_change());
Copy link
Member

Choose a reason for hiding this comment

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

So right now I think this will introduce a bug because below in our controller function I don't see that we've set up any watch mechanism that will re-enqueue the Gateway once the GatewayClass is finally accepted. We would need that to make sure that GatewayClass changes trigger their connected Gateways getting enqueued 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could we just re-enqueue here? Wouldn't that cause the gateway to keep re-enqueuing until it's GatewayClass was accepted?

That's not ideal because it's tantamount to naively long polling for changes to your GatewayClass that may never come versus the GatewayClass change driving the behavior.

Is there a an API for the watch mechanism you describe? If so, I can implement that.

Copy link
Member

Choose a reason for hiding this comment

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

Could we just re-enqueue here? Wouldn't that cause the gateway to keep re-enqueuing until it's GatewayClass was accepted?
That's not ideal because it's tantamount to naively long polling for changes to your GatewayClass that may never come versus the GatewayClass change driving the behavior.

It still wouldn't be quite right because of the problem you described, and in a case where something is broken it would just endlessly re-enqueue.

Is there a an API for the watch mechanism you describe? If so, I can implement that.

Yes, there are ways to watch for changes to related or owned objects, and then enqueue the related object when the watch object changes. Look at the owns() (for when there's specifically an owner relationship) and watches() functions and other related functions in kube::runtime::Controller, there are examples in there as well. I think what we'll probably want to do is run a watch on GatewayClass that re-enqueues any related Gateways when that class changes whenever it matches our controllerName 🤔

controlplane/src/gateway_controller.rs Show resolved Hide resolved
controlplane/src/gatewayclass_controller.rs Outdated Show resolved Hide resolved
controlplane/src/gatewayclass_controller.rs Outdated Show resolved Hide resolved
Comment on lines +44 to +49
let mut gwc = GatewayClass {
metadata: gateway_class.metadata.clone(),
spec: gateway_class.spec.clone(),
status: gateway_class.status.clone(),
// NOTE: Am I missing anything else here?
};
Copy link
Member

Choose a reason for hiding this comment

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

If the goal is to make a full clone, you should be able to do that with Deref and clone:

Suggested change
let mut gwc = GatewayClass {
metadata: gateway_class.metadata.clone(),
spec: gateway_class.spec.clone(),
status: gateway_class.status.clone(),
// NOTE: Am I missing anything else here?
};
let mut gwc = (*gateway_class).clone();

Copy link
Contributor Author

@EandrewJones EandrewJones Feb 5, 2025

Choose a reason for hiding this comment

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

This is indeed the aim, unless I can get away with not cloning at all. But we ultimately would need a thread-safe mutable reference (Arc<Mutex<GatewayClass>>) to our gateway_class input then, but AFAICT from the kube.rs docs, reconcile will always be passed an Arc<> of the resource it's watching. Is that so?

Copy link
Member

Choose a reason for hiding this comment

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

It will always be an Arc, so if you want the inner of an arc you can do a deref clone (as shown above) OR an as_ref().clone().

controlplane/src/gatewayclass_controller.rs Outdated Show resolved Hide resolved
controlplane/src/gatewayclass_controller.rs Show resolved Hide resolved
@shaneutt shaneutt added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Feb 5, 2025
EandrewJones and others added 4 commits February 5, 2025 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
Status: Review
Development

Successfully merging this pull request may close these issues.

Rust ControlPlane: GatewayClass Controller
3 participants