Skip to content

[client] Simplify entrypoint by running netbird up unconditionally#5652

Merged
mlsmaycon merged 1 commit intomainfrom
simplify-entrypoint-startup
Mar 23, 2026
Merged

[client] Simplify entrypoint by running netbird up unconditionally#5652
mlsmaycon merged 1 commit intomainfrom
simplify-entrypoint-startup

Conversation

@lixmal
Copy link
Collaborator

@lixmal lixmal commented Mar 22, 2026

Describe your changes

  • Replace login_if_needed polling loop with unconditional netbird up call, which is a no-op when already connected

  • Remove NB_ENTRYPOINT_LOGIN_TIMEOUT env var from entrypoint and both Dockerfiles since it is no longer needed

  • Stop running STUN/TURN probes for --check startup: the relay status recorder already tracks availability in real time, and checkStartup only checks relay connectivity (not STUN/TURN)

Issue ticket number and link

Follow-up to #5650

Stack

Checklist

  • Is it a bug fix
  • Is a typo/documentation fix
  • Is a feature enhancement
  • It is a refactor
  • Created tests that fail without the change (if possible)

By submitting this pull request, you confirm that you have read and agree to the terms of the Contributor License Agreement.

Documentation

Select exactly one:

  • I added/updated documentation for this change
  • Documentation is not needed for this change (explain why)

Entrypoint behavior change only. No user-facing CLI or API changes.

Docs PR URL (required if "docs added" is checked)

Paste the PR link from https://github.com/netbirdio/docs here:

https://github.com/netbirdio/docs/pull/__

Summary by CodeRabbit

  • Chores
    • Removed login timeout environment variable from container configurations
    • Simplified startup logic to unconditionally establish connection after daemon initialization
  • Bug Fixes
    • Adjusted health-check behavior to skip probe execution during status checks

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 22, 2026

📝 Walkthrough

Walkthrough

Removed NB_ENTRYPOINT_LOGIN_TIMEOUT from both client Dockerfiles. Replaced conditional, timeout-based login_if_needed() in client/netbird-entrypoint.sh with an unconditional connect() that runs netbird up after daemon startup. client/cmd/status.go now disables probes during Status RPC by setting ShouldRunProbes to false.

Changes

Cohort / File(s) Summary
Dockerfile Configuration
client/Dockerfile, client/Dockerfile-rootless
Removed NB_ENTRYPOINT_LOGIN_TIMEOUT env var; left NB_ENTRYPOINT_SERVICE_TIMEOUT="30" unchanged.
Entrypoint Script
client/netbird-entrypoint.sh
Removed timeout-based login_if_needed() and associated conditional waiting; added connect() that unconditionally runs netbird up after daemon startup; updated main() to call connect.
Health / Status Check
client/cmd/status.go
Changed runHealthCheck to call getStatus with ShouldRunProbes=false (previously tied to isStartup); fullPeerStatus still uses isStartup.

Sequence Diagram(s)

sequenceDiagram
    autonumber
    participant Entrypoint as "Container ENTRYPOINT\n(/netbird-entrypoint.sh)" rect rgba(150,200,255,0.5)
    participant Daemon as "Netbird daemon" rect rgba(200,255,200,0.5)
    participant CLI as "netbird CLI" rect rgba(255,230,150,0.5)
    participant StatusSvc as "Status RPC / Probes" rect rgba(255,200,200,0.5)

    Entrypoint->>Daemon: wait_for_daemon_startup()
    Daemon-->>Entrypoint: ready
    Entrypoint->>CLI: connect() -> run `netbird up`
    CLI->>Daemon: control plane connection / start
    CLI-->>Entrypoint: exit status

    Note over StatusSvc,Daemon: Health/status checks now call getStatus(..., ShouldRunProbes=false)
    StatusSvc->>Daemon: getStatus(request with ShouldRunProbes=false)
    Daemon-->>StatusSvc: status (no probe execution)
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested reviewers

  • pappz
  • mlsmaycon

Poem

🐰 Hopping in the container light,
Timeout trimmed to keep things tight,
connect() springs up, no pause to wait,
Netbird leaps — a simpler state. 🥕

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: replacing conditional login logic with unconditional netbird up execution in the entrypoint.
Description check ✅ Passed The description includes all required sections: changes explained, issue reference, checklist completed, and documentation rationale provided.

✏️ 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 simplify-entrypoint-startup

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.

Copy link
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 (1)
client/netbird-entrypoint.sh (1)

53-56: LGTM!

The simplified connect() function cleanly replaces the previous polling logic. Since set -eEuo pipefail is enabled, the script will exit on failure from netbird up.

Regarding the SonarCloud hint about adding an explicit return: this is optional given set -e, but adding return $? or return 0 could make the success path more explicit if preferred.

,

🔧 Optional: Add explicit return for clarity
 connect() {
   info "running 'netbird up'..."
   "${NETBIRD_BIN}" up
+  return $?
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@client/netbird-entrypoint.sh` around lines 53 - 56, The connect() function
currently runs "${NETBIRD_BIN}" up and relies on set -e for failure handling;
add an explicit return to make the success/failure outcome clear by returning
the command's exit status (e.g., use the existing NETBIRD_BIN invocation and
then return "$?" or return 0 on success) — update the connect() function to
capture/forward the exit code from "${NETBIRD_BIN}" up so callers see the
explicit result.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@client/netbird-entrypoint.sh`:
- Around line 53-56: The connect() function currently runs "${NETBIRD_BIN}" up
and relies on set -e for failure handling; add an explicit return to make the
success/failure outcome clear by returning the command's exit status (e.g., use
the existing NETBIRD_BIN invocation and then return "$?" or return 0 on success)
— update the connect() function to capture/forward the exit code from
"${NETBIRD_BIN}" up so callers see the explicit result.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 2bb61e99-cc8e-496f-b287-0a9f298d1fe1

📥 Commits

Reviewing files that changed from the base of the PR and between 91f0d5c and 7bdf930.

📒 Files selected for processing (3)
  • client/Dockerfile
  • client/Dockerfile-rootless
  • client/netbird-entrypoint.sh

@lixmal lixmal force-pushed the simplify-entrypoint-startup branch from 7bdf930 to e354e26 Compare March 22, 2026 18:09
@sonarqubecloud
Copy link

Copy link
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.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@client/cmd/status.go`:
- Line 221: The startup path is passing a hardcoded ShouldRunProbes=false into
getStatus, causing startup checks to use async ProbeAll (stale cached state)
instead of blocking ProbeAllWaitResult; update the call in status.go (the
getStatus invocation used by checkStartup) to pass a true/locking probe flag for
startup (e.g., use isStartup for the third arg or explicitly true when isStartup
is true) so startup calls run blocking probes while keeping live/ready checks
using async probes.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 46f15a1c-3e36-4680-a7ea-7f4da176e881

📥 Commits

Reviewing files that changed from the base of the PR and between 7bdf930 and e354e26.

📒 Files selected for processing (4)
  • client/Dockerfile
  • client/Dockerfile-rootless
  • client/cmd/status.go
  • client/netbird-entrypoint.sh
🚧 Files skipped from review as they are similar to previous changes (3)
  • client/Dockerfile
  • client/Dockerfile-rootless
  • client/netbird-entrypoint.sh

@mlsmaycon mlsmaycon merged commit fd9d430 into main Mar 23, 2026
46 checks passed
@mlsmaycon mlsmaycon deleted the simplify-entrypoint-startup branch March 23, 2026 08:39
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