From fcf9c360f8f97ec32ee8e70c88a9561b047d8bd8 Mon Sep 17 00:00:00 2001 From: fullsend-code <278716306+fullsend-ai-coder[bot]@users.noreply.github.com> Date: Thu, 10 Sep 2026 15:26:04 +0000 Subject: [PATCH 1/4] fix(ROSAENG-435): support PagerDuty incident lookup for HCP clusters MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit HCP clusters use region-based PD services instead of per-cluster services keyed by DNS base domain. For classic clusters, PD services are named after the cluster's DNS domain, so querying by baseDomain works. For HCP, the PD service corresponds to the AWS region (e.g. us-east-1), and incidents for multiple clusters share the same service. Changes: - Add WithClusterID() to the PD client builder so callers can set a cluster ID for incident filtering - Modify GetFiringAlertsForCluster() to include first_trigger_log_entries when a cluster ID is set, and filter incidents by matching the cluster ID in EventDetails - Add incidentMatchesCluster() helper that checks cluster_id, clusterID, and cluster-id keys in EventDetails - In cmd/cluster/context.go: override baseDomain with the region ID for HCP clusters, and pass externalClusterID (the external UUID) to WithClusterID — PD alerts reference the external UUID, not the internal OCM ID - In cmd/org/context.go: update NewPDClient signature to accept clusterID, detect HCP clusters and pass cluster.ExternalID() The critical fix is using ExternalID() rather than ID() when filtering incidents. PagerDuty alerts contain the cluster's external UUID in their EventDetails, so passing the internal OCM ID would cause incidentMatchesCluster to always return false, filtering out all incidents. Related to ROSAENG-435 --- cmd/cluster/context.go | 19 ++- cmd/org/context.go | 16 +- cmd/org/context_test.go | 23 +++ pkg/provider/pagerduty/pagerduty.go | 51 ++++++- pkg/provider/pagerduty/pagerduty_test.go | 187 +++++++++++++++++++++++ 5 files changed, 282 insertions(+), 14 deletions(-) diff --git a/cmd/cluster/context.go b/cmd/cluster/context.go index 2aa26fa79..b286053ca 100644 --- a/cmd/cluster/context.go +++ b/cmd/cluster/context.go @@ -195,6 +195,12 @@ func (o *contextOptions) setup() error { o.clusterID = o.cluster.ID() o.externalClusterID = o.cluster.ExternalID() o.baseDomain = o.cluster.DNS().BaseDomain() + // HCP clusters use region-based PD services rather than per-cluster + // services keyed by DNS base domain. Use the region ID as the PD + // service query for HCP clusters. + if o.cluster.Hypershift().Enabled() && o.cluster.Region() != nil && o.cluster.Region().ID() != "" { + o.baseDomain = o.cluster.Region().ID() + } o.infraID = o.cluster.InfraID() if o.usertoken == "" { @@ -374,12 +380,19 @@ func (o *contextOptions) generateContextData() (*contextData, []error) { // For PD query dependencies pdwg := sync.WaitGroup{} var skipPagerDutyCollection bool - pdProvider, err := pagerduty.NewClient(). + pdClientBuilder := pagerduty.NewClient(). WithUserToken(o.usertoken). WithOauthToken(o.oauthtoken). WithBaseDomain(o.baseDomain). - WithTeamIdList(viper.GetStringSlice(pagerduty.PagerDutyTeamIDsKey)). - Init() + WithTeamIdList(viper.GetStringSlice(pagerduty.PagerDutyTeamIDsKey)) + // For HCP clusters, set the cluster ID so PD incidents are filtered + // to only those belonging to this cluster within the region-based + // PD service. Use the external ID because PD alerts reference the + // cluster's external UUID, not the internal OCM ID. + if o.cluster.Hypershift().Enabled() { + pdClientBuilder = pdClientBuilder.WithClusterID(o.externalClusterID) + } + pdProvider, err := pdClientBuilder.Init() if err != nil { skipPagerDutyCollection = true dataErrors = append(dataErrors, fmt.Errorf("skipping PagerDuty context collection: %v", err)) diff --git a/cmd/org/context.go b/cmd/org/context.go index 64756cb08..be4298b5f 100644 --- a/cmd/org/context.go +++ b/cmd/org/context.go @@ -41,7 +41,7 @@ type DefaultContextFetcher struct { GetLimitedSupport func(*sdk.Connection, string) ([]*cmv1.LimitedSupportReason, error) GetServiceLogs func(string, time.Time, bool, bool) ([]*v1.LogEntry, error) GetJiraIssues func(clusterID, externalID, filter string) ([]jira.Issue, error) - NewPDClient func(baseDomain string) (PDClient, error) + NewPDClient func(baseDomain, clusterID string) (PDClient, error) } type PDClient interface { @@ -83,9 +83,10 @@ func NewDefaultContextFetcher() *DefaultContextFetcher { GetLimitedSupport: utils.GetClusterLimitedSupportReasons, GetServiceLogs: servicelog.GetServiceLogsSince, GetJiraIssues: utils.GetJiraIssuesForCluster, - NewPDClient: func(baseDomain string) (PDClient, error) { + NewPDClient: func(baseDomain, clusterID string) (PDClient, error) { return pdProvider.NewClient(). WithBaseDomain(baseDomain). + WithClusterID(clusterID). WithUserToken(viper.GetString(pdProvider.PagerDutyUserTokenConfigKey)). WithOauthToken(viper.GetString(pdProvider.PagerDutyOauthTokenConfigKey)). Init() @@ -207,7 +208,16 @@ func (f *DefaultContextFetcher) FetchContext(orgID string, output io.Writer) ([] }) // PagerDuty alerts dataEg.Go(func() error { - pdClient, err := f.NewPDClient(cluster.DNS().BaseDomain()) + // For HCP clusters, PD services are organized by region + // rather than per-cluster DNS domain. Use the region as + // the query and filter incidents by cluster external ID. + baseDomain := cluster.DNS().BaseDomain() + var clusterID string + if cluster.Hypershift().Enabled() && cluster.Region() != nil && cluster.Region().ID() != "" { + baseDomain = cluster.Region().ID() + clusterID = cluster.ExternalID() + } + pdClient, err := f.NewPDClient(baseDomain, clusterID) if err != nil { return fmt.Errorf("failed to build PD client") } diff --git a/cmd/org/context_test.go b/cmd/org/context_test.go index e2c3c396b..ee77c5e58 100644 --- a/cmd/org/context_test.go +++ b/cmd/org/context_test.go @@ -111,6 +111,29 @@ func TestFetchContext_NoSubscriptions(t *testing.T) { } } +func TestNewPDClient_PassesClusterID(t *testing.T) { + // Verify that the NewPDClient function signature accepts both + // baseDomain and clusterID parameters. + var calledBaseDomain, calledClusterID string + fetcher := &DefaultContextFetcher{ + NewPDClient: func(baseDomain, clusterID string) (PDClient, error) { + calledBaseDomain = baseDomain + calledClusterID = clusterID + return &fakePDClient{ + serviceIDs: []string{"svc-1"}, + incidents: map[string][]pd.Incident{}, + }, nil + }, + } + _, _ = fetcher.NewPDClient("us-east-1", "hcp-cluster-123") + if calledBaseDomain != "us-east-1" { + t.Errorf("expected baseDomain 'us-east-1', got %q", calledBaseDomain) + } + if calledClusterID != "hcp-cluster-123" { + t.Errorf("expected clusterID 'hcp-cluster-123', got %q", calledClusterID) + } +} + func TestFetchContext_ErrorCreateOCMClient(t *testing.T) { fetcher := &DefaultContextFetcher{ SearchSubscriptions: func(orgID string, status string, managedOnly bool) ([]*accountsv1.Subscription, error) { diff --git a/pkg/provider/pagerduty/pagerduty.go b/pkg/provider/pagerduty/pagerduty.go index 8c63734e4..9d56a3980 100644 --- a/pkg/provider/pagerduty/pagerduty.go +++ b/pkg/provider/pagerduty/pagerduty.go @@ -33,6 +33,7 @@ type pdClientInterface interface { type client struct { pdclient pdClientInterface baseDomain string + clusterID string teamIds []string userToken string oauthToken string @@ -47,6 +48,11 @@ func (c *client) WithBaseDomain(baseDomain string) *client { return c } +func (c *client) WithClusterID(clusterID string) *client { + c.clusterID = clusterID + return c +} + func (c *client) WithTeamIdList(teamIds []string) *client { c.teamIds = teamIds return c @@ -106,21 +112,33 @@ func (c *client) GetFiringAlertsForCluster(pdServiceIDs []string) (map[string][] var incidentListOffset uint = 0 for _, pdServiceID := range pdServiceIDs { for { + opts := pd.ListIncidentsOptions{ + ServiceIDs: []string{pdServiceID}, + Statuses: []string{"triggered", "acknowledged"}, + SortBy: "urgency:DESC", + Limit: incidentLimit, + Offset: incidentListOffset, + } + // For HCP clusters, include first_trigger_log_entries so + // we can filter incidents by cluster ID in EventDetails. + if c.clusterID != "" { + opts.Includes = []string{"first_trigger_log_entries"} + } + listIncidentsResponse, err := c.pdclient.ListIncidentsWithContext( context.TODO(), - pd.ListIncidentsOptions{ - ServiceIDs: []string{pdServiceID}, - Statuses: []string{"triggered", "acknowledged"}, - SortBy: "urgency:DESC", - Limit: incidentLimit, - Offset: incidentListOffset, - }, + opts, ) if err != nil { return nil, err } - incidents[pdServiceID] = append(incidents[pdServiceID], listIncidentsResponse.Incidents...) + for _, incident := range listIncidentsResponse.Incidents { + if c.clusterID != "" && !incidentMatchesCluster(incident, c.clusterID) { + continue + } + incidents[pdServiceID] = append(incidents[pdServiceID], incident) + } if !listIncidentsResponse.More { break @@ -131,6 +149,23 @@ func (c *client) GetFiringAlertsForCluster(pdServiceIDs []string) (map[string][] return incidents, nil } +// incidentMatchesCluster checks whether a PagerDuty incident belongs to the +// given cluster by inspecting the first trigger log entry's EventDetails for +// a matching cluster_id value. This is used for HCP clusters where PD services +// are region-based and contain incidents for multiple clusters. +func incidentMatchesCluster(incident pd.Incident, clusterID string) bool { + ed := incident.FirstTriggerLogEntry.EventDetails + if ed == nil { + return false + } + for _, key := range []string{"cluster_id", "clusterID", "cluster-id"} { + if v, ok := ed[key]; ok && v == clusterID { + return true + } + } + return false +} + func (c *client) GetHistoricalAlertsForCluster(pdServiceIDs []string) (map[string][]*IncidentOccurrenceTracker, error) { var currentOffset uint diff --git a/pkg/provider/pagerduty/pagerduty_test.go b/pkg/provider/pagerduty/pagerduty_test.go index 5625124ed..c4939f8d5 100644 --- a/pkg/provider/pagerduty/pagerduty_test.go +++ b/pkg/provider/pagerduty/pagerduty_test.go @@ -20,6 +20,78 @@ func generateIncident() pd.Incident { } } +var _ = Describe("incidentMatchesCluster", func() { + It("Returns true when cluster_id matches", func() { + incident := pd.Incident{ + FirstTriggerLogEntry: pd.FirstTriggerLogEntry{ + CommonLogEntryField: pd.CommonLogEntryField{ + EventDetails: map[string]string{ + "cluster_id": "abc-123", + }, + }, + }, + } + Expect(incidentMatchesCluster(incident, "abc-123")).To(BeTrue()) + }) + + It("Returns true when clusterID key matches", func() { + incident := pd.Incident{ + FirstTriggerLogEntry: pd.FirstTriggerLogEntry{ + CommonLogEntryField: pd.CommonLogEntryField{ + EventDetails: map[string]string{ + "clusterID": "abc-123", + }, + }, + }, + } + Expect(incidentMatchesCluster(incident, "abc-123")).To(BeTrue()) + }) + + It("Returns true when cluster-id key matches", func() { + incident := pd.Incident{ + FirstTriggerLogEntry: pd.FirstTriggerLogEntry{ + CommonLogEntryField: pd.CommonLogEntryField{ + EventDetails: map[string]string{ + "cluster-id": "abc-123", + }, + }, + }, + } + Expect(incidentMatchesCluster(incident, "abc-123")).To(BeTrue()) + }) + + It("Returns false when cluster ID does not match", func() { + incident := pd.Incident{ + FirstTriggerLogEntry: pd.FirstTriggerLogEntry{ + CommonLogEntryField: pd.CommonLogEntryField{ + EventDetails: map[string]string{ + "cluster_id": "different-cluster", + }, + }, + }, + } + Expect(incidentMatchesCluster(incident, "abc-123")).To(BeFalse()) + }) + + It("Returns false when EventDetails is nil", func() { + incident := pd.Incident{} + Expect(incidentMatchesCluster(incident, "abc-123")).To(BeFalse()) + }) + + It("Returns false when no cluster ID key is present", func() { + incident := pd.Incident{ + FirstTriggerLogEntry: pd.FirstTriggerLogEntry{ + CommonLogEntryField: pd.CommonLogEntryField{ + EventDetails: map[string]string{ + "some_other_key": "abc-123", + }, + }, + }, + } + Expect(incidentMatchesCluster(incident, "abc-123")).To(BeFalse()) + }) +}) + var _ = Describe("Tests the Pagerduty Provider", func() { var pdProvider *client BeforeEach(func() { @@ -70,6 +142,13 @@ var _ = Describe("Tests the Pagerduty Provider", func() { ctrl.Finish() }) + Context("WithClusterID", func() { + It("Should correctly populate the clusterID", func() { + pdProvider.WithClusterID("test-cluster-123") + Expect(pdProvider.clusterID).To(Equal("test-cluster-123")) + }) + }) + Context("GetPDServiceIDs", func() { It("Returns an error from the pd client if there's an error with the request", func() { m := pdMock.NewMockpdClientInterface(ctrl) @@ -190,6 +269,114 @@ var _ = Describe("Tests the Pagerduty Provider", func() { Expect(incs["baz"]).To(BeEmpty()) }) }) + + Context("HCP cluster ID filtering", func() { + It("Returns only incidents matching the cluster ID in EventDetails", func() { + matchingIncident := pd.Incident{ + IncidentNumber: 1, + Title: "MatchingAlert", + FirstTriggerLogEntry: pd.FirstTriggerLogEntry{ + CommonLogEntryField: pd.CommonLogEntryField{ + EventDetails: map[string]string{ + "cluster_id": "hcp-cluster-123", + }, + }, + }, + } + nonMatchingIncident := pd.Incident{ + IncidentNumber: 2, + Title: "OtherClusterAlert", + FirstTriggerLogEntry: pd.FirstTriggerLogEntry{ + CommonLogEntryField: pd.CommonLogEntryField{ + EventDetails: map[string]string{ + "cluster_id": "hcp-cluster-999", + }, + }, + }, + } + noDetailsIncident := pd.Incident{ + IncidentNumber: 3, + Title: "NoDetailsAlert", + } + mixedResponse := &pd.ListIncidentsResponse{ + Incidents: []pd.Incident{matchingIncident, nonMatchingIncident, noDetailsIncident}, + } + + m := pdMock.NewMockpdClientInterface(ctrl) + m.EXPECT().ListIncidentsWithContext(gomock.Any(), gomock.Any()).Return(mixedResponse, nil) + pdProvider.pdclient = m + pdProvider.clusterID = "hcp-cluster-123" + + incs, err := pdProvider.GetFiringAlertsForCluster([]string{"region-svc"}) + Expect(err).To(BeNil()) + Expect(incs["region-svc"]).To(HaveLen(1)) + Expect(incs["region-svc"][0].Title).To(Equal("MatchingAlert")) + }) + + It("Returns empty when no incidents match the cluster ID", func() { + nonMatchingResponse := &pd.ListIncidentsResponse{ + Incidents: []pd.Incident{ + { + IncidentNumber: 1, + Title: "OtherAlert", + FirstTriggerLogEntry: pd.FirstTriggerLogEntry{ + CommonLogEntryField: pd.CommonLogEntryField{ + EventDetails: map[string]string{ + "cluster_id": "different-cluster", + }, + }, + }, + }, + }, + } + + m := pdMock.NewMockpdClientInterface(ctrl) + m.EXPECT().ListIncidentsWithContext(gomock.Any(), gomock.Any()).Return(nonMatchingResponse, nil) + pdProvider.pdclient = m + pdProvider.clusterID = "hcp-cluster-123" + + incs, err := pdProvider.GetFiringAlertsForCluster([]string{"region-svc"}) + Expect(err).To(BeNil()) + Expect(incs["region-svc"]).To(BeEmpty()) + }) + + It("Supports alternate cluster ID key names", func() { + incident := pd.Incident{ + IncidentNumber: 1, + Title: "AlternateKeyAlert", + FirstTriggerLogEntry: pd.FirstTriggerLogEntry{ + CommonLogEntryField: pd.CommonLogEntryField{ + EventDetails: map[string]string{ + "clusterID": "hcp-cluster-alt", + }, + }, + }, + } + response := &pd.ListIncidentsResponse{ + Incidents: []pd.Incident{incident}, + } + + m := pdMock.NewMockpdClientInterface(ctrl) + m.EXPECT().ListIncidentsWithContext(gomock.Any(), gomock.Any()).Return(response, nil) + pdProvider.pdclient = m + pdProvider.clusterID = "hcp-cluster-alt" + + incs, err := pdProvider.GetFiringAlertsForCluster([]string{"region-svc"}) + Expect(err).To(BeNil()) + Expect(incs["region-svc"]).To(HaveLen(1)) + }) + + It("Does not filter when clusterID is empty (classic cluster behavior)", func() { + m := pdMock.NewMockpdClientInterface(ctrl) + m.EXPECT().ListIncidentsWithContext(gomock.Any(), gomock.Any()).Return(singleIncResponse, nil) + pdProvider.pdclient = m + pdProvider.clusterID = "" + + incs, err := pdProvider.GetFiringAlertsForCluster([]string{"classic-svc"}) + Expect(err).To(BeNil()) + Expect(incs["classic-svc"]).To(HaveLen(1)) + }) + }) }) }) }) From 04befce46d14b5fe6e602d12a6a3e834e36594aa Mon Sep 17 00:00:00 2001 From: fullsend-fix <278716306+fullsend-ai-coder[bot]@users.noreply.github.com> Date: Thu, 10 Sep 2026 16:04:19 +0000 Subject: [PATCH 2/4] fix: address review feedback on PR #962 - Add cluster-ID filtering to GetHistoricalAlertsForCluster for HCP clusters, matching the existing filtering in GetFiringAlertsForCluster. Requests first_trigger_log_entries and filters by incidentMatchesCluster when clusterID is set. - Fix pre-existing pagination bug: reset incidentListOffset to 0 at the start of each outer loop iteration over pdServiceIDs in GetFiringAlertsForCluster. - Make HCP detection in generateContextData() consistent with setup() by adding Region() != nil && Region().ID() != "" guards. Addresses #962 --- cmd/cluster/context.go | 6 +- pkg/provider/pagerduty/pagerduty.go | 31 +++++++--- pkg/provider/pagerduty/pagerduty_test.go | 75 ++++++++++++++++++++++++ 3 files changed, 101 insertions(+), 11 deletions(-) diff --git a/cmd/cluster/context.go b/cmd/cluster/context.go index b286053ca..6f753354c 100644 --- a/cmd/cluster/context.go +++ b/cmd/cluster/context.go @@ -388,8 +388,10 @@ func (o *contextOptions) generateContextData() (*contextData, []error) { // For HCP clusters, set the cluster ID so PD incidents are filtered // to only those belonging to this cluster within the region-based // PD service. Use the external ID because PD alerts reference the - // cluster's external UUID, not the internal OCM ID. - if o.cluster.Hypershift().Enabled() { + // cluster's external UUID, not the internal OCM ID. Guard on region + // availability to stay consistent with the baseDomain override in + // setup(). + if o.cluster.Hypershift().Enabled() && o.cluster.Region() != nil && o.cluster.Region().ID() != "" { pdClientBuilder = pdClientBuilder.WithClusterID(o.externalClusterID) } pdProvider, err := pdClientBuilder.Init() diff --git a/pkg/provider/pagerduty/pagerduty.go b/pkg/provider/pagerduty/pagerduty.go index 9d56a3980..2844908ff 100644 --- a/pkg/provider/pagerduty/pagerduty.go +++ b/pkg/provider/pagerduty/pagerduty.go @@ -109,8 +109,9 @@ func (c *client) GetFiringAlertsForCluster(pdServiceIDs []string) (map[string][] incidents := map[string][]pd.Incident{} var incidentLimit uint = 25 - var incidentListOffset uint = 0 + var incidentListOffset uint for _, pdServiceID := range pdServiceIDs { + incidentListOffset = 0 for { opts := pd.ListIncidentsOptions{ ServiceIDs: []string{pdServiceID}, @@ -176,15 +177,22 @@ func (c *client) GetHistoricalAlertsForCluster(pdServiceIDs []string) (map[strin for _, pdServiceID := range pdServiceIDs { for currentOffset = 0; true; currentOffset += limit { + opts := pd.ListIncidentsOptions{ + ServiceIDs: []string{pdServiceID}, + Statuses: []string{"resolved", "triggered", "acknowledged"}, + Offset: currentOffset, + Limit: limit, + SortBy: "created_at:desc", + } + // For HCP clusters, include first_trigger_log_entries so + // we can filter incidents by cluster ID in EventDetails. + if c.clusterID != "" { + opts.Includes = []string{"first_trigger_log_entries"} + } + liResponse, err := c.pdclient.ListIncidentsWithContext( ctx, - pd.ListIncidentsOptions{ - ServiceIDs: []string{pdServiceID}, - Statuses: []string{"resolved", "triggered", "acknowledged"}, - Offset: currentOffset, - Limit: limit, - SortBy: "created_at:desc", - }, + opts, ) if err != nil { @@ -195,7 +203,12 @@ func (c *client) GetHistoricalAlertsForCluster(pdServiceIDs []string) (map[strin break } - incidents = append(incidents, liResponse.Incidents...) + for _, incident := range liResponse.Incidents { + if c.clusterID != "" && !incidentMatchesCluster(incident, c.clusterID) { + continue + } + incidents = append(incidents, incident) + } } incidentCounter := make(map[string]*IncidentOccurrenceTracker) diff --git a/pkg/provider/pagerduty/pagerduty_test.go b/pkg/provider/pagerduty/pagerduty_test.go index c4939f8d5..3de358b63 100644 --- a/pkg/provider/pagerduty/pagerduty_test.go +++ b/pkg/provider/pagerduty/pagerduty_test.go @@ -378,5 +378,80 @@ var _ = Describe("Tests the Pagerduty Provider", func() { }) }) }) + + Context("GetHistoricalAlertsForCluster HCP filtering", func() { + It("Returns only incidents matching the cluster ID in historical data", func() { + matchingIncident := pd.Incident{ + IncidentNumber: 1, + Title: "MatchingAlert is firing", + CreatedAt: "2024-01-15T10:00:00Z", + FirstTriggerLogEntry: pd.FirstTriggerLogEntry{ + CommonLogEntryField: pd.CommonLogEntryField{ + EventDetails: map[string]string{ + "cluster_id": "hcp-cluster-123", + }, + }, + }, + } + nonMatchingIncident := pd.Incident{ + IncidentNumber: 2, + Title: "OtherClusterAlert is firing", + CreatedAt: "2024-01-15T11:00:00Z", + FirstTriggerLogEntry: pd.FirstTriggerLogEntry{ + CommonLogEntryField: pd.CommonLogEntryField{ + EventDetails: map[string]string{ + "cluster_id": "hcp-cluster-999", + }, + }, + }, + } + mixedResponse := &pd.ListIncidentsResponse{ + Incidents: []pd.Incident{matchingIncident, nonMatchingIncident}, + } + emptyResponse := &pd.ListIncidentsResponse{ + Incidents: []pd.Incident{}, + } + + m := pdMock.NewMockpdClientInterface(ctrl) + m.EXPECT().ListIncidentsWithContext(gomock.Any(), gomock.Any()).Return(mixedResponse, nil) + m.EXPECT().ListIncidentsWithContext(gomock.Any(), gomock.Any()).Return(emptyResponse, nil) + pdProvider.pdclient = m + pdProvider.clusterID = "hcp-cluster-123" + + result, err := pdProvider.GetHistoricalAlertsForCluster([]string{"region-svc"}) + Expect(err).To(BeNil()) + Expect(result["region-svc"]).To(HaveLen(1)) + Expect(result["region-svc"][0].IncidentName).To(Equal("MatchingAlert")) + }) + + It("Returns all incidents when clusterID is empty (classic cluster)", func() { + incident1 := pd.Incident{ + IncidentNumber: 1, + Title: "Alert1 is firing", + CreatedAt: "2024-01-15T10:00:00Z", + } + incident2 := pd.Incident{ + IncidentNumber: 2, + Title: "Alert2 is firing", + CreatedAt: "2024-01-15T11:00:00Z", + } + response := &pd.ListIncidentsResponse{ + Incidents: []pd.Incident{incident1, incident2}, + } + emptyResponse := &pd.ListIncidentsResponse{ + Incidents: []pd.Incident{}, + } + + m := pdMock.NewMockpdClientInterface(ctrl) + m.EXPECT().ListIncidentsWithContext(gomock.Any(), gomock.Any()).Return(response, nil) + m.EXPECT().ListIncidentsWithContext(gomock.Any(), gomock.Any()).Return(emptyResponse, nil) + pdProvider.pdclient = m + pdProvider.clusterID = "" + + result, err := pdProvider.GetHistoricalAlertsForCluster([]string{"classic-svc"}) + Expect(err).To(BeNil()) + Expect(result["classic-svc"]).To(HaveLen(2)) + }) + }) }) }) From 5c96b538f5c0a6e97effcebde43585f8c18fc2c4 Mon Sep 17 00:00:00 2001 From: fullsend-fix <278716306+fullsend-ai-coder[bot]@users.noreply.github.com> Date: Thu, 10 Sep 2026 16:35:40 +0000 Subject: [PATCH 3/4] fix: address review feedback on PR #962 - Fix pre-existing bug in GetHistoricalAlertsForCluster where the incidents slice was not reset between service ID iterations, causing inflated IncidentOccurrenceTracker counts for subsequent services - Add debug logging when incidents are filtered out by incidentMatchesCluster to aid HCP incident visibility debugging - Move incidentMatchesCluster tests inside existing Describe block for consistency with test file organization Addresses #962 --- pkg/provider/pagerduty/pagerduty.go | 3 + pkg/provider/pagerduty/pagerduty_test.go | 144 +++++++++++------------ 2 files changed, 75 insertions(+), 72 deletions(-) diff --git a/pkg/provider/pagerduty/pagerduty.go b/pkg/provider/pagerduty/pagerduty.go index 2844908ff..0ab653348 100644 --- a/pkg/provider/pagerduty/pagerduty.go +++ b/pkg/provider/pagerduty/pagerduty.go @@ -136,6 +136,7 @@ func (c *client) GetFiringAlertsForCluster(pdServiceIDs []string) (map[string][] for _, incident := range listIncidentsResponse.Incidents { if c.clusterID != "" && !incidentMatchesCluster(incident, c.clusterID) { + fmt.Printf("Skipping incident %d (%s): does not match cluster %s\n", incident.IncidentNumber, incident.Title, c.clusterID) continue } incidents[pdServiceID] = append(incidents[pdServiceID], incident) @@ -176,6 +177,7 @@ func (c *client) GetHistoricalAlertsForCluster(pdServiceIDs []string) (map[strin incidentMap := map[string][]*IncidentOccurrenceTracker{} for _, pdServiceID := range pdServiceIDs { + incidents = incidents[:0] for currentOffset = 0; true; currentOffset += limit { opts := pd.ListIncidentsOptions{ ServiceIDs: []string{pdServiceID}, @@ -205,6 +207,7 @@ func (c *client) GetHistoricalAlertsForCluster(pdServiceIDs []string) (map[strin for _, incident := range liResponse.Incidents { if c.clusterID != "" && !incidentMatchesCluster(incident, c.clusterID) { + fmt.Printf("Skipping incident %d (%s): does not match cluster %s\n", incident.IncidentNumber, incident.Title, c.clusterID) continue } incidents = append(incidents, incident) diff --git a/pkg/provider/pagerduty/pagerduty_test.go b/pkg/provider/pagerduty/pagerduty_test.go index 3de358b63..ede5a10eb 100644 --- a/pkg/provider/pagerduty/pagerduty_test.go +++ b/pkg/provider/pagerduty/pagerduty_test.go @@ -20,78 +20,6 @@ func generateIncident() pd.Incident { } } -var _ = Describe("incidentMatchesCluster", func() { - It("Returns true when cluster_id matches", func() { - incident := pd.Incident{ - FirstTriggerLogEntry: pd.FirstTriggerLogEntry{ - CommonLogEntryField: pd.CommonLogEntryField{ - EventDetails: map[string]string{ - "cluster_id": "abc-123", - }, - }, - }, - } - Expect(incidentMatchesCluster(incident, "abc-123")).To(BeTrue()) - }) - - It("Returns true when clusterID key matches", func() { - incident := pd.Incident{ - FirstTriggerLogEntry: pd.FirstTriggerLogEntry{ - CommonLogEntryField: pd.CommonLogEntryField{ - EventDetails: map[string]string{ - "clusterID": "abc-123", - }, - }, - }, - } - Expect(incidentMatchesCluster(incident, "abc-123")).To(BeTrue()) - }) - - It("Returns true when cluster-id key matches", func() { - incident := pd.Incident{ - FirstTriggerLogEntry: pd.FirstTriggerLogEntry{ - CommonLogEntryField: pd.CommonLogEntryField{ - EventDetails: map[string]string{ - "cluster-id": "abc-123", - }, - }, - }, - } - Expect(incidentMatchesCluster(incident, "abc-123")).To(BeTrue()) - }) - - It("Returns false when cluster ID does not match", func() { - incident := pd.Incident{ - FirstTriggerLogEntry: pd.FirstTriggerLogEntry{ - CommonLogEntryField: pd.CommonLogEntryField{ - EventDetails: map[string]string{ - "cluster_id": "different-cluster", - }, - }, - }, - } - Expect(incidentMatchesCluster(incident, "abc-123")).To(BeFalse()) - }) - - It("Returns false when EventDetails is nil", func() { - incident := pd.Incident{} - Expect(incidentMatchesCluster(incident, "abc-123")).To(BeFalse()) - }) - - It("Returns false when no cluster ID key is present", func() { - incident := pd.Incident{ - FirstTriggerLogEntry: pd.FirstTriggerLogEntry{ - CommonLogEntryField: pd.CommonLogEntryField{ - EventDetails: map[string]string{ - "some_other_key": "abc-123", - }, - }, - }, - } - Expect(incidentMatchesCluster(incident, "abc-123")).To(BeFalse()) - }) -}) - var _ = Describe("Tests the Pagerduty Provider", func() { var pdProvider *client BeforeEach(func() { @@ -149,6 +77,78 @@ var _ = Describe("Tests the Pagerduty Provider", func() { }) }) + Context("incidentMatchesCluster", func() { + It("Returns true when cluster_id matches", func() { + incident := pd.Incident{ + FirstTriggerLogEntry: pd.FirstTriggerLogEntry{ + CommonLogEntryField: pd.CommonLogEntryField{ + EventDetails: map[string]string{ + "cluster_id": "abc-123", + }, + }, + }, + } + Expect(incidentMatchesCluster(incident, "abc-123")).To(BeTrue()) + }) + + It("Returns true when clusterID key matches", func() { + incident := pd.Incident{ + FirstTriggerLogEntry: pd.FirstTriggerLogEntry{ + CommonLogEntryField: pd.CommonLogEntryField{ + EventDetails: map[string]string{ + "clusterID": "abc-123", + }, + }, + }, + } + Expect(incidentMatchesCluster(incident, "abc-123")).To(BeTrue()) + }) + + It("Returns true when cluster-id key matches", func() { + incident := pd.Incident{ + FirstTriggerLogEntry: pd.FirstTriggerLogEntry{ + CommonLogEntryField: pd.CommonLogEntryField{ + EventDetails: map[string]string{ + "cluster-id": "abc-123", + }, + }, + }, + } + Expect(incidentMatchesCluster(incident, "abc-123")).To(BeTrue()) + }) + + It("Returns false when cluster ID does not match", func() { + incident := pd.Incident{ + FirstTriggerLogEntry: pd.FirstTriggerLogEntry{ + CommonLogEntryField: pd.CommonLogEntryField{ + EventDetails: map[string]string{ + "cluster_id": "different-cluster", + }, + }, + }, + } + Expect(incidentMatchesCluster(incident, "abc-123")).To(BeFalse()) + }) + + It("Returns false when EventDetails is nil", func() { + incident := pd.Incident{} + Expect(incidentMatchesCluster(incident, "abc-123")).To(BeFalse()) + }) + + It("Returns false when no cluster ID key is present", func() { + incident := pd.Incident{ + FirstTriggerLogEntry: pd.FirstTriggerLogEntry{ + CommonLogEntryField: pd.CommonLogEntryField{ + EventDetails: map[string]string{ + "some_other_key": "abc-123", + }, + }, + }, + } + Expect(incidentMatchesCluster(incident, "abc-123")).To(BeFalse()) + }) + }) + Context("GetPDServiceIDs", func() { It("Returns an error from the pd client if there's an error with the request", func() { m := pdMock.NewMockpdClientInterface(ctrl) From 24c9674d841b19798f7c992c3515a601c79ebb10 Mon Sep 17 00:00:00 2001 From: fullsend-fix <278716306+fullsend-ai-coder[bot]@users.noreply.github.com> Date: Thu, 10 Sep 2026 17:41:53 +0000 Subject: [PATCH 4/4] fix: address review feedback on PR #962 - Remove fmt.Printf stdout debug logging that corrupts JSON output when HCP filtering skips incidents; add stderr warning only for nil EventDetails edge case (visibility for silent drops) - Deduplicate HCP region guard: compute once in setup() and store as isHCPRegionBased on contextOptions - Move WithClusterID test to "Client Creation" describe block Addresses #962 --- cmd/cluster/context.go | 10 +++++----- pkg/provider/pagerduty/pagerduty.go | 4 ++-- pkg/provider/pagerduty/pagerduty_test.go | 13 ++++++------- 3 files changed, 13 insertions(+), 14 deletions(-) diff --git a/cmd/cluster/context.go b/cmd/cluster/context.go index 6f753354c..c8a20d8c0 100644 --- a/cmd/cluster/context.go +++ b/cmd/cluster/context.go @@ -67,6 +67,7 @@ type contextOptions struct { jiratoken string teamIds []string regionID string + isHCPRegionBased bool } type contextData struct { @@ -198,7 +199,8 @@ func (o *contextOptions) setup() error { // HCP clusters use region-based PD services rather than per-cluster // services keyed by DNS base domain. Use the region ID as the PD // service query for HCP clusters. - if o.cluster.Hypershift().Enabled() && o.cluster.Region() != nil && o.cluster.Region().ID() != "" { + o.isHCPRegionBased = o.cluster.Hypershift().Enabled() && o.cluster.Region() != nil && o.cluster.Region().ID() != "" + if o.isHCPRegionBased { o.baseDomain = o.cluster.Region().ID() } o.infraID = o.cluster.InfraID() @@ -388,10 +390,8 @@ func (o *contextOptions) generateContextData() (*contextData, []error) { // For HCP clusters, set the cluster ID so PD incidents are filtered // to only those belonging to this cluster within the region-based // PD service. Use the external ID because PD alerts reference the - // cluster's external UUID, not the internal OCM ID. Guard on region - // availability to stay consistent with the baseDomain override in - // setup(). - if o.cluster.Hypershift().Enabled() && o.cluster.Region() != nil && o.cluster.Region().ID() != "" { + // cluster's external UUID, not the internal OCM ID. + if o.isHCPRegionBased { pdClientBuilder = pdClientBuilder.WithClusterID(o.externalClusterID) } pdProvider, err := pdClientBuilder.Init() diff --git a/pkg/provider/pagerduty/pagerduty.go b/pkg/provider/pagerduty/pagerduty.go index 0ab653348..620e09242 100644 --- a/pkg/provider/pagerduty/pagerduty.go +++ b/pkg/provider/pagerduty/pagerduty.go @@ -6,6 +6,7 @@ package pagerduty import ( "context" "fmt" + "os" "sort" "strings" "time" @@ -136,7 +137,6 @@ func (c *client) GetFiringAlertsForCluster(pdServiceIDs []string) (map[string][] for _, incident := range listIncidentsResponse.Incidents { if c.clusterID != "" && !incidentMatchesCluster(incident, c.clusterID) { - fmt.Printf("Skipping incident %d (%s): does not match cluster %s\n", incident.IncidentNumber, incident.Title, c.clusterID) continue } incidents[pdServiceID] = append(incidents[pdServiceID], incident) @@ -158,6 +158,7 @@ func (c *client) GetFiringAlertsForCluster(pdServiceIDs []string) (map[string][] func incidentMatchesCluster(incident pd.Incident, clusterID string) bool { ed := incident.FirstTriggerLogEntry.EventDetails if ed == nil { + fmt.Fprintf(os.Stderr, "Warning: incident %d (%s) has no EventDetails; cannot determine cluster ownership, skipping\n", incident.IncidentNumber, incident.Title) return false } for _, key := range []string{"cluster_id", "clusterID", "cluster-id"} { @@ -207,7 +208,6 @@ func (c *client) GetHistoricalAlertsForCluster(pdServiceIDs []string) (map[strin for _, incident := range liResponse.Incidents { if c.clusterID != "" && !incidentMatchesCluster(incident, c.clusterID) { - fmt.Printf("Skipping incident %d (%s): does not match cluster %s\n", incident.IncidentNumber, incident.Title, c.clusterID) continue } incidents = append(incidents, incident) diff --git a/pkg/provider/pagerduty/pagerduty_test.go b/pkg/provider/pagerduty/pagerduty_test.go index ede5a10eb..cfa82c791 100644 --- a/pkg/provider/pagerduty/pagerduty_test.go +++ b/pkg/provider/pagerduty/pagerduty_test.go @@ -44,6 +44,12 @@ var _ = Describe("Tests the Pagerduty Provider", func() { Expect(pdProvider.oauthToken).To(Equal("oauth_token")) }) }) + Context("WithClusterID", func() { + It("Should correctly populate the clusterID", func() { + pdProvider.WithClusterID("test-cluster-123") + Expect(pdProvider.clusterID).To(Equal("test-cluster-123")) + }) + }) Context("Building the Client", func() { It("Should build the user_token client when the user client is called", func() { err := pdProvider.WithUserToken("token").buildClient() @@ -70,13 +76,6 @@ var _ = Describe("Tests the Pagerduty Provider", func() { ctrl.Finish() }) - Context("WithClusterID", func() { - It("Should correctly populate the clusterID", func() { - pdProvider.WithClusterID("test-cluster-123") - Expect(pdProvider.clusterID).To(Equal("test-cluster-123")) - }) - }) - Context("incidentMatchesCluster", func() { It("Returns true when cluster_id matches", func() { incident := pd.Incident{