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.

Sign up for GitHub

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

Jump to bottom

update validations and status on route reconciler #563

Merged
merged 2 commits into from
Dec 7, 2023
Merged

update validations and status on route reconciler #563

merged 2 commits into from
Dec 7, 2023

Conversation

mikhail-aws
Copy link
Contributor

@mikhail-aws mikhail-aws commented Dec 7, 2023 •
edited
Loading

Note:
Add Gateway API Spec validations to route controller. Spec is not valid if there is at least single invalid property in ParentRef or BackendRef.

Tests:

Example for new status:

Status:
  Parents:
    Conditions:
      Last Transition Time:  2023-12-07T19:32:33Z
      Message:
      Observed Generation:   5
      Reason:                Accepted
      Status:                True
      Type:                  Accepted
      Last Transition Time:  2023-12-07T19:32:33Z
      Message:               backendRef name: inventory-ver1
      Observed Generation:   5
      Reason:                BackendNotFound
      Status:                False
      Type:                  ResolvedRefs
    Controller Name:         application-networking.k8s.aws/gateway-api-controller
    Parent Ref:
      Group:         gateway.networking.k8s.io
      Kind:          Gateway
      Name:          my-hotel
      Section Name:  http

close #544
close #540
close #277

@xWink
Copy link
Member

xWink commented Dec 7, 2023

Is the message of an Accepted status supposed to state the backendref's name in the spec, or is this a custom decision?

xWink
xWink reviewed Dec 7, 2023
View reviewed changes
pkg/controllers/route_controller.go
// There are 3 condition types: Accepted, PartiallyInvalid, ResolvedRefs.
// We dont support PartiallyInvalid for now and reject entire route if there is at least one invalid field.
// Accepted type is related to parentRefs, and ResolvedRefs to backendRefs. These 2 are validated independently.
func (r *routeReconciler) validateRoute(ctx context.Context, route core.Route) error {
Copy link
Member

Choose a reason for hiding this comment

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

Variable names in this function are hard to understand. Let's please stick to camelCase words

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

@mikhail-aws
Copy link
Contributor Author

Is the message of an Accepted status supposed to state the backendref's name in the spec, or is this a custom decision?

message is primarily for human readable error details

  • if it's not found indicate what is not found
  • if it's error kind indicate which kind it's and may be provide list of valid kinds
    etc.

xWink
xWink reviewed Dec 7, 2023
View reviewed changes
pkg/model/core/grpcroute.go Outdated
@@ -79,6 +79,13 @@ func (r *GRPCRoute) Inner() *gwv1alpha2.GRPCRoute {
return &r.r
}

func (r *GRPCRoute) GK() metav1.GroupKind {
Copy link
Member

Choose a reason for hiding this comment

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

Why GK() instead of GroupKind()?

Copy link
Contributor Author

@mikhail-aws mikhail-aws Dec 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

its GK or GKV in other places of kubernetes: controller-runtime for example

Copy link
Member

Choose a reason for hiding this comment

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

I am only able to find examples of GroupKind(), such as here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://pkg.go.dev/sigs.k8s.io/[email protected]/pkg/client/apiutil

if it bothers you I can change

Copy link
Member

Choose a reason for hiding this comment

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

That would be my preference, primarily for readability

@mikhail-aws mikhail-aws requested a review from xWink December 7, 2023 22:22
@mikhail-aws mikhail-aws merged commit 771efb5 into aws:main Dec 7, 2023
2 checks passed
@mikhail-aws mikhail-aws deleted the http-status branch December 7, 2023 22:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xWink xWink xWink approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@mikhail-aws @xWink