Skip to content

fix(ROSAENG-435): support PagerDuty incident lookup for HCP clusters #962

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

Open
fullsend-ai-coder wants to merge 4 commits into
base: master
Choose a base branch
from
+383 −23
Open

fix(ROSAENG-435): support PagerDuty incident lookup for HCP clusters #962

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fcf9c36
fix(ROSAENG-435): support PagerDuty incident lookup for HCP clusters
fullsend-ai-coder[bot] Sep 10, 2026
04befce
fix: address review feedback on PR #962
fullsend-ai-coder[bot] Sep 10, 2026
5c96b53
fix: address review feedback on PR #962
fullsend-ai-coder[bot] Sep 10, 2026
24c9674
fix: address review feedback on PR #962
fullsend-ai-coder[bot] Sep 10, 2026
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
21 changes: 18 additions & 3 deletions cmd/cluster/context.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ type contextOptions struct {
jiratoken string
teamIds []string
regionID string
isHCPRegionBased bool
}

type contextData struct {
Expand Down Expand Up @@ -195,6 +196,13 @@ 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

@fullsend-ai-review fullsend-ai-review Bot Sep 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[low] pattern-violation

The HCP detection guard appears identically in two places within the same file (setup() and generateContextData()) and once in cmd/org/context.go. Computing the HCP check once in setup() and storing the result on contextOptions would reduce the maintenance surface.

Suggested fix: Compute the HCP check once in setup() and store on contextOptions.

// services keyed by DNS base domain. Use the region ID as the PD
// service query for HCP clusters.

@fullsend-ai-review fullsend-ai-review Bot Sep 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[low] scope-coherence

baseDomain field is overloaded with the region ID for HCP clusters. Semantically baseDomain represents a DNS base domain but is repurposed as a PD service query string.

Suggested fix: Consider introducing a separate variable (e.g., pdServiceQuery) to distinguish between DNS base domain and PD service lookup key.

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()

if o.usertoken == "" {
Expand Down Expand Up @@ -374,12 +382,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

@fullsend-ai-review fullsend-ai-review Bot Sep 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[low] inconsistent HCP detection

In generateContextData(), the HCP check for setting the cluster ID uses only o.cluster.Hypershift().Enabled(), while setup() additionally guards with o.cluster.Region() != nil && o.cluster.Region().ID() != "". If an HCP cluster has no region, setup() would NOT override baseDomain, but generateContextData() would still call WithClusterID, enabling filtering against a baseDomain that holds the DNS domain.

Suggested fix: Use the same compound condition in generateContextData().

// PD service. Use the external ID because PD alerts reference the
// cluster's external UUID, not the internal OCM ID.
if o.isHCPRegionBased {
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))
Expand Down
16 changes: 13 additions & 3 deletions cmd/org/context.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ type DefaultContextFetcher struct {
GetLimitedSupport func(*sdk.Connection, string) ([]*cmv1.LimitedSupportReason, error)

@fullsend-ai-review fullsend-ai-review Bot Sep 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[low] design-direction

The NewPDClient signature change from one to two parameters is consistent with project patterns. If more parameters are needed in the future, consider refactoring to an options struct.

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)

@fullsend-ai-review fullsend-ai-review Bot Sep 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[low] api-design-coherence

NewPDClient function signature was changed to include clusterID as a parameter, while cmd/cluster/context.go uses the builder pattern directly via WithClusterID(). The two code paths use inconsistent approaches for threading the same value.

}

type PDClient interface {
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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")
}
Expand Down
23 changes: 23 additions & 0 deletions cmd/org/context_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,29 @@ func TestFetchContext_NoSubscriptions(t *testing.T) {
}
}

@fullsend-ai-review fullsend-ai-review Bot Sep 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[low] test-inadequate

TestNewPDClient_PassesClusterID only verifies the function signature accepts two parameters. While pagerduty_test.go has thorough unit tests for incidentMatchesCluster and HCP filtering, there is no integration-level test verifying that FetchContext correctly detects HCP clusters and passes the region as baseDomain and externalID as clusterID.

Suggested fix: Add an integration test for the FetchContext HCP detection path.

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) {
Expand Down
85 changes: 68 additions & 17 deletions pkg/provider/pagerduty/pagerduty.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ package pagerduty
import (
"context"
"fmt"
"os"
"sort"
"strings"
"time"
Expand Down Expand Up @@ -33,6 +34,7 @@ type pdClientInterface interface {
type client struct {
pdclient pdClientInterface
baseDomain string
clusterID string
teamIds []string
userToken string
oauthToken string
Expand All @@ -47,6 +49,11 @@ func (c *client) WithBaseDomain(baseDomain string) *client {
return c
}

func (c *client) WithClusterID(clusterID string) *client {

@fullsend-ai-review fullsend-ai-review Bot Sep 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[low] naming-convention

WithClusterID uses uppercase ID, while the existing WithTeamIdList uses lowercase Id. The new method follows Go naming conventions correctly (ID all-caps); the pre-existing WithTeamIdList is the one that deviates.

c.clusterID = clusterID
return c
}

func (c *client) WithTeamIdList(teamIds []string) *client {
c.teamIds = teamIds
return c
Expand Down Expand Up @@ -103,24 +110,37 @@ 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},
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
}

@fullsend-ai-review fullsend-ai-review Bot Sep 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[medium] missing filtering

GetHistoricalAlertsForCluster does not apply cluster-ID filtering for HCP clusters. Since the same client (with clusterID set) is used for both GetFiringAlertsForCluster and GetHistoricalAlertsForCluster, historical alerts for HCP clusters will include incidents from ALL clusters in the region-based PD service, not just the target cluster.

Suggested fix: Apply the same incidentMatchesCluster filtering in GetHistoricalAlertsForCluster when c.clusterID is set (and include first_trigger_log_entries in the options), or document that historical alerts intentionally show region-wide data for HCP.


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)
}

@fullsend-ai-review fullsend-ai-review Bot Sep 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[medium] error handling / output corruption

The fmt.Printf("Skipping incident ...") calls in GetFiringAlertsForCluster (line 143) and GetHistoricalAlertsForCluster (line 210) write to stdout. Both osdctl cluster context -o json and osdctl org context -o json also write structured JSON to stdout. When HCP filtering occurs, these debug messages corrupt the JSON output for downstream consumers (e.g., jq, scripts). This is also inconsistent with the codebase logging pattern — the pagerduty package uses fmt.Printf only for genuine error conditions.

Suggested fix: Replace fmt.Printf with fmt.Fprintf(os.Stderr, ...) or remove the debug logging entirely, since the filtering behavior is expected and validated by tests.


if !listIncidentsResponse.More {
break
Expand All @@ -131,6 +151,24 @@ 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 {

@fullsend-ai-review fullsend-ai-review Bot Sep 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[low] edge-case

The incidentMatchesCluster function performs exact string equality checks on the cluster ID. No logging or metrics are emitted when incidents are filtered out, which could make debugging HCP incident visibility issues harder in production.

@fullsend-ai-review fullsend-ai-review Bot Sep 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[low] edge-case

When EventDetails is nil in incidentMatchesCluster, the incident is silently filtered out. If PagerDuty doesn't populate EventDetails for certain incident types (e.g., incidents created via the UI rather than an integration), legitimate HCP incidents could be silently dropped.

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"} {
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
Expand All @@ -140,16 +178,24 @@ 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},
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 {
Expand All @@ -160,7 +206,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)
Expand Down
Loading
Loading