Add host_key_policy option to ComputeEngineSSHHook#66746
Open
potiuk wants to merge 1 commit into
Open
Conversation
Exposes paramiko's `MissingHostKeyPolicy` choice as a constructor argument so callers can opt into strict host-key verification on the SSH transport. The argument accepts the string aliases `"auto_add"`, `"reject"` and `"warning"` (which map to the matching `paramiko` policy classes) and also passes through any custom `paramiko.MissingHostKeyPolicy` instance — so a caller that wants to pin the remote host's key from GCE guest attributes / instance metadata can plug in a policy that loads it on the fly. The default is `"auto_add"`, preserving the historical behaviour of this hook; no migration is required for existing callers. The previous inline comment claiming the missing host-key check was unrelated to the local private key is removed — it conflated two different concerns and is replaced with a pointer to the new constructor argument. Generated-by: Claude Opus 4.7 (1M context) following the guidelines at https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#gen-ai-assisted-contributions
boring-cyborg
Bot
added
area:providers
provider:google
shahar1
reviewed
May 12, 2026
| callers can plug in a custom policy (e.g. one that loads pinned | ||
| host keys from GCE guest attributes). | ||
|
|
||
| Any other value raises :class:`AirflowException`. |
Contributor
There was a problem hiding this comment.
Suggested change
| Any other value raises :class:`AirflowException`. | |
| Any other value raises :class:`ValueError`. |
| """Tests for the ``host_key_policy`` constructor argument.""" | ||
|
|
||
| def test_default_is_auto_add(self): | ||
| import paramiko |
Contributor
There was a problem hiding this comment.
Is there a reason for making this import internal? (same goes for the other tests)
Comment on lines
+197
to
+201
| raise ValueError( | ||
| f"Unknown host_key_policy {self.host_key_policy!r}. " | ||
| "Expected one of 'auto_add', 'reject', 'warning', " | ||
| "or an instance of paramiko.MissingHostKeyPolicy." | ||
| ) |
Contributor
There was a problem hiding this comment.
Consider replacing with raise ValueError(...) from None as the KeyError is an implementation detail of the dict lookup
| cmd_timeout: int | ArgNotSet = NOTSET, | ||
| max_retries: int = 10, | ||
| impersonation_chain: str | None = None, | ||
| host_key_policy: str | paramiko.MissingHostKeyPolicy = "auto_add", |
Contributor
There was a problem hiding this comment.
Could you please add a docstring?
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.
Summary
Expose paramiko's
MissingHostKeyPolicychoice as a constructor argument onComputeEngineSSHHook, so callers can opt into strict host-key verification on the SSH transport. The hook previously hard-codedparamiko.AutoAddPolicy, which means callers who wanted the remote host authenticated had no way to ask for it.The new
host_key_policyargument accepts:"auto_add","reject"and"warning"— mapped to the matchingparamikopolicy classes;paramiko.MissingHostKeyPolicyinstance — so a caller that wants to pin the remote host's key from GCE guest attributes / instance metadata can plug in a policy that loads it on the fly.The default is
"auto_add", preserving the historical behaviour of the hook; no migration is required for existing callers.The previous inline comment that claimed the missing host-key check was unrelated to the local private key is removed — it conflated two different concerns — and replaced with a pointer to the new constructor argument.
Files changed
providers/google/src/airflow/providers/google/cloud/hooks/compute_ssh.py— newhost_key_policyparameter, helper resolver, applied in_connect_to_instance. Misleading comment removed.providers/google/tests/unit/google/cloud/hooks/test_compute_ssh.py— newTestHostKeyPolicyResolutionclass (4 cases: default, string aliases, custom instance, unknown-string error).Test plan
uv run --project providers/google pytest providers/google/tests/unit/google/cloud/hooks/test_compute_ssh.py— 22 / 22 passprek run --from-ref upstream/main --stage pre-commit— cleanMigration
None. Callers that don't pass
host_key_policyget the same paramikoAutoAddPolicybehaviour as before.Was generative AI tooling used to co-author this PR?
Generated-by: Claude Opus 4.7 (1M context) following the guidelines at https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#gen-ai-assisted-contributions