K8SPG-1083 improve platform detection to reduce unknown telemetry values#1655
Open
hors wants to merge 11 commits into
Open
K8SPG-1083 improve platform detection to reduce unknown telemetry values#1655hors wants to merge 11 commits into
hors wants to merge 11 commits into
Conversation
Replace CRD-based detection (which relied on optional addons being installed) with discovery API group checks and API server URL patterns. Add detection for AKS, DOKS, OKE, ACK, NKP, Platform9, Tanzu, and Rancher alongside the existing GKE, EKS, and OpenShift detections.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the operator’s platform/managed-Kubernetes detection used for telemetry, moving away from CRD-based heuristics toward API discovery group checks and API server host pattern checks to reduce “unknown” platform values.
Changes:
- Added a
hasAPIGrouphelper that detects platforms by checking for the presence of API groups via the discovery API. - Updated existing platform detection (notably GKE/EKS) to use API groups and API server host patterns rather than optional addon CRDs.
- Added detection and telemetry labels for AKS, DOKS, OKE, ACK, NKP, Platform9, Tanzu, and Rancher.
Comment on lines
+361
to
+370
| // hasAPIGroup returns true if the cluster exposes the given API group name. | ||
| // Uses the discovery API which is available to all authenticated users with no extra RBAC. | ||
| func hasAPIGroup(ctx context.Context, cfg *rest.Config, groupName string) bool { | ||
| client, err := discovery.NewDiscoveryClientForConfig(cfg) | ||
| assertNoError(err) | ||
|
|
||
| if err != nil { | ||
| return false | ||
| } | ||
| groups, err := client.ServerGroups() | ||
| if err != nil { | ||
| assertNoError(err) | ||
| return false |
Comment on lines
+389
to
+396
| func isEKS(ctx context.Context, cfg *rest.Config) bool { | ||
| log := logging.FromContext(ctx) | ||
| // EKS API server hostnames always end with .eks.amazonaws.com. | ||
| if strings.Contains(cfg.Host, ".eks.amazonaws.com") { | ||
| logging.FromContext(ctx).Info("detected EKS environment") | ||
| return true | ||
| } | ||
| return false | ||
| } |
Comment on lines
+470
to
+493
| case isTanzu(ctx, cfg): | ||
| return "tanzu" | ||
| case isRancher(ctx, cfg): | ||
| return "rancher" |
hors
force-pushed
the
detect-more-platforms
branch
from
June 26, 2026 13:53
44406c3 to
6cbdb1e
Compare
Comment on lines
+365
to
+369
| client, err := discovery.NewDiscoveryClientForConfig(cfg) | ||
| assertNoError(err) | ||
|
|
||
| if err != nil { | ||
| log.V(1).Info("platform detection: could not create discovery client", "error", err.Error()) | ||
| return false | ||
| } |
gkech
reviewed
Jun 29, 2026
gkech
reviewed
Jun 29, 2026
hors
force-pushed
the
detect-more-platforms
branch
from
June 29, 2026 10:14
6cbdb1e to
ef39113
Compare
hors
changed the title
hors
marked this pull request as ready for review
July 9, 2026 15:52
hors
requested review from
egegunes,
mayankshah1607,
nmarukovich,
oksana-grishchenko and
pooknull
as code owners
July 9, 2026 15:52
Comment on lines
373
to
377
| groups, err := client.ServerGroups() | ||
| if err != nil { | ||
| assertNoError(err) | ||
| log.V(1).Info("platform detection: could not list API groups", "error", err.Error()) | ||
| return false | ||
| } |
| if err != nil { | ||
| assertNoError(err) | ||
| if hasAPIGroup(ctx, cfg, "security.openshift.io") { | ||
| logging.FromContext(ctx).Info("detected Openshift environment") |
Comment on lines
+456
to
+461
| for _, probe := range platformProbes { | ||
| if probe.detect(ctx, cfg) { | ||
| logging.FromContext(ctx).Info("detected " + probe.label + " environment") | ||
| return probe.name | ||
| } | ||
| } |
Comment on lines
+433
to
+435
| host := strings.TrimPrefix(cfg.Host, "https://") | ||
| host = strings.TrimPrefix(host, "http://") | ||
| netConn, err := (&tls.Dialer{Config: tlsCfg}).DialContext(ctx, "tcp", host) |
egegunes
reviewed
Jul 10, 2026
egegunes
previously approved these changes
Jul 20, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (4)
cmd/postgres-operator/main.go:422
- Problem:
platformProbeslogs "Openshift", but the proper product name and existing codebase spelling is "OpenShift".
Why it matters: Keeps logs/telemetry consistent and avoids introducing a new spelling variant.
Fix: Update the label capitalization.
var platformProbes = []platformProbe{
{name: "openshift", label: "Openshift", apiGroups: []string{"security.openshift.io"}},
{name: "gke", label: "GKE", apiGroups: []string{"networking.gke.io"}},
cmd/postgres-operator/main.go:466
- Problem: New platform detection logic is not covered by unit tests, while this package already has tests (e.g.,
main_test.go).
Why it matters: Detection regressions will silently change telemetry and OpenShift-specific behavior.
Fix: Add table-driven unit tests that validate probe ordering/host-based matches and that error paths fall back to "unknown" without panicking.
func detectPlatform(ctx context.Context, cfg *rest.Config) string {
for _, probe := range platformProbes {
if probe.detect(ctx, cfg) {
logging.FromContext(ctx).Info("detected " + probe.label + " environment")
return probe.name
cmd/postgres-operator/main.go:377
- Problem:
hasAPIGroupcreates a new discovery client and callsServerGroups()on every invocation;detectPlatformcan trigger many discovery round-trips during startup.
Why it matters: Excessive discovery calls can slow startup and may fail on clusters where discovery is rate-limited.
Fix: Cache the discovery results for the duration of a singledetectPlatformcall (e.g., build amap[string]struct{}from oneServerGroups()call and check membership for all probes/groups).
// hasAPIGroup returns true if the cluster exposes the given API group name.
// Uses the discovery API; note that hardened clusters may restrict discovery access.
func hasAPIGroup(ctx context.Context, cfg *rest.Config, groupName string) bool {
log := logging.FromContext(ctx)
client, err := discovery.NewDiscoveryClientForConfig(cfg)
cmd/postgres-operator/main.go:447
- Problem:
detectAKSdials the API server with the process context (no timeout) and assumescfg.Hostalready includes a port.
Why it matters: A stalled TCP/TLS dial can block operator startup; and a host without an explicit port will makeDialContextfail.
Fix: Add a short timeout and normalize the dial address to include a default port when missing.
host := strings.TrimPrefix(cfg.Host, "https://")
host = strings.TrimPrefix(host, "http://")
netConn, err := (&tls.Dialer{Config: tlsCfg}).DialContext(ctx, "tcp", host)
if err != nil {
logging.FromContext(ctx).V(1).Info("platform detection: could not dial API server", "error", err.Error())
mayankshah1607
requested changes
Jul 22, 2026
| func isGKE(ctx context.Context, cfg *rest.Config) bool { | ||
| // hasAPIGroup returns true if the cluster exposes the given API group name. | ||
| // Uses the discovery API; note that hardened clusters may restrict discovery access. | ||
| func hasAPIGroup(ctx context.Context, cfg *rest.Config, groupName string) bool { |
Member
There was a problem hiding this comment.
We already have a helper utility GroupVersionKindExists here -#1655
I'd suggest we move this to a GroupVersionExists in the same package for consistency
gkech
reviewed
Jul 22, 2026
| {name: "openshift", label: "Openshift", apiGroups: []string{"security.openshift.io"}}, | ||
| {name: "gke", label: "GKE", apiGroups: []string{"networking.gke.io"}}, | ||
| // crd.k8s.amazonaws.com (VPC CNI) and metrics.eks.amazonaws.com are independent EKS signals. | ||
| {name: "eks", label: "EKS", apiGroups: []string{"crd.k8s.amazonaws.com", "metrics.eks.amazonaws.com"}}, |
Contributor
There was a problem hiding this comment.
if rancher run on AWS, what's the api group returned?
Contributor
There was a problem hiding this comment.
do we have a more eks specific signal?
Collaborator
Author
There was a problem hiding this comment.
The most reliable truly-EKS signal without extra RBAC requires both groups
return hasAPIGroup(ctx, cfg, "crd.k8s.amazonaws.com") &&
hasAPIGroup(ctx, cfg, "metrics.eks.amazonaws.com")
}},
The problem is that clusters without the metrics addon enabled would fall back to unknown :(
| } | ||
| host := strings.TrimPrefix(cfg.Host, "https://") | ||
| host = strings.TrimPrefix(host, "http://") | ||
| netConn, err := (&tls.Dialer{Config: tlsCfg}).DialContext(ctx, "tcp", host) |
Contributor
There was a problem hiding this comment.
Should we add a timeout here? Raw TLS connection potentially can hang which would block the operator startup
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| return false | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
[gofmt] reported by reviewdog 🐶
Suggested change
hors
requested review from
Copilot,
gkech,
mayankshah1607 and
oksana-grishchenko
July 22, 2026 10:20
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
cmd/postgres-operator/main.go:149
- The upgrade-check scheduler is now always told
openshift=false, which changes the telemetry/header generation behavior compared to the previous OpenShift detection when upgrade checking is enabled.
assertNoError(upgradecheck.ManagedScheduler(mgr,
false, os.Getenv("CHECK_FOR_UPGRADES_URL"), versionString, nil))
cmd/postgres-operator/main.go:441
tls.Dialer.DialContextrequires an address inhost:portform; kubeconfigs commonly setcfg.Hostwithout an explicit port (e.g.https://xxx.azmk8s.io). In that case this will fail with "missing port in address" and AKS detection will always return false.
host := strings.TrimPrefix(cfg.Host, "https://")
host = strings.TrimPrefix(host, "http://")
dialCtx, cancel := context.WithTimeout(ctx, 5*time.Second)
defer cancel()
netConn, err := (&tls.Dialer{Config: tlsCfg}).DialContext(dialCtx, "tcp", host)
| client, err := discovery.NewDiscoveryClientForConfig(cfg) | ||
| assertNoError(err) | ||
| var platformProbes = []platformProbe{ | ||
| {name: "openshift", label: "Openshift", apiGroups: []string{"security.openshift.io"}}, |
nmarukovich
approved these changes
Jul 22, 2026
Collaborator
commit: cc35cc4 |
oksana-grishchenko
approved these changes
Jul 22, 2026
egegunes
approved these changes
Jul 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replace CRD-based detection (which relied on optional addons being installed) with discovery API group checks and API server URL patterns.
Add detection for AKS, DOKS, OKE, ACK, NKP, Platform9, Tanzu, and Rancher alongside the existing GKE, EKS, and OpenShift detections.
CHANGE DESCRIPTION
Problem:
Short explanation of the problem.
Cause:
Short explanation of the root cause of the issue if applicable.
Solution:
Short explanation of the solution we are providing with this PR.
CHECKLIST
Jira
Needs Doc) and QA (Needs QA)?Tests
Config/Logging/Testability