-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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_cloud_metadata - Retrieve AKS cluster name and id #37685
add_cloud_metadata - Retrieve AKS cluster name and id #37685
Conversation
This pull request does not have a backport label.
To fixup this pull request, you need to add the backport labels for the needed
|
💔 Build Failed
Expand to view the summary
Build stats
Pipeline error
❕ Flaky test reportNo test was executed to be analysed. 🤖 GitHub commentsExpand to view the GitHub comments
To re-run your PR in the CI, just comment with:
|
💔 Tests Failed
Expand to view the summary
Build stats
Test stats 🧪
Test errors
Expand to view the tests failures
|
❕ Build Aborted
Expand to view the summary
Build stats
Test stats 🧪
Steps errors
Expand to view the steps failures
|
❕ Build Aborted
Expand to view the summary
Build stats
Test stats 🧪
Test errors
Expand to view the tests failures
|
💔 Tests Failed
Expand to view the summary
Build stats
Test stats 🧪
Test errors
Expand to view the tests failures
|
// hfetcher represents an http fetcher to retrieve metadata from azure metadata endpoint | ||
hfetcher, err := newMetadataFetcher(config, "azure", azHeaders, metadataHost, azHttpSchema, azMetadataURI) | ||
if err != nil { | ||
return hfetcher, err |
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.
Can we add fmt.Errorf
statements to the errors here? We put them in some of the later functions, so we should be consistent.
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.
@fearful-symmetry Done
@@ -53,4 +52,12 @@ | |||
path: cloud.region | |||
migration: true | |||
|
|||
- name: azure.resourcegroup |
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.
Is this consistent with what we have been using in Fleet integrations? I think I recall azure.resource.group
being used over there.
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 am also a little concerned about this processor beginning to produce fields outside of the ECS cloud
and orchestrator
namespaces.
Downstream consumers (e.g Fleet integrations) should ideally be updated to have this field included in their mappings since it's not part of ECS. This affects almost all integrations since Agent includes add_cloud_metadata
with most input types from Beats. (Imagine if we ran our CI on Azure for Fleet integration system testing, after this merges every integration's system tests would begin failing because this field was not defined in its mappings.)
The other concern is whether or not existing pipelines will safely handle the addition of this field. For example if an ingest pipeline is already using this field then does it safely handle the case where the field already exists.
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.
The azure.resource.group
, created by a pipeline? as I noticed is the resource group stripped. It only includes the group's name. The addition of the azure.resourcegroup.name
field I did was to me a nice to have and definitely not the reason of this PR.
I understand that it then needs to be added in all the integrations because it comes from the processor.
Also it is confusing having two similar named fields. I chose the naming to be compliant with Otel.
I hear your concerns and I will remove this field as it beats the purpose of this PR which was to add the orchestrator ecs fields.
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.
The azure.resource.group , created by a pipeline?
I'm not familiar with these integrations, but, yes, I see it being created by a grok in a few pipeline.
I understand that it then needs to be added in all the integrations because it comes from the processor.
This is a really general problem we have with Fleet integrations in that there is no easy or centralized way to account for the fields being added by the processors.
I hear your concerns and I will remove this field as it beats the purpose of this PR which was to add the orchestrator ecs fields.
👍
libbeat/processors/add_cloud_metadata/docs/add_cloud_metadata.asciidoc
Outdated
Show resolved
Hide resolved
libbeat/processors/add_cloud_metadata/docs/add_cloud_metadata.asciidoc
Outdated
Show resolved
Hide resolved
libbeat/processors/add_cloud_metadata/docs/add_cloud_metadata.asciidoc
Outdated
Show resolved
Hide resolved
…asciidoc Co-authored-by: Andrew Kroh <andrew.kroh@elastic.co>
…asciidoc Co-authored-by: Andrew Kroh <andrew.kroh@elastic.co>
Co-authored-by: Andrew Kroh <andrew.kroh@elastic.co>
Co-authored-by: Andrew Kroh <andrew.kroh@elastic.co>
Co-authored-by: Andrew Kroh <andrew.kroh@elastic.co>
@@ -53,4 +52,12 @@ | |||
path: cloud.region | |||
migration: true | |||
|
|||
- name: azure.resourcegroup |
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.
The azure.resource.group , created by a pipeline?
I'm not familiar with these integrations, but, yes, I see it being created by a grok in a few pipeline.
I understand that it then needs to be added in all the integrations because it comes from the processor.
This is a really general problem we have with Fleet integrations in that there is no easy or centralized way to account for the fields being added by the processors.
I hear your concerns and I will remove this field as it beats the purpose of this PR which was to add the orchestrator ecs fields.
👍
Co-authored-by: Andrew Kroh <andrew.kroh@elastic.co>
💔 Build Failed
Failed CI StepsHistory
|
💔 Build Failed
Failed CI StepsHistory
|
💔 Build Failed
Failed CI StepsHistory
|
💚 Build Succeeded
History
|
💚 Build Succeeded
History
|
💚 Build Succeeded
History
|
Proposed commit message
add_cloud_metadata
processor withorchestrator.cluster.name
andorchestrator.cluster.id
for AKS clusters.Checklist
CHANGELOG.next.asciidoc
orCHANGELOG-developer.next.asciidoc
.How to test this PR locally
In order to acquire a CLIENT_SECRET and CLIENT_ID follow the app_registration instructions.
GOOS=linux GOARCH=amd64 go build
kubectl cp ./metricbeat metricbeat-9lkhf:/usr/share/metricbeat/ -n kube-system
./metricbeat -c /etc/metricbeat.yml -e
orchestrator.cluster.name
andorchestrator.cluster.id
being populatedRelated issues
Use cases
Screenshots
Logs