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

[Metricbeat][Kubernetes] Remove mandatory permissions for namespace and node #38762

Merged
merged 10 commits into from
Apr 23, 2024
1 change: 1 addition & 0 deletions CHANGELOG-developer.next.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ The list below covers the major changes between 7.0.0-rc2 and main only.

==== Bugfixes

- Do not start namespace and node watchers on metricbeat autodiscover if `add_resource_metadata` is disabled along with `hints`.{pull}38762[38762]
constanca-m marked this conversation as resolved.
Show resolved Hide resolved
- Fix multiple metricbeat instances reporting same metrics when using autodiscover with provider kubernetes, and ensure leader elector is always running in autodiscover mode.{pull}38471[38471]
- Fix how Prometheus histograms are calculated when percentiles are provide.{pull}36537[36537]
- Stop using `mage:import` in community beats. This was ignoring the vendorized beats directory for some mage targets, using the code available in GOPATH, this causes inconsistencies and compilation problems if the version of the code in the GOPATH is different to the vendored one. Use of `mage:import` will continue to be unsupported in custom beats till beats is migrated to go modules, or mage supports vendored dependencies. {issue}13998[13998] {pull}14162[14162]
Expand Down
35 changes: 20 additions & 15 deletions libbeat/autodiscover/providers/kubernetes/pod.go
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ type pod struct {
func NewPodEventer(uuid uuid.UUID, cfg *conf.C, client k8s.Interface, publish func(event []bus.Event)) (Eventer, error) {
logger := logp.NewLogger("autodiscover.pod")

var replicaSetWatcher, jobWatcher kubernetes.Watcher
var replicaSetWatcher, jobWatcher, namespaceWatcher, nodeWatcher kubernetes.Watcher

config := defaultConfig()
err := cfg.Unpack(&config)
Expand Down Expand Up @@ -96,22 +96,27 @@ func NewPodEventer(uuid uuid.UUID, cfg *conf.C, client k8s.Interface, publish fu
return nil, fmt.Errorf("couldn't create watcher for %T due to error %w", &kubernetes.Pod{}, err)
}

options := kubernetes.WatchOptions{
SyncTimeout: config.SyncPeriod,
Node: config.Node,
Namespace: config.Namespace,
}

metaConf := config.AddResourceMetadata
nodeWatcher, err := kubernetes.NewNamedWatcher("node", client, &kubernetes.Node{}, options, nil)
if err != nil {
logger.Errorf("couldn't create watcher for %T due to error %+v", &kubernetes.Node{}, err)

if metaConf.Node.Enabled() || config.Hints.Enabled() {
options := kubernetes.WatchOptions{
SyncTimeout: config.SyncPeriod,
Node: config.Node,
Namespace: config.Namespace,
}
nodeWatcher, err = kubernetes.NewNamedWatcher("node", client, &kubernetes.Node{}, options, nil)
if err != nil {
logger.Errorf("couldn't create watcher for %T due to error %+v", &kubernetes.Node{}, err)
}
}
namespaceWatcher, err := kubernetes.NewNamedWatcher("namespace", client, &kubernetes.Namespace{}, kubernetes.WatchOptions{
SyncTimeout: config.SyncPeriod,
}, nil)
if err != nil {
logger.Errorf("couldn't create watcher for %T due to error %+v", &kubernetes.Namespace{}, err)

if metaConf.Namespace.Enabled() || config.Hints.Enabled() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we actually need config.Hints.Enabled()?
Especially that now metaConf.Namespace has default value true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe not, but we had this before and I think changing it could be a breaking change, correct?

namespaceWatcher, err = kubernetes.NewNamedWatcher("namespace", client, &kubernetes.Namespace{}, kubernetes.WatchOptions{
SyncTimeout: config.SyncPeriod,
}, nil)
if err != nil {
logger.Errorf("couldn't create watcher for %T due to error %+v", &kubernetes.Namespace{}, err)
}
}

// Resource is Pod so we need to create watchers for Replicasets and Jobs that it might belongs to
Expand Down
107 changes: 107 additions & 0 deletions libbeat/autodiscover/providers/kubernetes/pod_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2108,6 +2108,113 @@ func TestNodePodUpdater(t *testing.T) {
}
}

func TestPodEventer_Namespace_Node_Watcher(t *testing.T) {
client := k8sfake.NewSimpleClientset()
uuid, err := uuid.NewV4()
if err != nil {
t.Fatal(err)
}

tests := []struct {
cfg mapstr.M
expectedNil bool
name string
msg string
}{
{
cfg: mapstr.M{
"resource": "pod",
"node": "node-1",
"add_resource_metadata": mapstr.M{
"namespace.enabled": false,
"node.enabled": false,
},
"hints.enabled": false,
"builders": []mapstr.M{
{
"mock": mapstr.M{},
},
},
},
expectedNil: true,
name: "add_resource_metadata.namespace and add_resource_metadata.node disabled and hints disabled.",
msg: "Watcher should be nil.",
},
{
cfg: mapstr.M{
"resource": "pod",
"node": "node-1",
"add_resource_metadata": mapstr.M{
"namespace.enabled": false,
"node.enabled": false,
},
"hints.enabled": true,
},
expectedNil: false,
name: "add_resource_metadata.namespace and add_resource_metadata.node disabled and hints enabled.",
msg: "Watcher should not be nil.",
},
{
cfg: mapstr.M{
"resource": "pod",
"node": "node-1",
"add_resource_metadata": mapstr.M{
constanca-m marked this conversation as resolved.
Show resolved Hide resolved
"namespace.enabled": true,
"node.enabled": true,
},
"hints.enabled": false,
"builders": []mapstr.M{
{
"mock": mapstr.M{},
},
},
},
expectedNil: false,
name: "add_resource_metadata.namespace and add_resource_metadata.node enabled and hints disabled.",
msg: "Watcher should not be nil.",
},
{
cfg: mapstr.M{
"resource": "pod",
"node": "node-1",
"builders": []mapstr.M{
{
"mock": mapstr.M{},
},
},
},
expectedNil: false,
name: "add_resource_metadata default and hints default.",
msg: "Watcher should not be nil.",
},
}

for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
config := conf.MustNewConfigFrom(&test.cfg)
c := defaultConfig()
err = config.Unpack(&c)
assert.NoError(t, err)

eventer, err := NewPodEventer(uuid, config, client, nil)
if err != nil {
t.Fatal(err)
}

namespaceWatcher := eventer.(*pod).namespaceWatcher
nodeWatcher := eventer.(*pod).nodeWatcher

if test.expectedNil {
assert.Equalf(t, nil, namespaceWatcher, "Namespace "+test.msg)
assert.Equalf(t, nil, nodeWatcher, "Node "+test.msg)
} else {
assert.NotEqualf(t, nil, namespaceWatcher, "Namespace "+test.msg)
assert.NotEqualf(t, nil, nodeWatcher, "Node "+test.msg)
}
})
}
}

