feat(searchengine): support Basic Auth for ES connections#165
Conversation
ECK and most managed ES clusters require credentials. Add SEARCH_USERNAME and SEARCH_PASSWORD env vars on search-service and search-sync-worker (both default to empty so dev clusters without auth still work). Also refactor searchengine.New to take a Config struct rather than growing positional args (now five connection knobs: Backend, URL, Username, Password, TLSSkipVerify) — labeled fields at every call site and easy to extend. Test call sites (9) updated to the struct form. Drive-by: fix history-service/internal/mongorepo/room.go and service.go to import the Collection / NewCollection / WithProjection helpers from pkg/mongoutil (they were moved out of the local package in #157 but room.go and the compile-time check were left referencing the old package-local names, breaking make lint on main). https://claude.ai/code/session_01UkLD7hpaypxjeh5zbEWTjp
📝 WalkthroughWalkthroughThis PR introduces a configuration-based API for the search engine factory, consolidating backend selection, connection details, and credentials into a single Config struct. Concurrently, it migrates history-service's RoomRepo to use shared mongoutil helpers and reformats test struct definitions. ChangesSearch Engine Configuration Refactoring
Mongo Repository Utility Migration
Test Struct Formatting
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@pkg/searchengine/factory.go`:
- Around line 24-26: The New function's switch currently only accepts
"elasticsearch" and returns the "unsupported search backend" error despite the
docstring claiming "opensearch" is supported; update the switch in New to accept
"opensearch" as a valid case (mapping it to the same backend initialization as
"elasticsearch") or normalize the backend string to treat "opensearch"
equivalently, ensure the returned SearchEngine is still Ping()-verified, and
remove or adjust any hardcoded error message "unsupported search backend" to
only trigger for truly unknown values.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: d2aee8e4-9e0a-4638-8db6-a61391ac68ae
📒 Files selected for processing (9)
history-service/internal/mongorepo/room.gohistory-service/internal/service/service.gopkg/minioutil/minio_integration_test.gopkg/searchengine/factory.gosearch-service/integration_test.gosearch-service/main.gosearch-sync-worker/inbox_integration_test.gosearch-sync-worker/integration_test.gosearch-sync-worker/main.go
Summary
ECK and most managed Elasticsearch clusters require credentials. This PR adds Basic Auth support to
pkg/searchengineand plumbsSEARCH_USERNAME/SEARCH_PASSWORDenv vars through both consuming services.search-serviceSEARCH_USERNAME,SEARCH_PASSWORDsearch-sync-workerSEARCH_USERNAME,SEARCH_PASSWORDEmpty defaults preserve current dev-stack behavior (
make upagainst an unauthenticatedelasticsearch:9200); operators set them per-environment for ECK / SaaS clusters.Notable change:
searchengine.NewsignatureNew(ctx, backend, url, tlsSkipVerify)was already four positional args after the recent TLS toggle. Adding two more (username,password) would push call sites to six, all unlabeled. Refactored to aConfigstruct so each call site is labeled and the API can absorb future knobs without churning every caller:All 9 integration-test call sites updated to the struct form. The two
main.gocall sites use the struct verbatim.Drive-by fix
make lintwas failing onmain(typecheck error inhistory-service/internal/mongorepo/room.goandinternal/service/service.go) — PR #157 movedCollection/NewCollection/WithProjectionhelpers out of the localmongorepopackage intopkg/mongoutil, butroom.goand theservice.gocompile-time check were left referencing the old package-local names. Fixed by adding themongoutilimport + qualifier and the missingmongorepoimport on the service-side. Without this fix, the pre-commit hook would block any PR from this point forward.Test plan
make lintcleanmake testcleango vet -tags integration ./pkg/searchengine/... ./search-service/... ./search-sync-worker/...cleanSEARCH_USERNAME=elastic SEARCH_PASSWORD=<from k8s secret>authenticates successfully (CI doesn't cover this path; integration tests still hit unauthenticated containers via the empty-defaults code path)Notes for ops
user-room-*/messages-*forsearch-service; write on the same indexes forsearch-sync-worker) rather than the ECK-generatedelasticsuperuser.SEARCH_TLS_SKIP_VERIFY=trueonly on internal/self-signed clusters; default-off remains correct for prod with a properly signed cert.https://claude.ai/code/session_01UkLD7hpaypxjeh5zbEWTjp
Generated by Claude Code
Summary by CodeRabbit
New Features
Refactor
Tests