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.

Sign up for GitHub

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

Jump to bottom

[processor/resourcedetection] introduce kubeadm detector #35450

Open
wants to merge 37 commits into
base: main
Choose a base branch
from
Open

[processor/resourcedetection] introduce kubeadm detector #35450

wants to merge 37 commits into from

Conversation

odubajDT
Copy link
Contributor

@odubajDT odubajDT commented Sep 27, 2024 •
edited
Loading

Description:

  • introduce kubeadm detector to detect local cluster name

Link to tracking Issue: #35116

@odubajDT odubajDT force-pushed the resourcedetection-local-cluster branch 2 times, most recently from 1bdc2fb to 3c5422f Compare September 30, 2024 06:49
@odubajDT odubajDT marked this pull request as ready for review September 30, 2024 08:05
@odubajDT odubajDT requested a review from a team as a code owner September 30, 2024 08:05
crobert-1
crobert-1 reviewed Sep 30, 2024
View reviewed changes
processor/resourcedetectionprocessor/README.md Outdated Show resolved Hide resolved
internal/metadataproviders/kubeadm/metadata.go Outdated Show resolved Hide resolved
processor/resourcedetectionprocessor/README.md Outdated Show resolved Hide resolved
processor/resourcedetectionprocessor/internal/kubeadm/config.go Outdated Show resolved Hide resolved
processor/resourcedetectionprocessor/internal/kubeadm/kubeadm.go Outdated Show resolved Hide resolved
processor/resourcedetectionprocessor/internal/kubeadm/kubeadm.go
if d.ra.K8sClusterName.Enabled {
clusterName, err := d.provider.ClusterName(ctx)
if err != nil {
return pcommon.NewResource(), "", fmt.Errorf("failed getting k8s cluster name: %w", err)
Copy link
Member

Choose a reason for hiding this comment

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

Nit, feel free to drop: I'm wondering if it would be better to future-proof this by not returning the error right away, and returning the error at the end.

If we end up adding more resource attributes the existing logic would not record the other, unrelated resource attributes, even if they could be detected and emitted successfully.

@odubajDT odubajDT force-pushed the resourcedetection-local-cluster branch from 5dbc1c7 to 99bf118 Compare October 2, 2024 07:45
@odubajDT odubajDT force-pushed the resourcedetection-local-cluster branch 2 times, most recently from 062d966 to 80bd836 Compare October 4, 2024 05:50
@odubajDT odubajDT requested a review from crobert-1 October 4, 2024 10:55
crobert-1
crobert-1 reviewed Oct 4, 2024
View reviewed changes
processor/resourcedetectionprocessor/internal/kubeadm/kubeadm.go
Comment on lines +22 to +23
defaultConfigMapName = "kubeadm-config"
defaultConfigMapNamespace = "kube-system"
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Do these defaults need to be in the resource detection processor? Could they instead be moved to internal/metadataproviders/kubeadm/metadata.go? It doesn't appear to be necessary to pass these down to the NewProvider method, but maybe there's a scenario I'm not thinking of.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only scenario would be that we could make the configMap name and namespace configurable in the future. Currently that is not needed at all.

Technically we can move the default values to the internal package you mentioned, but I do not see much advantage technically there. Leaving it up to your decision.

@odubajDT odubajDT force-pushed the resourcedetection-local-cluster branch from 6436f3a to 0eda211 Compare October 9, 2024 06:30
@odubajDT
Copy link
Contributor Author

@crobert-1 any other suggestions?

@odubajDT
Copy link
Contributor Author

cc @crobert-1

@odubajDT odubajDT force-pushed the resourcedetection-local-cluster branch from aa63a96 to ad7502e Compare November 6, 2024 06:09
odubajDT added 10 commits November 7, 2024 13:00
@odubajDT
[processor/resourcedetection] introduce kubeadm detector
534cf01 
Signed-off-by: odubajDT <[email protected]>
@odubajDT
add chlog
bcf33b2 
Signed-off-by: odubajDT <[email protected]>
@odubajDT
fix chlog
e4f7663 
Signed-off-by: odubajDT <[email protected]>
@odubajDT
polish config implementation
3eb6a0a 
Signed-off-by: odubajDT <[email protected]>
@odubajDT
create test files
df3d293 
Signed-off-by: odubajDT <[email protected]>
@odubajDT
add test files to internal
11caca7 
Signed-off-by: odubajDT <[email protected]>
@odubajDT
add tests for metadata
02ee56c 
Signed-off-by: odubajDT <[email protected]>
@odubajDT
add unit tests
07fe879 
Signed-off-by: odubajDT <[email protected]>
@odubajDT
more unit tests
6a66681 
Signed-off-by: odubajDT <[email protected]>
@odubajDT
rename function
78c1575 
Signed-off-by: odubajDT <[email protected]>
@odubajDT
fir err msg
4cbbeb3 
Signed-off-by: odubajDT <[email protected]>
@odubajDT
move default values to kubeadm.go
023406d 
Signed-off-by: odubajDT <[email protected]>
@odubajDT
make generate
b8cd07c 
Signed-off-by: odubajDT <[email protected]>
@odubajDT
fix after rebase
9b0389f 
Signed-off-by: odubajDT <[email protected]>
@odubajDT odubajDT force-pushed the resourcedetection-local-cluster branch from 21ca676 to 9b0389f Compare November 7, 2024 12:03
@ChrsMark
Copy link
Member

@dashpole @Aneurysm9 could you also take a look?

ChrsMark
ChrsMark approved these changes Nov 26, 2024
View reviewed changes
Copy link
Member

@ChrsMark ChrsMark left a comment

Choose a reason for hiding this comment

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

Left 2 nits. Otherwise LGTM.

internal/metadataproviders/kubeadm/metadata.go Outdated Show resolved Hide resolved
processor/resourcedetectionprocessor/README.md Outdated Show resolved Hide resolved
@odubajDT
polishing
09b1548 
Signed-off-by: odubajDT <[email protected]>
@github-actions github-actions bot mentioned this pull request Dec 3, 2024
Closed
@github-actions github-actions bot mentioned this pull request Dec 10, 2024
Open
@github-actions github-actions bot mentioned this pull request Dec 17, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@crobert-1 crobert-1 crobert-1 left review comments

@ChrsMark ChrsMark ChrsMark approved these changes

@Aneurysm9 Aneurysm9 Awaiting requested review from Aneurysm9

@dashpole dashpole Awaiting requested review from dashpole dashpole is a code owner

Assignees

@crobert-1 crobert-1

Labels
internal/metadataproviders processor/resourcedetection Resource detection processor waiting-for-code-owners
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@odubajDT @ChrsMark @crobert-1