type mockUpdaterHandler struct {
objects []interface{}
}
Expand Down
20 changes: 11 additions & 9 deletions libbeat/autodiscover/providers/kubernetes/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -70,17 +70,19 @@ func NewServiceEventer(uuid uuid.UUID, cfg *conf.C, client k8s.Interface, publis
var namespaceMeta metadata.MetaGen
var namespaceWatcher kubernetes.Watcher

metaConf := metadata.GetDefaultResourceMetadataConfig()
namespaceWatcher, err = kubernetes.NewNamedWatcher("namespace", client, &kubernetes.Namespace{}, kubernetes.WatchOptions{
SyncTimeout: config.SyncPeriod,
Namespace: config.Namespace,
}, nil)
if err != nil {
return nil, fmt.Errorf("couldn't create watcher for %T due to error %w", &kubernetes.Namespace{}, err)
metaConf := config.AddResourceMetadata

if metaConf.Namespace.Enabled() || config.Hints.Enabled() {
namespaceWatcher, err = kubernetes.NewNamedWatcher("namespace", client, &kubernetes.Namespace{}, kubernetes.WatchOptions{
SyncTimeout: config.SyncPeriod,
Namespace: config.Namespace,
}, nil)
if err != nil {
return nil, fmt.Errorf("couldn't create watcher for %T due to error %w", &kubernetes.Namespace{}, err)
}
namespaceMeta = metadata.NewNamespaceMetadataGenerator(metaConf.Namespace, namespaceWatcher.Store(), client)
}

namespaceMeta = metadata.NewNamespaceMetadataGenerator(metaConf.Namespace, namespaceWatcher.Store(), client)

p := &service{
config: config,
uuid: uuid,
Expand Down
98 changes: 98 additions & 0 deletions libbeat/autodiscover/providers/kubernetes/service_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -432,6 +432,104 @@ func TestEmitEvent_Service(t *testing.T) {
}
}

func TestServiceEventer_NamespaceWatcher(t *testing.T) {
client := k8sfake.NewSimpleClientset()
uuid, err := uuid.NewV4()
if err != nil {
t.Fatal(err)
}

tests := []struct {
cfg mapstr.M
expectedNil bool
name string
msg string
}{
{
cfg: mapstr.M{
"resource": "service",
"node": "node-1",
"add_resource_metadata": mapstr.M{
"namespace.enabled": false,
},
"hints.enabled": false,
"builders": []mapstr.M{
{
"mock": mapstr.M{},
},
},
},
expectedNil: true,
name: "add_resource_metadata.namespace disabled and hints disabled.",
msg: "Namespace watcher should be nil.",
},
{
cfg: mapstr.M{
"resource": "service",
"node": "node-1",
"add_resource_metadata": mapstr.M{
"namespace.enabled": false,
},
"hints.enabled": true,
},
expectedNil: false,
name: "add_resource_metadata.namespace disabled and hints enabled.",
msg: "Namespace watcher should not be nil.",
},
{
cfg: mapstr.M{
"resource": "service",
"node": "node-1",
"add_resource_metadata": mapstr.M{
"namespace.enabled": true,
},
"hints.enabled": false,
"builders": []mapstr.M{
{
"mock": mapstr.M{},
},
},
},
expectedNil: false,
name: "add_resource_metadata.namespace enabled and hints disabled.",
msg: "Namespace watcher should not be nil.",
},
{
cfg: mapstr.M{
"resource": "pod",
"node": "node-1",
"builders": []mapstr.M{
{
"mock": mapstr.M{},
},
},
},
expectedNil: false,
name: "add_resource_metadata default and hints default.",
msg: "Watcher should not be nil.",
},
}

for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
config := conf.MustNewConfigFrom(&test.cfg)

eventer, err := NewServiceEventer(uuid, config, client, nil)
if err != nil {
t.Fatal(err)
}

namespaceWatcher := eventer.(*service).namespaceWatcher

if test.expectedNil {
assert.Equalf(t, nil, namespaceWatcher, test.msg)
} else {
assert.NotEqualf(t, nil, namespaceWatcher, test.msg)
}
})
}
}

