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][Aerospike Module] Add support for TLS #38126

Merged
merged 23 commits into from
Apr 13, 2024
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
b7221a6
Add TLS Support for Metricbeat module aerospike
herrBez Feb 22, 2024
7ba6b99
Module aerospike: add support for basic auth
herrBez Feb 23, 2024
d89729b
Move the initialization of the clientpolicy from the metricset to the…
herrBez Feb 23, 2024
b3f3677
Aerospike: add test to check the initialization of ssl and user/passw…
herrBez Feb 23, 2024
972a183
Aerospike: Add support for cluster_name setting
herrBez Feb 23, 2024
12d0878
Aerospike: Add test for ClusterName
herrBez Feb 23, 2024
59df82f
Aerospike module: add commented username, password, cluster_name and …
herrBez Feb 23, 2024
8965a52
Aerospike Module: Fix unit test
herrBez Feb 23, 2024
baec16b
Fix test to avoid using hard-coded value
herrBez Feb 28, 2024
0e7291c
Adjust configuration of aerospike to show username/password and TLS s…
herrBez Mar 7, 2024
4517d20
Executed goimports -w -local github.com/elastic metricbeat/module/aer…
herrBez Mar 7, 2024
d5c1279
Improve Test error messages to provide more context
herrBez Mar 15, 2024
26dc676
Drop support for Basic Auth that would
herrBez Mar 15, 2024
62ebf2a
Fix Documentation by adding SSL reference
herrBez Mar 15, 2024
5635e75
Update metricbeat/module/aerospike/aerospike_test.go
herrBez Apr 8, 2024
76772c9
Address reviewer's request:
herrBez Apr 8, 2024
7d057a4
Run mage config
herrBez Apr 8, 2024
95544f3
mage check
shmsr Apr 8, 2024
689c651
Merge branch 'main' into aerospike-tls-spike
shmsr Apr 8, 2024
4a15b5b
Replace "Optional SSL/TLS. By default is off." with disabled by default
herrBez Apr 8, 2024
5db38b6
Convert ClusterName to a simple string instead of pointer to a string
herrBez Apr 8, 2024
43f4a74
Merge branch 'main' into aerospike-tls-spike
herrBez Apr 13, 2024
c737807
Update x-pack/metricbeat.reference.yml
herrBez Apr 13, 2024
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
38 changes: 38 additions & 0 deletions metricbeat/module/aerospike/aerospike.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,47 @@ import (
"strconv"
"strings"

"github.com/elastic/elastic-agent-libs/transport/tlscommon"

as "github.com/aerospike/aerospike-client-go"
)

type Config struct {
ClusterName *string `config:"cluster_name"`
gpop63 marked this conversation as resolved.
Show resolved Hide resolved
User *string `config:"username"`
Password *string `config:"password"`
TLS *tlscommon.Config `config:"ssl"`
}

// DefaultConfig return default config for the aerospike module.
func DefaultConfig() Config {
return Config{}
}

func ParseClientPolicy(config Config) (*as.ClientPolicy, error) {
clientPolicy := as.NewClientPolicy()
if config.TLS.IsEnabled() {
tlsconfig, err := tlscommon.LoadTLSConfig(config.TLS)
if err != nil {
return nil, fmt.Errorf("could not initialize TLS configurations %w", err)
}
clientPolicy.TlsConfig = tlsconfig.ToConfig()
}
if config.User != nil && config.Password != nil {
clientPolicy.User = *config.User
clientPolicy.Password = *config.Password
} else if config.User != nil && config.Password == nil {
return nil, fmt.Errorf("if username is set, password should be set too")
} else if config.User == nil && config.Password != nil {
return nil, fmt.Errorf("if password is set, username should be set too")
}

if config.ClusterName != nil {
clientPolicy.ClusterName = *config.ClusterName
}
return clientPolicy, nil
}

func ParseHost(host string) (*as.Host, error) {
pieces := strings.Split(host, ":")
if len(pieces) != 2 {
Expand Down
102 changes: 102 additions & 0 deletions metricbeat/module/aerospike/aerospike_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,10 @@

import (
"errors"
"fmt"
"testing"

"github.com/elastic/elastic-agent-libs/transport/tlscommon"

Check failure on line 25 in metricbeat/module/aerospike/aerospike_test.go

View workflow job for this annotation

GitHub Actions / lint (windows)

File is not `goimports`-ed with -local github.com/elastic (goimports)
Copy link
Contributor Author

@herrBez herrBez Feb 23, 2024

Choose a reason for hiding this comment

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

Not sure what does the error mean, any guidance is much appreciated

Copy link
Contributor

Choose a reason for hiding this comment

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

The imports starting with github.com/elastic/ need to appear before other 3rd party imports, run goimports -w -local github.com/elastic metricbeat/module/aerospike

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you. Let's see if it's fixed now :)

"github.com/stretchr/testify/assert"

as "github.com/aerospike/aerospike-client-go"
Expand Down Expand Up @@ -96,3 +98,103 @@
assert.Equal(t, test.expected, result, test.Name)
}
}

func getStringPointer(value string) *string {
return &value
}

func getBoolPointer(value bool) *bool {
return &value
}
herrBez marked this conversation as resolved.
Show resolved Hide resolved

