From de7a4d6f214351a6864a5167cc5ebbdf8be66c32 Mon Sep 17 00:00:00 2001 From: fullsend-code <278716306+fullsend-ai-coder[bot]@users.noreply.github.com> Date: Tue, 4 Aug 2026 06:21:37 +0000 Subject: [PATCH] fix(#941): support PagerDuty incident lookup for HCP clusters HCP clusters use region-based PagerDuty services rather than per-cluster services keyed by DNS base domain. This caused osdctl cluster context and org context to return zero PD incidents for HCP clusters. For HCP clusters, use the cluster's region ID to query PD services and filter incidents by matching cluster ID in the first trigger log entry's EventDetails. Classic clusters continue to use the existing DNS-based lookup unchanged. Changes: - pkg/provider/pagerduty: add WithClusterID builder method and incidentMatchesCluster filter that checks EventDetails for cluster_id when clusterID is set on the client - cmd/cluster/context: detect HCP clusters in setup() to use region-based PD service query and cluster ID filtering - cmd/org/context: extend NewPDClient to accept clusterID, detect HCP clusters in FetchContext to use region-based lookup with cluster ID filtering Note: golangci-lint was not available in the sandbox. go vet and gofmt passed on all changed packages. Closes #941 --- cmd/cluster/context.go | 18 ++- 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, 281 insertions(+), 14 deletions(-) diff --git a/cmd/cluster/context.go b/cmd/cluster/context.go index 2aa26fa79..71c9d57d0 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,18 @@ 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. + if o.cluster.Hypershift().Enabled() { + pdClientBuilder = pdClientBuilder.WithClusterID(o.clusterID) + } + 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..d236fb479 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 ID. + baseDomain := cluster.DNS().BaseDomain() + var clusterID string + if cluster.Hypershift().Enabled() && cluster.Region() != nil && cluster.Region().ID() != "" { + baseDomain = cluster.Region().ID() + clusterID = cluster.ID() + } + 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..b543d4386 100644 --- a/cmd/org/context_test.go +++ b/cmd/org/context_test.go @@ -125,3 +125,26 @@ func TestFetchContext_ErrorCreateOCMClient(t *testing.T) { t.Errorf("expected error creating OCM client, got %v", err) } } + +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) + } +} 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..85e2a9c3f 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) @@ -126,6 +205,114 @@ var _ = Describe("Tests the Pagerduty Provider", func() { } }) + 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)) + }) + }) + It("Returns an error from the pd client if there's an error with the request", func() { m := pdMock.NewMockpdClientInterface(ctrl) m.EXPECT().ListIncidentsWithContext(gomock.Any(), gomock.Any()).Return(&pd.ListIncidentsResponse{}, fmt.Errorf("An error"))