Skip to content

Adds Config API Design#95

Merged
danehans merged 6 commits intoenvoyproxy:mainfrom
danehans:issue_51
Jun 28, 2022
Merged

Adds Config API Design#95
danehans merged 6 commits intoenvoyproxy:mainfrom
danehans:issue_51

Conversation

@danehans
Copy link
Copy Markdown
Contributor

@danehans danehans commented Jun 7, 2022

Adds a design doc detailing the initial Envoy Gateway configuration API.

Fixes #107

Signed-off-by: danehans daneyonhansen@gmail.com

@danehans danehans requested review from LukeShu, arkodg and youngnick June 7, 2022 00:30
@danehans
Copy link
Copy Markdown
Contributor Author

danehans commented Jun 7, 2022

xref initial config api and tooling: #51

Comment thread docs/design/CONFIG_API.md Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this spec should only expose fields to the user (admin in this case) that are relevant, EnabledServices isn't imho since its an internal implementation detail.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@arkodg since EG is designed to be a collection of loosely coupled services, how do you propose that we expose the ability to run different services? Here are example use cases:

  • As an admin, I want to run the message service and control plane services separately.
  • As an admin, I want to run all EG services together.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

For the default case (all services run together) we probably dont need to define anything
Thinking out loud, when we need to handle this case

As an admin, I want to run the message service and control plane services separately

we can define a MessageService struct with details about the service, publisher, subscriber

Comment thread docs/design/CONFIG_API.md Outdated
Comment thread docs/design/CONFIG_API.md Outdated
Comment thread docs/design/CONFIG_API.md Outdated
Comment thread docs/design/CONFIG_API.md Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

we probably need a top level platform field to toggle between kubernetes, local/stand-alone, custom (could be used to enable advanced use cases such as using the kubernetes controller to read configs but leverage a non kubernetes component for service discovery / resolver)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Instead of taking the above approach, EG could 1) check for kubeconfig 2) if kubeconfig exists, run in kubernetes "mode" 3) if kubeconfig doesn't exist, run in standAlone "mode". If we find this dynamic approach to be an issue in the future, we can always add the API fields you suggest in the future. I find it more difficult to remove fields than to add them.

Comment thread docs/design/CONFIG_API.md Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Is the plan to go with a managed or unmanaged deployment route (defined in kubernetes-sigs/gateway-api#892)?

put another way - does this say to actually go provision an Envoy or just set config for when one is provision

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@howardjohn in Envoy Gateway's current design, only "managed" infra is supported. A GatewayClass with a matching controllerName instructs Envoy Gateway to provision the Envoy infra using default settings. If the gatewayclass.spec.parametersRef references this API resource, e.g. BootstrapConfig, Envoy Gateway will provision the Envoy infra using these parameters instead of defaults. We currently have no plans to support "unmanaged" infra but feel free to open an issue or join the community meeting to discuss in more detail.

Comment thread docs/design/CONFIG_API.md Outdated
Comment thread docs/design/CONFIG_API.md Outdated
Comment thread docs/design/CONFIG_API.md Outdated
Comment thread docs/design/CONFIG_API.md Outdated
@danehans
Copy link
Copy Markdown
Contributor Author

danehans commented Jun 7, 2022

Still not a fan of Bootstrap as the API name. Since this API type will be ref'd by gateway class.spec.parameterRefs, I feel that GatewayConfig or GatewayParams provides a more natural association between resources.

@danehans
Copy link
Copy Markdown
Contributor Author

xref: #107 for API structure decision.
xref: #51 for initial config API deisgn.

Comment thread docs/design/CONFIG_API.md Outdated
Comment thread docs/design/CONFIG_API.md Outdated
Comment thread docs/design/CONFIG_API.md Outdated
Comment thread docs/design/CONFIG_API.md Outdated
Comment thread docs/design/CONFIG_API.md Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

im guessing DataPlane will contain platform agnostic fields, so if the user wanted to specify a service type for the envoy k8s service, that would be expressed using the top level Provider field ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@arkodg ControlPlane and DataPlane APIs expose provider-agnostic configuration. For example, spec.dataPlane.adminAddress and spec.dataPlane.adminPort could be used to configure Envoy's admin endpoint. I'm open to removing data plane configuration from the design spec until it's needed.

@danehans
Copy link
Copy Markdown
Contributor Author

@arkodg et al., I've updated this PR based on your feedback and the feedback from today's community call. I'm still in favor of using providers as an abstraction for EG runtime config. I understand that supporting different provider combinations will be a challenge from a testing perspective. Since EG is meant to be extensible, I would like to understand opposing approaches to achieving EGs extensibility without expanding the test matrix. IMHO contributors that add providers are responsible for ensuring the provider works with existing supported providers. For example, if a FooRatelimiter provider is introduced to add global rate limiting functionality, the contributor is responsible for adding the necessary e2e's to ensure proper functionality with Kubernetes and File providers.

@danehans danehans force-pushed the issue_51 branch 2 times, most recently from 8b29c5d to 9fc6897 Compare June 15, 2022 18:21
@danehans danehans mentioned this pull request Jun 15, 2022
Comment thread docs/design/CONFIG_API.md Outdated
Comment thread docs/design/CONFIG_API.md Outdated
Comment thread docs/design/CONFIG_API.md Outdated
@danehans danehans changed the title Adds Control Plane Config API Design Adds Config API Design Jun 23, 2022
Comment thread docs/design/CONFIG_API.md Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

if we had used protobufs this would be looked a little cleaner

provider:
  kubernetes:
    ....

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Understood. As we discussed, provider follows the union type conventions.

Comment thread docs/design/CONFIG_API.md Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: prefer if this field was called GatewayAPI

arkodg
arkodg previously approved these changes Jun 24, 2022
Copy link
Copy Markdown
Contributor

@arkodg arkodg left a comment

Choose a reason for hiding this comment

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

LGTM , thanks !

danehans added 5 commits June 27, 2022 16:29
Signed-off-by: danehans <daneyonhansen@gmail.com>
Signed-off-by: danehans <daneyonhansen@gmail.com>
Signed-off-by: danehans <daneyonhansen@gmail.com>
Signed-off-by: danehans <daneyonhansen@gmail.com>
Signed-off-by: danehans <daneyonhansen@gmail.com>
@danehans danehans force-pushed the issue_51 branch 2 times, most recently from ccbacbd to 1cc2506 Compare June 28, 2022 15:37
Signed-off-by: danehans <daneyonhansen@gmail.com>
Comment thread docs/design/CONFIG_API.md
Copy link
Copy Markdown
Contributor

@youngnick youngnick left a comment

Choose a reason for hiding this comment

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

This LGTM, especially to get started with.

@danehans danehans merged commit 33631b8 into envoyproxy:main Jun 28, 2022
@arkodg arkodg mentioned this pull request Jun 29, 2022
@danehans danehans deleted the issue_51 branch July 12, 2022 19:20
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.

Decision: Config API structure and design

7 participants