func TestParseClientPolicy(t *testing.T) {
sampleUser := "Test"
samplePassword := "MySecretPassword"
sampleClusterName := "TestCluster"

UserPasswordClientPolicy := as.NewClientPolicy()
UserPasswordClientPolicy.User = sampleUser
UserPasswordClientPolicy.Password = samplePassword

TLSPolicy := as.NewClientPolicy()
tlsconfig, _ := tlscommon.LoadTLSConfig(&tlscommon.Config{Enabled: getBoolPointer(true)})
TLSPolicy.TlsConfig = tlsconfig.ToConfig()

ClusterNamePolicy := as.NewClientPolicy()
ClusterNamePolicy.ClusterName = sampleClusterName

tests := []struct {
Name string
Config Config
expectedClientPolicy *as.ClientPolicy
expectedErr error
}{
{
Name: "No configurations lead to default policy",
Config: Config{},
expectedClientPolicy: as.NewClientPolicy(),
expectedErr: nil,
},
{
Name: "Username and password are honored",
Config: Config{
User: getStringPointer("Test"),
Password: getStringPointer("MySecretPassword"),
},
expectedClientPolicy: UserPasswordClientPolicy,
expectedErr: nil,
},
{
Name: "Username is set but Password is not",
Config: Config{
User: getStringPointer(sampleUser),
},
expectedClientPolicy: UserPasswordClientPolicy,
expectedErr: fmt.Errorf("if username is set, password should be set too"),
},
{
Name: "Password is set but Username is not",
Config: Config{
Password: getStringPointer(samplePassword),
},
expectedClientPolicy: UserPasswordClientPolicy,
expectedErr: fmt.Errorf("if password is set, username should be set too"),
},
{
Name: "TLS Declaration",
Config: Config{
TLS: &tlscommon.Config{
Enabled: getBoolPointer(true),
},
},
expectedClientPolicy: TLSPolicy,
expectedErr: nil,
},
{
Name: "Cluster Name Setting",
Config: Config{
ClusterName: getStringPointer(sampleClusterName),
},
expectedClientPolicy: ClusterNamePolicy,
expectedErr: nil,
},
}

for _, test := range tests {
result, err := ParseClientPolicy(test.Config)
if err != nil {
if test.expectedErr != nil {
assert.Equal(t, test.expectedErr.Error(), err.Error())
continue
}
t.Error(err)
continue
}
assert.Equal(t, test.expectedClientPolicy.User, result.User, test.Name)
assert.Equal(t, test.expectedClientPolicy.Password, result.Password, test.Name)
assert.Equal(t, test.expectedClientPolicy.ClusterName, result.ClusterName, test.Name)
Copy link
Contributor

Choose a reason for hiding this comment

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

would you mind switching to Equalf and providing a description of what didn't match?

Right now if this fails in CI we will get an error like "A" does not equal "B". It is a lot easier to debug if we get something like Aerospike policy username is wrong. got: "A" expected: "B"

Copy link
Contributor Author

@herrBez herrBez Mar 14, 2024

Choose a reason for hiding this comment

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

Hi,
Thank you for the feedback. I don't mind at all ^^. So something like:

assert.Equalf(t, test.expectedClientPolicy.User, result.User, "Aerospike policy [Test '%s'] username is wrong: expected '%s' got '%s'", test.Name, test.expectedClientPolicy.User, result.User)

Or something like:

assert.Equalf(t, test.expectedClientPolicy.User, result.User, "Aerospike policy [Test '%s'] username is wrong", test.Name)

??

Should I also modify the existing tests to use Equalf?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think you need the test name, when it errors out go test will give us that. I prefer the ones that have expected and got (or something like that).

One thing I think that helps is to fail the test and just double check that the error message gives you enough context that you have some idea of where the problem is. For example telling me that the Username is what didn't match, the value we got, and the expected value goes a long way to helping understand where the problem might be.

I think you can just focus on the tests you are adding. That helps limit the scope of the PR.

if test.Config.TLS.IsEnabled() {
assert.NotNil(t, result.TlsConfig)
}
}
}
15 changes: 11 additions & 4 deletions metricbeat/module/aerospike/namespace/namespace.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,15 +42,16 @@ func init() {
// multiple fetch calls.
type MetricSet struct {
mb.BaseMetricSet
host *as.Host
client *as.Client
host *as.Host
clientPolicy *as.ClientPolicy
client *as.Client
}

// New create a new instance of the MetricSet
// Part of new is also setting up the configuration by processing additional
// configuration entries if needed.
func New(base mb.BaseMetricSet) (mb.MetricSet, error) {
config := struct{}{}
config := aerospike.DefaultConfig()
if err := base.Module().UnpackConfig(&config); err != nil {
return nil, err
}
Expand All @@ -60,9 +61,15 @@ func New(base mb.BaseMetricSet) (mb.MetricSet, error) {
return nil, fmt.Errorf("Invalid host format, expected hostname:port: %w", err)
}

clientPolicy, err := aerospike.ParseClientPolicy(config)
if err != nil {
return nil, fmt.Errorf("could not initialize aerospike client policy: %w", err)
}

return &MetricSet{
BaseMetricSet: base,
host: host,
clientPolicy: clientPolicy,
}, nil
}

Expand Down Expand Up @@ -105,7 +112,7 @@ func (m *MetricSet) Fetch(reporter mb.ReporterV2) error {
// create an aerospike client if it doesn't exist yet
func (m *MetricSet) connect() error {
gpop63 marked this conversation as resolved.
Show resolved Hide resolved
if m.client == nil {
client, err := as.NewClientWithPolicyAndHost(as.NewClientPolicy(), m.host)
client, err := as.NewClientWithPolicyAndHost(m.clientPolicy, m.host)
if err != nil {
return err
}
Expand Down
12 changes: 12 additions & 0 deletions metricbeat/modules.d/aerospike.yml.disabled
herrBez marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,15 @@
# - namespace
period: 10s
hosts: ["localhost:3000"]

# Username of hosts. Empty by default.
# username: root

# Password of hosts. Empty by default.
# password: secret

# Aerospike Cluster Name
# cluster_name: myclustername

# Read more about SSL Configurations https://www.elastic.co/guide/en/beats/metricbeat/current/configuration-ssl.html
# ssl.enabled: false
Loading