Skip to content

Fix unicast authorizer for observer node#8547

Open
zhangchiqing wants to merge 1 commit intomasterfrom
leo/fix-unicast-for-observer
Open

Fix unicast authorizer for observer node#8547
zhangchiqing wants to merge 1 commit intomasterfrom
leo/fix-unicast-for-observer

Conversation

@zhangchiqing
Copy link
Copy Markdown
Member

@zhangchiqing zhangchiqing commented Apr 21, 2026

The changes from this PR breaks for the observer node receiving block responses from access node.

This PR fix is by allowing messages to be sent to observer node.

Summary by CodeRabbit

  • Chores
    • Updated network authorization configuration with conditional logic for observer mode. Authorization behavior for network communication streams has been adjusted based on operational mode.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 21, 2026

📝 Walkthrough

Walkthrough

The pull request adds conditional authorization configuration for unicast stream senders in the Flow network initialization. When observer mode is enabled, the authorizer defaults to always-authorized sender role; otherwise it falls back to nil for default authorization behavior.

Changes

Cohort / File(s) Summary
Network Configuration
cmd/scaffold.go
Added import for github.com/onflow/flow-go/network/message and configured underlay.NetworkConfig.UnicastStreamAuthorizer with conditional logic based on fnb.ObserverMode to use either AlwaysAuthorizedUnicastSenderRole or nil.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

  • #8526: Modifies the same UnicastStreamAuthorizer configuration with identical import, suggesting related network authorization changes.

Suggested reviewers

  • fxamacker
  • peterargue

Poem

🐰 A tiny hop in network's flow,
Observer mode now set just so,
Authorization rules refined and clean,
The safest senders ever seen! ✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Fix unicast authorizer for observer node' directly and specifically describes the main change in the pull request, which configures the unicast stream authorizer conditionally for observer nodes.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch leo/fix-unicast-for-observer

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 and usage tips.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 21, 2026

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Scanned Files

None

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Apr 21, 2026

Codecov Report

❌ Patch coverage is 0% with 6 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
cmd/scaffold.go 0.00% 6 Missing ⚠️

📢 Thoughts on this report? Let us know!

@zhangchiqing zhangchiqing marked this pull request as ready for review April 21, 2026 19:56
@zhangchiqing zhangchiqing requested a review from a team as a code owner April 21, 2026 19:56
@zhangchiqing zhangchiqing force-pushed the leo/fix-unicast-for-observer branch from e9d40d0 to f4dad7f Compare April 21, 2026 19:56
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (2)
cmd/scaffold.go (2)

695-701: Consider adding test coverage for the observer-mode branch.

Codecov flagged this block (6 lines) as uncovered. Given the prior regression motivating this PR (observer failing to receive block responses from access nodes), a targeted unit/integration test asserting that UnicastStreamAuthorizer resolves to AlwaysAuthorizedUnicastSenderRole when ObserverMode=true (and to nil/IsAuthorizedUnicastSenderRole otherwise) would help prevent future regressions.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@cmd/scaffold.go` around lines 695 - 701, Add unit tests covering the
observer-mode branch so UnicastStreamAuthorizer resolves correctly: write tests
that create the scaffold/config with fnb.ObserverMode=true and assert
UnicastStreamAuthorizer returns message.AlwaysAuthorizedUnicastSenderRole, and a
test with ObserverMode=false asserting it returns nil (i.e. the default
IsAuthorizedUnicastSenderRole behavior). Target the code that constructs
UnicastStreamAuthorizer (reference the symbol UnicastStreamAuthorizer and the
fnb.ObserverMode flag) and assert the resolved function identity or behavior for
both branches to ensure coverage and prevent regressions.

695-701: Optional: simplify the IIFE.

The immediately-invoked function is unnecessary — a direct value works and is easier to read. The default-fallback via NetworkConfig.Validate() (→ IsAuthorizedUnicastSenderRole) is preserved either way.

♻️ Proposed refactor
-		UnicastStreamAuthorizer: func() func(flow.Role, flow.Role) bool {
-			if fnb.ObserverMode {
-				// observer mode uses public network where peers are not authorized based on role
-				return message.AlwaysAuthorizedUnicastSenderRole
-			}
-			return nil // use default (IsAuthorizedUnicastSenderRole)
-		}(),
+		// In observer mode, peers on the public network are not authorized based on role.
+		// Otherwise leave nil so NetworkConfig.Validate() falls back to IsAuthorizedUnicastSenderRole.
+		UnicastStreamAuthorizer: func() func(flow.Role, flow.Role) bool {
+			if fnb.ObserverMode {
+				return message.AlwaysAuthorizedUnicastSenderRole
+			}
+			return nil
+		}(),

Or, hoist to a local variable before the struct literal for maximum clarity:

var unicastAuth func(flow.Role, flow.Role) bool
if fnb.ObserverMode {
    unicastAuth = message.AlwaysAuthorizedUnicastSenderRole
}
// ... then use unicastAuth in the NetworkConfig literal.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@cmd/scaffold.go` around lines 695 - 701, Replace the unnecessary
immediately-invoked function for UnicastStreamAuthorizer with a direct value or
a precomputed local variable: check fnb.ObserverMode and set a local unicastAuth
variable to message.AlwaysAuthorizedUnicastSenderRole when true (leave it nil
otherwise), then assign unicastAuth to UnicastStreamAuthorizer in the
NetworkConfig literal so NetworkConfig.Validate() still falls back to
IsAuthorizedUnicastSenderRole by default.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@cmd/scaffold.go`:
- Around line 695-701: Add unit tests covering the observer-mode branch so
UnicastStreamAuthorizer resolves correctly: write tests that create the
scaffold/config with fnb.ObserverMode=true and assert UnicastStreamAuthorizer
returns message.AlwaysAuthorizedUnicastSenderRole, and a test with
ObserverMode=false asserting it returns nil (i.e. the default
IsAuthorizedUnicastSenderRole behavior). Target the code that constructs
UnicastStreamAuthorizer (reference the symbol UnicastStreamAuthorizer and the
fnb.ObserverMode flag) and assert the resolved function identity or behavior for
both branches to ensure coverage and prevent regressions.
- Around line 695-701: Replace the unnecessary immediately-invoked function for
UnicastStreamAuthorizer with a direct value or a precomputed local variable:
check fnb.ObserverMode and set a local unicastAuth variable to
message.AlwaysAuthorizedUnicastSenderRole when true (leave it nil otherwise),
then assign unicastAuth to UnicastStreamAuthorizer in the NetworkConfig literal
so NetworkConfig.Validate() still falls back to IsAuthorizedUnicastSenderRole by
default.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 673dcc89-daaf-410e-9683-1865e4c6a67d

📥 Commits

Reviewing files that changed from the base of the PR and between a469a19 and f4dad7f.

📒 Files selected for processing (1)
  • cmd/scaffold.go

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants