-
Notifications
You must be signed in to change notification settings - Fork 2
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
Add OpenShift version detector #168
Conversation
0a90056
to
d01617b
Compare
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.
Overall I like the idea, I left some nits and 1 comment about merging the workflow into already existing platform detection one.
Latest updates stuffs it into the workflow that handles cluster versions, with the discussed change to make providers run once in a separate (currently local) branch. Moved the designated namespace/pod constants into here since it was requested and I don't think there's any great place to put them. Re creating a field to store the distro in general, I don't know that we have need to build out scaffolding for as-yet unplanned data points, but figure we can discuss it in sync. Still fetches the expected version info in tests, waiting to confirm on an actual OpenShift as my test environment is being a brat. |
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.
While the code in here is good to be merged I'd suggest that we pause for a bit and think about using that provider in the potential distro detector provider.
The reason for this is that we might want to discovery different platform at some point. Would we then jam another "XYZdistroversionprovider" next to this one? And then yet another one? There can only be one "version" logically hence that would indicate that all others would be sent as none/unknown which doesn't sound like a great design.
Adding a new distroflavor
and distroversion
would make sense to me where the logic of detecting OpenShift and its version would be used. Even if the only value that it could take was OpenShift
for now.
We're gonna need some UTs for that as well, something like https://github.com/Kong/kubernetes-telemetry/blob/main/pkg/provider/k8sclusterversionprovider_test.go or https://github.com/Kong/kubernetes-telemetry/blob/main/pkg/provider/k8scloudproviderprovider_test.go
Scan a Pod for OpenShift environment variables that indicate the OpenShift cluster version. If this information is available, include an openshift_version key in metrics.
I don't think we want to use a single field for hypothetical additional distribution metrics. They aren't comparable values since they all release independently, and dashboard views would only ever display a spread for one, i.e. you'd add a widget that displays OpenShift versions only, not OpenShift and K3S versions both. I also don't know that we'd even add the others. AFAIK we have a particular interest in OpenShift and the others either haven't really come up or don't change the environment as much as OpenShift does, but that's more a question for @mheap. Process-wise, this sort of discussion should happen during grooming, before Kong/kubernetes-ingress-controller#3149 was moved into ready. |
Openshift is the only one we've been asked about recently. I'm +1 on having separate fields and the flavour encoded in the field name. Understood that this could lead to many fields, but we don't intend to aggregate across multiple flavours and this prevents accidental aggregation |
Co-authored-by: Patryk Małek <patryk.malek@konghq.com>
Scan a Pod for OpenShift environment variables that indicate the OpenShift cluster version.
See research in Kong/kubernetes-ingress-controller#3149 (comment). This implements the Pod environment variable option, which may be less accurate but requires no new permissions.
Confirmed able to report version on a local OpenShift 4.13 instance using Kong/kubernetes-ingress-controller#4211:
but looking for initial review on this approach (versus other resources that require new permissions) before building out the tests.