[doc] HLD: Static analysis CI gates for SONiC repositories - #2503
Open
bhouse-nexthop wants to merge 11 commits into
Open
[doc] HLD: Static analysis CI gates for SONiC repositories#2503bhouse-nexthop wants to merge 11 commits into
bhouse-nexthop wants to merge 11 commits into
Conversation
Adds a high-level design for a static analysis merge gate covering Python, C/C++, Rust, Go and shell across SONiC's own repositories, wired into the Azure pipelines each repository already runs rather than into new CI jobs. SONiC has no working static analysis gate today. The one lint check that exists (flake8 in sonic-utilities) is declared continueOnError, and the CodeQL and Semgrep workflows report without gating: sonic-buildimage has over 3,100 open CodeQL alerts, 98% of them code quality rather than security, and the Semgrep workflow has failed 100 of its last 100 pushes to master with 338 blocking findings nobody acts on. Measured against the current tree, that has left real defects in place -- 445 undefined-name findings that are latent NameError crashes in platform error paths, and 56 files still written in Python 2 that raise SyntaxError on import under any supported image. The design proposes a shared sonic-ci repository holding the analyzer configuration and one manifest deciding which rules block a merge, so each repository carries about ten lines. Rules either block anywhere in the repository, or block only defects a pull request introduces -- measured by comparing findings against the target branch rather than by filtering on changed lines, which testing showed misses regressions whose symptom lands on an untouched line. Also adds doc/security/README.md indexing this directory and pointing at the existing security-related HLDs elsewhere in doc/. Signed-off-by: Brad House <bhouse@nexthop.ai> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Collaborator
|
/azp run |
One or more co-authors of this pull request were not found. You must specify co-authors in commit message trailer via: Supported
Alternatively, if the co-author should not be included, remove the Please update your commit message(s) by doing |
|
Commenter does not have sufficient privileges for PR 2503 in repo sonic-net/SONiC |
The design described how a pull request consumes the target-branch baseline but not how that baseline comes to exist, which left the failure modes unspecified. Adds: baseline mode runs on every merge to the target branch with all rules enabled and nothing gating; pull requests download the most recent completed artifact, so a PR still costs one analysis pass rather than two. A weekly scheduled run repairs the cases a merge-triggered baseline misses -- a branch with no recent merges, a build that failed before publishing, and analyzer behaviour that changes without the code changing. sonic-swss, sonic-sairedis and sonic-swss-common already carry a suitable weekly cron; sonic-utilities needs one added. Also specifies what happens when the baseline is missing, stale, or was produced under different analyzer versions or a different rule set. In each case the regression-only rules degrade to advisory and say why, while repo-wide gating rules are evaluated from the pull request's own findings and are unaffected -- so an unusable baseline can never let a gating defect through, and can never block a pull request spuriously. Signed-off-by: Brad House <bhouse@nexthop.ai> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Collaborator
|
/azp run |
|
No pipelines are associated with this pull request. |
Two gaps. Fingerprinting was asserted rather than specified. Now gives the exact key -- path, rule, hash of the stripped source line, and an occurrence counter for repeats -- states that it must be computed during analysis rather than reconstructed from stored line numbers, and covers the cases that were previously unaddressed: multi-line findings, findings with no source line, and the same header analysed through several source files. Behaviour is tabulated against measured results: 20 lines inserted above four findings reports 0 new, re-indenting reports 0, deleting one of four identical findings reports 0, and a genuine new defect reports 1. Second, the design replaced tools SONiC already runs without saying what that changed. Adds 7.4.6 accounting for all three: sonic-utilities' flake8 (line-length 120 and its default ignore set are carried across; W503/W504 are dropped as no-ops since ruff does not implement them; C90 was never active), sonic-mgmt-framework's pylint (unchanged -- it is a build target -- with ten of its eleven checks mapped to ruff equivalents and W0621 noted as having none), and sonic-dash-ha's pre-commit config. That last one exposed a real gap: it covers file hygiene, config-file validity, private-key detection, actionlint and cargo fmt, none of which this design had. Those are cheap and worth having everywhere, so they become two shared entries in the manifest rather than staying in one repository. Also notes that the existing flake8 hook diffs HEAD^..HEAD, so on a multi-commit pull request it only ever lints the final commit. Baseline comparison covers the whole pull request. Signed-off-by: Brad House <bhouse@nexthop.ai> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Collaborator
|
/azp run |
|
No pipelines are associated with this pull request. |
Go linters are noisy by default, and measurement confirms it. Running govet, staticcheck, errcheck, ineffassign, unused and revive over the 27 sonic-gnmi packages that build without cgo produced 122 findings, of which errcheck and revive accounted for 100. Most of that is not defect: 35 of revive's 50 are missing doc comments, and 29 of errcheck's 50 are unchecked Close/Flush/Sync on cleanup paths. Enabling that set as-is would bury 22 useful findings -- including SA5011 nil dereferences and unchecked json.Unmarshal, io.Copy and os.Remove -- under a hundred nobody reads, which is how linters end up switched off. The configuration now disables revive's documentation rules and excludes the conventional errcheck functions, taking 122 raw findings to roughly 43 actionable ones without losing a single staticcheck, govet, unused or ineffassign result. Separately, section 4.2 presented Python lint findings as the evidence for a document filed under doc/security/. Those findings are real, but they are crashes and logic errors, not vulnerabilities, and ruff would not catch a security issue in the first place. The section now separates the two: the security argument rests on C/C++ memory safety across ~1,700 files that have never been analysed, and is explicitly stated as not yet measured because it needs the build environment the pilot provides. What has been measured is labelled as correctness, and the Go sample is included so the evidence is not Python-only. Also drops the downstream-fork requirement and its test case. Signed-off-by: Brad House <bhouse@nexthop.ai> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Collaborator
|
/azp run |
|
No pipelines are associated with this pull request. |
…agram Local reproduction was referenced in five places -- a requirement, a filename, two example commands and a test -- but never designed, so the hard parts were unanswered: how a developer gets analyzers that live in the slave image, how C/C++ analysis works without a compilation database, and how anyone compares against a baseline that exists only as a pipeline artifact. Section 7.9 answers those. sonic-analyze bootstraps the pinned tool versions locally, so a developer cannot accidentally run a different ruff than CI does. Python, shell, Rust and the hygiene checks need no build. C/C++ and Go do, and the document says so plainly, giving both the local path (bear + make) and the container path rather than pretending a compiled language can be linted without compiling. A --since flag reconstructs a baseline from the merge base for developers who want to see specifically what their branch introduces, opt-in because it costs a second pass. Per-repository documentation is now part of adoption rather than a follow-up: the right commands differ by repository, so each Phase 2 PR adds a local-usage section to that repository's README, and a repository is not considered adopted until it exists. Adds R6a and ST10/ST11. Also replaces the architecture diagram. The old one showed one repository being referenced by three others, which the surrounding sentence already said. The new one shows the mechanism that is actually non-obvious: master publishing a baseline, a pull request downloading it, and the three rule scopes resolving against the comparison. Signed-off-by: Brad House <bhouse@nexthop.ai> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The argument for comparing finding sets rather than filtering on changed lines was a bold lead buried inside 7.3.1, so it did not appear in the table of contents and was easy to miss. It is the one part of this design that is genuinely counter-intuitive -- filtering on the diff is the obvious implementation and it silently misses regressions -- so it needs to be findable. Splits that material into four numbered sections: gate scope, why baseline differencing, fingerprinting, and producing and consuming the baseline. All four now appear in the table of contents. 7.3.1 closes with a pointer into the three that follow. Also folds away the "Known behaviours" list, which had become mostly a restatement of the fingerprinting and baseline sections written above it. The two items that were not duplicated -- baseline drift and the reporting of fixed findings -- are kept. Signed-off-by: Brad House <bhouse@nexthop.ai> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Collaborator
|
/azp run |
|
No pipelines are associated with this pull request. |
…lot repos Reordering the trixie migration to follow the pilot means sonic-swss can no longer be a pilot repository, since it is one of the pipelines being migrated. Choosing replacements surfaced a constraint worth recording. Surveying which slave image each repository actually uses shows three groups, not two. Six repositories already run trixie and can adopt immediately. Four are parameterised and default to bookworm while already running trixie jobs alongside. Ten pin sonic-slave-bookworm directly and need a real port -- a larger set than the five previously identified, because repositories with a hardcoded image do not carry a debian_version parameter to flip. The pilot is now sonic-utilities, sonic-gnmi and dhcpmon, all of which run on trixie today. sonic-utilities is unaffected either way: its analysis runs in a standalone stage with no container. Two gaps follow from the reordering and are stated rather than papered over. Rust cannot be piloted at all, because every Rust repository is awaiting migration. C/C++ can only be piloted at dhcpmon's 9-file scale, against sonic-swss's 564, so the wall-clock and peak-RSS figures R11 depends on are not produced by the pilot. Both close in a new Phase 3, where sonic-swss adopts C/C++ and Rust together and produces those measurements. Fleet rollout is gated on Phase 3 rather than on Phase 1, so nothing rolls out fleet-wide on unmeasured cost. Python 2 removal and rule promotion renumber to Phases 5 and 6. Signed-off-by: Brad House <bhouse@nexthop.ai> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Collaborator
|
/azp run |
|
No pipelines are associated with this pull request. |
The three sections covering how a pull request's findings are told apart from pre-existing ones were subsections of the severity manifest, which made them look like details of rule configuration. They are one inter-related argument and belong at the same level as the manifest itself. Adds 7.4 "Deciding what a pull request introduced" as a peer of 7.3, holding what were 7.3.2 through 7.3.4: why baseline differencing beats changed-line filtering, how findings are matched across revisions, and where the comparison point comes from. Its introduction states why the three are read together. 7.3.1 keeps the scope definition, since that is part of the manifest, and now carries a one-line pointer instead of the paragraph that duplicated the new introduction. Everything from the old 7.4 onward shifts by one: per-language design is now 7.5, suppressions 7.6, build dependency 7.7, rollout 7.8, items not changed 7.9, and local use 7.10. All cross-references and the table of contents are updated. Signed-off-by: Brad House <bhouse@nexthop.ai> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Collaborator
|
/azp run |
|
No pipelines are associated with this pull request. |
Investigating the concern behind deferring the trixie work shows it applies to sonic-swss-common but not to sonic-swss, and that the design had conflated three different actions. Attaching the gate to an existing trixie job is blocked by nothing. Flipping the debian_version default is blocked by nothing as long as the bookworm job still publishes. Only retiring the bookworm jobs is disruptive, and only for repositories whose bookworm artifact something else still fetches. sonic-swss-common-bookworm is fetched by four repositories -- sonic-dash-ha, sonic-bmp, sonic-utilities and linkmgrd -- so its retirement waits on them. sonic-swss-bookworm has no external consumers at all. The docker layer coupling is real but lives inside sonic-buildimage: docker-swss-layer-bookworm.mk and docker-swss-layer-trixie.mk both depend on the same $(SWSS), and which is built is selected by BLDENV from the slave container buildimage itself runs in. sonic-buildimage also builds swss from submodule source rather than from its published artifact, so no submodule pipeline setting reaches it. sonic-swss therefore returns to the pilot, attaching to the BuildTrixie stage it already runs on every pull request. That restores C/C++ at 564-file scale and Rust coverage, so the wall-clock and peak-RSS figures R11 depends on come from the pilot again rather than from a later phase. dhcpmon stays as a second, smaller C/C++ case. Retiring the bookworm jobs becomes Phase 2, ordered by who still fetches what. Fleet, Python 2 removal and promotion renumber to Phases 3, 4 and 5. Notes that BuildTrixie is chained behind Build and BuildArm in both repositories, so a gate attached there reports late and is skipped if an earlier stage fails. Signed-off-by: Brad House <bhouse@nexthop.ai> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Collaborator
|
/azp run |
|
No pipelines are associated with this pull request. |
The case is that these files cannot run on any supported image, so deleting them loses nothing. That did not need a paragraph establishing they are non-functional, a second one citing the cavium removal as precedent and arguing SONiC has no deprecation process to follow, and four bullets prescribing what each PR description must contain. Cuts Phase 4 from roughly thirty lines to eight, keeping the vendor grouping and the backlog figure, and trims the same reasoning where it was restated in Appendix B and Section 10. Signed-off-by: Brad House <bhouse@nexthop.ai> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Collaborator
|
/azp run |
|
No pipelines are associated with this pull request. |
The design had gate-new fall back to advisory when the baseline was missing or unusable, and claimed as a safety property that the gate never depends on the baseline being available. That was wrong in two ways. It is inconsistent with how SONiC pipelines already behave. The DownloadPipelineArtifact tasks that fetch libswsscommon, sairedis and libnl carry no failure tolerance, and allowPartiallySucceededBuilds only relaxes which build to source from, not whether the artifact must exist. A missing build asset already fails the job everywhere else. More importantly it was a hole. A broken baseline job would quietly switch off a whole class of enforcement with nothing failing to indicate it, which is the failure mode this design exists to remove. The baseline is now treated as a build dependency like any other: absent or incomparable means the job fails. The two situations that could produce that are handled by sequencing instead. A repository adopts in two steps -- repo-wide gate rules first, which need no baseline, then gate-new once the branch build has published one -- so the "no baseline yet" case does not arise in steady state. A version or rule-set mismatch fails and reports that the target branch must rebuild, a window of one build cycle after a sonic-ci tag bump. Updates UT13 and UT14, and the pilot and fleet criteria to require both adoption steps. Signed-off-by: Brad House <bhouse@nexthop.ai> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Collaborator
|
/azp run |
|
No pipelines are associated with this pull request. |
nh-grecs Bot
pushed a commit
to nexthop-ai/SONiC
that referenced
this pull request
Aug 23, 2026
rebuild-source: sonic-net#2503 @ bhouse-nexthop/SONiC 4f61909 [case: upstream:open]
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request Template
What is being proposed
A high-level design for a static analysis merge gate across SONiC's own repositories, covering Python, C/C++, Rust, Go and shell.
The analysis runs inside the Azure pipelines each repository already has — extra steps in the existing build job for compiled languages, and a small standalone stage for Python and shell, which need no build. No new CI system, no new build agents.
Rendered:
doc/security/static-analysis-ci-hld.mdThis is a design document only. No code, pipeline, or build change is included.
Why
SONiC has no working static analysis gate. The only lint check in the tree,
flake8insonic-utilities, is declaredcontinueOnError: trueand cannot block a merge. The CodeQL and Semgrep workflows report but do not gate, and the evidence that nobody acts on them is unambiguous:sonic-buildimagehas over 3,100 open CodeQL alerts. Of the first 3,000 sampled, 50 carry asecuritytag and 2,950 do not. CodeQL is also configuredlanguage: [ 'python' ]in every repository that has it, includingsonic-swssandsonic-sairedis— so it analyses none of their ~1,150 C/C++ files.semgrep cidiff-scopes on PR events.The document is careful to separate two claims that are easy to conflate, and §4.2 says so explicitly:
The security argument rests on C/C++, and is not yet measured. Memory safety across ~1,700 files in
sonic-swss,sonic-sairedis,sonic-swss-common,linkmgrdand others — none of which has ever been run through a static analyzer — is what a security-motivated gate is actually for. Producing those numbers needs the full build environment, which is what the pilot provides. That measurement is a pilot exit criterion, not a claim made up front.What has been measured is correctness, not security. 445 latent
NameErrorsites (raw_input,unicode, an unimportedsyslog) clustered in platform error paths; 56 files still written in Python 2 that raiseSyntaxErroron import and cannot have worked since 202012; and on the Go side, nil dereferences and uncheckedjson.Unmarshal/io.Copy/os.Remove. Real defects, but crashes and logic errors rather than vulnerabilities.How it works
A shared
sonic-cirepository holds the analyzer configuration and one manifest deciding what blocks a merge. Consuming repositories carry ~10 lines, pinned by tag, and never edit them again to change rules or tool versions.Rules fall into three scopes:
gate— blocks anywhere in the repository. A small, high-confidence set whose backlog is cleared during rollout.gate-new— blocks only defects a pull request introduces. This is what makes a 35,862-finding backlog survivable without a cleanup nobody would do.advisory— reported, never blocks."Introduced" is decided by comparing finding sets against a baseline the target branch publishes, not by filtering on changed lines. §7.4.1 shows why with a worked case: removing a null check reports the dereference several lines away, on a line the PR never touched, so a line filter waves a real null dereference through. Findings are matched by content fingerprint so they survive line shifts and re-indentation; the baseline is produced on every merge and consumed via the same artifact mechanism pipelines already use for
libswsscommonandsairedis, so a PR still costs one analysis pass rather than two.Design decisions backed by measurement
clang-tidyrather thanscan-build— 119 of its checks are the Clang Static Analyzer, verified to reproduce the identical path-sensitive diagnostics, plus ~419 pattern checksscan-buildhas no equivalent for. Same cost, strict superset.sonic-gnmipackages that build without cgo produced 122 findings, of whicherrcheckandrevivewere 100 — 35 of those merely missing doc comments, 29 uncheckedClose/Flushon cleanup paths. Disabling revive's documentation rules and excluding the conventional errcheck functions yields ~43 actionable findings while losing nostaticcheck,govet,unusedorineffassignresult.sonic-utilities' flake8 line-length and ignore set carry across toruff;sonic-mgmt-framework's pylint keeps running because it is a build target;sonic-dash-ha's pre-commit config — file hygiene,detect-private-key,actionlint,cargo fmt— is generalised into the shared set rather than left in one repository.Developer experience
Everything runs locally through the same entry point CI uses, reading the same manifest, with the pinned tool versions bootstrapped automatically. C/C++ and Go need a build, and the document says so plainly rather than pretending otherwise, giving both the local and container paths. Per-repository instructions land with each adoption PR — a repository is not considered adopted until its README section exists.
Scope and rollout
masteronly; release branches are not backported to. Vendor-hosted platform submodules and vendored upstream projects (FRR, scapy, ptf) are excluded — the community cannot merge changes there.Five phases: foundation; a four-repository pilot (
sonic-utilities,sonic-gnmi,sonic-swss,dhcpmon), all of which already run trixie so none waits on a migration; retiring the bookworm jobs, ordered by which artifacts other repositories still fetch; fleet rollout, gated on the pilot's cost measurements being within budget; Python 2 removal; then rule promotion.Review notes
The sections most worth scrutiny:
Also adds
doc/security/README.mdindexing the directory and cross-referencing the security HLDs already elsewhere indoc/. Those are not moved.