Add authorization header to Elasticsearch query #100
Conversation
Added a function to encode username and password for Basic Authentication headers.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #100 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 21 21
Lines 660 671 +11
=========================================
+ Hits 660 671 +11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR adds authentication support for Elasticsearch connections by implementing Basic Authentication headers across the application. The changes enable the application to work with Elasticsearch instances that have security enabled, which is now required for the test environment running in Docker Compose.
Key Changes:
- Added username/password-based authentication header generation for Elasticsearch requests
- Updated all Elasticsearch HTTP requests to include authorization headers when credentials are configured
- Enabled Elasticsearch security in the Docker Compose test environment
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/config.py | Added auth header encoding function and configuration for Elasticsearch credentials |
| src/utils/wait_for_service.py | Updated service health check to support optional authentication token |
| src/server/main.py | Passed auth token to Elasticsearch connection check |
| src/es_client/query.py | Added authorization header to search queries |
| src/search2_rpc/service.py | Added authorization header to index listing requests |
| tests/helpers/init_elasticsearch.py | Extracted header generation to helper function and applied to all test index operations |
| docker-compose.yaml | Enabled Elasticsearch security and configured authentication credentials |
| .github/workflows/test.yml | Added environment variables for Elasticsearch authentication in CI |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
Just to be clear - now all the tests run with auth? |
The tests in GHA do. There are some tests that require an SSH tunnel to CI that I haven't tried out |
|
@MrCreosote are there any requested changes? |
|
This is open: #100 (comment) |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
CodeQL is still dumb |
| stderr=container_err, | ||
| cwd=cwd) | ||
| wait_for_service(app_url, "search2") | ||
| wait_for_service(app_url, "search2", {}) |
There was a problem hiding this comment.
How does this work without the headers?
There was a problem hiding this comment.
Because it already didn't send any headers when querying search2. Only elastic requires auth
There was a problem hiding this comment.
Oh, I'm dumb. I was thinking this function is exclusively for ES
| start = time.time() | ||
| with pytest.raises(SystemExit) as se: | ||
| wait_for_service(url, 'foo', timeout=timeout) | ||
| wait_for_service(url, 'foo', {}, timeout=timeout) |
There was a problem hiding this comment.
Same question - not sure how this can work without headers if auth is always on
There was a problem hiding this comment.
Auth is always on for elastic, not for any other services.
Co-authored-by: MrCreosote <MrCreosote@users.noreply.github.com>
Co-authored-by: MrCreosote <MrCreosote@users.noreply.github.com>
Co-authored-by: MrCreosote <MrCreosote@users.noreply.github.com>
So these tests do call the running elastic search, because they start it up with docker compose. So since I enabled auth on it and added those env vars to the github actions, tests are required to use credentials to talk to elastic.