-
Notifications
You must be signed in to change notification settings - Fork 550
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: adding k8 support for container id detection #1962
base: main
Are you sure you want to change the base?
Changes from 8 commits
55b2e8d
b3e4365
8a4c557
10e407f
c69f689
1b95768
acf2c55
e6df278
009dccb
cd5f7b5
5062374
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 |
---|---|---|
|
@@ -25,6 +25,8 @@ import * as fs from 'fs'; | |
import * as util from 'util'; | ||
import { diag } from '@opentelemetry/api'; | ||
|
||
const { KubeConfig, CoreV1Api } = require('@kubernetes/client-node'); | ||
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. Any reason to use require instead of |
||
|
||
export class ContainerDetector implements Detector { | ||
readonly CONTAINER_ID_LENGTH = 64; | ||
readonly DEFAULT_CGROUP_V1_PATH = '/proc/self/cgroup'; | ||
|
@@ -36,7 +38,12 @@ export class ContainerDetector implements Detector { | |
|
||
async detect(_config?: ResourceDetectionConfig): Promise<Resource> { | ||
try { | ||
const containerId = await this._getContainerId(); | ||
let containerId = ''; | ||
if (this._isInKubernetesEnvironment()) { | ||
containerId = await this._getContainerIdK8(); | ||
} else { | ||
containerId = (await this._getContainerId()) || ''; | ||
} | ||
return !containerId | ||
? Resource.empty() | ||
: new Resource({ | ||
|
@@ -100,6 +107,43 @@ export class ContainerDetector implements Detector { | |
return containerIdStr || ''; | ||
} | ||
|
||
private _isInKubernetesEnvironment(): boolean { | ||
return process.env.KUBERNETES_SERVICE_HOST !== undefined; | ||
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 don't know k8s well enough to have confidence in its standard envvars and its client API usage. I'd be happier if others with k8s experience could weigh in. It might help to add some comments linking to official k8s docs justifying the envvars and APIs being used. 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 purely based out of my research - let me find other PRs or articles supporting this. Thanks for the review 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. |
||
} | ||
|
||
private async _getContainerIdK8(): Promise<string> { | ||
const kubeconfig = new KubeConfig(); | ||
kubeconfig.loadFromDefault(); | ||
|
||
const api = kubeconfig.makeApiClient(CoreV1Api); | ||
const namespace: string = process.env.NAMESPACE || 'default'; | ||
const containerName: string | undefined = process.env.CONTAINER_NAME; | ||
if (!containerName) { | ||
throw new Error('Container name not specified in environment'); | ||
} | ||
|
||
const response = await api.listNamespacePod(namespace); | ||
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. Is this listing all pods in the namespace? What if there are multiple pods in the namespace with the same container name (as would happen with a k8s depoyment)? Will each detect a random container ID? Listing all pods in a namespace is also much more expensive than getting a single pod. 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. Hello @dashpole May be I can provide a condition where it will look for a specific pod defined as environment variable. 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 a typo, should be |
||
|
||
const podWithContainer = response.body.items.find( | ||
(pod: { spec: { containers: any[] } }) => { | ||
return pod.spec.containers.some( | ||
container => container.name === containerName | ||
); | ||
} | ||
); | ||
|
||
if (!podWithContainer) { | ||
throw new Error(`No pods found with container name '${containerName}'.`); | ||
} | ||
|
||
const container = podWithContainer.spec.containers.find( | ||
(container: { name: string }) => container.name === containerName | ||
); | ||
const containerId = container ? container.containerID || '' : ''; | ||
|
||
return containerId; | ||
} | ||
|
||
/* | ||
cgroupv1 path would still exist in case of container running on v2 | ||
but the cgroupv1 path would no longer have the container id and would | ||
|
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.
A concern here is that this is a large dependency, but mostly that it still uses the deprecated
request
HTTP client library. This would bring in some CVEs (kubernetes-client/javascript#754 (comment)).There is an effort (https://github.com/kubernetes-client/javascript/blob/master/FETCH_MIGRATION.md kubernetes-client/javascript#754) to move away from
request
tofetch()
. That looks active. There was a recent release-candidate release with that work:'1.0.0-rc4': '2023-12-23T15:06:31.075Z'
. However, I haven't looked deeply and I'm not sure how soon that might come.Even if that came, an issue with
@kubernetes/[email protected]
might be that I believe it dropped compat with Node.js v14 in kubernetes-client/javascript@3a995fbThat commit unfortunately doesn't mention the Node.js version, nor is it tied to a PR with any discussion.
That same commit is dropping an AbortController shim for older Node.js version support. AbortController is avaialble starting in Node.js v14.17.0, so it is possible that
@kubernetes/[email protected]
would work withnode >=14.17.0
. They don't test with Node v14 though, so that is tenuous support.I haven't read through this PR yet. Would it be possible to do whatever k8s client calls without using
@kubernetes/client-node
? I don't know if it is a very complex API to call manually.Another option would possibly be to wait for
@kubernetes/[email protected]
and the OTel SDK 2.0 milestone (https://github.com/open-telemetry/opentelemetry-js/milestone/17), because that will drop support for Node.js v14.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 believe - This api takes care of adding filepath required for certifcates etc to be present.
I think we can certainly manually do all the operations that this does. It may be bit more code . I can look into it.
I see the migration to fetch and partial compatibility for node 14 as well. I think officially node 14 is not supported now.
Let me take a look at the effort.
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 was looking at the library itself.
It is officially maintained by Kubernetes SIG API Machinery.
I suggest we use this and create an issue there regarding node 14 and request --> fetch ( which is I believe is already in progress )
@trentm let me know what you think.