Adds Support for Managing Gateway Status Addresses #352

danehans · 2022-09-07T02:15:49Z

Updates gateway controller to watch services and trigger a reconciliation for those with the EG Gateway owning labels.
Moves label and label selector funcs to a common file, labels.go.
Updates status manager to manage the Gateway "Ready" condition based on the availability of Gateway status addresses.
Applies Gateway ns/name owning labels to the Envoy service.
Updates the quickstart guide to use the Gateway status address instead of the Envoy service address.

Requires #350

Fixes #330

codecov-commenter · 2022-09-07T16:28:37Z

Codecov Report

Merging #352 (8d71f6f) into main (9d75f27) will decrease coverage by 0.65%.
The diff coverage is 47.15%.

@@            Coverage Diff             @@
##             main     #352      +/-   ##
==========================================
- Coverage   60.62%   59.96%   -0.66%     
==========================================
  Files          32       33       +1     
  Lines        3220     3317      +97     
==========================================
+ Hits         1952     1989      +37     
- Misses       1159     1217      +58     
- Partials      109      111       +2

Impacted Files	Coverage Δ
internal/envoygateway/config/config.go	`0.00% <ø> (ø)`
internal/gatewayapi/helpers.go	`64.91% <0.00%> (-1.16%)`	⬇️
internal/ir/infra.go	`66.27% <0.00%> (-4.10%)`	⬇️
internal/status/conditions.go	`81.42% <0.00%> (-13.58%)`	⬇️
internal/status/gateway.go	`0.00% <0.00%> (ø)`
internal/provider/kubernetes/gateway.go	`70.80% <55.81%> (-6.25%)`	⬇️
internal/infrastructure/kubernetes/deployment.go	`84.37% <100.00%> (-0.84%)`	⬇️
internal/infrastructure/kubernetes/infra.go	`74.62% <100.00%> (+4.47%)`	⬆️
internal/infrastructure/kubernetes/labels.go	`100.00% <100.00%> (ø)`
internal/infrastructure/kubernetes/service.go	`81.39% <100.00%> (+2.72%)`	⬆️
... and 2 more

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

skriss · 2022-09-07T16:57:03Z

internal/gatewayapi/translator.go

+	// OwningGatewayNameLabel is the owner reference label used for managed infra.
+	// The value should be the name of the Gateway used to CRUD the Service.
+	OwningGatewayNameLabel = "gateway.envoyproxy.io/owning-gateway-name"
+	// OwningGatewayNamespaceLabel is the owner reference label used for managed infra.
+	// The value should be the namespace of the Gateway used to CRUD the Service.
+	OwningGatewayNamespaceLabel = "gateway.envoyproxy.io/owning-gateway-namespace"


Doesn't the infra correspond to a GatewayClass, which could have >1 Gateways? I would think these labels should identify the GatewayClass.

@skriss since a Gateway supports multiple addresses, each Gateway corresponds to a Service. Therefore, a Service name is constructed as envoy-${GW_NAME}-${GW_NS}, where the variables are derived from these labels.

The owning Gateway labels are required so the gateway controller can reconcile the appropriate Gateway from CRUD's of a watched Service. As you know, we need to watch the Envoy service to get the status.ingresses[].ip to populate gateway.status.addresses.

internal/provider/kubernetes/gateway.go

danehans · 2022-09-07T21:47:14Z

Added release-note label to indicate that user-defined gateway addresses will not be supported in v0.2.0 unless #360 is fixed.

docs/user/QUICKSTART.md

internal/infrastructure/kubernetes/infra_test.go

internal/provider/kubernetes/gateway.go

internal/status/gateway.go

arkodg · 2022-09-09T16:15:51Z

@danehans curious if this PR unblocks the first few GatewayAPI conformance tests

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

danehans · 2022-09-09T16:21:58Z

@danehans curious if this PR unblocks the first few GatewayAPI conformance tests

@arkodg I have not looked into the tests so I'm unsure. From our discussions during the last few community meetings, I do know this PR will unblock our ability to run conformance tests. This is becasue the test client uses gateway.status.addresses to connect to the Gateway(s) under test.

internal/provider/kubernetes/gateway.go

arkodg · 2022-09-09T18:39:17Z

@danehans curious if this PR unblocks the first few GatewayAPI conformance tests

@arkodg I have not looked into the tests so I'm unsure. From our discussions during the last few community meetings, I do know this PR will unblock our ability to run conformance tests. This is becasue the test client uses gateway.status.addresses to connect to the Gateway(s) under test.

cool, would be great if you can attach the Gateway output highlighting the updated status with the LB address, TIA

skriss

Mostly LGTM, one nit. I'm a bit unclear on what's going on with labels -- if we have a single service per GatewayClass, wouldn't we want to label the Service with the GatewayClass info? https://github.com/envoyproxy/gateway/blob/main/internal/gatewayapi/translator.go#L114-L121 appears to only keep labels for the last Gateway that's seen since it's continually overwriting them for each Gateway, which seems weird. In the end I'm not sure how much it matters, since for every Reconcile call we're reconciling everything, and not actually using the service's labels to look up the relevant service for a Gateway, but it may be confusing.

internal/provider/kubernetes/gateway.go

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

danehans · 2022-09-10T00:21:00Z

I'm a bit unclear on what's going on with labels -- if we have a single service per GatewayClass, wouldn't we want to label the Service with the GatewayClass info?

@skriss thanks for the review. I have refactored the PR to use a GatewayClass owning label. The service watch event handler of the gateway controller now looks for the GatewayClass owning label. If found, it will load all the Gateways from the resource map and create a reconcile request for each Gateway that references a GatewayClass that matches the owning label.

