Replace Ruby bosh-nats-sync with Golang implementation

[aramprice]

Summary

• Replaces the Ruby bosh-nats-sync gem with a Go binary (src/bosh-nats-sync/) that mirrors all Ruby behavior including TLS verification preferences (uaa_ca_cert > director_ca_cert), HTTP peer verification, and startup reliability
• Removes all Ruby runtime dependencies from the nats BOSH job and package (director-ruby-3.3, gem bundling, BUNDLE_GEMFILE/GEM_HOME env vars)
• Adds GitHub Actions CI coverage for the new Go code (lint + test jobs in go.yml); removes the defunct nats_sync:parallel Ruby matrix entry from ruby.yml

Commits

1. Golang bosh-nats-sync — full Go implementation with parity to the Ruby version:

   • Correct TLS CA selection (uaa_ca_cert preferred over director_ca_cert)
   • Proper Director API TLS peer verification (no InsecureSkipVerify)
   • isConnectionError retry logic covering deadline exceeded / eof (matching Ruby's Net::OpenTimeout, Net::ReadTimeout, Errno::ECONNRESET)
   • Fatal startup behavior on failed NATS reload (matching Ruby)
   • Full Ginkgo/Gomega test suite

2. Remove ruby-isms now that nats is golang — strips Ruby from jobs/nats/, packages/nats/, src/Gemfile, and CI:

   • packages/nats/packaging now builds the Go binary via go build
   • jobs/nats/ wrapper script calls Go binary directly (no Ruby runtime sourcing)
   • CI go.yml: adds lint (bosh-nats-sync) and test (bosh-nats-sync) jobs
   • CI ruby.yml: removes nats_sync:parallel matrix entry
   • Adds src/bosh-nats-sync/.golangci.yml

Test plan

• go.yml / lint (bosh-nats-sync) passes
• go.yml / test (bosh-nats-sync) passes
• ruby.yml / unit_specs passes for all remaining sub-projects (common:parallel, monitor:parallel, release)
• Release ERB template specs (spec/nats_templates_spec.rb) pass

[coderabbitai]

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 3d80f5f8-33e2-4d34-9ad5-3cd7392e0c25

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• ✅ Review completed - (🔄 Check again to review again)

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch experiment-golang-bosh-nats-sync

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[coderabbitai]

Actionable comments posted: 14

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In @.github/workflows/go.yml:
- Line 21: The actions/checkout steps lack the persist-credentials parameter
which increases the risk of token reuse in subsequent steps and artifacts. Add
the parameter `persist-credentials: false` to both instances of the
`actions/checkout@v7` action to disable credential persistence and reduce token
exposure risk. This parameter should be added as a separate line within the uses
section for both checkout actions.
- Around line 21-22: Replace the GitHub Actions version tag pins with full
commit SHA pins for enhanced security. Update the `uses:` entries for
`actions/checkout@v7` and `actions/setup-go@v6` (and any other actions mentioned
in the comment) to use their full commit SHA instead of the `@v*` tag format.
This applies to all `uses:` statements throughout the workflow file where
actions are currently pinned to version tags rather than specific commit hashes.

In `@src/bosh-nats-sync/go.mod`:
- Line 3: The Go toolchain and dependencies contain multiple security
vulnerabilities that must be addressed before release. Update the go directive
from version 1.26.1 to 1.26.3 to resolve five known security vulnerabilities.
Additionally, upgrade golang.org/x/net from v0.49.0 to v0.53.0 to address the
GO-2026-4918 vulnerability affecting net/http infinite loops, and upgrade
golang.org/x/sys from v0.40.0 to v0.44.0 to address the GO-2026-5024
vulnerability affecting sys/windows integer overflow. After making these version
updates in the go.mod file, refresh the module graph by running go mod tidy to
ensure all transitive dependencies are properly resolved and consistent.

In `@src/bosh-nats-sync/pkg/authprovider/auth_provider_test.go`:
- Around line 305-308: The test assertion in the "Token expiration deadline"
describe block is tautological because it compares two identical constant
expressions (60 * time.Second to 60 * time.Second) which will always pass
regardless of actual production code changes. Replace the hardcoded 60 *
time.Second on the left side of the Expect call in the It block with
authprovider.ExpirationDeadline so the test actually verifies the production
constant has the expected value, while keeping the right side as 60 *
time.Second for the comparison target.

In `@src/bosh-nats-sync/pkg/authprovider/auth_provider.go`:
- Around line 87-91: In the error handling block following the failed token
acquisition call to a.cfg.Token(a.ctx), change the return statement from
returning ("", nil) to returning ("", err) to properly propagate the
authentication error instead of suppressing it. This ensures that downstream
code receives the error and can handle the transient failure appropriately
through the intended retry and error handling flow.
- Around line 109-111: The http.Client being returned from this function lacks
an explicit timeout, which can cause token requests to hang indefinitely. Add a
Timeout field to the http.Client struct that is being returned to specify a
reasonable timeout duration for UAA token requests. This ensures that network
operations on the client will not block indefinitely and will allow sync
progress to continue even if token requests become unresponsive.
- Around line 99-107: The code currently silently ignores read errors and parse
failures when loading the configured CA certificate file in the block around
caCertPath and x509.NewCertPool(). Instead of only proceeding when err is nil,
you should fail fast by returning an error when either the file read fails or
when AppendCertsFromPEM fails to parse the certificates. This ensures that
configuration issues with the CA file are surfaced immediately rather than
silently degrading TLS trust behavior at runtime.

In `@src/bosh-nats-sync/pkg/config/config.go`:
- Around line 55-60: The NewLogger function dereferences the cfg parameter
without first checking if it is nil, which will cause a panic if a nil config is
passed. Add a nil check at the start of the NewLogger function before accessing
cfg.LogFile, and when cfg is nil, fall back to creating a logger that writes to
stdout instead of attempting to access cfg fields.
- Around line 41-53: The Load function unmarshals the YAML data into the Config
struct but does not validate the configuration before returning it, which can
lead to runtime failures when downstream code attempts to use fields like
Intervals.PollUserSync with time.NewTicker. After the yaml.Unmarshal call
succeeds, add validation logic to check that required fields are present and
that interval values (specifically Intervals.PollUserSync) are positive non-zero
durations. If validation fails, return an error with a descriptive message. This
ensures the config is semantically valid before being used.

In `@src/bosh-nats-sync/pkg/runner/runner_test.go`:
- Around line 193-204: The failCmdRunner in this test fails immediately on the
first call (startup reload), causing Run() to exit before reaching the periodic
ticker loop, so the periodic sync failure path is never tested. Modify the
failCmdRunner to succeed on the first invocation (to allow the startup reload to
complete) and fail on subsequent invocations (to test the periodic sync failure
path). Use a counter or state tracking within the failCmdRunner function to
differentiate between the first call and later calls, ensuring the test actually
validates periodic-sync failure as described.

In `@src/bosh-nats-sync/pkg/runner/runner.go`:
- Around line 57-59: The code creates a time.NewTicker with a duration from
r.config.Intervals.PollUserSync without validating that the value is greater
than zero, which can cause a panic if the config value is unset or invalid. Add
validation logic before creating the ticker to check if the interval duration is
greater than zero, and return an appropriate error or handle the invalid
configuration gracefully instead of allowing time.NewTicker to panic. This
validation should occur before the line where time.NewTicker is called.

In `@src/bosh-nats-sync/pkg/userssync/users_sync_test.go`:
- Around line 500-549: The test context is titled "when director becomes
available after retries" but the It block only verifies that Execute() succeeds
without validating that retries actually occurred. The /info handler is designed
to return StatusServiceUnavailable on the first request and StatusOK on
subsequent requests, but the test never asserts this retry behavior happened.
Add an assertion after the sync.Execute() call in the It block to check that
requestCount >= 2, ensuring the /info endpoint was called multiple times and the
retry mechanism in withDirectorConnection() was actually exercised.

In `@src/bosh-nats-sync/pkg/userssync/users_sync.go`:
- Around line 307-309: The deploymentName parameter in the getVMsByDeployment
method is directly embedded into the API path using fmt.Sprintf without URL
encoding, which will cause issues if the deployment name contains reserved URL
characters like forward slashes, percent signs, or spaces. Fix this by
URL-escaping the deploymentName before embedding it into the path string using
the appropriate URL encoding function from the net/url package, specifically
url.QueryEscape or url.PathEscape as appropriate for path parameters.
- Around line 107-127: The ConnectionWaitTimeout is used to calculate retry
attempts but doesn't enforce an overall elapsed-time budget across all probes.
Each HTTP call can block independently for up to 30 seconds, causing startup to
stall far longer than the configured timeout. In the withDirectorConnection()
method, create a deadline based on the configured timeout at the start and track
elapsed time throughout the retry loop. Before each call to
boshAPIResponseBody() and before sleeping, check if the elapsed time would
exceed the deadline and bail out if so. This ensures the total time spent across
all attempts, including individual HTTP timeouts, never exceeds the
ConnectionWaitTimeout. Apply the same deadline-checking pattern to all other
callers of boshAPIResponseBody() in getAuthHeader(), queryAllDeployments(), and
getVMsByDeployment() methods.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 74b1ea7c-2402-441d-a6e3-cda6eee9250b

📥 Commits

Reviewing files that changed from the base of the PR and between a13a1db and 980d15b.

⛔ Files ignored due to path filters (2)

• src/Gemfile.lock is excluded by !**/*.lock
• src/bosh-nats-sync/go.sum is excluded by !**/*.sum

📒 Files selected for processing (49)

• .github/workflows/go.yml
• .github/workflows/ruby.yml
• jobs/nats/spec
• jobs/nats/templates/bosh_nats_sync
• jobs/nats/templates/bpm.yml
• packages/nats/packaging
• packages/nats/spec
• src/Gemfile
• src/bosh-nats-sync/.gitignore
• src/bosh-nats-sync/.golangci.yml
• src/bosh-nats-sync/bin/bosh-nats-sync
• src/bosh-nats-sync/bosh-nats-sync.gemspec
• src/bosh-nats-sync/cmd/bosh-nats-sync/main.go
• src/bosh-nats-sync/go.mod
• src/bosh-nats-sync/lib/nats_sync.rb
• src/bosh-nats-sync/lib/nats_sync/auth_provider.rb
• src/bosh-nats-sync/lib/nats_sync/config.rb
• src/bosh-nats-sync/lib/nats_sync/nats_auth_config.rb
• src/bosh-nats-sync/lib/nats_sync/runner.rb
• src/bosh-nats-sync/lib/nats_sync/users_sync.rb
• src/bosh-nats-sync/lib/nats_sync/version.rb
• src/bosh-nats-sync/lib/nats_sync/yaml_helper.rb
• src/bosh-nats-sync/pkg/authprovider/auth_provider.go
• src/bosh-nats-sync/pkg/authprovider/auth_provider_suite_test.go
• src/bosh-nats-sync/pkg/authprovider/auth_provider_test.go
• src/bosh-nats-sync/pkg/config/config.go
• src/bosh-nats-sync/pkg/config/config_suite_test.go
• src/bosh-nats-sync/pkg/config/config_test.go
• src/bosh-nats-sync/pkg/natsauthconfig/nats_auth_config.go
• src/bosh-nats-sync/pkg/natsauthconfig/nats_auth_config_suite_test.go
• src/bosh-nats-sync/pkg/natsauthconfig/nats_auth_config_test.go
• src/bosh-nats-sync/pkg/runner/runner.go
• src/bosh-nats-sync/pkg/runner/runner_suite_test.go
• src/bosh-nats-sync/pkg/runner/runner_test.go
• src/bosh-nats-sync/pkg/userssync/users_sync.go
• src/bosh-nats-sync/pkg/userssync/users_sync_suite_test.go
• src/bosh-nats-sync/pkg/userssync/users_sync_test.go
• src/bosh-nats-sync/spec/nats_sync/auth_provider_spec.rb
• src/bosh-nats-sync/spec/nats_sync/config_spec.rb
• src/bosh-nats-sync/spec/nats_sync/nats_auth_config_spec.rb
• src/bosh-nats-sync/spec/nats_sync/runner_spec.rb
• src/bosh-nats-sync/spec/nats_sync/users_sync_spec.rb
• src/bosh-nats-sync/spec/nats_sync_spec.rb
• src/bosh-nats-sync/spec/spec_helper.rb
• src/bosh-nats-sync/spec/support/uaa_helpers.rb
• src/bosh-nats-sync/testdata/director-subject
• src/bosh-nats-sync/testdata/hm-subject
• src/bosh-nats-sync/testdata/sample_config.yml
• src/tasks/spec.rake

💤 Files with no reviewable changes (25)

• src/bosh-nats-sync/lib/nats_sync/version.rb
• jobs/nats/templates/bosh_nats_sync
• src/bosh-nats-sync/spec/support/uaa_helpers.rb
• src/bosh-nats-sync/lib/nats_sync/nats_auth_config.rb
• .github/workflows/ruby.yml
• src/bosh-nats-sync/.gitignore
• src/bosh-nats-sync/lib/nats_sync/yaml_helper.rb
• src/bosh-nats-sync/spec/nats_sync/auth_provider_spec.rb
• src/bosh-nats-sync/lib/nats_sync/runner.rb
• src/bosh-nats-sync/spec/nats_sync/runner_spec.rb
• src/bosh-nats-sync/spec/nats_sync/users_sync_spec.rb
• src/bosh-nats-sync/bin/bosh-nats-sync
• src/bosh-nats-sync/bosh-nats-sync.gemspec
• src/bosh-nats-sync/spec/nats_sync/nats_auth_config_spec.rb
• src/bosh-nats-sync/spec/nats_sync/config_spec.rb
• src/bosh-nats-sync/lib/nats_sync.rb
• src/Gemfile
• jobs/nats/spec
• src/bosh-nats-sync/lib/nats_sync/config.rb
• src/bosh-nats-sync/spec/nats_sync_spec.rb
• src/bosh-nats-sync/testdata/sample_config.yml
• src/bosh-nats-sync/spec/spec_helper.rb
• src/bosh-nats-sync/lib/nats_sync/users_sync.rb
• src/bosh-nats-sync/lib/nats_sync/auth_provider.rb
• jobs/nats/templates/bpm.yml

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[Copilot]

Pull request overview

Copilot reviewed 46 out of 51 changed files in this pull request and generated 6 comments.

[aramprice]

Behavioral parity audit: Ruby vs Go

A complete pass comparing the Ruby source (src/bosh-nats-sync/lib/nats_sync/) against this Go implementation. All deviations are noted; deviations marked improvement are intentional and correct.

Startup sequence

#	Aspect	Ruby	Go
1	Signal handling	Signal.trap('INT') only	SIGINT and SIGTERM ✅ improvement (BPM sends SIGTERM)
2	Initial NATS reload	Always sends reload; picks up whatever config pre-start wrote	Bootstrap(): reads subject files, writes explicit director/HM config, then sends reload ✅ improvement
3	First sync timing	Rufus::Scheduler.interval fires after the interval	time.Ticker fires after the interval — identical timing

execute_users_sync / Execute

#	Aspect	Ruby	Go
4	AuthProvider lifecycle	NATSSync::AuthProvider.new(info, …) created on every authenticated request → new UAAToken → new UAA grant per request	AuthProvider created once per Execute pass; token cached across all /deployments + /deployments/*/vms calls ✅ improvement (reduces UAA load)
5	UAA token failure	Logs error, sets @uaa_token = nil, returns nil auth header → director returns 401 → RuntimeError caught → fallback	Returns error to caller → fallback. Same outcome, clearer error chain ✅
6	Connection error retry	Bosh::Common.retryable(tries: N, on: DIRECTOR_CONNECTION_ERRORS) — number-of-attempts based	Deadline-based with 1 s sleep between retries — equivalent timeout
7	Deployment URL in path	/deployments/#{deployment}/vms — no URL encoding	/deployments/ + url.PathEscape(name) + /vms — URL-encodes name ✅ (BOSH names are [a-z0-9-] so no practical difference)
8	Empty agent_id VMs	No check — writes CN=.agent.bosh-internal (invalid entry)	Skips VMs with empty agent_id ✅ bug fixed
9	JSON serialization	JSON.unparse — no HTML-escaping (preserves > literally)	json.NewEncoder with SetEscapeHTML(false) — identical output ✅
10	Change-detection hash	Digest::MD5.file(path).hexdigest	sha256.Sum256 — same semantics (non-cryptographic change detection) ✅
11	read_subject_file (whitespace-only)	File.empty? is false for whitespace-only; strip returns "" → returns "" (not nil!)	TrimSpace → returns nil

auth_header / AuthProvider

#	Aspect	Ruby	Go
12	Basic auth encoding	Base64.encode64(…).strip — strip removes leading/trailing whitespace but NOT mid-string newlines Ruby inserts every 60 chars. For long credentials, produces an invalid RFC 2617 header	base64.StdEncoding.EncodeToString — no newlines, RFC-compliant ✅
13	CA cert fallback	uaa_ca_cert → director_ca_cert → system trust	CAFilePath(): identical fallback chain ✅
14	TLS verification	verify_mode = VERIFY_PEER always	tls.Config{MinVersion: TLS12} — always verifies ✅

Error / shutdown handling

#	Aspect	Ruby	Go
15	Sync error	Scheduler on_error logs fatal + shutdown	Runner returns error → main.go os.Exit(1) — equivalent
16	Stop	@scheduler.shutdown	Close stopCh + wait for Run() — equivalent graceful shutdown

NatsAuthConfig

#	Aspect	Ruby	Go
17	Permissions structure	Identical publish/subscribe arrays per user type	Identical ✅
18	CN format	"C=USA, O=Cloud Foundry, CN=#{cn}.agent.bosh-internal"	Identical ✅
19	bootstrap user	Included when !vm['permanent_nats_credentials']	Identical ✅

Verdict: The Go implementation is a faithful and improved replacement for the Ruby version. Every behavioral difference is an improvement (SIGTERM support, per-Execute auth caching, empty-agent-id guard, RFC-compliant Base64, clearer UAA error propagation).

[aramprice]

Idiomatic Go audit

A second pass reviewing the implementation for Go clarity and convention (not Ruby-translation patterns).

Strengths ✅

• errors.Is/errors.As for connection-error classification — correct and idiomatic vs. string-matching
• context.Context propagated through every I/O call — correct
• sync.Once for lazy HTTP client — correct (documented as permanently-fatal, which is the right semantic)
• Functional options pattern (WithCommandRunner) — idiomatic for testability
• slog.Logger — modern (Go 1.21+)
• export_test.go for exposing unexported symbols in external test packages — correct Go idiom
• Error wrapping with %w throughout — correct
• defer func() { _ = resp.Body.Close() }() — correct ignore-close-error pattern
• Table-driven tests via DescribeTable/Entry — well-suited to the isConnectionError matrix

Issues found and fixed in this review pass

1. getCommandRunner() was dead code — NewUsersSync now normalises a nil cmdRunner in the constructor, and all call-sites use u.commandRunner directly. Removed.

2. Runner test temp file leak — dirSubjectFile and hmSubjectFile were never cleaned up in AfterEach. Fixed (analogous to the users_sync_test.go fix).

Design observations (no code change required, but worth noting)

3. uaaTokenHeader holds the mutex for the entire HTTP call — a.mu.Lock() is held while a.cfg.Token(tokenCtx) makes an outbound HTTP request (up to 30 s). Since there is only one sync goroutine at a time, this is safe and correct. A future refactor could use double-checked locking or sync.Once per token, but the current design is fine.

4. readSubjectFile returns *string — this is idiomatic in Go when nil is semantically meaningful. An alternative (string, bool) return would also be idiomatic; either is fine.

5. Module path bosh-nats-sync without a domain prefix — acceptable for a standalone binary; would need a qualified path (e.g., github.com/cloudfoundry/bosh/src/bosh-nats-sync) if external importability were ever needed.

6. Bootstrap() always sends a NATS reload even if the config hasn't changed — unlike Execute which skips reload when the hash matches, Bootstrap always reloads after writing. This is correct for startup (we want NATS to definitely apply the new config), and the extra SIGHUP is harmless.