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

5579 extended http methods #9633

Merged
merged 37 commits into from
Jun 25, 2024
Merged

5579 extended http methods #9633

merged 37 commits into from
Jun 25, 2024

Conversation

ashishb-solo
Copy link
Contributor

@ashishb-solo ashishb-solo commented Jun 17, 2024

Description

Enable extended HTTP methods. Prior to this change, Envoy only allowed a
limited set of HTTP methods, and anything not allowed in a hard-coded list
would be rejected. This change allows users to configure Envoy so that extended
methods, such as LABEL or UPDATE can be passed.

The code changes are fairly straightforward, but potential compatibility issues with future envoy releases are not straightforward, so I would appreciate if reviewers could focus in particular on this area.

API changes

  • Created a new header validation settings message and exposed it at the HTTP Listener options level.

Code changes

  • Create the HeaderValidation plugin to manage settings related to header validation

CI changes

  • Added e2e tests (unit tests still needed)

Docs changes

  • See documentation in .proto files

Context

https://soloio.slab.com/posts/extended-http-methods-design-doc-40j7pjeu

Interesting decisions

See slab document regarding status of option settings in upstream Envoy.

Testing steps

TODO

Notes for reviewers

Please read the design document linked on slab carefully to understand the current state of this feature in upstream Envoy, especially how it pertains to Universal Header Validation and the deprecation status of the features that we're using in upstream Envoy.

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

BOT NOTES:
resolves #5579

Includes an e2e test and an attempt at implementing the fix. However, the test
fails, so clearly the fix doesn't actually work
Currently it's a bit of a hack as I just stuffed the functionality into the hcm
plugin. I would like to move it elsewhere
@ashishb-solo ashishb-solo requested a review from a team as a code owner June 17, 2024 17:53
@ashishb-solo ashishb-solo marked this pull request as draft June 17, 2024 17:53
Copy link
Contributor

@bewebi bewebi left a comment

Choose a reason for hiding this comment

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

Overall seems like a good approach to me

We may also want to call out the potential breaking changes where we specify the Envoy version (ie here) so that there are no surprises, but I think there is/will be enough tests that will fail if/when allow_custom_methods is deprecated in Envoy that we aren't at risk of shipping something broken

The real risk is that there will be hidden required work at some point when we bump Envoy versions, but we're calling it out as loudly as possible while also describing the path forward, so I think the pain/surprise will be minimized

projects/gloo/api/v1/options.proto Outdated Show resolved Hide resolved
projects/gloo/pkg/plugins/registry/registry.go Outdated Show resolved Hide resolved
…on.proto

Co-authored-by: Bernie Birnbaum <bewebi@earthlink.net>
projects/gloo/pkg/plugins/README.md Show resolved Hide resolved
projects/gloo/pkg/plugins/hcm/plugin.go Outdated Show resolved Hide resolved
projects/gloo/pkg/plugins/header_validation/plugin.go Outdated Show resolved Hide resolved
projects/gloo/pkg/plugins/registry/registry.go Outdated Show resolved Hide resolved
test/e2e/header_validation_test.go Show resolved Hide resolved
test/e2e/header_validation_test.go Outdated Show resolved Hide resolved
@ashishb-solo
Copy link
Contributor Author

After a bit of discussion yesterday, we made the following decisions:

  1. only enable this on the gateway (and possibly in settings) for now. we cannot decide exactly how we want lower level configurations (like route table) to override higher level configurations (ie. which one takes precedence), so let's hold off on making a decision here until we have to.

  2. we can just pop all this functionality into the existing gloo plugin for hcm. no need to create a new one.

@solo-changelog-bot
Copy link

Issues linked to changelog:
#5579

@ashishb-solo ashishb-solo marked this pull request as ready for review June 20, 2024 20:33
Copy link

github-actions bot commented Jun 20, 2024

Visit the preview URL for this PR (updated for commit 257e0ac):

https://gloo-edge--pr9633-5579-extended-http-m-awemr4gz.web.app

