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

Introduce a v1alpha1 DirectResponse CRD #9960

Merged
merged 20 commits into from
Sep 10, 2024

Conversation

timflannagan
Copy link
Contributor

@timflannagan timflannagan commented Aug 27, 2024

Description

Introduces a new v1alpha1 DirectResponseRoute API to the Gloo project. This API allows GW API users to configure direct response functionality through the HTTPRoute resource via extensionRef:

---
apiVersion: gateway.networking.k8s.io/v1
kind: HTTPRoute
metadata:
  name: httpbin
  namespace: httpbin
  labels:
    app: httpbin
spec:
  hostnames:
  - "www.example.com"
  parentRefs:
  - name: http
    namespace: gloo-system
  rules:
  - matches:
    - path:
        type: PathPrefix
        value: /v1
    filters:
    - type: ExtensionRef
      extensionRef:
        group: gateway.gloo.solo.io
        kind: DirectResponse
        name: v1-deprecated-endpoint
  - backendRefs:
    - name: httpbin
      port: 8000
    matches:
    - path:
        type: PathPrefix
        value: /v2
    filters:
    - type: URLRewrite
      urlRewrite:
        path:
          replacePrefixMatch: /
          type: ReplacePrefixMatch
  - backendRefs:
    - name: httpbin
      port: 8000
    matches:
    - path:
        type: PathPrefix
        value: /
---
apiVersion: gateway.gloo.solo.io/v1alpha1
kind: DirectResponse
metadata:
  name: v1-deprecated-endpoint
  namespace: httpbin
spec:
  status: 301
  body: "The /v1 endpoint is deprecated. Please use /v2 instead."

Introducing this functionality helps bridge the gap between what's supported with the classic Edge APIs and the newer GW API integration. Long term, we plan to migrate users to the first-class functionality if upstream decides to adopt it.

Open Questions

  • Do we want to rename this API to DirectResponse or DirectResponseRule?
  • Do we want to model the spec.body field to match Istio or Envoy's configuration? Right now, it's a string type and upstream defines it a struct to provide better extensibility. As an aside, the classic Edge API has skewed by the envoy definition over time
  • Do we want to support users referencing a DRR within a rule's backendRef.filters configuration? Right now, that configuration is ignored by the implementation and no warning/error is bubbled up to the HTTPRoute status

Answered Questions

  • Q: How do we want to handle the scenario where a user configures multiple route actions within a single rule? A: we decided to prohibit this scenario, e.g. backendRef or the rewrite filter are specified, due to how other ecosystem projects implement this. We also want to maintain a stronger validation story to remain backwards compatible to the behavior that upstream implements when/if this functionality gets pushed on upstream
  • Q: Why are implementing a net new API vs. extending existing APIs (e.g. RouteOptions)? We felt it was easier from a medium/long maintenance perspective that it's easier to 1.) deprecate a single API vs. deprecating a single field in a stable API, and 2.) we didn't want to introduce non-policy behavior to the RouteOption API, and 3.) it was easier to migrate users away from this approach in case upstream adopts this functionality. Additionally, we can't entertain the "synthetic reference" approach to avoid introducing or extending APIs as direct response functionality may require multiple inputs (e.g. status code and response body) that's not possible with this approach
  • Q: Do we want to limit the max spec.body length that a user can configure? A: Yes, at least to avoid making backwards incompatible behavior changes in case upstream decides to adopt a different implementation. Right now, it's set to a 4KB limit, which models Envoy's default value. Istio and GME/GMG supports a higher limit of 1MB.

TODO

  • DRY up the translator unit tests
  • Determine whether Edge has a spec.body max length. At a glance, it doesn't since it skewed by envoy's definition over time

API changes

// DirectResponse contains configuration for defining direct response routes.
//
// +kubebuilder:object:root=true
// +kubebuilder:metadata:labels={app=gloo-gateway,app.kubernetes.io/name=gloo-gateway}
// +kubebuilder:resource:categories=gloo-gateway,shortName=drr
// +kubebuilder:subresource:status
type DirectResponse struct {
	metav1.TypeMeta   `json:",inline"`
	metav1.ObjectMeta `json:"metadata,omitempty"`

	Spec   DirectResponseSpec   `json:"spec,omitempty"`
	Status DirectResponseStatus `json:"status,omitempty"`
}

// +kubebuilder:object:root=true
type DirectResponseList struct {
	metav1.TypeMeta `json:",inline"`
	metav1.ListMeta `json:"metadata,omitempty"`
	Items           []DirectResponse `json:"items"`
}

// DirectResponseSpec describes the desired state of a DirectResponse.
type DirectResponseSpec struct {
	// Status defines the HTTP status code to return for this route.
	//
	// +kubebuilder:validation:Required
	// +kubebuilder:validation:Minimum=200
	// +kubebuilder:validation:Maximum=599
	Status uint32 `json:"status"`
	// Body defines the content to be returned in the HTTP response body.
	// The maximum length of the body is restricted to prevent excessively large responses.
	//
	// +kubebuilder:validation:MaxLength=4096
	// +kubebuilder:validation:Optional
	Body string `json:"body,omitempty"`
}

