Fix/245 hypervisor connection uri validation#249
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical fix by implementing robust validation for hypervisor connection URIs. It introduces a database migration to ensure that Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a database migration to enforce a unique constraint on the connection_uri for hypervisors, preventing duplicate entries. New functional tests are added to validate this constraint, covering various scenarios including null and duplicate values. The testing infrastructure is also enhanced with new fixtures and utility classes in conftest.py and utils.py for better database and migration management during tests.
The changes are well-structured and effectively implement the desired validation. My review includes a couple of suggestions for the new test utilities in genesis_core/tests/functional/utils.py to improve robustness and adherence to best practices regarding database query construction and interaction with third-party library internals.
|
|
||
| def teardown(self) -> None: | ||
| if isinstance(self._engine, engines.PgSQLEngine): | ||
| self._engine._pool.close() |
There was a problem hiding this comment.
Accessing the private attribute _pool directly is brittle and couples this test utility to the internal implementation of PgSQLEngine. It's better to use a public API for closing the connection pool if one exists. This will make the code more robust to changes in the underlying library.
There was a problem hiding this comment.
Engine doesn't have a close method. Pool closing occurs in __del__, which is somewhat hacky and requires a mandatory call to the garbage collector. It looks like there are issues with the Engine lifecycle.
| def create_db(self) -> None: | ||
| create_db = self.manager_config.create_db | ||
| if create_db is None: | ||
| return | ||
|
|
||
| with self.connection(autocommit=True) as connection: | ||
| with connection.cursor() as cursor: | ||
| cursor.execute(f"CREATE DATABASE \"{create_db}\"") | ||
|
|
||
| def drop_db(self) -> None: | ||
| create_db = self.manager_config.create_db | ||
| if create_db is None: | ||
| return | ||
|
|
||
| with self.connection(autocommit=True) as connection: | ||
| with connection.cursor() as cursor: | ||
| cursor.execute(f"DROP DATABASE \"{create_db}\"") | ||
|
|
There was a problem hiding this comment.
The create_db and drop_db methods use f-strings to construct CREATE DATABASE and DROP DATABASE queries. This is a potential SQL injection vulnerability. While the risk is low in a test environment where the database name is controlled, this is a dangerous pattern that should be avoided. Database identifiers should be properly quoted using a mechanism provided by the database driver/ORM to prevent injection. For example, if restalchemy is built on SQLAlchemy, you could use sqlalchemy.sql.compiler.IdentifierPreparer.
There was a problem hiding this comment.
Since this happens in tests and not production, it is not critical.
atin65536
requested review from
a team,
akremenetsky,
gmelikov and
phantomii
and removed request for
a team
| BEGIN | ||
| RETURN t::jsonb; | ||
| EXCEPTION | ||
| WHEN invalid_text_representation THEN |
There was a problem hiding this comment.
I thought about why do we need such function, and looks like the answer is that driver_spec is not a jsonb field. Mayeb convert it to jsonb, so if it's not valid - we won't have a trash in DB @akremenetsky ?
driver_spec field will be validated in RA already https://github.com/infraguys/genesis_core/blob/master/genesis_core/compute/dm/models.py#L147 (but it uses old way to work with json fields, that's why it's varchar in DB).
It's not a blocker for this PR, but looks like a tech debt.
2 participants
No description provided.