fix(jira): stop reporting Jira failures as missing issues - #1123
Conversation
GetJiraIssueInfo treated anything short of a non-404 HTTP response as "the issue does not exist". When go-jira returns an error with no response at all - DNS failure, TLS error, timeout, connection refused - that guard is false, issue is nil, and the function returns IssueExists=false with a nil error. A network blip was therefore reported to the user as a missing Jira issue, and under `attest jira --assert` it failed their pipeline with a message pointing at their commit's Jira references rather than at the network. Classify the outcome instead. A 404 is the only status that answers the question, and stays a missing issue. A nil response is a reachability error naming the base URL; 401 and 403 name the credential in play, since "check the API token of user x" is the actionable part and go-jira's own message carries neither; any other status keeps the error go-jira returned. Tests cover each branch against a stubbed Jira, including the transport failure that returned no error at all before this change.
Jira will not confirm that an issue exists to a caller who may not see it, so the issue endpoint answers 404 both for an issue that is absent and for a credential that has expired or lost access. `attest jira` read that 404 as "issue not found" either way, so when the shared Atlassian API token needed recycling, CI failed. This sends the reader looking for a deleted Jira issue instead of a dead token. Customers hit the same message on their own pipelines whenever a token lapses, and Atlassian API tokens now expire by default. Ask Jira who we are before believing the 404. `VerifyCredentials` calls the identity endpoint, where a rejected credential does get a straight answer, and names the credential to check in the error. attest jira consults it only when some reference was not found - where every reference resolved, the credential has already proved itself - and fails before reporting, since issue_exists=false is not a fact we established and does not belong in a compliance trail.
|
Claude finished @mbevc1's task in 4m 56s —— View job Review — PR #1123
Static checks
I could not run Where the change has landedAcross seven commits this has converged on a defensible split, and it's worth stating it plainly because the PR body no longer describes it:
The retreat from Findings
What's good
None of the five findings is a blocker on the classification logic itself, which reads correct. (1) and (2) are the two I'd want addressed before merge — the help currently misleads half its readers, and |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Review of #1123 caught that the credential check replaced one way for a third-party blip to fail a pipeline with another. VerifyCredentials returned an opaque error for three different outcomes - rejected, unreachable, 5xx - and the caller aborted before reporting on all of them. A commit referencing a genuinely absent PROJ-9999 plus one failed /myself call now exited non-zero and recorded nothing, where before it reported the issue as non-existing and exited 0. Only a negative answer is worth failing on. VerifyCredentials returns *CredentialNotVerifiedError when Jira refuses the credential (401/403) or answers in a way that cannot be read as verification (404); anything else stays a plain error. The caller matches with errors.As: a negative answer aborts, and a check that did not complete is logged as a warning and the attestation still reported. A type rather than a sentinel because the wording has to differ. A 404 also comes back from a base URL that is not a Jira, so pointing only at the credential there would send the reader off to rotate a token that was never the problem: that message names both candidates. Its comment no longer asserts that Jira answers /myself with 404 for rejected credentials - we have not observed that, and the branch is defensive. Also fixes the help text, which still described the old behaviour ("the attestation is reported in all cases", wrong credentials "reported as non existing Jira issue"), and replaces the start-a-server-then-close-it trick in the unreachable-Jira tests with port 0, which cannot be claimed by another package's tests mid-run. The two CLI-level cases move out of AttestJiraCommandTestSuite, whose SetupTest skips without the shared Jira secrets: both stub Jira, one stubs the attestation endpoint too, so they need neither the secrets nor the local Kosli server and run wherever `go test` does.
Second review round caught that the 404 branch broke this PR's own rule. "Only a rejected credential stops the attestation" was applied to the one status that is not a rejection, and the one that had never been observed: a 404 hard-failed the run while a real, observed infrastructure failure - 503, unreachable - only warned. Everything that realistically answers /myself with 404 is shaped like a misconfiguration rather than a credential: a base URL that is not a Jira, one missing a Data Center context path, a proxy or SSO gateway that 404s paths it does not recognise. None of them say anything about the credential, which is the criterion the other branches use, and a proxy that started 404ing would fail pipelines over issues that really are missing - the failure this PR removed, re-entering through its most speculative branch. So 404 joins the plain-error bucket: the caller warns, names --jira-base-url as a candidate, and still reports the 404 the issue endpoint gave. Only 401 and 403, where Jira looked at the credential and said no, stop the attestation. The type is CredentialRejectedError accordingly - it now means what its name says, rather than "not verified", which was covering two outcomes that want opposite treatment. Also from the round: - The warning leads with the actionable part: "could not verify the Jira credential (%s); issues Jira did not return are reported as not found". - Suite case 09 references a non-existent issue, so it takes the verification path with an exact golden; a rate-limited /myself would prepend the warning and fail it. Matched rather than compared exactly now, like cases 07 and 25. - A third stub test covers the path most at risk from the new call: credential verifies, issue genuinely missing, attestation still reported and --assert still failing on it. The suite cases for that skip without the Jira secrets. - Help text follows the behaviour: refusal fails, anything else warns.
The check was still allowed to decide an outcome: a refused credential aborted before the attestation was reported. That is the one change a user would notice, and it is not worth making - a token that lapses overnight would fail pipelines that had been passing, for a diagnosis the run can simply state. It is now diagnostic only. Every outcome of VerifyCredentials becomes one warning on stderr and nothing else: the attestation is reported as before, the exit code is untouched, and --assert still fails on the issues that were not found - with the warning explaining why they were not. So the fix keeps what it was for, telling a stale credential apart from a missing issue, and gives up the part that could break a working pipeline. CredentialRejectedError goes with it. With no caller branching on the outcome the type had no consumer, and the statuses now differ only in wording: 401 and 403 name the credential, a 404 names --jira-base-url alongside it, and the rest name the Jira. That also retires the question of which bucket a 404 belongs in, since nothing turns on it any more. Help text leads with the behaviour rather than the mechanism, and says the warning goes to stderr and does not change the exit code. Review of efb1a5f, also addressed: - GetJiraIssueInfo's default branch wrapped go-jira's message, which names neither the Jira nor the issue: on a 5xx it read as an unattributable "request failed". It now says which call it was, and the transport branch names the issue too. - The three fmt.Errorf sites use %w rather than %s, so a caller can still reach a net.Error or context.DeadlineExceeded underneath "failed to reach Jira". Tested by unwrapping to *net.OpError. - The stale-credential test used the real local Kosli server as its host while its comment claimed it needed no server, so "nothing was POSTed" rested on a string check: a regression moving the POST above the check would have attested a bogus issue_exists=false into the live attest-jira flow. All three tests now share a counting stub of the attestation endpoint and assert the request count, so what was sent is checked structurally.
The help described the credential warning in detail but not the change a user is more likely to hit: since the first commit, a Jira that cannot be reached, or that answers the issue lookup with anything other than a 404, fails the command instead of recording the issue as non existing. That is deliberate - an unanswered lookup is not evidence that an issue is missing - but it means `attest jira` can exit non-zero on a Jira outage even without `--assert`, which it could not do before, and nothing in --help said so.
Exit codes now match what they were before this branch, in every case. The first commit had made one outcome newly fatal: a Jira that never answers the issue lookup. Before, that came back as issue_exists=false and the run passed; after, it aborted with no attestation. Nothing else moved - a 401, 403 or 5xx on the lookup failed the command before this branch too, and still does. So the transport failure goes back to being non-fatal, and stops being silent, which was the actual defect. JiraIssueInfo carries LookupFailure: set when Jira never answered, excluded from the payload, and reported by the caller as a warning on stderr. IssueExists stays false and the issue still counts as not found for --assert, exactly as it always has. Keeping it a field rather than an error is the point: there is no way for a caller to accidentally turn a Jira outage into a failed pipeline, and no classification to get wrong. The cause stays wrapped, so a caller can still tell a timeout from a refused connection. Two things follow: - The credential check now runs only when Jira actually answered "not found" for something. Where nothing was answered there is no ambiguous 404 to explain, and asking again only produced a second way of saying Jira could not be reached. - The help text says which outcome does what, and no longer claims the command fails when Jira cannot be reached. TestAttestJiraUnreachable covers it end to end against port 0: no error, the attestation still reported (asserted on the stub's request count), the lookup warning on stderr, and no credential warning.
| If your Jira credentials are wrong, or ^--jira-base-url^ does not point at your Jira, the | ||
| issues are reported as non existing. This is because Jira returns the same 404 whether an | ||
| issue does not exist or your credentials may not see it. | ||
| When that happens a warning naming the likely cause is written to stderr, so a run whose | ||
| issues all came back missing says whether the credentials were the reason. The warning does | ||
| not change the exit code; ^--assert^ still fails on the issues that were not found. | ||
|
|
||
| If Jira cannot be reached at all, the issues it did not answer for are reported as non | ||
| existing, as before, with a warning on stderr saying that the lookup never got an answer. | ||
| If Jira answers the lookup with an error instead - a rejected credential, a server error - | ||
| the command fails, as it has always done for those. |
There was a problem hiding this comment.
These two paragraphs give opposite answers to the same question.
Line 72: "If your Jira credentials are wrong … the issues are reported as non existing."
Line 81: "If Jira answers the lookup with an error instead - a rejected credential, a server error - the command fails."
Both describe wrong credentials, and they promise different outcomes. Which one a user gets depends on something the help never mentions and they can't control: whether their Jira answers the issue lookup with 404 (Cloud, and the case that motivated this PR) or 401/403 (self-hosted, or an SSO gateway in front). A reader with a Data Center Jira follows paragraph one and is surprised by a non-zero exit; a reader with Cloud follows paragraph four and wonders why nothing failed.
The rest of the change is careful to key behaviour off the status rather than off a guess at the cause — the help should do the same:
| If your Jira credentials are wrong, or ^--jira-base-url^ does not point at your Jira, the | |
| issues are reported as non existing. This is because Jira returns the same 404 whether an | |
| issue does not exist or your credentials may not see it. | |
| When that happens a warning naming the likely cause is written to stderr, so a run whose | |
| issues all came back missing says whether the credentials were the reason. The warning does | |
| not change the exit code; ^--assert^ still fails on the issues that were not found. | |
| If Jira cannot be reached at all, the issues it did not answer for are reported as non | |
| existing, as before, with a warning on stderr saying that the lookup never got an answer. | |
| If Jira answers the lookup with an error instead - a rejected credential, a server error - | |
| the command fails, as it has always done for those. | |
| The found issue references will be checked against Jira to confirm their existence. | |
| The attestation is reported in all cases, and its compliance status depends on referencing | |
| existing Jira issues. | |
| Jira returns the same 404 whether an issue does not exist or your credentials may not see | |
| it - it will not confirm that an issue exists to a caller who may not see it - so wrong | |
| credentials, or a ^--jira-base-url^ that does not point at your Jira, show up as issues | |
| reported non existing. When that happens a warning naming the likely cause is written to | |
| stderr, so a run whose issues all came back missing says whether the credentials were the | |
| reason. The warning does not change the exit code; ^--assert^ still fails on the issues | |
| that were not found. | |
| If Jira cannot be reached at all, the issues it did not answer for are reported as non | |
| existing, as before, with a warning on stderr saying that the lookup never got an answer. | |
| If Jira answers the lookup with any other error - 401, 403, or a server error - the command | |
| fails, as it has always done for those. |
| // did before - with the warning now explaining why they were not. | ||
| if answeredNotFound > 0 { | ||
| if err := jc.VerifyCredentials(); err != nil { | ||
| logger.Warn("%s. Issues Jira did not return are reported as not found", err) |
There was a problem hiding this comment.
Now that the diagnosis is the only thing this change produces, --quiet deletes the whole feature.
logger.Warn returns early when QuietEnabled (internal/logger/logger.go:56-58, set from global.Quiet && !global.Debug). Both new signals go through it — this line and the per-issue LookupFailure warning at line 333 — and neither has a fallback.
So kosli attest jira --assert --quiet (a very ordinary CI invocation; -q is one character) with an expired token fails with exactly the pre-PR output:
Error: missing Jira issues from references found in commit message or branch name
EX-1: issue not found
That's the incident this PR was written for, reproduced in full. And in the unreachable case with --quiet there is now no output at all about Jira being down — the run passes and silently attests issue_exists=false, which is the behaviour the first commit set out to stop being silent about.
--quiet is documented as suppressing non-critical warnings, and "the reason your compliance data is wrong" isn't that. issueLog is built a few lines up and always reaches the user when --assert fails, so carrying the diagnosis into it costs little and survives -q:
credentialNote := ""
if answeredNotFound > 0 {
if err := jc.VerifyCredentials(); err != nil {
logger.Warn("%s. Issues Jira did not return are reported as not found", err)
credentialNote = fmt.Sprintf("\n\t%s", err)
}
}…appended to issueLog in the assert branch at line 403. The same applies to LookupFailure: fmt.Sprintf("\n\t%s: %s", result.IssueID, issueExistLog) could say issue not found (lookup unanswered) rather than relying solely on a suppressible warning.
| // unchanged, so a stale token cannot fail a pipeline that would have passed, and neither | ||
| // can a blip on this call. --assert still fails on the issues that were not found, as it | ||
| // did before - with the warning now explaining why they were not. | ||
| if answeredNotFound > 0 { |
There was a problem hiding this comment.
answeredNotFound > 0 narrows the guard usefully — the unreachable case no longer asks twice — but it still fires when a lookup already came back 200, and a single 200 proves the credential exactly as conclusively as ten do.
The mixed case isn't hypothetical; it's the false-positive shape this command's own help calls out at lines 55–62. see note-1 for context, fixes PROJ-42 looks up NOTE-1 and PROJ-42: PROJ-42 → 200, NOTE-1 → 404, and the CLI still spends a round-trip on /myself. Since the credential just demonstrably worked, the call can only return nil (no value) or blip — and a blip prints "could not check the Jira credential … Issues Jira did not return are reported as not found", casting doubt on a result the 200 already settled.
| if answeredNotFound > 0 { | |
| if issueFoundCount == 0 && answeredNotFound > 0 { |
Note the comment above already states the stronger rule — "where every reference resolved the credential has already proved itself" — so this is the code catching up with the reasoning. A stub test with one 200 and one 404 asserting no credential warning would pin it; TestAttestJiraVerifiedCredentialMissingIssue is nearly that test already.
| // LookupFailure is set when Jira never answered the lookup at all, so IssueExists is | ||
| // false because nothing was learned rather than because the issue is absent. It is | ||
| // reported rather than returned as an error, so that an unreachable Jira cannot fail a | ||
| // run that would otherwise have passed - the exit code stays what it was before this | ||
| // distinction existed. Excluded from the payload: it describes this attempt, not the | ||
| // issue, and the cause stays wrapped so a caller can still tell a timeout from a | ||
| // refused connection. | ||
| LookupFailure error `json:"-"` |
There was a problem hiding this comment.
Worth naming the residual gap the json:"-" creates, since it's the one place the PR title's promise doesn't reach.
An unanswered lookup still POSTs issue_exists: false to the trail, so the durable record — the thing someone reads back in six months, and the thing the compliance status is computed from — still says "this commit references an issue that does not exist", when in fact nobody ever asked. The correction lives only in a stderr warning that the CI log rotates away (and that --quiet drops outright).
Keeping the exit code unchanged is clearly right. But "don't fail the run" and "record a fact we didn't establish" are separable, and the second is the more permanent of the two. If the attestation schema has room for it — a lookup_failed alongside issue_exists, or omitting the unanswered entries from JiraResults — that would close the loop. If it doesn't, that's a fine answer too; it's a server-side conversation rather than something to hold this PR for. Either way the tradeoff deserves a sentence in this comment, because the current wording ("reported rather than returned as an error") reads as though the caller receives it, and the only caller that does is stderr.
| // would quietly attest a bogus issue_exists=false into the real attest-jira flow, and only | ||
| // then fail on a missing string. | ||
| func newStubKosliAttestation(t *testing.T) (*httpfake.HTTPFake, *atomic.Int32) { | ||
| t.Helper() |
There was a problem hiding this comment.
Nit, carried over from the last round: t is taken only for t.Helper(), and all four callers still write their own defer kosliStub.Close(). t.Cleanup(fake.Close) here would earn the parameter and drop a line from each caller.
Also the org/flow/trail are spelled out here and rebuilt in every caller's --flow/--org/--trail args. It fails loudly if they drift (the POST 404s, reported stays 0), so it's not a correctness risk — but a shared const would keep the two definitions honest.
|
Closed in favour of #1125 |
2 participants
GetJiraIssueInfo treated anything short of a non-404 HTTP response as "the issue
does not exist".
When go-jira returns an error with no response at all - DNS failure, TLS error,
timeout, connection refused - that guard is false, issue is nil, and the function
returns IssueExists=false with a nil error. A network blip was therefore reported
to the user as a missing Jira issue, and under
attest jira --assertit failedtheir pipeline with a message pointing at their commit's Jira references rather
than at the network.
Classify the outcome instead. A 404 is the only status that answers the question,
and stays a missing issue. A nil response is a reachability error naming the base
URL; 401 and 403 name the credential in play, since "check the API token of user
x" is the actionable part and go-jira's own message carries neither; any other
status keeps the error go-jira returned.
Tests cover each branch against a stubbed Jira, including the transport failure
that returned no error at all before this change.
Follow-up from #1118 troubleshooting issues
Checklist
charts/k8s-reporter/) updated, if needed. Note: these changes live in a separate PR