-
Notifications
You must be signed in to change notification settings - Fork 7
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
Make cluster stats names more parseable #10439
Changes from 12 commits
df7f10b
68e0607
ed45de6
b760f4e
613400e
8f26f0d
e82e1e1
02ffad7
a39c1d8
d9201dc
fcdb24e
5b2a85d
d612dda
0e92ee7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
changelog: | ||
- type: BREAKING_CHANGE | ||
issueLink: https://github.com/solo-io/solo-projects/issues/7105 | ||
resolvesIssue: false | ||
description: >- | ||
When using the Kubernetes Gateway API and routing to Kubernetes Services or Kubernetes Upstreams, the | ||
envoy cluster stats names are now in a new more parseable format that allows us to extract info about | ||
the Service: `upstreamName_upstreamNs_svcNs_svcName_svcPort` (underscore-separated). Note that as long | ||
as Kubernetes Gateway integration is enabled (i.e. Gloo Gateway is installed with `kubeGateway.enabled=true`), | ||
both Edge and Kubernetes Gateway proxies will use the new stats name format for these clusters. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,7 +16,6 @@ import ( | |
|
||
v1 "github.com/solo-io/gloo/projects/gloo/pkg/api/v1" | ||
plugins "github.com/solo-io/gloo/projects/gloo/pkg/api/v1/options" | ||
"github.com/solo-io/gloo/projects/gloo/pkg/translator" | ||
) | ||
|
||
var errorUndetectableUpstream = errors.New("upstream type cannot be detected") | ||
|
@@ -115,7 +114,7 @@ func (u *Updater) UpstreamUpdated(upstream *v1.Upstream) { | |
|
||
func (u *Updater) UpstreamAdded(upstream *v1.Upstream) { | ||
// upstream already tracked. ignore. | ||
key := translator.UpstreamToClusterName(upstream.GetMetadata().Ref()) | ||
key := upstream.GetMetadata().Ref().Key() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this was part of the cleanup i carried over from #10403. in some plugins (etc) we are just using the "cluster name" as key for a map used internally, and i changed those instances to use Ref.Key instead |
||
if _, ok := u.activeUpstreams[key]; ok { | ||
return | ||
} | ||
|
@@ -144,7 +143,7 @@ func (u *Updater) UpstreamAdded(upstream *v1.Upstream) { | |
} | ||
|
||
func (u *Updater) UpstreamRemoved(upstream *v1.Upstream) { | ||
key := translator.UpstreamToClusterName(upstream.GetMetadata().Ref()) | ||
key := upstream.GetMetadata().Ref().Key() | ||
if upstreamState, ok := u.activeUpstreams[key]; ok { | ||
upstreamState.cancel() | ||
delete(u.activeUpstreams, key) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,6 +15,8 @@ import ( | |
"github.com/golang/protobuf/ptypes/wrappers" | ||
"github.com/rotisserie/eris" | ||
"github.com/solo-io/gloo/pkg/utils/api_conversion" | ||
"github.com/solo-io/gloo/pkg/utils/envutils" | ||
"github.com/solo-io/gloo/projects/gloo/constants" | ||
v1 "github.com/solo-io/gloo/projects/gloo/pkg/api/v1" | ||
v1_options "github.com/solo-io/gloo/projects/gloo/pkg/api/v1/options" | ||
"github.com/solo-io/gloo/projects/gloo/pkg/api/v1/ssl" | ||
|
@@ -69,7 +71,7 @@ func (t *translatorInstance) computeClusters( | |
} | ||
|
||
// This function is intented to be used when translating a single upstream outside of the context of a full snapshot. | ||
// This happens in GGv2 krt implementation. | ||
// This happens in the kube gateway krt implementation. | ||
func (t *translatorInstance) TranslateCluster( | ||
params plugins.Params, | ||
upstream *v1.Upstream, | ||
|
@@ -156,6 +158,10 @@ func (t *translatorInstance) initializeCluster( | |
DnsRefreshRate: dnsRefreshRate, | ||
PreconnectPolicy: preconnect, | ||
} | ||
// for kube gateway, use new stats name format | ||
if envutils.IsEnvTruthy(constants.GlooGatewayEnableK8sGwControllerEnv) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we do things like this elsewhere but after looking at it I feel like we should be checking these once at the start of a translation rather than checking mid translation where hypothetically some clusters may have this set true and some set false. Not a problem with this pr more of a thought for our general strategies There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah we have this check and/or the proxy type check sprinkled throughout the codebase and it's a bit messy..but luckily we should be able to rip them out soon! this is the only place that i saw that created a Cluster (besides some plugins that create Clusters for other purposes but with well-defined names that don't need this alt stat name) |
||
out.AltStatName = UpstreamToClusterStatsName(upstream) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is the most important part |
||
} | ||
|
||
if sslConfig := upstream.GetSslConfig(); sslConfig != nil { | ||
applyDefaultsToUpstreamSslConfig(sslConfig, t.settings.GetUpstreamOptions()) | ||
|
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.
i'm not sure if this should be considered breaking or not, given that it only affects stats names
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.
Well, dashboards and alerts can break, so I'd say it is a breaking change.