Fix Jira integration for Atlassian Cloud migration#916
Fix Jira integration for Atlassian Cloud migration#916clcollins wants to merge 1 commit intoopenshift:mainfrom
Conversation
Red Hat migrated Jira from on-prem (issues.redhat.com) to Atlassian Cloud (redhat.atlassian.net). This broke access request creation because: 1. The default Jira base URL pointed to the old instance 2. PAT authentication is not supported by Atlassian Cloud - it requires Basic Auth (email + API token) 3. URL parsing in ohssService hardcoded ".com" domain suffix Changes: - Update default JiraBaseURL to redhat.atlassian.net - Switch from PATAuthTransport to BasicAuthTransport (email + API token) - Add JiraEmail config field with JIRA_EMAIL env var support - Fix URL parsing in ohssService to use net/url instead of string split - Add tests for URL parsing, ClusterID extraction, and JiraEmail config Mirrors the approach taken in osdctl commit 54dbc7aefe1f. Created with assistance from Claude 🤖 <claude@anthropic.com> Signed-off-by: Christopher Collins <collins.christopher@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: clcollins The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
WalkthroughThis PR adds Jira email configuration support, updates the default Jira URL endpoint from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@clcollins: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #916 +/- ##
==========================================
+ Coverage 53.93% 53.97% +0.04%
==========================================
Files 88 88
Lines 6656 6662 +6
==========================================
+ Hits 3590 3596 +6
- Misses 2596 2597 +1
+ Partials 470 469 -1
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/jira/issueService.go (1)
82-99: Add direct unit tests forcreateIssueService()auth behavior.The migration logic is good, but this path is currently untested directly (email-required validation + BasicAuth transport selection). Add focused tests here so future auth regressions are caught earlier.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/jira/issueService.go` around lines 82 - 99, Add unit tests that directly exercise createIssueService(): mock or stub config.GetBackplaneConfiguration to return configurations with (a) empty JiraEmail and a valid JiraToken to assert it returns the expected validation error about missing email, and (b) valid JiraEmail and JiraToken to assert it successfully constructs a jira.BasicAuthTransport (verify the returned IssueService or inspect the transport.Username/Password values used). Use unique symbols createIssueService, config.GetBackplaneConfiguration, bpConfig.JiraEmail, bpConfig.JiraToken, and jira.BasicAuthTransport to locate code under test and ensure the tests fail on regressions in auth selection and email-required validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/cli/config/config.go`:
- Line 65: The current normalization in pkg/accessrequest/accessRequest.go
relies on getJiraBaseURL() (and the constant JiraBaseURLDefaultValue) to
TrimPrefix(issueID, getJiraBaseURL()+"/browse/"), which breaks when Jira hosts
differ (.com vs .net); change the extraction to be domain-agnostic by locating
the "/browse/" path segment (or using a regex to capture the segment after
"/browse/") and extracting the issue key regardless of host, updating the logic
in the function that performs this normalization so it no longer depends on
getJiraBaseURL() or the JiraBaseURLDefaultValue.
In `@pkg/jira/ohssService.go`:
- Around line 75-77: The WebURL construction using url.Parse(issue.Self) can
produce malformed values when Parse returns a relative URL (empty Scheme/Host);
update the logic around url.Parse(issue.Self) (the block that sets
formatIssue.WebURL) to validate that u.Scheme is "http" or "https" and u.Host is
non-empty before formatting "%s://%s/browse/%s"; if those checks fail, do not
build the WebURL (leave it empty or use a safe fallback), and add unit tests for
relative/malformed issue.Self inputs to cover this case.
---
Nitpick comments:
In `@pkg/jira/issueService.go`:
- Around line 82-99: Add unit tests that directly exercise createIssueService():
mock or stub config.GetBackplaneConfiguration to return configurations with (a)
empty JiraEmail and a valid JiraToken to assert it returns the expected
validation error about missing email, and (b) valid JiraEmail and JiraToken to
assert it successfully constructs a jira.BasicAuthTransport (verify the returned
IssueService or inspect the transport.Username/Password values used). Use unique
symbols createIssueService, config.GetBackplaneConfiguration,
bpConfig.JiraEmail, bpConfig.JiraToken, and jira.BasicAuthTransport to locate
code under test and ensure the tests fail on regressions in auth selection and
email-required validation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 96c52a5e-ba3b-4c80-9f4b-2752f192b622
📒 Files selected for processing (6)
pkg/cli/config/config.gopkg/cli/config/config_test.gopkg/info/info.gopkg/jira/issueService.gopkg/jira/ohssService.gopkg/jira/ohssService_test.go
| JiraConfigForAccessRequestsKey = "jira-config-for-access-requests" | ||
| prodEnvNameDefaultValue = "production" | ||
| JiraBaseURLDefaultValue = "https://issues.redhat.com" | ||
| JiraBaseURLDefaultValue = "https://redhat.atlassian.net" |
There was a problem hiding this comment.
Default domain switch exposes domain-coupled issue ID parsing in access requests.
After Line 65 changes the default to .net, pkg/accessrequest/accessRequest.go (Line 82) still strips URLs using strings.TrimPrefix(issueID, getJiraBaseURL()+"/browse/"). Full legacy .com browse URLs won’t be normalized and can fail later issue-key handling. Consider making issue-ID extraction domain-agnostic (parse /browse/<KEY> from any Jira host).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/cli/config/config.go` at line 65, The current normalization in
pkg/accessrequest/accessRequest.go relies on getJiraBaseURL() (and the constant
JiraBaseURLDefaultValue) to TrimPrefix(issueID, getJiraBaseURL()+"/browse/"),
which breaks when Jira hosts differ (.com vs .net); change the extraction to be
domain-agnostic by locating the "/browse/" path segment (or using a regex to
capture the segment after "/browse/") and extracting the issue key regardless of
host, updating the logic in the function that performs this normalization so it
no longer depends on getJiraBaseURL() or the JiraBaseURLDefaultValue.
| if u, err := url.Parse(issue.Self); err == nil { | ||
| formatIssue.WebURL = fmt.Sprintf("%s://%s/browse/%s", u.Scheme, u.Host, issue.Key) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Current WebURL parsing logic =="
rg -n -C3 'url\.Parse\(issue\.Self\)|WebURL' pkg/jira/ohssService.go
echo
echo "== Existing Self URL test cases =="
rg -n -C2 'Self\s*=' pkg/jira/ohssService_test.go
echo
echo "== Check for malformed/relative Self coverage =="
rg -n -C2 'malformed|invalid|relative|scheme|host' pkg/jira/ohssService_test.go || trueRepository: openshift/backplane-cli
Length of output: 1321
Harden WebURL construction by validating parsed URL components.
At lines 75-77, url.Parse() succeeds for relative URLs without returning an error, but leaves u.Scheme and u.Host empty. This produces malformed WebURLs like :///browse/<key>. Add explicit checks for HTTP(S) scheme and non-empty host.
Current code has no test coverage for relative or malformed Self values.
Proposed fix
- if u, err := url.Parse(issue.Self); err == nil {
+ if u, err := url.Parse(issue.Self); err == nil &&
+ (u.Scheme == "http" || u.Scheme == "https") &&
+ u.Host != "" {
formatIssue.WebURL = fmt.Sprintf("%s://%s/browse/%s", u.Scheme, u.Host, issue.Key)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if u, err := url.Parse(issue.Self); err == nil { | |
| formatIssue.WebURL = fmt.Sprintf("%s://%s/browse/%s", u.Scheme, u.Host, issue.Key) | |
| } | |
| if u, err := url.Parse(issue.Self); err == nil && | |
| (u.Scheme == "http" || u.Scheme == "https") && | |
| u.Host != "" { | |
| formatIssue.WebURL = fmt.Sprintf("%s://%s/browse/%s", u.Scheme, u.Host, issue.Key) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/jira/ohssService.go` around lines 75 - 77, The WebURL construction using
url.Parse(issue.Self) can produce malformed values when Parse returns a relative
URL (empty Scheme/Host); update the logic around url.Parse(issue.Self) (the
block that sets formatIssue.WebURL) to validate that u.Scheme is "http" or
"https" and u.Host is non-empty before formatting "%s://%s/browse/%s"; if those
checks fail, do not build the WebURL (leave it empty or use a safe fallback),
and add unit tests for relative/malformed issue.Self inputs to cover this case.
2 participants
Problem
Red Hat migrated Jira from on-prem (
https://issues.redhat.com) to Atlassian Cloud (https://redhat.atlassian.net). This brokeocm-backplane accessrequest create— users receive a 403 when the CLI tries to create Jira notification issues because:ohssService.gohardcoded a.comdomain suffix viastrings.SplitAfter(issue.Self, ".com"), which breaks for.netdomainsPractical Impact
SREs have been unable to use AccessRequests since the Jira migration a few weeks ago. SREs were unaware they were able to break-glass to access clusters when AccessRequests were not working, resulting in delayed incident response and cluster access issues.
Changes
pkg/cli/config/config.go: Update defaultJiraBaseURLtohttps://redhat.atlassian.net. AddJiraEmailfield toBackplaneConfigurationwithJIRA_EMAILenv var support and config file fallback.pkg/info/info.go: AddBackplaneJiraEmailEnvNameconstant for theJIRA_EMAILenvironment variable.pkg/jira/issueService.go: Switch fromjira.PATAuthTransporttojira.BasicAuthTransportusing email + API token. Add validation for missingJiraEmail.pkg/jira/ohssService.go: Replace brittlestrings.SplitAfter(issue.Self, ".com")withnet/url.Parse()for domain-agnostic URL construction.New Tests
pkg/jira/ohssService_test.go: Tests for Atlassian Cloud.netURL parsing, legacy.comURL parsing, emptySelffield, andClusterIDcustom field extraction.pkg/cli/config/config_test.go: Tests forJIRA_EMAILenv var loading, env var precedence over config file, and config file fallback.User Migration
After this change, users need to configure their Jira email:
ocm-backplane config set jira-email user@redhat.comOr via environment variable:
export JIRA_EMAIL=user@redhat.comContext
Test Plan
go test ./pkg/jira/...— 7/7 passgo test ./pkg/cli/config/...— all passgo test ./pkg/accessrequest/...— all passocm-backplane accessrequest createwithJIRA_EMAILandJIRA_API_TOKENset against a cluster with access protection enabled