Fix/pr 567 issue 682 followup by yahlifried · Pull Request #727 · openperouter/openperouter

yahlifried · 2026-09-03T10:28:26Z

fixing some leftover comments from this PR #567,
fixes #682

/kind cleanup

What this PR does / why we need it:

Special notes for your reviewer:

NONE

AI Guidelines Acknowledgment:

I have reviewed all changes in this PR, including any AI-generated content, and I take full responsibility for its accuracy and correctness.

Summary by CodeRabbit

Security
- Sensitive BGP and FRR passwords are now redacted in debug logs and configuration-related output.
- Passwords embedded in raw FRR configuration snippets are also protected from accidental exposure.
Validation
- Duplicate neighbor identities are rejected during configuration validation.
- Invalid neighbor passwords are reported more comprehensively, while valid neighbors can still be retained.
- Plaintext passwords take precedence over password references, and associated secret references are cleared when applicable.

Restore the FRR config debug log using a redacted copy, preserving debugging value without exposing resolved BGP passwords or raw FRR credentials. Resolves review threads openperouter#44 and openperouter#45. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: yahlifried <yfriedla@redhat.com>

Route API and static config debug payloads through the shared redaction helpers so resolved BGP passwords and raw FRR credentials are never logged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: yahlifried <yfriedla@redhat.com>

Reject cross-field NeighborID collisions during common underlay validation while preserving the existing specific listen-range errors. Resolves review threads openperouter#64 and openperouter#43. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: yahlifried <yfriedla@redhat.com>

Move password-secret clearing into static underlay construction, retain plaintext precedence, and rely on common reconciliation validation for duplicate identities. Resolves review threads openperouter#26, openperouter#30, openperouter#31, openperouter#57 and openperouter#59. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: yahlifried <yfriedla@redhat.com>

Deep-compare all surviving neighbors and resolved passwords, use specific error assertions, and cover an empty Secret value. Resolves review threads openperouter#55, openperouter#40, openperouter#56 and openperouter#47. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: yahlifried <yfriedla@redhat.com>

Keep the shared password-secret fixture constants together. Follow-up to review thread openperouter#71. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: yahlifried <yfriedla@redhat.com>

coderabbitai · 2026-09-03T10:29:14Z

Warning

Review limit reached

Next included review available in 18 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Team

Run ID: 69fa3533-30f7-4239-a36e-beaa0ddf7882

📥 Commits

Reviewing files that changed from the base of the PR and between 9f6ade3 and 638e82c.

📒 Files selected for processing (2)

internal/frr/config.go
internal/frr/config_redact_test.go

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Team

Run ID: 0d72eec9-3358-4ed7-a771-cbae2b244556

📥 Commits

Reviewing files that changed from the base of the PR and between c866030 and 9f6ade3.

📒 Files selected for processing (12)

e2etests/tests/password_secret.go
internal/controller/routerconfiguration/frr.go
internal/controller/routerconfiguration/password_secret_test.go
internal/controller/routerconfiguration/static_configuration_reader.go
internal/controller/routerconfiguration/static_configuration_reader_test.go
internal/controller/routerconfiguration/underlay_vni_controller.go
internal/conversion/api.go
internal/conversion/api_test.go
internal/conversion/validate_underlay.go
internal/conversion/validate_underlay_test.go
internal/frr/config.go
internal/frr/config_redact_test.go

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.

📝 Walkthrough

Walkthrough

The changes redact passwords from configuration logs, relocate duplicate and empty-neighbor validation to conversion, clear conflicting static secret references, and expand password-resolution coverage with exact state and error assertions.

Changes

Configuration safety

Layer / File(s)	Summary
Redacted configuration logging `internal/conversion/api.go`, `internal/frr/...`, `internal/controller/routerconfiguration/...`	New redaction helpers protect resolved and raw FRR passwords in logs. FRR password matching now handles additional formatting.
Static neighbor conversion and validation `internal/controller/routerconfiguration/static_configuration_reader.go`, `internal/conversion/validate_underlay.go`, `internal/.../*_test.go`	Plaintext passwords clear `PasswordSecret`. Conversion validation rejects duplicate and empty neighbor identities.
Password resolution behavior `internal/controller/routerconfiguration/password_secret_test.go`, `e2etests/tests/password_secret.go`, `internal/controller/routerconfiguration/underlay_vni_controller.go`	Tests compare complete resolved state and exact errors. Shared test constants and password precedence documentation were updated.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Merge Risk: ⚪ Minimal · up to 9f6ad

