-
Notifications
You must be signed in to change notification settings - Fork 0
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
[ENVOY] feat: Envoy to set Service ID header from subdomain #102
Conversation
Co-authored-by: Daniel Olshansky <[email protected]>
…le-endpoint-id-passing
…ldwithgrove/path into configurable-endpoint-id-passing
…der from subdomain
253043d
to
023d44f
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.
LGTM!
Will approve after we merge the base and rebase this.
…ble-endpoint-id-passing
…rse-services-follow-up
…thgrove/path into fix-morse-e2e
# tl;dr This PR makes PATH default to a NoOp QoS for services without a QoS implementation instead of rejecting requests, streamlining configuration and simplifying PATH's service config to focus on QoS mappings only. This PR also removes the requirement for defining services in the PATH config YAML, as the responsibility for "allowed services" now resides with Envoy Proxy (this change is handled in PR #102 ). _Example Test_ <img width="1295" alt="Screenshot 2024-12-19 at 11 09 20" src="https://github.com/user-attachments/assets/683c60d7-fcf1-4905-ab32-bbd2e12877ba" /> ## Summary This PR expands upon the draft functionality introduced by @Olshansk in PR #112, which itself builds upon @adshmh 's work in #106. It achieves several primary goals (see reasoning for these changes in the next section): 1. Removes the requirement for the `services` field in the config YAML structure. 2. Allows the `Parser` struct to default to the `NoOp` QoS implementation if the request's service ID does not have a corresponding QoS implementation. 3. Renames `config/service_alias.go` to `config/service_qos.go` 4. As a direct requirement of [1], it updates the `cmd/qos.go` file's initialization logic to create the `gatewayQoSService` map from the values in `config/service_qos.go` instead of from the services specified in the config YAML. 5. Improves commented documentation related to this change, including introducing the new "authoritative service ID" terminology. ## Reasoning The reasoning for this change is as follows: 1. PATH's current behaviour is to force operators to define which services their PATH instance supports in the config YAML file. This includes defining a string `alias` for service IDs (eg. `eth` aliases the authoritative service ID `F00C`). 2. In discussions with @adshmh in Discord and Github it was decided that the responsibility for both defining service aliases and determining which service IDs PATH supports should be moved to the Envoy Proxy level. 3. This means PATH only receives the "authoritative service ID" (eg. `F00C`, `anvil`, etc.), never an alias and it only ever receives it as the `target-service-id` header, never as the subdomain. 4. The change to handle this at the Envoy Level is implemented in PR#102: #102. This PR contains work parallel and related to this PR. 5. As described in [1], PATH's current behaviour is to reject requests for services not configured in both the config YAML and the `config/service_qos.go` (previously `config/service_alias.go`) file. 6. With the introduction of the `NoOp QoS` functionality, this has has changed to instead use a QoS service that selects a random endpoint and performs no QoS checks, rather than outright rejecting the request. 7. Because of [2] and [4], the source of truth for "allowed services" will soon reside at the Envoy level. This means that the `config/service_qos.go` should be concerned only with which services correspond to which QoS implementations (eg. `F00C -> EVM QoS`, `proto-anvil -> NoOp QoS`, etc.) and not with whether the service is "allowed" by PATH. 8. This also means that we no longer need to configure which services are allowed by the PATH instance in the config YAML as all services are "allowed" at the PATH level; forcing operators to configure services at both the Envoy and PATH level is confusing and redundant and should be removed. _(Note that the `hydrator_config` that determines which services the hydrator runs for remains in place and may still be configured by the operator; only the `services` YAML field is removed.)_ ## Origin ![Screenshot 2024-12-19 at 10 16 43](https://github.com/user-attachments/assets/c3480528-a6a1-4c52-b8dd-ecde9f20976e) https://discord.com/channels/824324475256438814/1263851886486880356/1318695800754405466 <img width="834" alt="Screenshot 2024-12-19 at 10 17 31" src="https://github.com/user-attachments/assets/0ba00538-0c78-4f37-a169-951d0c013d69" /> #106 (review) ## Type of change Select one or more from the following: - [x] New feature, functionality or library - [x] Code health or cleanup ## Testing - [x] **All Tests**: `make test_all` ## Sanity Checklist - [x] I have tested my changes using the available tooling - [x] I have commented my code - [x] I have performed a self-review of my own code; both comments & source code - [x] I create and reference any new tickets, if applicable --------- Co-authored-by: Daniel Olshansky <[email protected]>
@@ -6,29 +6,101 @@ | |||
# Get the directory of the script |
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.
Just as a heads up - I'm skipping this in leu of time but will edit when going through the E2E flow again.
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.
Just as a heads up - I'm skipping this in leu of time but will edit when going through the E2E flow again.
SGTM but just FYI this file it not currently involved in the E2E at all.
I started playing around with how we could implement a FULL E2E suite utilizing Tilt to spin up the full stack including Envoy but it's non-trivial and will have to wait until the new year.
But just FYI I have been thinking about it and tentatively started prototyping.
…ay endpoints file
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.
@commoddity I updated the docs and made sure we can render mermaid diagrams, etc (it wasn't working before).
Let's merge it in!
This PR makes three main changes to PATH:
subdomain
orheader
"alias"
from PATH's code; instead all aliases are handled at the Envoy level..allowed-services.lua
to configure which services are allowed by a PATH instance.Further details below.
Adds a new Lua HTTP filter to the Envoy YAML template to set the
target-service-id
header from the subdomain or the header (including resolving service aliases to authoritative service IDs - see point 2.)."anvil.gateway.pokt.network" -> Header: "target-service-id: anvil"
"eth.gateway.pokt.network" -> Header: "target-service-id: F00C"
This allows us to continue supporting the legacy method of passing chain IDs to middleware via the subdomain.
eg.
and
both work when hitting Envoy (in the latter example, the new Envoy HTTP filter sets the
Target-Service-ID
header fromanvil.localhost:3001
)."alias"
concept from PATHtl;dr all service IDs forwarded to PATH from Envoy are 1. in the
target-service-id
header and 2. authoritative IDs, not aliasesAll handling of service aliases requests is done at the Envoy Proxy level now, inside the Lua filter.
Allowed services for a PATH instance are now configured inside a new
.allowed-services.lua
file which is mounted into the Envoy Proxy container and read from inside the Lua filter.If requests are made for a service that does not exist in this file they will be rejected.
Aliases are also configured here and the Lua filter ensures that requests made for aliases are forwarded to PATH with the correct
authoritative service ID
set as thetarget-service-id
header.NOTE: This PR also updates all header string consts in the Go code to be lowercase as the variable casing was causing issues with Envoy. Suggest that all header consts defined in the Go code be lower case going forward to avoid the potential for casing issues with Envoy. It also removes the deprecated
x-
prefix from header names.