-
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 complete k8s metadata through composable provider #27691
Conversation
Signed-off-by: chrismark <chrismarkou92@gmail.com>
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
Trends 🧪💚 Flaky test reportTests succeeded. Expand to view the summary
Test stats 🧪
|
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
/test |
Pinging @elastic/integrations (Team:Integrations) |
I'm opening this for an early review. I will need to add/tune tests as well as do extensive manual testing, however an early review would be more than welcome so as to spot any possible foundational issues early on. |
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Heads-up on this: Unit tests were tuned accordingly and I'm running several manual testing scenarios to verify nothing is broken. I'm planning to work on adding e2e tests right after this one is merged (issue: elastic/e2e-testing#1090) I have particularly tried to "port" the logic from the old autodiscovery feature to this one and try to cover cases like short-living jobs etc. Review parts:
Maybe in the same release we can tune the metadata part (in follow-ups) in order to be aligned with the outcomes of #13911 and more specifically #16483 and #14875 |
ExcludeLabels []string `config:"exclude_labels"` | ||
|
||
LabelsDedot bool `config:"labels.dedot"` | ||
AnnotationsDedot bool `config:"annotations.dedot"` |
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 this just be the default? Do we need this configurable now?
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.
AnnotationsDedot bool `config:"annotations.dedot"` | ||
|
||
// Undocumented settings, to be deprecated in favor of `drop_fields` processor: | ||
IncludeCreatorMetadata bool `config:"include_creator_metadata"` |
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.
If undocumented and deprecated, why add it? Being this is all new to Elastic Agent, it could be the time to remove it.
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.
You are right this one is not exposed so it should be removed.
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.
Actually this touches code that is used by beats too. I will remove it in follow up PR target 8.0
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.
PR: #28006
|
||
AddResourceMetadata *metadata.AddResourceMetadataConfig `config:"add_resource_metadata"` | ||
IncludeLabels []string `config:"include_labels"` | ||
ExcludeLabels []string `config:"exclude_labels"` |
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.
Where is IncludeLabels
and ExcludeLabels
used? I was not able to find them in the diff.
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.
These are passed and used deeper in meta Generators, which is already existent codebase.
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.
@jsoriano it would be super helpful if you could review the discovery lifecycle start/stop events , proper termination etc since you had worked on this recently :)
This part looks good (waiting for e2e tests for confirmation 🙂), added only some thoughts, nothing really blocking.
@@ -9,6 +9,8 @@ import ( | |||
|
|||
k8s "k8s.io/client-go/kubernetes" | |||
|
|||
"github.com/elastic/beats/v7/libbeat/common" | |||
|
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.
Nit.
"github.com/elastic/beats/v7/libbeat/common/kubernetes/metadata" | ||
"github.com/elastic/beats/v7/libbeat/common/safemapstr" |
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.
Nit. Move beats imports to the last group of imports.
x-pack/elastic-agent/pkg/composable/providers/kubernetes/kubernetes.go
Outdated
Show resolved
Hide resolved
for _, c := range pod.Spec.EphemeralContainers { | ||
c := kubernetes.Container(c.EphemeralContainerCommon) | ||
containers = append(containers, &containerInPod{spec: c}) | ||
} |
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.
There are two places now where we collect all the containers in the pod and their statuses (the other here), this can be error-prone in future refactors. I wonder if this could be unified.
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.
Fair point. I will tune it! :)
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
@jsoriano @blakerouse I tested this extensively (manually) and I think is good to go. Would you mind giving it another review? Next steps will be to add 2e2 tests (in this iteration/release) and verify that we are aligned with #13911 (cc @MichaelKatsoulis ). |
/test |
Signed-off-by: chrismark <chrismarkou92@gmail.com>
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 this looks good to me.
I would prefer to see the dedot
feature just set to default and the options to turn if off removed. But I will leave that up to you to determine.
Signed-off-by: chrismark <chrismarkou92@gmail.com>
(cherry picked from commit 46d17b4)
* upstream/master: (658 commits) Add complete k8s metadata through composable provider (elastic#27691) Revert "Fix issue where --insecure didn't propogate to Fleet Server ES connection (elastic#27969)" (elastic#27997) Remove deprecated kafka fields (elastic#27938) [Filebeat] Add Base64 encoded HMAC & UUID template functions to httpjson input (elastic#27873) Improve httpjson template function join (elastic#27996) Remove kubernetes.container.image alias (elastic#27898) [Elastic Agent] Golden files for program tests (elastic#27862) [Elastic Agent] Disable modules.d in metricbeat (elastic#27860) libbeat/common/seccomp: provide default policy for linux arm64 (elastic#27955) Fix logger statement in aws-s3 input (elastic#27982) Fix wrong merge (elastic#27976) Fix issue where --insecure didn't propogate to Fleet Server ES connection (elastic#27969) Forward-port 7.14.2 changelog to master (elastic#27975) [Filebeat] Removing duplicate modules (aliases) Observability (elastic#27919) Fix path in vagrant windows script (elastic#27966) [Filebeat] Removing duplicate modules (aliases) and Cyberark (elastic#27915) No changelog for 8.0.0-alpha2 (elastic#27961) Add write access to 'url.value' from 'request.transforms'. (elastic#27937) Docker: remove deprecated fields (elastic#27933) Filebeat: Make all filesets disabled in default configuration (elastic#27762) ...
What does this PR do?
Add all k8s metadata via the provider.
Why is it important?
To support full metadata access like in Beats.
Checklist
CHANGELOG.next.asciidoc
orCHANGELOG-developer.next.asciidoc
.TODOs
How to test this PR locally
./elastic-agent -c ./elastic-agent.yml inspect output -o default
Using annotations (Note: annotations are not exposed to meta fields by default but are exposed in variable resolution mechanism)
Specify a condition based on an annotation like
condition: ${kubernetes.annotations.level} == 'production' AND ${kubernetes.container.port_name} == 'web'
and verify that input is created again. (note: tune Redis Pod manifest accordingly)Short-living pods
Define a log input with condition based on container's name:
Deploy elastic-agent and verify that it collects logs for the following short-living pod:
Short living Init containers are covered
Ensure logs are captured even if Pod is marked for deletion
Related issues
Notes for the reviewer
Most of the functionality is ported from
libbeat/autodiscover/providers/kubernetes
andlibbeat/common/kubernetes/metadata
.