From 8620adfc8c864943ded9e7a940aed8ec3036df75 Mon Sep 17 00:00:00 2001 From: Petr Vobornik Date: Thu, 26 Oct 2023 13:33:13 +0200 Subject: [PATCH] fix: do not try to notify if required data are missing (#14) Adds a check which controls if notification will happen or not. The check validates presence of the most required parts and creates a basis for adding more. Now it checks for: - having at least one sample to send - HostInfo present and containing HostID and ExternalOrganization Signed-off-by: Petr Vobornik --- daemon/daemon.go | 10 ++-- daemon/daemon_test.go | 42 +++++++++++++--- notify/policy.go | 37 ++++++++++++++ notify/policy_test.go | 110 ++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 190 insertions(+), 9 deletions(-) create mode 100644 notify/policy.go create mode 100644 notify/policy_test.go diff --git a/daemon/daemon.go b/daemon/daemon.go index e9d2388..a5ea64d 100644 --- a/daemon/daemon.go +++ b/daemon/daemon.go @@ -20,6 +20,7 @@ type Daemon struct { metricsLog *notify.MetricsLog certWatcher hostinfo.CertWatcher notifier notify.Notifier + notifyPolicy notify.NotifyPolicy stopCh chan os.Signal started bool } @@ -30,6 +31,7 @@ func NewDaemon(config *config.Config) (*Daemon, error) { config: config, notifier: notify.NewPrometheusNotifier(config), hostInfoProvider: &hostinfo.SubManInfoProvider{}, + notifyPolicy: ¬ify.GeneralNotifyPolicy{}, } d.certWatcher, err = hostinfo.NewINotifyCertWatcher(d.config.HostCertPath) if err != nil { @@ -208,11 +210,13 @@ func (d *Daemon) notify() error { if d.config.MetricsMaxAge > 0 { samples = notify.FilterSamplesByAge(samples, d.config.MetricsMaxAge) } - count := len(samples) - if count == 0 { - logger.Debugln("No samples to send") + err = d.notifyPolicy.ShouldNotify(samples, d.hostInfo) + if err != nil { + logger.Warnf("Cannot notify: %s\n", err.Error()) return nil } + + count := len(samples) logger.Debugf("Sending %d sample(s)...\n", count) err = d.notifier.Notify(samples, d.hostInfo) if err != nil { diff --git a/daemon/daemon_test.go b/daemon/daemon_test.go index 15de5c5..e36f819 100644 --- a/daemon/daemon_test.go +++ b/daemon/daemon_test.go @@ -152,6 +152,7 @@ func TestNotify(t *testing.T) { daemon, notifier, metricsLog, hiProvider := createDaemon(t) daemon.config.MetricsMaxAge = 10 * time.Second daemon.hostInfo, _ = hiProvider.Load() + notifyPolicy := daemon.notifyPolicy.(*mockNotifyPolicy) // Test that notifier is called when there are some samples metricsLog.WriteSampleNow(1) @@ -159,6 +160,7 @@ func TestNotify(t *testing.T) { err := daemon.notify() checkError(t, err, "failed to notify") notifier.CheckWasCalled(t) + notifyPolicy.CheckWasCalled(t) // Test that notifier was called with the sample and hostinfo if len(notifier.calledWith.samples) != 1 { @@ -335,14 +337,16 @@ func createDaemon(t *testing.T) (*Daemon, *mockNotifier, *notify.MetricsLog, *mo notifier := &mockNotifier{} notifier.ExpectSuccess() daemon.notifier = notifier + daemon.notifyPolicy = NewMockNotifyPolicy(false) daemon.certWatcher = &mockCertWatcher{make(chan hostinfo.CertEvent)} hiProvider := newMockHostInfoProvider(&hostinfo.HostInfo{ - CpuCount: 2, - HostId: "testhost-id", - SocketCount: "1", - Product: "testproduct", - Support: "testsupport", - Usage: "testusage", + CpuCount: 2, + HostId: "testhost-id", + ExternalOrganization: "testorg", + SocketCount: "1", + Product: "testproduct", + Support: "testsupport", + Usage: "testusage", Billing: hostinfo.BillingInfo{ Model: "testmodel", Marketplace: "testmarketplace", @@ -426,6 +430,32 @@ func (n *mockNotifier) WaitForCall(t *testing.T, timeout time.Duration) { } } +// Mock NotifyPolicy +type mockNotifyPolicy struct { + Called uint + Fake bool +} + +func (m *mockNotifyPolicy) ShouldNotify(samples []prompb.Sample, hostinfo *hostinfo.HostInfo) error { + m.Called++ + if !m.Fake { + policy := ¬ify.GeneralNotifyPolicy{} + return policy.ShouldNotify(samples, hostinfo) + } + return nil +} + +func (m *mockNotifyPolicy) CheckWasCalled(t *testing.T) { + t.Helper() + if m.Called == 0 { + t.Fatalf("expected notify policy to be called") + } +} + +func NewMockNotifyPolicy(fake bool) *mockNotifyPolicy { + return &mockNotifyPolicy{0, fake} +} + // Mock HostInfo provider type mockHostInfoProvider struct { diff --git a/notify/policy.go b/notify/policy.go new file mode 100644 index 0000000..85ae217 --- /dev/null +++ b/notify/policy.go @@ -0,0 +1,37 @@ +package notify + +import ( + "fmt" + + "github.com/prometheus/prometheus/prompb" + + "github.com/RedHatInsights/host-metering/hostinfo" +) + +type NotifyPolicy interface { + ShouldNotify([]prompb.Sample, *hostinfo.HostInfo) error +} + +type GeneralNotifyPolicy struct{} + +func (p *GeneralNotifyPolicy) ShouldNotify(samples []prompb.Sample, hostinfo *hostinfo.HostInfo) error { + + count := len(samples) + if count == 0 { + return fmt.Errorf("no samples to send") + } + + if hostinfo == nil { + return fmt.Errorf("missing internal HostInfo") + } + + if hostinfo.HostId == "" { + return fmt.Errorf("missing HostId") + } + + if hostinfo.ExternalOrganization == "" { + return fmt.Errorf("missing ExternalOrganization") + } + + return nil +} diff --git a/notify/policy_test.go b/notify/policy_test.go new file mode 100644 index 0000000..b29fd10 --- /dev/null +++ b/notify/policy_test.go @@ -0,0 +1,110 @@ +package notify + +import ( + "strings" + "testing" + + "github.com/RedHatInsights/host-metering/hostinfo" + "github.com/prometheus/prometheus/prompb" +) + +type ShouldNotifyTestCase struct { + name string + samples []prompb.Sample + hostInfo *hostinfo.HostInfo + expected string +} + +func TestGeneralNotifyPolicy(t *testing.T) { + + p := &GeneralNotifyPolicy{} + correctSamples := []prompb.Sample{{}} + correctHostInfo := fullyDefinedMockHostInfo() + + emptyHostIdHI := fullyDefinedMockHostInfo() + emptyHostIdHI.HostId = "" + + emptyExternalOrganizationHI := fullyDefinedMockHostInfo() + emptyExternalOrganizationHI.ExternalOrganization = "" + + testCases := []ShouldNotifyTestCase{ + { + name: "No samples", + samples: []prompb.Sample{}, + hostInfo: correctHostInfo, + expected: "no samples to send", + }, + { + name: "Nil hostInfo", + samples: correctSamples, + hostInfo: nil, + expected: "missing internal HostInfo", + }, + { + name: "Empty HostId", + samples: correctSamples, + hostInfo: emptyHostIdHI, + expected: "missing HostId", + }, + { + name: "Empty ExternalOrganization", + samples: correctSamples, + hostInfo: emptyExternalOrganizationHI, + expected: "missing ExternalOrganization", + }, + } + + t.Run("Test ShouldNotify", func(t *testing.T) { + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + // when + err := p.ShouldNotify(tc.samples, tc.hostInfo) + + // then + expectErrorContains(t, err, tc.expected) + }) + } + + t.Run("Possitive", func(t *testing.T) { + // when + err := p.ShouldNotify(correctSamples, correctHostInfo) + + // then + if err != nil { + t.Fatalf("expected no error, got '%s'", err.Error()) + } + }) + }) +} + +func fullyDefinedMockHostInfo() *hostinfo.HostInfo { + return &hostinfo.HostInfo{ + HostId: "hostid", + ExternalOrganization: "externalorganization", + SocketCount: "socketcount", + Product: "product", + Support: "support", + Usage: "usage", + Billing: fullyDefinedMockBillingInfo(), + } +} + +func fullyDefinedMockBillingInfo() hostinfo.BillingInfo { + return hostinfo.BillingInfo{ + Model: "model", + Marketplace: "marketplace", + MarketplaceAccount: "marketplaceaccount", + MarketplaceInstanceId: "marketplaceinstanceid", + } +} + +func expectErrorContains(t *testing.T, err error, expected string) { + t.Helper() + if err == nil { + t.Fatalf("expected error, got nil") + } + if !strings.Contains(err.Error(), expected) { + t.Fatalf("expected error to contain '%s', got '%s'", expected, err.Error()) + } +}