-
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
[Agent] Support Node and Service autodiscovery in k8s provider #26801
Changes from 19 commits
8e06537
8737d7b
8ff2e13
743348b
a7e6ccf
eb4ffa0
bd6956c
5dd5b94
6e71318
fc494de
4f12589
929c183
43f5136
54c912d
6002dd1
865eabb
31211cf
975746d
ea014a1
88260fd
2823bb6
f778936
6f39378
d42013f
701236c
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 |
---|---|---|
|
@@ -9,24 +9,72 @@ package kubernetes | |
|
||
import ( | ||
"time" | ||
|
||
"github.com/elastic/beats/v7/libbeat/logp" | ||
) | ||
|
||
// Config for kubernetes provider | ||
type Config struct { | ||
Scope string `config:"scope"` | ||
Resources Resources `config:"resources"` | ||
|
||
// expand config settings at root level of config too | ||
KubeConfig string `config:"kube_config"` | ||
Namespace string `config:"namespace"` | ||
SyncPeriod time.Duration `config:"sync_period"` | ||
CleanupTimeout time.Duration `config:"cleanup_timeout" validate:"positive"` | ||
|
||
// Needed when resource is a pod | ||
// Needed when resource is a Pod or Node | ||
Node string `config:"node"` | ||
} | ||
|
||
// Scope of the provider (cluster or node) | ||
Scope string `config:"scope"` | ||
// Resources config section for resources' config blocks | ||
type Resources struct { | ||
Pod *ResourceConfig `config:"pod"` | ||
Node *ResourceConfig `config:"node"` | ||
Service *ResourceConfig `config:"service"` | ||
} | ||
|
||
// ResourceConfig for kubernetes resource | ||
type ResourceConfig struct { | ||
KubeConfig string `config:"kube_config"` | ||
Namespace string `config:"namespace"` | ||
SyncPeriod time.Duration `config:"sync_period"` | ||
CleanupTimeout time.Duration `config:"cleanup_timeout" validate:"positive"` | ||
|
||
// Needed when resource is a Pod or Node | ||
Node string `config:"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. I'm wondering about use cases for resource specific settings, do you have anyone in mind? 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. Now that I'm thinking of it again, maybe it is over-engineering to provide this option at the moment since the base config shared for all the resources should cover the cases. Flexibility for different accesses per resource or variant settings options would be nice to think of but we can and see if users actually need them. So, I will change it and move to single config for all of the resources. |
||
} | ||
|
||
// InitDefaults initializes the default values for the config. | ||
func (c *Config) InitDefaults() { | ||
c.SyncPeriod = 10 * time.Minute | ||
c.CleanupTimeout = 60 * time.Second | ||
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. We may be hitting this issue #20543, not sure how we can do something now depending on the kind of data we are collecting. 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. Yeap, the provider is not really aware of the inputs right now, but maybe it could be handled better in the future if we introduce a "smart" controller which enables providers according to the inputs. |
||
c.SyncPeriod = 10 * time.Minute | ||
c.Scope = "node" | ||
} | ||
|
||
// Validate ensures correctness of config | ||
func (c *Config) Validate() error { | ||
// Check if resource is service. If yes then default the scope to "cluster". | ||
if c.Resources.Service != nil { | ||
if c.Scope == "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. It's interesting that you can override almost all settings per resource, but not scope 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. See #26801 (comment). |
||
logp.L().Warnf("can not set scope to `node` when using resource `Service`. resetting scope to `cluster`") | ||
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. Should this logger be namespaced? I also see |
||
} | ||
c.Scope = "cluster" | ||
} | ||
baseCfg := &ResourceConfig{ | ||
CleanupTimeout: c.CleanupTimeout, | ||
SyncPeriod: c.CleanupTimeout, | ||
KubeConfig: c.KubeConfig, | ||
Namespace: c.Namespace, | ||
Node: c.Node, | ||
} | ||
if c.Resources.Pod == nil { | ||
c.Resources.Pod = baseCfg | ||
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. what if the user only overrides 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. See #26801 (comment) |
||
} | ||
if c.Resources.Node == nil { | ||
c.Resources.Node = baseCfg | ||
} | ||
|
||
return nil | ||
} |
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.
Does it make sense to have
kube_config
andnode
for every type? Is the idea here that you can get pods from one cluster and different nodes from another cluster? Or is this needed for scoped permissions in eachkube_config
?This also will make the configuration of the provider change, which is a breaking configuration change.
Now it will be:
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.
Yeap, I had in mind to provide such kind of flexibility and give the option for possible different permissions per resource. However the generic use case here is with
inCluster
configs which means that after the improvements of #26947,kube_config
andnode
settings won't be needed in the general use case since defaults would be enough.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.
Could we still have a top-level defaults for all of these options? So it works as it does today, but if you want to provide specifics for each resource type you can? That would prevent changing what we have today as it would stay working with the same configurations, as well as provide the flexibility that you are adding here.