func NewMockServiceEventerManager(svc *service) EventManager {
em := &eventerManager{}
em.eventer = svc
Expand Down
57 changes: 48 additions & 9 deletions metricbeat/module/kubernetes/util/kubernetes.go
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,14 @@
func getExtraWatchers(resourceName string, addResourceMetadata *metadata.AddResourceMetadataConfig) []string {
switch resourceName {
case PodResource:
extra := []string{NamespaceResource, NodeResource}
extra := []string{}
if addResourceMetadata.Node.Enabled() {
extra = append(extra, NodeResource)
}
if addResourceMetadata.Namespace.Enabled() {
Copy link
Contributor

@gizas gizas Apr 16, 2024

Choose a reason for hiding this comment

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

This made think of the scenario:
a) kubernetes.provider has add_resource.metadata.namespace.enabled: false and the module add_resource.metadata.namespace.enabled: true --> This means that the watcher will start. Always the module config is more specific correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I believe the flow is provider > module > metricset.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you are talking about kubernetes module, it does not use the kubernetes provider. The kubernetes autodiscover provider can be used to start different modules (like nginx based on hints or templates) and also is used in log collection.

extra = append(extra, NamespaceResource)
}

// We need to create watchers for ReplicaSets and Jobs that it might belong to,
// in order to be able to retrieve 2nd layer Owner metadata like in case of:
// Deployment -> Replicaset -> Pod
Expand All @@ -179,23 +186,55 @@
}
return extra
case ServiceResource:
return []string{NamespaceResource}
extra := []string{}
if addResourceMetadata.Namespace.Enabled() {
extra = append(extra, NamespaceResource)
}
return extra
case DeploymentResource:
return []string{NamespaceResource}
extra := []string{}
if addResourceMetadata.Namespace.Enabled() {
extra = append(extra, NamespaceResource)
}
return extra
case ReplicaSetResource:
return []string{NamespaceResource}
extra := []string{}
if addResourceMetadata.Namespace.Enabled() {
extra = append(extra, NamespaceResource)
}
return extra
case StatefulSetResource:
return []string{NamespaceResource}
extra := []string{}
if addResourceMetadata.Namespace.Enabled() {
extra = append(extra, NamespaceResource)
}
return extra
case DaemonSetResource:
return []string{NamespaceResource}
extra := []string{}
if addResourceMetadata.Namespace.Enabled() {
extra = append(extra, NamespaceResource)
}
return extra
case JobResource:
return []string{NamespaceResource}
extra := []string{}
if addResourceMetadata.Namespace.Enabled() {
extra = append(extra, NamespaceResource)
}
return extra
case CronJobResource:
return []string{NamespaceResource}
extra := []string{}
if addResourceMetadata.Namespace.Enabled() {
extra = append(extra, NamespaceResource)
}
return extra
case PersistentVolumeResource:
return []string{}
case PersistentVolumeClaimResource:
return []string{NamespaceResource}
extra := []string{}
if addResourceMetadata.Namespace.Enabled() {
extra = append(extra, NamespaceResource)
}
return extra
case StorageClassResource:
return []string{}
case NodeResource:
Expand Down Expand Up @@ -551,8 +590,8 @@
return &nilEnricher{}
}

var specificMetaGen metadata.MetaGen

Check failure on line 593 in metricbeat/module/kubernetes/util/kubernetes.go

View workflow job for this annotation

GitHub Actions / lint (darwin)

specificMetaGen declared and not used (typecheck)
var generalMetaGen *metadata.Resource

Check failure on line 594 in metricbeat/module/kubernetes/util/kubernetes.go

View workflow job for this annotation

GitHub Actions / lint (darwin)

generalMetaGen declared and not used (typecheck)
// Create the metadata generator to be used in the watcher's event handler.
// Both specificMetaGen and generalMetaGen implement Generate method for metadata collection.
if resourceName == ServiceResource || resourceName == PodResource {
Expand Down
Loading
Loading