skriss

This makes sense to me based on the current relationship between GatewayClass and infra/a Service, can always revisit if/when needed.

danehans requested a review from a team as a code owner September 7, 2022 02:15

danehans marked this pull request as draft September 7, 2022 02:15

danehans added provider/kubernetes Issues related to the Kubernetes provider area/infra-mgr Issues related to the provisioner used for provisioning the managed Envoy Proxy fleet. labels Sep 7, 2022

danehans added this to the 0.2.0-rc2 milestone Sep 7, 2022

danehans force-pushed the issue_330 branch from 143e152 to 53299d4 Compare September 7, 2022 16:24

skriss reviewed Sep 7, 2022

View reviewed changes

internal/provider/kubernetes/gateway.go Outdated Show resolved Hide resolved

danehans force-pushed the issue_330 branch from 53299d4 to 9aed3a1 Compare September 7, 2022 19:33

danehans mentioned this pull request Sep 7, 2022

Add Support for User-Defined Gateway Addresses #360

Closed

danehans added the release-note Indicates a required release note label Sep 7, 2022

danehans force-pushed the issue_330 branch from 9aed3a1 to f8fd0fa Compare September 7, 2022 22:01

danehans marked this pull request as ready for review September 7, 2022 22:01

danehans changed the title ~~WIP: Adds Support for Managing Gateway Status Addresses~~ Adds Support for Managing Gateway Status Addresses Sep 7, 2022

danehans mentioned this pull request Sep 7, 2022

Delete xDs resources when GatewayClass is deleted #355

Merged

danehans force-pushed the issue_330 branch from f8fd0fa to 3be8de6 Compare September 7, 2022 22:18

This was referenced Sep 7, 2022

Split Proxy Infra CRUD #362

Closed

Set Gateway.Status.Addresses #330

Closed

skriss mentioned this pull request Sep 8, 2022

Initial Gateway API Conformance #65

Closed

danehans force-pushed the issue_330 branch 2 times, most recently from 2cf81ce to 8d71f6f Compare September 8, 2022 19:38

arkodg reviewed Sep 8, 2022

View reviewed changes

docs/user/QUICKSTART.md Outdated Show resolved Hide resolved

arkodg reviewed Sep 8, 2022

View reviewed changes

internal/infrastructure/kubernetes/infra_test.go Outdated Show resolved Hide resolved

arkodg reviewed Sep 8, 2022

View reviewed changes

internal/provider/kubernetes/gateway.go Outdated Show resolved Hide resolved

arkodg reviewed Sep 8, 2022

View reviewed changes

internal/provider/kubernetes/gateway.go Outdated Show resolved Hide resolved

arkodg reviewed Sep 8, 2022

View reviewed changes

internal/status/gateway.go Outdated Show resolved Hide resolved

This was referenced Sep 8, 2022

Labels or Annotations for Service-to-Gateway Object References #368

Closed

Should Infra Manager Watch Services and Update Status #371

Closed

danehans force-pushed the issue_330 branch from 8d71f6f to 8b04ca5 Compare September 9, 2022 16:08

Adds Support for Managing Gateway Status Addresses

ae4aed2

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

danehans force-pushed the issue_330 branch from 8b04ca5 to ae4aed2 Compare September 9, 2022 16:18

arkodg reviewed Sep 9, 2022

View reviewed changes

internal/provider/kubernetes/gateway.go Outdated Show resolved Hide resolved

skriss reviewed Sep 9, 2022

View reviewed changes

internal/provider/kubernetes/gateway.go Outdated Show resolved Hide resolved

Refactors for GatewayClass Owning Labels

afc1b7b

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

danehans force-pushed the issue_330 branch from 713b3e7 to afc1b7b Compare September 10, 2022 00:14

skriss approved these changes Sep 12, 2022

View reviewed changes

danehans merged commit bb05385 into envoyproxy:main Sep 12, 2022

danehans deleted the issue_330 branch September 12, 2022 16:05

danehans mentioned this pull request Sep 12, 2022

Add Support for Gateway Ready Condition #219

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds Support for Managing Gateway Status Addresses #352

Adds Support for Managing Gateway Status Addresses #352

danehans commented Sep 7, 2022 •

edited

Loading

codecov-commenter commented Sep 7, 2022 •

edited

Loading

skriss Sep 7, 2022

danehans Sep 7, 2022 •

edited

Loading

danehans commented Sep 7, 2022

arkodg commented Sep 9, 2022

danehans commented Sep 9, 2022

arkodg commented Sep 9, 2022

skriss left a comment •

edited

Loading

danehans commented Sep 10, 2022

skriss left a comment

Adds Support for Managing Gateway Status Addresses #352

Adds Support for Managing Gateway Status Addresses #352

Conversation

danehans commented Sep 7, 2022 • edited Loading

codecov-commenter commented Sep 7, 2022 • edited Loading

Codecov Report

skriss Sep 7, 2022

Choose a reason for hiding this comment

danehans Sep 7, 2022 • edited Loading

Choose a reason for hiding this comment

danehans commented Sep 7, 2022

arkodg commented Sep 9, 2022

danehans commented Sep 9, 2022

arkodg commented Sep 9, 2022

skriss left a comment • edited Loading

Choose a reason for hiding this comment

danehans commented Sep 10, 2022

skriss left a comment

Choose a reason for hiding this comment

danehans commented Sep 7, 2022 •

edited

Loading

codecov-commenter commented Sep 7, 2022 •

edited

Loading

danehans Sep 7, 2022 •

edited

Loading

skriss left a comment •

edited

Loading