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.

Sign up for GitHub

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
Merged

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
ea974d7
Ensure that assocConf is always read from the annotation
thbkrkr Mar 15, 2022
7860dc4
More factorization
thbkrkr Mar 16, 2022
40d8142
Naming
thbkrkr Mar 16, 2022
f9b0052
go fmt :blowfish:
thbkrkr Mar 16, 2022
660f6e8
rm FetchWithAssociations
thbkrkr Mar 21, 2022
eb8bfb9
Add test case
thbkrkr Mar 21, 2022
0d00a2f
Apply review's input
thbkrkr Mar 22, 2022
0fb9923
Refactoring so that AssociationConf() returns an error
thbkrkr Mar 22, 2022
90352a6
Merge remote-tracking branch 'upstream/main' into fix-assoc-conf
thbkrkr Mar 22, 2022
8c113d8
lint
thbkrkr Mar 22, 2022
e9b9663
Update pkg/controller/apmserver/controller.go
thbkrkr Mar 24, 2022
b6bd14b
Update pkg/apis/common/v1/conf.go
thbkrkr Mar 24, 2022
1d93178
Update pkg/apis/common/v1/conf.go
thbkrkr Mar 24, 2022
9c8ce24
Read values from a nil map is safe
thbkrkr Mar 24, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 4 additions & 11 deletions pkg/apis/agent/v1alpha1/agent_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (
"k8s.io/apimachinery/pkg/types"

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

const (
Expand Down Expand Up @@ -320,15 +321,7 @@ func (aea *AgentESAssociation) AssociationConfAnnotationName() string {
}

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

return &assocConf
return assocutils.GetAndSetAssociationConfByRef(aea, aea.ref, aea.esAssocConfs)
}

func (aea *AgentESAssociation) SetAssociationConf(conf *commonv1.AssociationConf) {
Expand All @@ -347,7 +340,7 @@ type AgentKibanaAssociation struct {
var _ commonv1.Association = &AgentKibanaAssociation{}

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

func (a *AgentKibanaAssociation) SetAssociationConf(conf *commonv1.AssociationConf) {
Expand Down Expand Up @@ -387,7 +380,7 @@ type AgentFleetServerAssociation struct {
var _ commonv1.Association = &AgentFleetServerAssociation{}

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

func (a *AgentFleetServerAssociation) SetAssociationConf(conf *commonv1.AssociationConf) {
Expand Down
5 changes: 3 additions & 2 deletions pkg/apis/beat/v1beta1/beat_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

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

const (
Expand Down Expand Up @@ -250,7 +251,7 @@ func (b *BeatESAssociation) AssociationConfAnnotationName() string {
}

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

func (b *BeatESAssociation) SetAssociationConf(conf *commonv1.AssociationConf) {
Expand All @@ -268,7 +269,7 @@ type BeatKibanaAssociation struct {
var _ commonv1.Association = &BeatKibanaAssociation{}

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

func (b *BeatKibanaAssociation) SetAssociationConf(conf *commonv1.AssociationConf) {
Expand Down
11 changes: 2 additions & 9 deletions pkg/apis/elasticsearch/v1/elasticsearch_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (
"k8s.io/apimachinery/pkg/types"

commonv1 "github.com/elastic/cloud-on-k8s/pkg/apis/common/v1"
"github.com/elastic/cloud-on-k8s/pkg/controller/association/utils"
"github.com/elastic/cloud-on-k8s/pkg/controller/common/hash"
"github.com/elastic/cloud-on-k8s/pkg/utils/pointer"
"github.com/elastic/cloud-on-k8s/pkg/utils/set"
Expand Down Expand Up @@ -543,15 +544,7 @@ 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
return utils.GetAndSetAssociationConfByRef(ema, ema.ref, ema.AssocConfs)
}

func (ema *EsMonitoringAssociation) SetAssociationConf(assocConf *commonv1.AssociationConf) {
Expand Down
75 changes: 75 additions & 0 deletions pkg/apis/elasticsearch/v1/elasticsearch_types_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ 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"
assocutils "github.com/elastic/cloud-on-k8s/pkg/controller/association/utils"
"github.com/elastic/cloud-on-k8s/pkg/utils/pointer"
"github.com/elastic/cloud-on-k8s/pkg/utils/set"
)
Expand Down Expand Up @@ -314,3 +316,76 @@ func TestElasticsearch_DisabledPredicates(t *testing.T) {
})
}
}

// Test_AssociationConfs tests that if something resets the AssocConfs map, then AssociationConf() reinitializes
// the map from the annotation.
thbkrkr marked this conversation as resolved.
Show resolved Hide resolved
func Test_AssociationConfs(t *testing.T) {
// simple es without associations
es := &Elasticsearch{
ObjectMeta: metav1.ObjectMeta{
Name: "es",
Namespace: "default",
},
}

// set assoc conf even if no assoc
assert.Equal(t, 0, len(es.AssocConfs))
for _, association := range es.GetAssociations() {
assocConf, err := assocutils.GetAssociationConf(association)
assert.NoError(t, err)
association.SetAssociationConf(assocConf)
}
thbkrkr marked this conversation as resolved.
Show resolved Hide resolved
assert.Equal(t, 0, len(es.AssocConfs))

// checks that assocConfs is nil
for _, assoc := range es.GetAssociations() {
thbkrkr marked this conversation as resolved.
Show resolved Hide resolved
assert.Nil(t, assoc.AssociationConf())
}

// es with associations
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{{
Name: "metrics",
Namespace: "default"},
},
},
Logs: LogsMonitoring{
ElasticsearchRefs: []commonv1.ObjectSelector{{
Name: "logs",
Namespace: "default"},
},
},
},
},
}

// set assoc conf
assert.Equal(t, 0, len(esMon.AssocConfs))
for _, association := range esMon.GetAssociations() {
assocConf, err := assocutils.GetAssociationConf(association)
assert.NoError(t, err)
association.SetAssociationConf(assocConf)
}
assert.Equal(t, 2, len(esMon.AssocConfs))
thbkrkr marked this conversation as resolved.
Show resolved Hide resolved

// 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

// checks that AssociationConf are not nil when AssociationConf() is called
assert.Equal(t, 0, len(esMon.AssocConfs))
for _, assoc := range esMon.GetAssociations() {
assert.NotNil(t, assoc.AssociationConf())
}
assert.Equal(t, 2, len(esMon.AssocConfs))
}
3 changes: 2 additions & 1 deletion pkg/apis/enterprisesearch/v1/enterprisesearch_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

commonv1 "github.com/elastic/cloud-on-k8s/pkg/apis/common/v1"
assocutils "github.com/elastic/cloud-on-k8s/pkg/controller/association/utils"
common_name "github.com/elastic/cloud-on-k8s/pkg/controller/common/name"
)

Expand Down Expand Up @@ -100,7 +101,7 @@ func (ent *EnterpriseSearch) AssociationRef() commonv1.ObjectSelector {
}

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

func (ent *EnterpriseSearch) SetAssociationConf(assocConf *commonv1.AssociationConf) {
Expand Down
14 changes: 4 additions & 10 deletions pkg/apis/kibana/v1/kibana_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
"k8s.io/apimachinery/pkg/types"

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

const (
Expand Down Expand Up @@ -300,7 +301,7 @@ func (kbes *KibanaEsAssociation) AssociationRef() commonv1.ObjectSelector {
}

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

func (kbes *KibanaEsAssociation) SetAssociationConf(assocConf *commonv1.AssociationConf) {
Expand Down Expand Up @@ -347,7 +348,7 @@ func (kbent *KibanaEntAssociation) AssociationRef() commonv1.ObjectSelector {
}

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

func (kbent *KibanaEntAssociation) SetAssociationConf(assocConf *commonv1.AssociationConf) {
Expand Down Expand Up @@ -396,14 +397,7 @@ 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
return assocutils.GetAndSetAssociationConfByRef(kbmon, kbmon.ref, kbmon.monitoringAssocConfs)
}

func (kbmon *KbMonitoringAssociation) SetAssociationConf(assocConf *commonv1.AssociationConf) {
Expand Down
26 changes: 26 additions & 0 deletions pkg/apis/kibana/v1/kibana_types_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,36 @@ 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"},
},
}

assert.Nil(t, kb.EntAssociation().AssociationConf())
assert.NotNil(t, kb.EsAssociation().AssociationConf())
assert.Equal(t, "https://metrics-es-http.default.svc:9200", kb.EsAssociation().AssociationConf().URL)
}
3 changes: 2 additions & 1 deletion pkg/apis/maps/v1alpha1/maps_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

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

const (
Expand Down Expand Up @@ -89,7 +90,7 @@ func (m *ElasticMapsServer) ServiceAccountName() string {
}

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

func (m *ElasticMapsServer) SetAssociationConf(assocConf *commonv1.AssociationConf) {
Expand Down
45 changes: 2 additions & 43 deletions pkg/controller/association/conf.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import (
"context"
"encoding/json"
"fmt"
"reflect"
"strings"
"unsafe"

Expand All @@ -22,6 +21,7 @@ import (
"sigs.k8s.io/controller-runtime/pkg/reconcile"

commonv1 "github.com/elastic/cloud-on-k8s/pkg/apis/common/v1"
assocutils "github.com/elastic/cloud-on-k8s/pkg/controller/association/utils"
"github.com/elastic/cloud-on-k8s/pkg/controller/common/events"
"github.com/elastic/cloud-on-k8s/pkg/controller/common/tracing"
"github.com/elastic/cloud-on-k8s/pkg/controller/common/version"
Expand All @@ -43,7 +43,7 @@ func FetchWithAssociations(
}

for _, association := range associated.GetAssociations() {
assocConf, err := GetAssociationConf(association)
assocConf, err := assocutils.GetAssociationConf(association)
if err != nil {
return err
}
Expand Down Expand Up @@ -145,17 +145,6 @@ func AllowVersion(resourceVersion version.Version, associated commonv1.Associate
return true
}

// GetAssociationConf extracts the association configuration from the given object by reading the annotations.
func GetAssociationConf(association commonv1.Association) (*commonv1.AssociationConf, error) {
accessor := meta.NewAccessor()
annotations, err := accessor.Annotations(association)
if err != nil {
return nil, err
}

return extractAssociationConf(annotations, association.AssociationConfAnnotationName())
}

// SingleAssociationOfType returns single association from the provided slice that matches provided type. Returns
// nil if such association can't be found. Returns an error if more than one association matches the type.
func SingleAssociationOfType(
Expand All @@ -176,24 +165,6 @@ func SingleAssociationOfType(
return result, nil
}

func extractAssociationConf(annotations map[string]string, annotationName string) (*commonv1.AssociationConf, error) {
if len(annotations) == 0 {
return nil, nil
}

var assocConf commonv1.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
}

// RemoveObsoleteAssociationConfs removes all no longer needed annotations on `associated` matching
// `associationConfAnnotationNameBase` prefix.
func RemoveObsoleteAssociationConfs(
Expand Down Expand Up @@ -291,18 +262,6 @@ func UpdateAssociationConf(
return client.Update(context.Background(), obj)
}

// unsafeStringToBytes converts a string to a byte array without making extra allocations.
// since we read potentially large strings from annotations on every reconcile loop, this should help
// reduce the amount of garbage created.
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,
}))
}

// unsafeBytesToString converts a byte array to string without making extra allocations.
func unsafeBytesToString(b []byte) string {
return *(*string)(unsafe.Pointer(&b))
Expand Down
Loading