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

Make sure to read association configuration again from annotations if it was cleared #5489

Merged
merged 14 commits into from
Mar 24, 2022
20 changes: 6 additions & 14 deletions pkg/apis/agent/v1alpha1/agent_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -339,16 +339,8 @@ func (aea *AgentESAssociation) AssociationConfAnnotationName() string {
return commonv1.ElasticsearchConfigAnnotationName(aea.ref)
}

func (aea *AgentESAssociation) AssociationConf() *commonv1.AssociationConf {
if aea.esAssocConfs == nil {
return nil
}
assocConf, found := aea.esAssocConfs[aea.ref]
if !found {
return nil
}

return &assocConf
func (aea *AgentESAssociation) AssociationConf() (*commonv1.AssociationConf, error) {
return commonv1.GetAndSetAssociationConfByRef(aea, aea.ref, aea.esAssocConfs)
}

func (aea *AgentESAssociation) SetAssociationConf(conf *commonv1.AssociationConf) {
Expand All @@ -370,8 +362,8 @@ func (a *AgentKibanaAssociation) ElasticServiceAccount() (commonv1.ServiceAccoun
return "", nil
}

func (a *AgentKibanaAssociation) AssociationConf() *commonv1.AssociationConf {
return a.kbAssocConf
func (a *AgentKibanaAssociation) AssociationConf() (*commonv1.AssociationConf, error) {
return commonv1.GetAndSetAssociationConf(a, a.kbAssocConf)
}

func (a *AgentKibanaAssociation) SetAssociationConf(conf *commonv1.AssociationConf) {
Expand Down Expand Up @@ -414,8 +406,8 @@ func (a *AgentFleetServerAssociation) ElasticServiceAccount() (commonv1.ServiceA
return "", nil
}

func (a *AgentFleetServerAssociation) AssociationConf() *commonv1.AssociationConf {
return a.fsAssocConf
func (a *AgentFleetServerAssociation) AssociationConf() (*commonv1.AssociationConf, error) {
return commonv1.GetAndSetAssociationConf(a, a.fsAssocConf)
}

func (a *AgentFleetServerAssociation) SetAssociationConf(conf *commonv1.AssociationConf) {
Expand Down
8 changes: 4 additions & 4 deletions pkg/apis/apm/v1/apmserver_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -219,8 +219,8 @@ func (aes *ApmEsAssociation) AssociationRef() commonv1.ObjectSelector {
return aes.Spec.ElasticsearchRef.WithDefaultNamespace(aes.Namespace)
}

func (aes *ApmEsAssociation) AssociationConf() *commonv1.AssociationConf {
return aes.esAssocConf
func (aes *ApmEsAssociation) AssociationConf() (*commonv1.AssociationConf, error) {
return commonv1.GetAndSetAssociationConf(aes, aes.esAssocConf)
}

func (aes *ApmEsAssociation) SetAssociationConf(assocConf *commonv1.AssociationConf) {
Expand Down Expand Up @@ -268,8 +268,8 @@ func (akb *ApmKibanaAssociation) RequiresAssociation() bool {
return akb.Spec.KibanaRef.Name != ""
}

func (akb *ApmKibanaAssociation) AssociationConf() *commonv1.AssociationConf {
return akb.kibanaAssocConf
func (akb *ApmKibanaAssociation) AssociationConf() (*commonv1.AssociationConf, error) {
return commonv1.GetAndSetAssociationConf(akb, akb.kibanaAssocConf)
}

func (akb *ApmKibanaAssociation) SetAssociationConf(assocConf *commonv1.AssociationConf) {
Expand Down
8 changes: 4 additions & 4 deletions pkg/apis/beat/v1beta1/beat_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -253,8 +253,8 @@ func (b *BeatESAssociation) AssociationConfAnnotationName() string {
return commonv1.ElasticsearchConfigAnnotationNameBase
}

func (b *BeatESAssociation) AssociationConf() *commonv1.AssociationConf {
return b.esAssocConf
func (b *BeatESAssociation) AssociationConf() (*commonv1.AssociationConf, error) {
return commonv1.GetAndSetAssociationConf(b, b.esAssocConf)
}

func (b *BeatESAssociation) SetAssociationConf(conf *commonv1.AssociationConf) {
Expand All @@ -271,8 +271,8 @@ type BeatKibanaAssociation struct {

var _ commonv1.Association = &BeatKibanaAssociation{}

func (b *BeatKibanaAssociation) AssociationConf() *commonv1.AssociationConf {
return b.kbAssocConf
func (b *BeatKibanaAssociation) AssociationConf() (*commonv1.AssociationConf, error) {
return commonv1.GetAndSetAssociationConf(b, b.kbAssocConf)
}

func (b *BeatKibanaAssociation) SetAssociationConf(conf *commonv1.AssociationConf) {
Expand Down
2 changes: 1 addition & 1 deletion pkg/apis/common/v1/association.go
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ type Association interface {
AssociationConfAnnotationName() string

// AssociationConf is the configuration of the Association allowing to connect to the Association resource.
AssociationConf() *AssociationConf
AssociationConf() (*AssociationConf, error)
SetAssociationConf(*AssociationConf)

// AssociationID uniquely identifies this Association among all Associations of the same type belonging to Associated()
Expand Down
77 changes: 77 additions & 0 deletions pkg/apis/common/v1/conf.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
// Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
// or more contributor license agreements. Licensed under the Elastic License 2.0;
// you may not use this file except in compliance with the Elastic License 2.0.

package v1

import (
"encoding/json"
"reflect"
"unsafe"

"github.com/pkg/errors"
"k8s.io/apimachinery/pkg/types"
)

// GetAndSetAssociationConf returns the association configuration if it is not nil, else the association configured is
// read from the annotation and put back in the given association.
func GetAndSetAssociationConf(assoc Association, assocConf *AssociationConf) (*AssociationConf, error) {
if assocConf == nil {
return setAssocConfFromAnnotation(assoc)
}
return assocConf, nil
}

// GetAndSetAssociationConfByRef returns the association configuration corresponding to the namespace name of the
thbkrkr marked this conversation as resolved.
Show resolved Hide resolved
// referenced resource if it is found in the given map of association configurations.
// Because the map of association configurations is not persisted and can be cleared by an update of the parent resource
// (see https://github.com/elastic/cloud-on-k8s/issues/4709#issuecomment-1042898108). If we detect that the map is empty,
// we try to populate it again from the annotation.
thbkrkr marked this conversation as resolved.
Show resolved Hide resolved
func GetAndSetAssociationConfByRef(assoc Association, ref types.NamespacedName, assocConfs map[types.NamespacedName]AssociationConf) (*AssociationConf, error) {
if len(assocConfs) == 0 {
return setAssocConfFromAnnotation(assoc)
}
thbkrkr marked this conversation as resolved.
Show resolved Hide resolved
assocConf, found := assocConfs[ref]
if !found {
return setAssocConfFromAnnotation(assoc)
}
return &assocConf, nil
}

// setAssocConfFromAnnotation sets the association configuration extracted from the annotations in the given association.
func setAssocConfFromAnnotation(assoc Association) (*AssociationConf, error) {
assocConf, err := extractAssocConfFromAnnotation(assoc.Associated().GetAnnotations(), assoc.AssociationConfAnnotationName())
if err != nil {
return nil, err
}
assoc.SetAssociationConf(assocConf)
return assocConf, nil
}

// extractAssocConfFromAnnotation extracts the association configuration from annotations and an annotation name.
func extractAssocConfFromAnnotation(annotations map[string]string, annotationName string) (*AssociationConf, error) {
if len(annotations) == 0 {
thbkrkr marked this conversation as resolved.
Show resolved Hide resolved
return nil, nil
}

var assocConf AssociationConf
serializedConf, exists := annotations[annotationName]
if !exists || serializedConf == "" {
return nil, nil
}

if err := json.Unmarshal(unsafeStringToBytes(serializedConf), &assocConf); err != nil {
return nil, errors.Wrapf(err, "failed to extract association configuration")
}

return &assocConf, nil
}

func unsafeStringToBytes(s string) []byte {
hdr := *(*reflect.StringHeader)(unsafe.Pointer(&s)) //nolint:govet
return *(*[]byte)(unsafe.Pointer(&reflect.SliceHeader{ //nolint:govet
Data: hdr.Data,
Len: hdr.Len,
Cap: hdr.Len,
}))
}
12 changes: 2 additions & 10 deletions pkg/apis/elasticsearch/v1/elasticsearch_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -546,16 +546,8 @@ func (ema *EsMonitoringAssociation) AssociationRef() commonv1.ObjectSelector {
}
}

func (ema *EsMonitoringAssociation) AssociationConf() *commonv1.AssociationConf {
if ema.AssocConfs == nil {
return nil
}
assocConf, found := ema.AssocConfs[ema.ref]
if !found {
return nil
}

return &assocConf
func (ema *EsMonitoringAssociation) AssociationConf() (*commonv1.AssociationConf, error) {
return commonv1.GetAndSetAssociationConfByRef(ema, ema.ref, ema.AssocConfs)
}

func (ema *EsMonitoringAssociation) SetAssociationConf(assocConf *commonv1.AssociationConf) {
Expand Down
82 changes: 82 additions & 0 deletions pkg/apis/elasticsearch/v1/elasticsearch_types_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import (
"github.com/stretchr/testify/require"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

commonv1 "github.com/elastic/cloud-on-k8s/pkg/apis/common/v1"
"github.com/elastic/cloud-on-k8s/pkg/utils/pointer"
"github.com/elastic/cloud-on-k8s/pkg/utils/set"
)
Expand Down Expand Up @@ -314,3 +315,84 @@ func TestElasticsearch_DisabledPredicates(t *testing.T) {
})
}
}

// Test_AssociationConfs tests that if the association configuration map in an associated object is cleared, then
// AssociationConf() is rebuilt from the annotation.
func Test_AssociationConfs(t *testing.T) {
// simple es without associations
es := &Elasticsearch{
ObjectMeta: metav1.ObjectMeta{
Name: "es",
Namespace: "default",
},
}
assert.Equal(t, 0, len(es.GetAssociations()))
assert.Equal(t, 0, len(es.AssocConfs))

// es with associations
metricsEsRef := commonv1.ObjectSelector{
Name: "metrics",
Namespace: "default",
}
esMon := &Elasticsearch{
ObjectMeta: metav1.ObjectMeta{
Name: "esmon",
Namespace: "default",
Annotations: map[string]string{
"association.k8s.elastic.co/es-conf-864518565": `{"authSecretName":"es-default-metrics-beat-es-mon-user","authSecretKey":"default-es-default-esmon-beat-es-mon-user","caCertProvided":true,"caSecretName":"es-es-monitoring-default-metrics-ca","url":"https://metrics-es-http.default.svc:9200","version":"8.0.0"}`,
"association.k8s.elastic.co/es-conf-1654136115": `{"authSecretName":"es-default-logs-beat-es-mon-user","authSecretKey":"default-es-default-esmon-beat-es-mon-user","caCertProvided":true,"caSecretName":"es-es-monitoring-default-logs-ca","url":"https://logs-es-http.default.svc:9200","version":"8.0.0"}`,
},
},
Spec: ElasticsearchSpec{
Monitoring: Monitoring{
Metrics: MetricsMonitoring{
ElasticsearchRefs: []commonv1.ObjectSelector{metricsEsRef},
},
Logs: LogsMonitoring{
ElasticsearchRefs: []commonv1.ObjectSelector{{
Name: "logs",
Namespace: "default"},
},
},
},
},
}
assert.Equal(t, 2, len(esMon.GetAssociations()))

// map should be initially empty
assert.Equal(t, 0, len(esMon.AssocConfs))

// get and set assoc conf
for _, assoc := range esMon.GetAssociations() {
assocConf, err := assoc.AssociationConf()
assert.NotNil(t, assocConf)
assert.NoError(t, err)
}
// map should have been populated by the call to AssociationConf()
assert.Equal(t, 2, len(esMon.AssocConfs))

// simulate the case where the assocConfs map is reset, which can happen if the resource is updated
Copy link
Contributor

Choose a reason for hiding this comment

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

simulate the case where the assocConfs map is reset, which can happen if the resource is updated

Sorry, I'm not sure to understand. assocConfs is not persisted, I think it is expected for it to be empty before FetchWithAssociations kicks in ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue happens after FetchWithAssociations, when the resource is updated, it resets the map. This test shows that if it is reset after SetAssociationConf(), the map is repopulated on the fly by AssociationConf().
Maybe I should go further and delete SetAssociationConf() to avoid confusion. We could even delete the FetchWithAssociations() and just rely on AssociationConf() called in different places.

Copy link
Contributor

Choose a reason for hiding this comment

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

We could even delete the FetchWithAssociations() and just rely on AssociationConf() called in different places.

Thanks for the clarification ! My understanding is that it means that FetchWithAssociations is unreliable 😕 I wonder if it's really worth keeping it along with the unserialized annotations ? IIUC they are only here to spare a few CPU cycles by not using json.Unmarshal when the association config. is needed ?

esMon.AssocConfs = nil
assert.Equal(t, 0, len(esMon.AssocConfs))

// get and set assoc conf
for _, assoc := range esMon.GetAssociations() {
assocConf, err := assoc.AssociationConf()
assert.NotNil(t, assocConf)
assert.NoError(t, err)
}
// checks that all map entries are set again
assert.Equal(t, 2, len(esMon.AssocConfs))

// delete just one entry in the map
delete(esMon.AssocConfs, metricsEsRef.NamespacedName())
assert.Equal(t, 1, len(esMon.AssocConfs))

// checks that the missing entry is set again
for _, assoc := range esMon.GetAssociations() {
assocConf, err := assoc.AssociationConf()
assert.NotNil(t, assocConf)
assert.NoError(t, err)
}
assert.Equal(t, 2, len(esMon.AssocConfs))
}
4 changes: 2 additions & 2 deletions pkg/apis/enterprisesearch/v1/enterprisesearch_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -99,8 +99,8 @@ func (ent *EnterpriseSearch) AssociationRef() commonv1.ObjectSelector {
return ent.Spec.ElasticsearchRef.WithDefaultNamespace(ent.Namespace)
}

func (ent *EnterpriseSearch) AssociationConf() *commonv1.AssociationConf {
return ent.assocConf
func (ent *EnterpriseSearch) AssociationConf() (*commonv1.AssociationConf, error) {
return commonv1.GetAndSetAssociationConf(ent, ent.assocConf)
}

func (ent *EnterpriseSearch) SetAssociationConf(assocConf *commonv1.AssociationConf) {
Expand Down
4 changes: 2 additions & 2 deletions pkg/apis/enterprisesearch/v1beta1/enterprisesearch_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -95,8 +95,8 @@ func (ent *EnterpriseSearch) AssociationRef() commonv1.ObjectSelector {
return ent.Spec.ElasticsearchRef.WithDefaultNamespace(ent.Namespace)
}

func (ent *EnterpriseSearch) AssociationConf() *commonv1.AssociationConf {
return ent.assocConf
func (ent *EnterpriseSearch) AssociationConf() (*commonv1.AssociationConf, error) {
return commonv1.GetAndSetAssociationConf(ent, ent.assocConf)
}

func (ent *EnterpriseSearch) SetAssociationConf(assocConf *commonv1.AssociationConf) {
Expand Down
19 changes: 6 additions & 13 deletions pkg/apis/kibana/v1/kibana_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -316,8 +316,8 @@ func (kbes *KibanaEsAssociation) AssociationRef() commonv1.ObjectSelector {
return kbes.Spec.ElasticsearchRef.WithDefaultNamespace(kbes.Namespace)
}

func (kbes *KibanaEsAssociation) AssociationConf() *commonv1.AssociationConf {
return kbes.assocConf
func (kbes *KibanaEsAssociation) AssociationConf() (*commonv1.AssociationConf, error) {
return commonv1.GetAndSetAssociationConf(kbes, kbes.assocConf)
}

func (kbes *KibanaEsAssociation) SetAssociationConf(assocConf *commonv1.AssociationConf) {
Expand Down Expand Up @@ -367,8 +367,8 @@ func (kbent *KibanaEntAssociation) AssociationRef() commonv1.ObjectSelector {
return kbent.Spec.EnterpriseSearchRef.WithDefaultNamespace(kbent.Namespace)
}

func (kbent *KibanaEntAssociation) AssociationConf() *commonv1.AssociationConf {
return kbent.entAssocConf
func (kbent *KibanaEntAssociation) AssociationConf() (*commonv1.AssociationConf, error) {
return commonv1.GetAndSetAssociationConf(kbent, kbent.entAssocConf)
}

func (kbent *KibanaEntAssociation) SetAssociationConf(assocConf *commonv1.AssociationConf) {
Expand Down Expand Up @@ -420,15 +420,8 @@ func (kbmon *KbMonitoringAssociation) AssociationRef() commonv1.ObjectSelector {
}
}

func (kbmon *KbMonitoringAssociation) AssociationConf() *commonv1.AssociationConf {
if kbmon.monitoringAssocConfs == nil {
return nil
}
assocConf, found := kbmon.monitoringAssocConfs[kbmon.ref]
if !found {
return nil
}
return &assocConf
func (kbmon *KbMonitoringAssociation) AssociationConf() (*commonv1.AssociationConf, error) {
return commonv1.GetAndSetAssociationConfByRef(kbmon, kbmon.ref, kbmon.monitoringAssocConfs)
}

func (kbmon *KbMonitoringAssociation) SetAssociationConf(assocConf *commonv1.AssociationConf) {
Expand Down
30 changes: 30 additions & 0 deletions pkg/apis/kibana/v1/kibana_types_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,40 @@ package v1
import (
"testing"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

commonv1 "github.com/elastic/cloud-on-k8s/pkg/apis/common/v1"
)

func TestApmEsAssociation_AssociationConfAnnotationName(t *testing.T) {
k := Kibana{}
require.Equal(t, "association.k8s.elastic.co/es-conf", k.EsAssociation().AssociationConfAnnotationName())
}

// Test_AssociationConf tests that AssociationConf reads the conf from the annotation.
func Test_AssociationConf(t *testing.T) {
kb := &Kibana{
ObjectMeta: metav1.ObjectMeta{
Name: "kb",
Namespace: "default",
Annotations: map[string]string{
"association.k8s.elastic.co/es-conf": `{"authSecretName":"es-default-es-beat-es-mon-user","authSecretKey":"default-es-default-esmon-beat-es-mon-user","caCertProvided":true,"caSecretName":"es-es-monitoring-default-metrics-ca","url":"https://metrics-es-http.default.svc:9200","version":"8.0.0"}`,
},
},
Spec: KibanaSpec{
ElasticsearchRef: commonv1.ObjectSelector{
Name: "es",
Namespace: "default"},
},
}

entAssocConf, err := kb.EntAssociation().AssociationConf()
assert.NoError(t, err)
assert.Nil(t, entAssocConf)
esAssocConf, err := kb.EsAssociation().AssociationConf()
assert.NoError(t, err)
assert.NotNil(t, esAssocConf)
assert.Equal(t, "https://metrics-es-http.default.svc:9200", esAssocConf.URL)
}
4 changes: 2 additions & 2 deletions pkg/apis/maps/v1alpha1/maps_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -88,8 +88,8 @@ func (m *ElasticMapsServer) ServiceAccountName() string {
return m.Spec.ServiceAccountName
}

func (m *ElasticMapsServer) AssociationConf() *commonv1.AssociationConf {
return m.assocConf
func (m *ElasticMapsServer) AssociationConf() (*commonv1.AssociationConf, error) {
return commonv1.GetAndSetAssociationConf(m, m.assocConf)
}

func (m *ElasticMapsServer) SetAssociationConf(assocConf *commonv1.AssociationConf) {
Expand Down
Loading