Fix OpenSearch connection error with empty port parameter#1004
Merged
Conversation
Handle empty es_port and es_host parameters gracefully to prevent 'invalid literal for int()' error when ElasticSearch/OpenSearch connection parameters are not provided in Jenkins jobs. Changes: - Add validation to check for empty host/port before conversion - Convert port to integer once and reuse the value - Provide clearer error message when connection parameters are missing - Allow jobs to continue with es=None when ES is not configured Co-authored-by: Cursor <cursoragent@cursor.com>
📝 WalkthroughWalkthrough
ChangesES/OpenSearch client initialization and operational guards
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 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
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cloud_governance/common/elasticsearch/elasticsearch_operations.py (2)
56-69:⚠️ Potential issue | 🟠 MajorTLS certificate verification is disabled on secure connections.
When HTTPS/SSL is used,
verify_certs=False(line 59 for Elasticsearch and line 68 for OpenSearch) weakens transport security and enables MITM risk. Make certificate verification configurable via environment variable with secure defaults.Suggested direction
+ es_verify_certs = str(self.__environment_variables_dict.get('ES_VERIFY_CERTS', 'true')).lower() == 'true' ... - self.__es = Elasticsearch(hosts, basic_auth=basic_auth, verify_certs=False, + self.__es = Elasticsearch(hosts, basic_auth=basic_auth, verify_certs=es_verify_certs, request_timeout=self.__timeout, max_retries=2) ... if es_port_int == 443: add_host['use_ssl'] = True - add_host['verify_certs'] = False + add_host['verify_certs'] = es_verify_certs🤖 Prompt for 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. In `@cloud_governance/common/elasticsearch/elasticsearch_operations.py` around lines 56 - 69, The TLS certificate verification is hardcoded to False for both Elasticsearch and OpenSearch client connections, which disables critical security checks. Modify the code to make certificate verification configurable through an environment variable while maintaining secure defaults. For the Elasticsearch constructor call, replace the hardcoded verify_certs=False parameter with a configurable value read from an environment variable that defaults to True. Similarly, for the OpenSearch constructor call, add configuration for certificate verification through an environment variable with a secure default. This will allow operators to configure the behavior without compromising security by default.
54-67:⚠️ Potential issue | 🟠 MajorUse the parsed port for OpenSearch host construction too.
Line 63 passes
self.__es_port(string) instead ofes_port_int. The OpenSearch Python client requires the port parameter to be an integer; passing a string will cause configuration errors. The Elasticsearch path correctly uses the parsed integer at line 57, so the OpenSearch path should do the same for consistency and correctness.Suggested fix
- add_host = {'host': self.__es_host, 'port': self.__es_port} + add_host = {'host': self.__es_host, 'port': es_port_int}🤖 Prompt for 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. In `@cloud_governance/common/elasticsearch/elasticsearch_operations.py` around lines 54 - 67, In the OpenSearch configuration block (the else clause), the add_host dictionary is being constructed with self.__es_port as a string value, but it should use the already-parsed es_port_int variable instead. On the line where add_host is assigned with the host and port, replace self.__es_port with es_port_int to ensure the port parameter is passed as an integer, matching the approach used in the Elasticsearch path above it and ensuring compatibility with the OpenSearch Python client requirements.
🤖 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.
Outside diff comments:
In `@cloud_governance/common/elasticsearch/elasticsearch_operations.py`:
- Around line 56-69: The TLS certificate verification is hardcoded to False for
both Elasticsearch and OpenSearch client connections, which disables critical
security checks. Modify the code to make certificate verification configurable
through an environment variable while maintaining secure defaults. For the
Elasticsearch constructor call, replace the hardcoded verify_certs=False
parameter with a configurable value read from an environment variable that
defaults to True. Similarly, for the OpenSearch constructor call, add
configuration for certificate verification through an environment variable with
a secure default. This will allow operators to configure the behavior without
compromising security by default.
- Around line 54-67: In the OpenSearch configuration block (the else clause),
the add_host dictionary is being constructed with self.__es_port as a string
value, but it should use the already-parsed es_port_int variable instead. On the
line where add_host is assigned with the host and port, replace self.__es_port
with es_port_int to ensure the port parameter is passed as an integer, matching
the approach used in the Elasticsearch path above it and ensuring compatibility
with the OpenSearch Python client requirements.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 571da4e1-2ba8-432d-b3ab-1786c1fd07a7
📒 Files selected for processing (1)
cloud_governance/common/elasticsearch/elasticsearch_operations.py
ebattat
reviewed
Jun 18, 2026
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 `@cloud_governance/common/elasticsearch/elasticsearch_operations.py`:
- Around line 52-57: The no-ES mode is incompletely protected as self.__es can
be None but public methods like get_elasticsearch_index_by_id, get_index_hits,
fetch_data_by_es_query, delete_data_in_between_in_es, delete_data_in_es, and
post_query all directly dereference self.__es without guarding against None. Add
a consistent check at the start of each of these public methods to verify that
self.__es is not None before attempting to use it, and return a safe default
value (such as an empty result or None) or raise a consistent, informative
exception when ElasticSearch is not configured, ensuring graceful degradation in
no-ES mode.
🪄 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: CHILL
Plan: Enterprise
Run ID: a3adf2a8-a067-405e-bda0-fbe930ffeafb
📒 Files selected for processing (1)
cloud_governance/common/elasticsearch/elasticsearch_operations.py
github-project-automation
Bot
moved this from In progress
to Done
in Cloud-Governance project
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Type of change
Note: Fill x in []
Description
Handle empty es_port and es_host parameters gracefully to prevent 'invalid literal for int()' error when ElasticSearch/OpenSearch connection parameters are not provided in Jenkins jobs.
Changes:
For security reasons, all pull requests need to be approved first before running any automated CI
Assisted-by: Cursor