Skip to content

Adds xDS Server Configuration API #224

Closed
danehans wants to merge 1 commit intoenvoyproxy:mainfrom
danehans:issue_201_followup
Closed

Adds xDS Server Configuration API #224
danehans wants to merge 1 commit intoenvoyproxy:mainfrom
danehans:issue_201_followup

Conversation

@danehans
Copy link
Copy Markdown
Contributor

@danehans danehans commented Aug 23, 2022 •
edited
Loading

Adds xDS Server Configuration API and generates deepcopy funcs, manifests, etc.

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

@danehans danehans requested a review from a team as a code owner August 23, 2022 16:07
@danehans danehans mentioned this pull request Aug 23, 2022
Merged
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Aug 23, 2022 •
edited
Loading

Codecov Report

Merging #224 (98feabd) into main (009d125) will decrease coverage by 0.72%.
The diff coverage is 33.33%.

@@            Coverage Diff             @@
##             main     #224      +/-   ##
==========================================
- Coverage   57.40%   56.67%   -0.73%     
==========================================
  Files          30       30              
  Lines        2702     2765      +63     
==========================================
+ Hits         1551     1567      +16     
- Misses       1058     1104      +46     
- Partials       93       94       +1
Impacted Files Coverage Δ
internal/envoygateway/config/config.go 0.00% <ø> (ø)
internal/ir/zz_generated.deepcopy.go 0.00% <0.00%> (ø)
internal/message/types.go 68.51% <0.00%> (-13.71%) ⬇️
internal/status/gatewayclass.go 0.00% <0.00%> (ø)
internal/provider/kubernetes/gatewayclass.go 52.59% <23.80%> (-16.32%) ⬇️
internal/ir/infra.go 68.13% <55.55%> (-3.11%) ⬇️
internal/gatewayapi/translator.go 86.07% <100.00%> (+0.02%) ⬆️
internal/infrastructure/kubernetes/deployment.go 82.38% <100.00%> (ø)
internal/status/conditions.go 95.00% <100.00%> (ø)
... and 3 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@arkodg
Copy link
Copy Markdown
Contributor

arkodg commented Aug 23, 2022

isn't the comment referring to making

gateway/internal/infrastructure/kubernetes/deployment.go

Line 98 in 9c34a8c