(expires Tue, 02 Jul 2024 18:31:00 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: 77c2b86e287749579b7ff9cadb81e099042ef677

@ashishb-solo
Copy link
Contributor Author

ashishb-solo commented Jun 20, 2024

In the current iteration of the API, this is the configuration that would be used to allow custom http methods:

apiVersion: gateway.solo.io/v1
kind: Gateway
metadata:
  name: gateway-proxy
  namespace: gloo-system
spec:
  bindAddress: '::'
  bindPort: 8080
  httpGateway:
    options:
      headerValidationSettings:
        allowCustomMethods: {}

Copy link
Contributor

@sheidkamp sheidkamp left a comment

Choose a reason for hiding this comment

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

A couple notes on the tests.

Would like to see a demo and/or testing steps before approval.

projects/gloo/pkg/plugins/hcm/plugin_test.go Show resolved Hide resolved
test/e2e/example_test.go Outdated Show resolved Hide resolved
@ashishb-solo ashishb-solo merged commit 9876d02 into main Jun 25, 2024
20 checks passed
@ashishb-solo ashishb-solo deleted the 5579-extended-http-methods branch June 25, 2024 18:41
ashishb-solo added a commit that referenced this pull request Jun 25, 2024
* Check-in before codegen

* Initial commit

Includes an e2e test and an attempt at implementing the fix. However, the test
fails, so clearly the fix doesn't actually work

* Get the implementation working

Currently it's a bit of a hack as I just stuffed the functionality into the hcm
plugin. I would like to move it elsewhere

* Add plugin README

* Move configuration to a new plugin

* Codegen/formatting updates

* Add makefile documentation on building docker images

* Update projects/gloo/api/v1/options/header_validation/header_validation.proto

Co-authored-by: Bernie Birnbaum <bewebi@earthlink.net>

* Address some review comments

Namely, remove the new plugin and stuff the functionality into the existing HCM
plugin. Also, add a little more documentation on expected breaking changes when
UHV is enabled.

* Only allow header validation on gateway

* Update documentation

* Update protobuf API to use a oneof

* Add unit test

* Add changelog

* Fix a compilation error

* Add http/2 test

* Revert "Add http/2 test"

This reverts commit 28f0fe5.

* Re-run codegen

* Move changelog

* Update e2e test documentation

* Update API and documentation

* Rename API to disable_method_validation

* Rename custom_methods `oneof`

to header_method_validation

* Change disableMethodValidation to disableHttp1MethodValidation

* Update a renamed proto variable

* Update e2e test to use new framework

* Fix some ginkgo methods

* Remove duplicated test

* Update projects/gloo/api/v1/options/header_validation/header_validation.proto

Co-authored-by: Seth Heidkamp <61526534+sheidkamp@users.noreply.github.com>

* Add negative test to plugin unit test

* Add an additional test

* Re-run codegen

---------

Co-authored-by: Bernie Birnbaum <bewebi@earthlink.net>
Co-authored-by: Seth Heidkamp <61526534+sheidkamp@users.noreply.github.com>
Co-authored-by: Nathan Fudenberg <nathan.fudenberg@solo.io>
soloio-bulldozer bot added a commit that referenced this pull request Jun 26, 2024
* 5579 extended http methods (#9633)

* Check-in before codegen

* Initial commit

Includes an e2e test and an attempt at implementing the fix. However, the test
fails, so clearly the fix doesn't actually work

* Get the implementation working

Currently it's a bit of a hack as I just stuffed the functionality into the hcm
plugin. I would like to move it elsewhere

* Add plugin README

* Move configuration to a new plugin

* Codegen/formatting updates

* Add makefile documentation on building docker images

* Update projects/gloo/api/v1/options/header_validation/header_validation.proto

Co-authored-by: Bernie Birnbaum <bewebi@earthlink.net>

* Address some review comments

Namely, remove the new plugin and stuff the functionality into the existing HCM
plugin. Also, add a little more documentation on expected breaking changes when
UHV is enabled.

* Only allow header validation on gateway

* Update documentation

* Update protobuf API to use a oneof

* Add unit test

* Add changelog

* Fix a compilation error

* Add http/2 test

* Revert "Add http/2 test"

This reverts commit 28f0fe5.

* Re-run codegen

* Move changelog

* Update e2e test documentation

* Update API and documentation

* Rename API to disable_method_validation

* Rename custom_methods `oneof`

to header_method_validation

* Change disableMethodValidation to disableHttp1MethodValidation

* Update a renamed proto variable

* Update e2e test to use new framework

* Fix some ginkgo methods

* Remove duplicated test

* Update projects/gloo/api/v1/options/header_validation/header_validation.proto

Co-authored-by: Seth Heidkamp <61526534+sheidkamp@users.noreply.github.com>

* Add negative test to plugin unit test

* Add an additional test

* Re-run codegen

---------

Co-authored-by: Bernie Birnbaum <bewebi@earthlink.net>
Co-authored-by: Seth Heidkamp <61526534+sheidkamp@users.noreply.github.com>
Co-authored-by: Nathan Fudenberg <nathan.fudenberg@solo.io>

* Adding changelog file to new location

* Deleting changelog file from old location

---------

Co-authored-by: Bernie Birnbaum <bewebi@earthlink.net>
Co-authored-by: Seth Heidkamp <61526534+sheidkamp@users.noreply.github.com>
Co-authored-by: Nathan Fudenberg <nathan.fudenberg@solo.io>
Co-authored-by: soloio-bulldozer[bot] <48420018+soloio-bulldozer[bot]@users.noreply.github.com>
Co-authored-by: changelog-bot <changelog-bot>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Extended HTTP Methods
4 participants