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

feat: implment BackendRefs in EnvoyProxy #3080

Merged
merged 11 commits into from
Apr 11, 2024

Conversation

zirain
Copy link
Contributor

@zirain zirain commented Apr 2, 2024

xref: #3041

@zirain zirain requested a review from a team as a code owner April 2, 2024 02:50
@zirain zirain force-pushed the proxy-telemetry-backend branch from b693668 to 27fadb3 Compare April 2, 2024 03:16
Copy link

codecov bot commented Apr 2, 2024

Codecov Report

Attention: Patch coverage is 88.23529% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 66.59%. Comparing base (29946b0) to head (2b0c4e9).
Report is 29 commits behind head on main.

❗ Current head 2b0c4e9 differs from pull request most recent head f2c446e. Consider uploading reports for the commit f2c446e to get more accurate results

Files Patch % Lines
internal/gatewayapi/listener.go 88.00% 2 Missing and 1 partial ⚠️
internal/utils/net/backendref.go 66.66% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3080      +/-   ##
==========================================
+ Coverage   66.51%   66.59%   +0.08%     
==========================================
  Files         161      158       -3     
  Lines       22673    21996     -677     
==========================================
- Hits        15080    14648     -432     
+ Misses       6720     6503     -217     
+ Partials      873      845      -28     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@zirain zirain force-pushed the proxy-telemetry-backend branch 2 times, most recently from 7085d61 to cc9a69c Compare April 2, 2024 05:04
tools/make/golang.mk Outdated Show resolved Hide resolved
@@ -96,11 +96,14 @@ type FileEnvoyProxyAccessLog struct {

// OpenTelemetryEnvoyProxyAccessLog defines the OpenTelemetry access log sink.
//
// +kubebuilder:validation:XValidation:message="BackendRef only support Service Kind.",rule="!has(self.backendRef) || !has(self.backendRef.kind) || self.backendRef.kind == 'Service'"
// +kubebuilder:validation:XValidation:message="host or backendRef needs to be set",rule="has(self.host) || has(self.backendRef)"
// +kubebuilder:validation:XValidation:message="backendRef only support Service Kind.",rule="!has(self.backendRef) || self.backendRef.kind == 'Service'"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TBH, the previous version was more rigorous, but fails on 1.28.0.
Good news, there's dafault value on kind.

@arkodg
Copy link
Contributor

arkodg commented Apr 5, 2024

blocked on #3091

Signed-off-by: zirain <zirain2009@gmail.com>
@zirain zirain force-pushed the proxy-telemetry-backend branch from 577f8aa to a4dc637 Compare April 8, 2024 05:26
zirain added 5 commits April 8, 2024 21:11
Signed-off-by: zirain <zirain2009@gmail.com>
Signed-off-by: zirain <zirain2009@gmail.com>
Signed-off-by: zirain <zirain2009@gmail.com>
Signed-off-by: zirain <zirain2009@gmail.com>
@zirain zirain changed the title feat: implment BackendRef in EnvoyProxy feat: implment BackendRefs in EnvoyProxy Apr 10, 2024
zirain added 3 commits April 10, 2024 11:59
Signed-off-by: zirain <zirain2009@gmail.com>
Signed-off-by: zirain <zirain2009@gmail.com>
@zirain
Copy link
Contributor Author

zirain commented Apr 10, 2024

/retest

@@ -407,3 +408,10 @@ type KubernetesPatchSpec struct {
// Object contains the raw configuration for merged object
Value apiextensionsv1.JSON `json:"value"`
}

// BackendRef defines how an ObjectReference that is specific to BackendRef.
type BackendRef struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

🚀

if sink.OpenTelemetry.Host != nil {
host, port = *sink.OpenTelemetry.Host, uint32(sink.OpenTelemetry.Port)
}
if len(sink.OpenTelemetry.BackendRefs) > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe a TODO here to support multiple backendRefs in the future

Copy link
Contributor Author

Choose a reason for hiding this comment

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

EG support multi sinks, I don't think that future exists.

gwapiv1 "sigs.k8s.io/gateway-api/apis/v1"
)

func BackendHostAndPort(backendRef gwapiv1.BackendObjectReference, defaultNamespace string) (string, uint32) {
Copy link
Contributor

Choose a reason for hiding this comment

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

can we find a way to reuse

func (t *Translator) processDestination(backendRefContext BackendRefContext,
instead ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

let's address this first and then fix it with follow up PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

there is a working implementation using endpoints for the backendRef on the #3062 draft that I'd put together while waiting on APIs to be settled so I could test things out end to end while I had time to focus on it.

might be able to pull pieces of that and use them here, particularly the common bits in the controller that are required to watch the services and endpoints for backendRefs on EnvoyProxies

Signed-off-by: zirain <zirain2009@gmail.com>
@zirain
Copy link
Contributor Author

zirain commented Apr 10, 2024

/retest

@guydc guydc modified the milestones: v1.1.0, v1.1.0-rc1 Apr 10, 2024
Copy link
Contributor

@guydc guydc left a comment

Choose a reason for hiding this comment

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

LGTM

@zirain
Copy link
Contributor Author

zirain commented Apr 11, 2024

/retest

@zirain zirain merged commit 0cb3299 into envoyproxy:main Apr 11, 2024
20 checks passed
@zirain zirain deleted the proxy-telemetry-backend branch April 11, 2024 04:45
zhaohuabing pushed a commit to zhaohuabing/gateway that referenced this pull request Apr 11, 2024
* implment BackendRef in EnvoyProxy

Signed-off-by: zirain <zirain2009@gmail.com>
zhaohuabing pushed a commit to zhaohuabing/gateway that referenced this pull request Apr 11, 2024
* implment BackendRef in EnvoyProxy

Signed-off-by: zirain <zirain2009@gmail.com>
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.

4 participants