Skip to content

jir-ogc #555

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jirhiker merged 1 commit into from
Feb 27, 2026
Merged

jir-ogc #555

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions alembic/env.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,6 @@ def run_migrations_online() -> None:

instance_name = os.environ.get("CLOUD_SQL_INSTANCE_NAME")
user = os.environ.get("CLOUD_SQL_USER")
password = os.environ.get("CLOUD_SQL_PASSWORD")
database = os.environ.get("CLOUD_SQL_DATABASE")
use_iam_auth = get_bool_env("CLOUD_SQL_IAM_AUTH", True)
ip_type = os.environ.get("CLOUD_SQL_IP_TYPE", "public")
Expand All @@ -147,10 +146,11 @@ def getconn():
"ip_type": ip_type,
"enable_iam_auth": use_iam_auth,
Copy link
Copy Markdown
Contributor

Copilot AI Feb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the code now enforces that IAM auth must be enabled (raising an error if it's not), the enable_iam_auth parameter in connect_kwargs should be hardcoded to True instead of using the use_iam_auth variable. This makes the intent clearer and avoids passing a value that's always True at this point in the code.

Suggested change
"enable_iam_auth": use_iam_auth,
"enable_iam_auth": True,

}
if use_iam_auth:
connect_kwargs["password"] = get_iam_login_token()
else:
connect_kwargs["password"] = password
if not use_iam_auth:
raise RuntimeError(
Comment on lines +149 to +150
Copy link
Copy Markdown
Contributor

Copilot AI Feb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The get_bool_env function can return a string value if the environment variable is set to something other than recognized boolean values ("true", "1", "yes", "false", "0", "no"). This could lead to unexpected behavior in the if not use_iam_auth: check. Consider adding validation to ensure use_iam_auth is actually a boolean, or handle the case where it might be an unexpected string value.

"CLOUD_SQL_IAM_AUTH must be true when DB_DRIVER=cloudsql."
)
Comment on lines +149 to +152
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot Feb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Keep Alembic online migrations compatible with non-IAM auth

The migration connector now hard-fails when CLOUD_SQL_IAM_AUTH is false, so alembic upgrade cannot run in Cloud SQL setups that use DB password auth instead of IAM tokens. This also leaves inconsistent behavior in the same file because build_database_url() still has a non-IAM/password branch, indicating non-IAM mode is expected to exist, but online migrations now crash before connecting.

Useful? React with 👍 / 👎.

connect_kwargs["password"] = get_iam_login_token()
return connector.connect(
instance_name,
"pg8000",
Expand Down
20 changes: 10 additions & 10 deletions db/engine.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,6 @@ def asyncify_connection():

instance_name = os.environ.get("CLOUD_SQL_INSTANCE_NAME")
user = os.environ.get("CLOUD_SQL_USER")
password = os.environ.get("CLOUD_SQL_PASSWORD")
database = os.environ.get("CLOUD_SQL_DATABASE")
use_iam_auth = get_bool_env("CLOUD_SQL_IAM_AUTH", True)
ip_type = os.environ.get("CLOUD_SQL_IP_TYPE", "public")
Expand All @@ -80,10 +79,11 @@ def asyncify_connection():
"enable_iam_auth": use_iam_auth,
Copy link
Copy Markdown
Contributor

Copilot AI Feb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the code now enforces that IAM auth must be enabled (raising an error if it's not), the enable_iam_auth parameter in connect_kwargs should be hardcoded to True instead of using the use_iam_auth variable. This makes the intent clearer and avoids passing a value that's always True at this point in the code.

Suggested change
"enable_iam_auth": use_iam_auth,
"enable_iam_auth": True,

"ip_type": ip_type,
}
if use_iam_auth:
connect_kwargs["password"] = get_iam_login_token()
else:
connect_kwargs["password"] = password
if not use_iam_auth:
raise RuntimeError(
"CLOUD_SQL_IAM_AUTH must be true when DB_DRIVER=cloudsql."
)
Comment on lines +82 to +85
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot Feb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Re-enable password auth when CLOUD_SQL_IAM_AUTH is false

This branch now throws whenever CLOUD_SQL_IAM_AUTH=false, which removes previously supported Cloud SQL password authentication and causes runtime failures for any deployment that explicitly disables IAM auth. The prior implementation used CLOUD_SQL_PASSWORD in this path, and repository logic still treats IAM as a default that can be overridden (for example, transfers/transfer.py says it should default to IAM unless explicitly disabled), so this is a behavioral regression that can break existing cloudsql environments on first DB use.

Useful? React with 👍 / 👎.

connect_kwargs["password"] = get_iam_login_token()

connection = connector.connect_async(instance_name, "asyncpg", **connect_kwargs)

Expand All @@ -106,7 +106,6 @@ def asyncify_connection():
def init_connection_pool(connector):
instance_name = os.environ.get("CLOUD_SQL_INSTANCE_NAME")
user = os.environ.get("CLOUD_SQL_USER")
password = os.environ.get("CLOUD_SQL_PASSWORD")
database = os.environ.get("CLOUD_SQL_DATABASE")
use_iam_auth = get_bool_env("CLOUD_SQL_IAM_AUTH", True)
ip_type = os.environ.get("CLOUD_SQL_IP_TYPE", "public")
Expand All @@ -118,10 +117,11 @@ def getconn():
"ip_type": ip_type,
"enable_iam_auth": use_iam_auth,
Copy link
Copy Markdown
Contributor

Copilot AI Feb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the code now enforces that IAM auth must be enabled (raising an error if it's not), the enable_iam_auth parameter in connect_kwargs should be hardcoded to True instead of using the use_iam_auth variable. This makes the intent clearer and avoids passing a value that's always True at this point in the code.

}
if use_iam_auth:
connect_kwargs["password"] = get_iam_login_token()
else:
connect_kwargs["password"] = password
if not use_iam_auth:
raise RuntimeError(
Comment on lines +120 to +121
Copy link
Copy Markdown
Contributor

Copilot AI Feb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The get_bool_env function can return a string value if the environment variable is set to something other than recognized boolean values ("true", "1", "yes", "false", "0", "no"). This could lead to unexpected behavior in the if not use_iam_auth: check. Consider adding validation to ensure use_iam_auth is actually a boolean, or handle the case where it might be an unexpected string value.

"CLOUD_SQL_IAM_AUTH must be true when DB_DRIVER=cloudsql."
)
connect_kwargs["password"] = get_iam_login_token()

conn = connector.connect(
instance_name, # The Cloud SQL instance name
Expand Down