var (
as a const.
If we plan on retrieving the xds address from the config package, it should at least be part of the config API imho, but for now I see most of the definitions in

gateway/internal/infrastructure/kubernetes/deployment.go

Line 20 in 9c34a8c

const (
which is fine for now

@LukeShu
Copy link
Copy Markdown
Contributor

LukeShu commented Aug 23, 2022

What Arko said is how I intended it. I just saw a global value that didn't look like it should change declared as var when it could have been declared as const.

youngnick
youngnick previously approved these changes Aug 25, 2022
View reviewed changes
@youngnick
Copy link
Copy Markdown
Contributor

youngnick commented Aug 25, 2022

Ah, sorry, I reviewed without reading the comments. I'll unapprove and leave y'all to sort this one out.

@youngnick youngnick requested review from youngnick and removed request for youngnick August 25, 2022 01:28
@youngnick
Copy link
Copy Markdown
Contributor

youngnick commented Aug 25, 2022

hmm, seems I can't unapprove, sorry. More caffeine required for me today.

@danehans danehans changed the title Makes EnvoyGatewayService a Constant Manages Envoy xDS Server Address Through EnvoyProxy API Aug 25, 2022
@danehans danehans force-pushed the issue_201_followup branch 2 times, most recently from 98feabd to 55fc448 Compare August 25, 2022 16:40
danehans
danehans commented Aug 25, 2022
View reviewed changes
api/config/v1alpha1/helpers.go Outdated
Copy link
Copy Markdown
Contributor Author

@danehans danehans Aug 25, 2022

Choose a reason for hiding this comment

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

Note: This function was unused.

@danehans danehans mentioned this pull request Aug 25, 2022
Merged
@danehans danehans force-pushed the issue_201_followup branch 2 times, most recently from d255d87 to e53053b Compare August 25, 2022 17:16
@arkodg
Copy link
Copy Markdown
Contributor

arkodg commented Aug 25, 2022

@danehans would prefer if you split up the work in this PR into multiple smaller PRs so it would be easier for reviewers to review this code, for e.g. would be great if there was 1 PR was just the API changes for introducing the EnvoyProxy Kind resource. TIA

@arkodg arkodg requested a review from a team August 25, 2022 17:30
@danehans
Adds xDS Server Configurtion to EnvoyProxy API
34bdda6 
Signed-off-by: danehans <daneyonhansen@gmail.com>
@danehans
Copy link
Copy Markdown
Contributor Author

danehans commented Aug 25, 2022

@arkodg most of the changes are related to implementing the API. I don't think pulling the API changes into a separate PR will make much of a difference in the size of the implementation PR, but I'll go ahead separate the two. The PR looks daunting mostly due to testdata updates.

@danehans danehans force-pushed the issue_201_followup branch from e53053b to 34bdda6 Compare August 25, 2022 17:44
@danehans danehans changed the title Manages Envoy xDS Server Address Through EnvoyProxy API Adds xDS Server Configuration API Aug 25, 2022
@arkodg
Copy link
Copy Markdown
Contributor

arkodg commented Aug 25, 2022

thanks for just including the API changes in this PR ! reviewing rn ....

@danehans danehans mentioned this pull request Aug 25, 2022
Closed
arkodg
arkodg reviewed Aug 25, 2022
View reviewed changes
api/config/v1alpha1/envoyproxy_types.go
@@ -18,8 +23,21 @@ type EnvoyProxy struct {

// EnvoyProxySpec defines the desired state of EnvoyProxy.
type EnvoyProxySpec struct {
Copy link
Copy Markdown
Contributor

@arkodg arkodg Aug 25, 2022

Choose a reason for hiding this comment

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

Just refreshed my memory by going through https://github.com/envoyproxy/gateway/blob/main/docs/design/CONFIG_API.md#data-plane-api

based on the agreed upon design, the EnvoyGateway config, passed via the cmdline arg, decides the configuration for the control plane components/runners/services
and the EnvoyProxy resources passed into K8s, linked to the GatewayClass decides the configuration of the dataplane Envoy proxy fleet.

The Xds Server address & port (not included here but is required) is needed by the proxy to connect to the server and the same address (optional, can bind to all addresses) and port is required by the server to listen for xds client connections, so such a configuration is needed in EnvoyGateway too and the values in both must match .
It has been hardcoded in the xds-server for now

gateway/internal/xds/server/runner/runner.go

Line 57 in d99f32f

// TODO: Make the listening address and port configurable

Copy link
Copy Markdown
Contributor Author

@danehans danehans Aug 25, 2022

Choose a reason for hiding this comment

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

@arkodg agreed that we should have logic to ensure the EnvoyProxy xds config aligns with how EG is running. IMO we need to be careful to not outsmart ourselves as different client/server config permutations will work. I think we track this as an issue to improve in the future. In the meantime, the API docs should suffice or I can elaborate on the docs if needed. THoughts @arkodg @LukeShu @youngnick @skriss

@danehans
Copy link
Copy Markdown
Contributor Author

danehans commented Aug 25, 2022 •
edited
Loading

Closing since we don't want to expose config to users in v0.2.0. This can be reopened or used as a reference EnvoyProxy API implementation in the future.

@danehans danehans closed this Aug 25, 2022
@danehans danehans mentioned this pull request Aug 26, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@arkodg arkodg arkodg left review comments

@youngnick youngnick Awaiting requested review from youngnick

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@danehans @codecov-commenter @arkodg @LukeShu @youngnick