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

Interactive mode in interpreter sessions #44

Merged

Conversation

razvan-agape
Copy link
Contributor

@razvan-agape razvan-agape commented Apr 15, 2024

Problem description

secrets-init cannot be used in scenarios when you need to kubectl exec -i -t <pod-name> -- /bin/sh|python3 into a pod and run commands that require Secrets Manager secrets (say you want to query a DB and you need the credentials). That is because secrets-init is launching a new process with a dedicated process group for it and only sets stdout and stderr of the process to the "calling" process (you can see the logs/errors). Since stdin is not connected, the process exits immediately after the process is started.
When you exec into the pod, it would be nice to use secrets-init to fetch the secrets and keep the terminal console open.

Current behaviour

Say there is a running pod called terminal in the default namespace, running python3 image, with an env var configured:

- name: GSM_SECRET
  value: aws/gcp-sm-secret-ref

From local machine, running kubectl exec -i -t terminal -- python3, the command opens the python interpreter but secret env vars are not populated.

Running kubectl exec -i -t terminal -- secrets-init --provider <aws/gcp> python3 exits without any exit code/reason.

Desired behaviour

Running kubectl exec -i -t terminal -- secrets-init --provider <aws/gcp> python3, it would be nice to connect to the python console process and run python commands. Running

import os
os.environ['GSM_SECRET']

should return the "secret" value obtained by secrets-init from the cloud secrets manager.

Proposed solution

This PR adds a new command line argument to secrets-init (--interactive/-i) which will bind the launched process stdin to the "calling" process stdin.

### However...

The changes present in this pr are changes that worked for us (meaning that we can run secrets-init in interactive mode and do stuff), however, there are two problems:

  1. The process fails to treat window size change events. We see this error: ERRO[0000] failed to send system signal to the process args="[python3]" error="no such process" path=/usr/local/bin/python3 pid=93 signal=SIGWINCH
  2. The only way we managed to keep the interpreter console opened is by not switching to another GPID. And this in my opinion is breaking one of the core functions of secrets-init to be an init system.

I would like to receive some feedback from anyone who has any idea on how to fix the above issues and get this change merged.

Update 8th of May

I managed to fix the issue with the SIGWINCH signal by setting the new process to run in foreground. So, in interactive mode, the only changes to the process config are:

  • bind stdin
  • set process to run in foreground

Tested this configuration on Google Kubernetes Engine v1.27.11-gke.1062001 using kube-secrets-init:0.5.0 and secrets-init works well. In non-interactive mode, the behaviour is the same, so backwards compatibility is not an issue. Opening a shell in the pod (kubectl -n <ns> exec -it secret-init-terminal-test -- /helper/bin/secrets-init -i --provider=google python3).

Additionally, I updated the Dockerfile to run the container under a non root user.

Bind stdin during interactive mode
@iamsaso
Copy link

iamsaso commented Apr 15, 2024

@razvan-agape you can probably ignore SIGWINCH by extending the existing filter in the Goroutine for signals forwarding

if sig == syscall.SIGCHLD || sig == syscall.SIGURG || sig == syscall.SIGWINCH {
  continue
}

@razvan-agape
Copy link
Contributor Author

@razvan-agape you can probably ignore SIGWINCH by extending the existing filter in the Goroutine for signals forwarding

if sig == syscall.SIGCHLD || sig == syscall.SIGURG || sig == syscall.SIGWINCH {
  continue
}

Hey, @iamsaso. I had a deeper look into this and I think I found a solution for what we want to achieve. tldr, the solution was to set the process to run as 'foreground', which will bind the TTY to the child process. This way, secrets-init functions as normal, by launching the child process in a dedicated process group. Plus, the issue with the SIGWINCH signal no longer occurs.
I will continue to test this to make sure it works as expected in multiple use-cases, but this might be the final form :)

@razvan-agape
Copy link
Contributor Author

@alexei-led can you provide some input here?

Copy link
Collaborator

@alexei-led alexei-led left a comment

Choose a reason for hiding this comment

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

LGTM

@alexei-led alexei-led merged commit 73eaa84 into doitintl:master May 13, 2024
1 check failed
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.

3 participants