GoogleCloudPlatform / berglas

A tool for managing secrets on Google Cloud
https://cloud.google.com/secret-manager
Apache License 2.0
1.24k stars 96 forks source link

Change berglas exec to use sys.Exec? #171

Closed thegoodlittlescript closed 2 years ago

thegoodlittlescript commented 2 years ago

We are using berglas exec in k8s, and we've run into the same issue outlined in these issues:

Looks to me like you've taken some steps to address this 0.6.0, but we're still experiencing it in 0.6.2. The end result is that occasionally our CronJob exits, errors as above (non-zero), and re-runs.

I wonder, should berglas exec actually exec as in sys.Exec? That way the command would replace berglas and all signal handling would be done directly on the process. Is that an approach you'd be open to?

sethvargo commented 2 years ago

Hi @thegoodlittlescript

The primary reason it behaves this way is that I had a vision (not implemented) that you could signal the berglas process and it would re-process the secrets (to check for updates) and re-spawn the process if values had changed. I'm not opposed to switching to exec, except I think it doesn't work on Windows?

thegoodlittlescript commented 2 years ago

Hi @sethvargo - I imagine you're right. I wonder however if working on windows should be a goal of this command. Basically I wonder if there should be different commands for the two different platforms.

berglas exec
berglas win-exec

For example, the origin of all this is signal handling but from everything I see signals are not really a windows thing:

Maybe that's a hint that exec on windows/unix should be allowed to be different? In my own work I've leaned on unix exec so that I don't have to get into things like signal handling. But honestly I haven't had to design anything cross-platform and I don't know good patterns for it.

sethvargo commented 2 years ago

I'll look into it.

sethvargo commented 2 years ago

Let me know if that works for you ^

thegoodlittlescript commented 2 years ago

I built this, deployed it, and have been watching it all day. It seems fine -- the process is still working. Secrets appear to be set as before and as expected there are no more error messages around signals.

Thanks for the quick turnaround! I look forward to seeing this in a future release. Also fwiw I think your design around berglas refs is excellent. Being able to treat secrets like configs has been very helpful. Both our devs and SREs appreciate how visible everything is now.

github-actions[bot] commented 2 years ago

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.