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.

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 support for kubernetes provider to recognize namespace level defaults #16321

Merged
merged 3 commits into from
Mar 10, 2020

Conversation

vjsamuel
Copy link
Contributor

@vjsamuel vjsamuel commented Feb 14, 2020

This PR allows namespace level defaults to be configured on the namespace object for either service or pod hints autodiscover. This will need:

add_resource_metadata.namespace.enabled: true

to be set on the kubernetes autodiscover provider.

This PR has a known gap where a pod is deployed before the namespace annotations are added. In this case, autodiscover wouldnt be able to add in the defaults. To fix that behavior we would need to ensure that we handle resync events as updates. Currently we ignore resync events as the resource version remains the same.

@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

1 similar comment
@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@vjsamuel vjsamuel changed the title Add support for kubernetes provider to recognize namespace level defa… Add support for kubernetes provider to recognize namespace level defaults Feb 14, 2020
@vjsamuel vjsamuel force-pushed the add_namespace_defaults branch from dcbe36e to 6ac6d69 Compare February 14, 2020 07:17
@blakerouse blakerouse added the containers Related to containers use case label Feb 18, 2020
@blakerouse
Copy link
Contributor

We have a template that is generally used when creating a PR, could you update this PR to fill out all those sections.

Also did (or could) you file a bug for the behavior your trying to solve and can you provide steps on how to test this PR?


// Fall back to defaults on the namespace if there were no hints on the pods
if len(hints) == 0 {
if rawAnn, ok := kubeMeta["defaults"]; ok {
Copy link
Contributor

Choose a reason for hiding this comment

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

It sounds to me that we should do a mix of namespace hints + pod specific hints? Example:

I may have a setting to do json decode for all pods, but for one specific pod in the namespace I may want to tune how multiline goes, without affecting the JSON parsing?

Copy link
Contributor

@exekias exekias left a comment

Choose a reason for hiding this comment

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

Thank you for contributing! I commented in the PR. I think we need to define how namespace hints and Pod hints should interact with each other (order of precedence, full override..), let's keep the conversation going!

Once settled, this will need tests & docs

@vjsamuel vjsamuel force-pushed the add_namespace_defaults branch from 24d5180 to 21af677 Compare February 24, 2020 22:24
@vjsamuel
Copy link
Contributor Author

I have added changes such that annotations that are missing on the pod/service are defaulted with the help of namespace level annotations. i have added test cases for the same. @exekias PTAL

event["config"] = config
} else {

Copy link
Contributor

Choose a reason for hiding this comment

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

empty else?

Copy link
Contributor

@exekias exekias left a comment

Choose a reason for hiding this comment

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

Thank you for working on this! I left a few comments

for k, v := range namespace.GetAnnotations() {
safemapstr.Put(defaults, k, v)
}
kubemeta["defaults"] = defaults
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need to create a new key for this. metadata package is already able to put namespace annotations under kubernetes.namespace.annotations right? We can add that to the kubemeta object, in the same way we always add pod annotations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

onl in 8.0 we will have namespace as a MapStr. until then it would be namespace_annotations

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

good catch, that's true! I think we should do the same here

}

// Look at all the namespace level default annotations and do a merge with priority going to the pod annotations.
if rawNsAnn, ok := kubeMeta["defaults"]; ok {
Copy link
Contributor

Choose a reason for hiding this comment

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

we would be getting namespace.annotations here

@exekias
Copy link
Contributor

exekias commented Feb 27, 2020

Thank you for the changes, I think this is looking good! let's have the docs in and it's ready to go 🎉

@vjsamuel vjsamuel force-pushed the add_namespace_defaults branch from 7440678 to 9ae203a Compare March 3, 2020 06:26
@vjsamuel
Copy link
Contributor Author

vjsamuel commented Mar 3, 2020

@exekias added docs.

@exekias
Copy link
Contributor

exekias commented Mar 3, 2020

ok to test

@vjsamuel vjsamuel force-pushed the add_namespace_defaults branch from 9ae203a to 54d0b5c Compare March 10, 2020 06:00
@exekias exekias merged commit 004d36c into elastic:master Mar 10, 2020
@vjsamuel vjsamuel deleted the add_namespace_defaults branch March 10, 2020 17:20
@exekias exekias added test-plan Add this PR to be manual test plan and removed needs tests needs_docs labels Mar 17, 2020
exekias pushed a commit to exekias/beats that referenced this pull request Mar 17, 2020
…ults (elastic#16321)

* Add support for kubernetes provider to recognize namespace level default hints

(cherry picked from commit 004d36c)
@exekias exekias added the v7.7.0 label Mar 17, 2020
@andresrc andresrc added the test-plan-added This PR has been added to the test plan label Mar 21, 2020
exekias pushed a commit that referenced this pull request Mar 24, 2020
…ognize namespace level defaults (#17054)

* Add support for kubernetes provider to recognize namespace level defaults (#16321)

* Add support for kubernetes provider to recognize namespace level default hints

(cherry picked from commit 004d36c)

* Update CHANGELOG.next.asciidoc

Co-authored-by: Vijay Samuel <[email protected]>
@exekias
Copy link
Contributor

exekias commented Mar 26, 2020

How to test this:

Deploy Filebeat (or Metricbeat, but Filebeat makes more sense here) in kubernetes, enable hints based autodiscover with add_resource_metadata.namespace.enabled: true. Now adding namespace hints should apply to all pods in the namespace.

For instance, adding this annotation to the namespace:

co.elastic.logs/enabled: false

Should result on Filebeat stop retrieving logs from the pods in that namespace, while the others should still be retrieved. Also we should test that this can be overridden per pod, for instance, disable collection for a whole namespace, but enable it for one of the pods only

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autodiscovery containers Related to containers use case enhancement review Team:Platforms Label for the Integrations - Platforms team test-plan Add this PR to be manual test plan test-plan-added This PR has been added to the test plan v7.7.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants