Skip to content

Add function to handle Rundeck environment variables and options for execution in pod#161

Open
greenmath wants to merge 3 commits intorundeck-plugins:masterfrom
greenmath:add-rd-env-handling
Open

Add function to handle Rundeck environment variables and options for execution in pod#161
greenmath wants to merge 3 commits intorundeck-plugins:masterfrom
greenmath:add-rd-env-handling

Conversation

@greenmath
Copy link
Copy Markdown

I like to improve the script and command execution in pods in the way, that the environment variables set by Rundeck like the possible options of a job, are propagated to the pod. And create with this a behavior like running jobs on the local machine. For this, I wrote two functions.

  • common.handle_rundeck_environment_variables: This function is called in pods-run-script.py and pods-node-executor.py. In the function all environment variables of the current Rundeck process are filtered for just variables where the key starts with RD_. Also the function looks for variables that starts with RD_FILE_ that are uploaded files. This files are needed to uploaded to the pod too. The default directory is /tmp or what in RD_NODE_FILE_COPY_DESTINATION_DIR is defined.
    The function returns two arrays. The first array including all environment variables and can be added in front of the execution command. The first element is the command env and the following elements in the array are strings in the format 'key=value'. The second array contains all paths of uploaded files. This array can be used for my second function.

  • common.clean_up_temporary_files: This function is called in the end of pods-run-script.py and pods-node-executor.py. Like the name says the function delete uploaded files from the pod.
    In the pods-run-script function the temporary created script is added to the list of to deleting files.

My Background:
I like to use Rundeck in a kind “as a service” for multiple departments in my organization. To separate the environment of every department I like to use pods in my k8s. With this separation the departments can’t break or delete setups from other departments. But to use pods like running jobs on the local Rundeck machine it is needed to also have all Rundeck environment variables and uploaded files inside the pod.

@fdevans
Copy link
Copy Markdown
Contributor

fdevans commented Mar 6, 2026

Thank you for this contribution! The use case is clear - you want to use Kubernetes pods as isolated execution environments while maintaining the same Rundeck environment variable and file option behavior as local execution.

However, we have several concerns with the current implementation:

1. Breaking change - should be opt-in

This changes behavior for all users by always propagating environment variables. This should be an optional feature controlled by a configuration parameter (e.g., propagate_environment: true/false).

2. Security concerns

Propagating ALL RD_* variables could expose sensitive information. Consider:

  • RD_JOB_PASSWORD and other credential variables
  • Internal Rundeck configuration that shouldn't be in pods
  • Potential for environment variable injection

You might want to either:

  • Allow filtering/excluding certain variables
  • Only propagate specific variables via configuration
  • Document the security implications

3. Error handling issues

In handle_rundeck_environment_variables(), the try/finally block always sets the file path even if the copy fails:

try:
    copy_file(...)
finally:
    rundeck_variables[file_key] = full_path  # Sets even if copy failed!
    files_copied.append(full_path)

This should be try/except with proper error handling.

4. Cleanup reliability

In pods-run-script.py, if the main command fails and exits early (line 148), the cleanup never runs. Consider using a try/finally pattern to ensure cleanup always happens.

5. Missing documentation

This feature needs:

  • Documentation in the README explaining the feature
  • Examples of when to use it
  • Security considerations
  • Configuration options

Recommendation Summary

  1. Make this an opt-in feature with a configuration parameter
  2. Fix the error handling in the file copy logic
  3. Add proper cleanup with try/finally
  4. Document the feature and security implications
  5. Consider allowing variable filtering/exclusion
  6. Rebase on the latest master branch

If you're willing to make these changes, we'd be happy to reconsider this PR. However, please note that even with these improvements, we may still have concerns around the security implications and overall approach. We want to be transparent that addressing these items doesn't guarantee acceptance, but it would allow us to have a more thorough discussion about whether this feature fits the plugin's direction.

Thanks for the contribution and for understanding!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants