Skip to content

feat: consolidate duplicate port validation#407

Merged
jason-lynch merged 3 commits into
mainfrom
fix/PLAT-611/system-port-conflict-validation
Jun 16, 2026
Merged

feat: consolidate duplicate port validation#407
jason-lynch merged 3 commits into
mainfrom
fix/PLAT-611/system-port-conflict-validation

Conversation

@jason-lynch

Copy link
Copy Markdown
Member

Summary

We were validating duplicate ports in a few spots, each with different error message shapes, and neither could catch duplicate ports from nodes deployed to the same host.

This PR consolidates and improves those checks so that we'll catch duplicate ports from Postgres, Patroni, or other services on the same host.

Changes

This PR is split into two commits:

  • refactor: add validation package - refactors the validation error and path utilities to a new validation package. This starts to clean up our validation code and lets me use a Unique validator I originally implemented on another branch.
  • feat: consolidate duplicate port validation - Adds a Unique validator to the validation package and uses it to validate unique ports across hosts. This replaces the disparate duplicate-port validation we had for both nodes and services.

Testing

# This request would be accepted on main, but would result in an error
# You can use any environment (compose, dev-lima, lima, etc.) to test
cp1-req create-database <<EOF
{
    "id": "appdb",
    "spec": {
      "database_name": "appdb",
      "database_users": [
        {
          "username": "appdb_admin",
          "password": "password",
          "db_owner": true,
          "attributes": ["SUPERUSER", "LOGIN"]
        }
      ],
      "port": 5432,
      "patroni_port": 8426,
      "nodes": [
        {
          "name": "n1",
          "host_ids": ["host-1"]
        },
        {
          "name": "n2",
          "host_ids": ["host-1"]
        }
      ]
    }
  }
EOF

# You should get an error about duplicate ports

Notes for Reviewers

The ticket mentions systemd, but the same issue affected Swarm as well.

PLAT-611

Moves the validation error and path to a new `validation` package. We'll
add validation helper functions to this package and possibly move it to
a top level package in a future commit.

PLAT-611
We were validating duplicate ports in a few spots, each with different
error message shapes, and neither was able to catch duplicated ports
from nodes deployed to the same host.

This commit consolidates and improves those checks so that we'll catch
duplicated ports from either postgres, patroni, or a service on the same
host.

PLAT-611
@coderabbitai

coderabbitai Bot commented Jun 16, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@jason-lynch, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 57 minutes and 10 seconds. Learn how PR review limits work.

Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file).

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 87656d82-d3cf-4cd0-a74b-ffb2b16df831

📥 Commits

Reviewing files that changed from the base of the PR and between 83a6d9e and 2a90ed1.

📒 Files selected for processing (1)
  • changes/unreleased/Fixed-20260616-150152.yaml
📝 Walkthrough

Walkthrough

This change introduces structured validation paths and path-aware errors, rewires API validators to use them, replaces port conflict checks with duplicate port aggregation, updates API error conversion for the new error type, and adjusts tests to match the new validation output.

Changes

Validation path migration

Layer / File(s) Summary
Validation path and uniqueness primitives
server/internal/validation/error.go, server/internal/validation/validators.go, server/internal/validation/error_test.go, server/internal/ds/set.go
Adds validation.Path, validation.Error, and generic duplicate tracking with deterministic path formatting, plus tests and sorted set-to-string support.
API validators emit structured errors
server/internal/api/apiv1/validate.go, server/internal/api/apiv1/errors.go
Migrates database, node, service, repo, orchestrator, backup, restore, and script validators to validation.Path/validation.NewError, adds aggregated duplicate-port validation, and maps *validation.Error through apiErr.
Tests aligned with new error output
server/internal/api/apiv1/validate_test.go
Replaces port-conflict tests with duplicate-port coverage, updates expected validation messages, removes inherited-port cases, and drops the old API-layer validation error test now covered in server/internal/validation/error_test.go.

Poem

🐇 I hopped through paths of dot and bracket,
and tucked each error in a tidy packet.
Ports that clashed now line up in a row,
with duplicate trails that clearly show.
Carrot stamps on tests: “all set, let’s go!”

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 14.29% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title 'feat: consolidate duplicate port validation' is specific and directly summarizes the main change - consolidating duplicate port validation logic.
Description check ✅ Passed The PR description includes all required template sections: Summary, Changes, Testing, Checklist (mostly complete), and Notes for Reviewers. Information is clear and well-structured.
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 unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/PLAT-611/system-port-conflict-validation

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.

@codacy-production

codacy-production Bot commented Jun 16, 2026

Copy link
Copy Markdown

Up to standards ✅

🟢 Issues 4 medium

Results:
4 new issues

Category Results
Complexity 4 medium

View in Codacy

🟢 Metrics 21 complexity · 0 duplication

Metric Results
Complexity 21
Duplication 0

View in Codacy

NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.

@jason-lynch jason-lynch marked this pull request as ready for review June 16, 2026 18:22
@jason-lynch jason-lynch requested a review from moizpgedge June 16, 2026 18:22

@moizpgedge moizpgedge left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good and it fixes the bug. I tested on my dev env: two nodes on the same host with the same ports got rejected, and same ports on different hosts still worked.

@jason-lynch jason-lynch merged commit 79c56ed into main Jun 16, 2026
3 checks passed
@jason-lynch jason-lynch deleted the fix/PLAT-611/system-port-conflict-validation branch June 16, 2026 20:17
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