Skip to content
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.

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

Only do substitutions for container path conversions with resolved paths #12313

Merged
merged 2 commits into from
Jun 9, 2022

Conversation

AlanCoding
Copy link
Member

SUMMARY

We have a report that, on some systems, there is a symlink /var/run -> ../run

I've added tests here that create a symlink for case-in-point like /tmp/dst -> /tmp/src

Before this change, running jobs with AWX_ISOLATION_BASE_PATH set to a symlink of this pattern would error like:

Traceback (most recent call last):
  File "/awx_devel/awx/main/tasks/jobs.py", line 469, in run
    credential.credential_type.inject_credential(credential, env, self.safe_cred_env, args, private_data_dir)
  File "/awx_devel/awx/main/models/credential/__init__.py", line 507, in inject_credential
    container_path = to_container_path(path, private_data_dir)
  File "/awx_devel/awx/main/utils/execution_environments.py", line 62, in to_container_path
    raise RuntimeError(f'Cannot convert path {path} unless it is a subdir of {private_data_dir}')
RuntimeError: Cannot convert path /tmp/foobar/awx_117_j_ds0t3g/env/tmpw093dey9 unless it is a subdir of /tmp/foobar/awx_117_j_ds0t3g

After this change, that works fine.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME
  • API

@AlanCoding AlanCoding marked this pull request as ready for review June 2, 2022 15:45
@AlanCoding
Copy link
Member Author

I'm pretty happy with this patch. It takes some thought, but this code seems clearly safer.

I moved the method to_host_path because it was only used in the tests. If our method to convert from host->container path needs updating for these reason, then it would stand to reason this method would need updating too. But we don't use it except for in the tests, so it would be pointless to do so.

@AlanCoding AlanCoding merged commit 4543f69 into ansible:devel Jun 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants