From e5abbda3a025c556564b461c3da557744736ec64 Mon Sep 17 00:00:00 2001 From: chrismark Date: Fri, 16 Oct 2020 10:16:16 +0300 Subject: [PATCH 1/8] Protect keystore map with mutex Signed-off-by: chrismark --- .../kubernetes/k8skeystore/kubernetes_keystore.go | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/libbeat/common/kubernetes/k8skeystore/kubernetes_keystore.go b/libbeat/common/kubernetes/k8skeystore/kubernetes_keystore.go index 616525b432a..c12bfba577a 100644 --- a/libbeat/common/kubernetes/k8skeystore/kubernetes_keystore.go +++ b/libbeat/common/kubernetes/k8skeystore/kubernetes_keystore.go @@ -20,6 +20,7 @@ package k8skeystore import ( "context" "strings" + "sync" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" k8s "k8s.io/client-go/kubernetes" @@ -38,6 +39,7 @@ type KubernetesKeystoresRegistry struct { kubernetesKeystores KubernetesKeystores logger *logp.Logger client k8s.Interface + keystoreMapLock sync.RWMutex } // KubernetesSecretsKeystore allows to retrieve passwords from Kubernetes secrets for a given namespace @@ -59,6 +61,7 @@ func NewKubernetesKeystoresRegistry(logger *logp.Logger, client k8s.Interface) k kubernetesKeystores: KubernetesKeystores{}, logger: logger, client: client, + keystoreMapLock: sync.RWMutex{}, } } @@ -76,11 +79,16 @@ func (kr *KubernetesKeystoresRegistry) GetKeystore(event bus.Event) keystore.Key } if namespace != "" { // either retrieve already stored keystore or create a new one for the namespace + kr.keystoreMapLock.RLock() if storedKeystore, ok := kr.kubernetesKeystores[namespace]; ok { + kr.keystoreMapLock.RUnlock() return storedKeystore } + kr.keystoreMapLock.RUnlock() k8sKeystore, _ := Factoryk8s(namespace, kr.client, kr.logger) - kr.kubernetesKeystores["namespace"] = k8sKeystore + kr.keystoreMapLock.Lock() + kr.kubernetesKeystores[namespace] = k8sKeystore + kr.keystoreMapLock.Unlock() return k8sKeystore } kr.logger.Debugf("Cannot retrieve kubernetes namespace from event: %s", event) From ef269ab26bc9c5025dbba04b1cef0e218d0fa45a Mon Sep 17 00:00:00 2001 From: chrismark Date: Fri, 16 Oct 2020 10:54:21 +0300 Subject: [PATCH 2/8] Add changelog Signed-off-by: chrismark --- CHANGELOG.next.asciidoc | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.next.asciidoc b/CHANGELOG.next.asciidoc index 37a3366318f..c443966cb05 100644 --- a/CHANGELOG.next.asciidoc +++ b/CHANGELOG.next.asciidoc @@ -373,6 +373,7 @@ https://github.com/elastic/beats/compare/v7.0.0-alpha2...master[Check the HEAD d - Fix retrieving resources by ID for the azure module. {pull}21711[21711] {issue}21707[21707] - Use timestamp from CloudWatch API when creating events. {pull}21498[21498] - Report the correct windows events for system/filesystem {pull}21758[21758] +- Protect kubernetes keystore map with mutex {pull}21880[21880] *Packetbeat* From 21d61b17c60445d1b29e1a6da47450be99663f42 Mon Sep 17 00:00:00 2001 From: chrismark Date: Fri, 16 Oct 2020 13:54:05 +0300 Subject: [PATCH 3/8] lock once on read/write level Signed-off-by: chrismark --- .../common/kubernetes/k8skeystore/kubernetes_keystore.go | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/libbeat/common/kubernetes/k8skeystore/kubernetes_keystore.go b/libbeat/common/kubernetes/k8skeystore/kubernetes_keystore.go index c12bfba577a..1b96923c9c3 100644 --- a/libbeat/common/kubernetes/k8skeystore/kubernetes_keystore.go +++ b/libbeat/common/kubernetes/k8skeystore/kubernetes_keystore.go @@ -79,16 +79,13 @@ func (kr *KubernetesKeystoresRegistry) GetKeystore(event bus.Event) keystore.Key } if namespace != "" { // either retrieve already stored keystore or create a new one for the namespace - kr.keystoreMapLock.RLock() + kr.keystoreMapLock.Lock() + defer kr.keystoreMapLock.Unlock() if storedKeystore, ok := kr.kubernetesKeystores[namespace]; ok { - kr.keystoreMapLock.RUnlock() return storedKeystore } - kr.keystoreMapLock.RUnlock() k8sKeystore, _ := Factoryk8s(namespace, kr.client, kr.logger) - kr.keystoreMapLock.Lock() kr.kubernetesKeystores[namespace] = k8sKeystore - kr.keystoreMapLock.Unlock() return k8sKeystore } kr.logger.Debugf("Cannot retrieve kubernetes namespace from event: %s", event) From c811fa8ee4034dbec2ba4071864e805575bfd830 Mon Sep 17 00:00:00 2001 From: chrismark Date: Fri, 16 Oct 2020 17:27:05 +0300 Subject: [PATCH 4/8] Make use of LoadOrStore Signed-off-by: chrismark --- .../k8skeystore/kubernetes_keystore.go | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/libbeat/common/kubernetes/k8skeystore/kubernetes_keystore.go b/libbeat/common/kubernetes/k8skeystore/kubernetes_keystore.go index 1b96923c9c3..5d6f3ac8f4e 100644 --- a/libbeat/common/kubernetes/k8skeystore/kubernetes_keystore.go +++ b/libbeat/common/kubernetes/k8skeystore/kubernetes_keystore.go @@ -31,15 +31,15 @@ import ( "github.com/elastic/beats/v7/libbeat/logp" ) -type KubernetesKeystores map[string]keystore.Keystore +//type KubernetesKeystores map[string]keystore.Keystore +type KubernetesKeystores sync.Map // KubernetesKeystoresRegistry holds KubernetesKeystores for known namespaces. Once a Keystore for one k8s namespace // is initialized it will be reused every time it is needed. type KubernetesKeystoresRegistry struct { - kubernetesKeystores KubernetesKeystores + kubernetesKeystores sync.Map logger *logp.Logger client k8s.Interface - keystoreMapLock sync.RWMutex } // KubernetesSecretsKeystore allows to retrieve passwords from Kubernetes secrets for a given namespace @@ -58,10 +58,9 @@ func Factoryk8s(keystoreNamespace string, ks8client k8s.Interface, logger *logp. // NewKubernetesKeystoresRegistry initializes a KubernetesKeystoresRegistry func NewKubernetesKeystoresRegistry(logger *logp.Logger, client k8s.Interface) keystore.Provider { return &KubernetesKeystoresRegistry{ - kubernetesKeystores: KubernetesKeystores{}, + kubernetesKeystores: sync.Map{}, logger: logger, client: client, - keystoreMapLock: sync.RWMutex{}, } } @@ -79,14 +78,9 @@ func (kr *KubernetesKeystoresRegistry) GetKeystore(event bus.Event) keystore.Key } if namespace != "" { // either retrieve already stored keystore or create a new one for the namespace - kr.keystoreMapLock.Lock() - defer kr.keystoreMapLock.Unlock() - if storedKeystore, ok := kr.kubernetesKeystores[namespace]; ok { - return storedKeystore - } k8sKeystore, _ := Factoryk8s(namespace, kr.client, kr.logger) - kr.kubernetesKeystores[namespace] = k8sKeystore - return k8sKeystore + storedKeystore, _ := kr.kubernetesKeystores.LoadOrStore(namespace, k8sKeystore) + return storedKeystore.(keystore.Keystore) } kr.logger.Debugf("Cannot retrieve kubernetes namespace from event: %s", event) return nil From f0a8cd04ce90291cbc00dd58284ef74136825efa Mon Sep 17 00:00:00 2001 From: chrismark Date: Mon, 19 Oct 2020 10:17:05 +0300 Subject: [PATCH 5/8] review comments Signed-off-by: chrismark --- .../k8skeystore/kubernetes_keystore.go | 20 ++++++------------- 1 file changed, 6 insertions(+), 14 deletions(-) diff --git a/libbeat/common/kubernetes/k8skeystore/kubernetes_keystore.go b/libbeat/common/kubernetes/k8skeystore/kubernetes_keystore.go index 5d6f3ac8f4e..e5309060b43 100644 --- a/libbeat/common/kubernetes/k8skeystore/kubernetes_keystore.go +++ b/libbeat/common/kubernetes/k8skeystore/kubernetes_keystore.go @@ -20,7 +20,6 @@ package k8skeystore import ( "context" "strings" - "sync" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" k8s "k8s.io/client-go/kubernetes" @@ -31,15 +30,10 @@ import ( "github.com/elastic/beats/v7/libbeat/logp" ) -//type KubernetesKeystores map[string]keystore.Keystore -type KubernetesKeystores sync.Map - -// KubernetesKeystoresRegistry holds KubernetesKeystores for known namespaces. Once a Keystore for one k8s namespace -// is initialized it will be reused every time it is needed. +// KubernetesKeystoresRegistry implements a Provider for Keystore. type KubernetesKeystoresRegistry struct { - kubernetesKeystores sync.Map - logger *logp.Logger - client k8s.Interface + logger *logp.Logger + client k8s.Interface } // KubernetesSecretsKeystore allows to retrieve passwords from Kubernetes secrets for a given namespace @@ -58,9 +52,8 @@ func Factoryk8s(keystoreNamespace string, ks8client k8s.Interface, logger *logp. // NewKubernetesKeystoresRegistry initializes a KubernetesKeystoresRegistry func NewKubernetesKeystoresRegistry(logger *logp.Logger, client k8s.Interface) keystore.Provider { return &KubernetesKeystoresRegistry{ - kubernetesKeystores: sync.Map{}, - logger: logger, - client: client, + logger: logger, + client: client, } } @@ -79,8 +72,7 @@ func (kr *KubernetesKeystoresRegistry) GetKeystore(event bus.Event) keystore.Key if namespace != "" { // either retrieve already stored keystore or create a new one for the namespace k8sKeystore, _ := Factoryk8s(namespace, kr.client, kr.logger) - storedKeystore, _ := kr.kubernetesKeystores.LoadOrStore(namespace, k8sKeystore) - return storedKeystore.(keystore.Keystore) + return k8sKeystore } kr.logger.Debugf("Cannot retrieve kubernetes namespace from event: %s", event) return nil From 8af93be23f2e5801e4036ea8398398921b511c29 Mon Sep 17 00:00:00 2001 From: chrismark Date: Mon, 19 Oct 2020 15:13:03 +0300 Subject: [PATCH 6/8] fix changelog Signed-off-by: chrismark --- CHANGELOG.next.asciidoc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.next.asciidoc b/CHANGELOG.next.asciidoc index c443966cb05..ee489f90eef 100644 --- a/CHANGELOG.next.asciidoc +++ b/CHANGELOG.next.asciidoc @@ -373,7 +373,7 @@ https://github.com/elastic/beats/compare/v7.0.0-alpha2...master[Check the HEAD d - Fix retrieving resources by ID for the azure module. {pull}21711[21711] {issue}21707[21707] - Use timestamp from CloudWatch API when creating events. {pull}21498[21498] - Report the correct windows events for system/filesystem {pull}21758[21758] -- Protect kubernetes keystore map with mutex {pull}21880[21880] +- Stop storing stateless kubernetes keystores {pull}21880[21880] *Packetbeat* From 3a03732f03e4119529d37fc3528778446b7c14ec Mon Sep 17 00:00:00 2001 From: chrismark Date: Mon, 19 Oct 2020 15:18:35 +0300 Subject: [PATCH 7/8] fix leftover comment Signed-off-by: chrismark --- libbeat/common/kubernetes/k8skeystore/kubernetes_keystore.go | 1 - 1 file changed, 1 deletion(-) diff --git a/libbeat/common/kubernetes/k8skeystore/kubernetes_keystore.go b/libbeat/common/kubernetes/k8skeystore/kubernetes_keystore.go index e5309060b43..e17b4258232 100644 --- a/libbeat/common/kubernetes/k8skeystore/kubernetes_keystore.go +++ b/libbeat/common/kubernetes/k8skeystore/kubernetes_keystore.go @@ -70,7 +70,6 @@ func (kr *KubernetesKeystoresRegistry) GetKeystore(event bus.Event) keystore.Key namespace = ns.(string) } if namespace != "" { - // either retrieve already stored keystore or create a new one for the namespace k8sKeystore, _ := Factoryk8s(namespace, kr.client, kr.logger) return k8sKeystore } From 69a42c66dce4764dd1b5172802e54109c0ec191c Mon Sep 17 00:00:00 2001 From: Chris Mark Date: Mon, 19 Oct 2020 16:30:23 +0300 Subject: [PATCH 8/8] Update CHANGELOG.next.asciidoc Co-authored-by: Jaime Soriano Pastor --- CHANGELOG.next.asciidoc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.next.asciidoc b/CHANGELOG.next.asciidoc index ee489f90eef..90d4df554e3 100644 --- a/CHANGELOG.next.asciidoc +++ b/CHANGELOG.next.asciidoc @@ -373,7 +373,7 @@ https://github.com/elastic/beats/compare/v7.0.0-alpha2...master[Check the HEAD d - Fix retrieving resources by ID for the azure module. {pull}21711[21711] {issue}21707[21707] - Use timestamp from CloudWatch API when creating events. {pull}21498[21498] - Report the correct windows events for system/filesystem {pull}21758[21758] -- Stop storing stateless kubernetes keystores {pull}21880[21880] +- Fix panic in kubernetes autodiscover related to keystores {issue}21843[21843] {pull}21880[21880] *Packetbeat*