This change redacts BGP credentials in configuration logging, validates invalid neighbor identities, and preserves documented static-password behavior. The supplied coverage indicates these changes are ready to merge without an identified current production risk.

Sequence Diagram(s)

sequenceDiagram
  participant Controller
  participant Conversion
  participant FRR
  Controller->>Conversion: redact API configuration
  Conversion->>FRR: redact raw FRR passwords
  FRR-->>Conversion: return redacted snippets
  Conversion-->>Controller: return redacted configuration for logging

Suggested reviewers: maiqueb, andreaskaris, fedepaol

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 17.24% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 29 functions across 12 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check	❓ Inconclusive	The title identifies the follow-up PR and issue numbers, but it does not describe the primary changes, such as password redaction and duplicate-neighbor validation.	Use a concise descriptive title, such as "Redact sensitive logs and validate duplicate neighbors".

✅ Passed checks (3 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The changes address the coding objectives in issue [`#682`], including review follow-ups for credential redaction, duplicate-neighbor validation, static underlay handling, password-secret resolution, an…
Out of Scope Changes check	✅ Passed	The code and test changes remain within the cleanup and review-follow-up scope of issue [`#682`]. The added validation, redaction, static configuration, and test updates directly support the stated obje…
Description check	✅ Passed	The description includes the cleanup classification, the follow-up purpose, issue reference, release note, and AI acknowledgment. The main description section is empty, but the opening text provides t…

Full details: Linked Issues check

Explanation

The changes address the coding objectives in issue [#682], including review follow-ups for credential redaction, duplicate-neighbor validation, static underlay handling, password-secret resolution, and related tests. The static secret representation is raised for discussion rather than stated as a required implementation.

Full details: Out of Scope Changes check

Explanation

The code and test changes remain within the cleanup and review-follow-up scope of issue [#682]. The added validation, redaction, static configuration, and test updates directly support the stated objectives.

Full details: Description check

Explanation

The description includes the cleanup classification, the follow-up purpose, issue reference, release note, and AI acknowledgment. The main description section is empty, but the opening text provides the essential context.

✨ Finishing Touches 💡 1

🛠️ Fix failing CI checks 💡

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

Redact the complete password directive rather than only its first token, so quoted or whitespace-containing raw-FRR values cannot leak through logs. Handle case-insensitive command and diff formats, and document static-password precedence and the Linux MD5 key limit. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: yahlifried <yfriedla@redhat.com>

yahlifried and others added 6 commits September 1, 2026 12:57

test(e2e): hoist custom key constant

80bb2a8

Keep the shared password-secret fixture constants together. Follow-up to review thread openperouter#71. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: yahlifried <yfriedla@redhat.com>

github-actions Bot added kind/cleanup and removed kind/cleanup labels Sep 3, 2026

yahlifried mentioned this pull request Sep 3, 2026

fix: redact BGP passwords in logs and implement passwordSecret #567

Merged

1 task

github-actions Bot added kind/cleanup and removed kind/cleanup labels Sep 3, 2026

yahlifried force-pushed the fix/pr-567-issue-682-followup branch from 9f6ade3 to 638e82c Compare September 3, 2026 11:09

Navigation Menu

Uh oh!

Fix/pr 567 issue 682 followup - #727

Fix/pr 567 issue 682 followup#727
yahlifried wants to merge 7 commits into
openperouter:mainfrom
yahlifried:fix/pr-567-issue-682-followup

yahlifried commented Sep 3, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Sep 3, 2026 •

edited

Loading

Review limit reached

Walkthrough

Changes

Sequence Diagram(s)

❌ Failed checks (1 warning, 1 inconclusive)

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

yahlifried commented Sep 3, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Sep 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review limit reached

Walkthrough

Changes

Sequence Diagram(s)

❌ Failed checks (1 warning, 1 inconclusive)

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

yahlifried commented Sep 3, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Sep 3, 2026 •

edited

Loading