Known Limitations

  • Users cannot configure multiple route actions for a single rule. This includes configuring a backendRef or the RequestRedirect filter in the same rule that references the DRR extension filter. In this case, the route will be replaced and produce an error on the HTTPRoute status
    • Similarly, users referencing a DRR in the backendRef.filters field is currently unsupported and that configuration will be ignored by Gloo.
  • An HTTPRoute rule that references multiple DRR extensions will produce an error and be route replaced
  • Schema validation
    • The spec.body field supports values up to 4KB. This behavior is consistent with Envoy's default
    • The spec.status field supports the 200-599 status codes. This behavior is consistent with Istio/Envoy
  • No status is being populated for the new DRR resource
  • The generated CRD does not have populated field descriptions.
    • This is a known gap for any kubebuilder-based APIs. Right now, in generate.go we set the crd:maxDescLen=0 controller-gen marker that disables this functionality
    • We can revisit this behavior as potential follow-up work
  • This API does not work with classic Edge APIs. It's only applicate to the GW API extension
  • TODO: incompatible with URLRewrite and RequestHeaderModifier. Nina pointed out that those filters require the backendRef to be configured, and configuring a backendRef and DR on the same rule is unsupported.

Code changes

Introduces a new route plugin that's responsible for translating DRR resources that are being referenced by valid HTTPRoute route rules. We discussed implementing this functionality directly in the HTTP route table translator, but decided against that approach since 1.) we implemented the redirect filter as a plugin, which also modifies the output route action, and 2.) we're implementing this functionality as an extension filter. Longer term, we can determine whether this approach is still feasible if upstream introduces this as a first-class route rule.

CI changes

Introduces a new test/kubernetes/e2e feature suite that runs through some basic use examples.

Docs changes

N/A. This is being tracked in https://github.com/solo-io/docs/issues/450.

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works

@solo-changelog-bot
Copy link

Issues linked to changelog:
#9774

@timflannagan timflannagan force-pushed the feat/introduce-direct-response branch 2 times, most recently from 095d21b to 526f927 Compare August 28, 2024 22:36
@timflannagan timflannagan marked this pull request as ready for review August 28, 2024 22:37
This commit introduces a new v1alpha1 CRD that allows users
to configure direct response functionality within the K8S
Gateway API integration.

Signed-off-by: timflannagan <[email protected]>
Signed-off-by: timflannagan <[email protected]>
@timflannagan timflannagan added the keep pr updated signals bulldozer to keep pr up to date with base branch label Sep 3, 2024
Copy link
Contributor

@lgadban lgadban left a comment

Choose a reason for hiding this comment

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

looks great; dropping some comments for my first pass

//
// +kubebuilder:validation:MaxLength=4096
// +kubebuilder:validation:Optional
Body string `json:"body,omitempty"`
Copy link
Contributor Author

@timflannagan timflannagan Sep 3, 2024

Choose a reason for hiding this comment

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

We may want to model the Istio/Envoy approach and support an enum value here instead:

message HTTPDirectResponse {
  // Specifies the HTTP response status to be returned.
  uint32 status = 1 [(google.api.field_behavior) = REQUIRED];


  // Specifies the content of the response body. If this setting is omitted,
  // no body is included in the generated response.
  HTTPBody body = 2;
}


message HTTPBody {
  oneof specifier {
    // response body as a string
    string string = 1;


    // response body as base64 encoded bytes.
    bytes bytes = 2;
  }
}

https://github.com/istio/api/blob/0a8281c5589825c93389ddf1849725bb2bf07c4f/networking/v1alpha3/virtual_service.proto#L1167-L1184

Copy link
Contributor

@npolshakova npolshakova left a comment

Choose a reason for hiding this comment

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

Looks good! 🎉 Just had a couple questions!

@timflannagan timflannagan changed the title Introduce a v1alpha1 DirectResponseRoute CRD Introduce a v1alpha1 DirectResponse CRD Sep 6, 2024
Copy link
Contributor

@lgadban lgadban left a comment

Choose a reason for hiding this comment

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

non-blocking final comments/questions

Copy link
Contributor

@lgadban lgadban left a comment

Choose a reason for hiding this comment

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

looks good to me, great work!

}))
Expect(routes[0].Matchers[0].PathSpecifier).To(Equal(&matchers.Matcher_Prefix{Prefix: "/"}))

routeStatus := reportsMap.BuildRouteStatus(ctx, route, "")
Copy link
Contributor

Choose a reason for hiding this comment

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

we would also have ResolvedRefs false condition right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I'll do a fast follow that asserts on both the Accepted and ResolvedRef types. I think we'd want to do this for every test case throughout the suite.

@timflannagan timflannagan enabled auto-merge (squash) September 10, 2024 13:55
@timflannagan timflannagan merged commit 4b42112 into solo-io:main Sep 10, 2024
17 checks passed
@timflannagan timflannagan deleted the feat/introduce-direct-response branch September 10, 2024 15:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
keep pr updated signals bulldozer to keep pr up to date with base branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants