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

Bosh env unset #520

Merged
merged 3 commits into from
Oct 28, 2020
Merged

Bosh env unset #520

merged 3 commits into from
Oct 28, 2020

Conversation

iplay88keys
Copy link

@iplay88keys iplay88keys commented Oct 27, 2020

Resolves #457

@cf-gitbot
Copy link
Member

We have created an issue in Pivotal Tracker to manage this. Unfortunately, the Pivotal Tracker project is private so you may be unable to view the contents of the story.

The labels on this github issue will be updated when the story is started.

@iplay88keys
Copy link
Author

Unfortunately, I haven't been able to test the Powershell aspect manually. I do see evidence that it will work based on this stack overflow question, though: https://stackoverflow.com/questions/50064358/setting-environment-variables-to-an-empty-string-in-powershell

@kcboyle
Copy link
Contributor

kcboyle commented Oct 28, 2020

Some functionality tests:

→ go run main.go --env env.yml bosh-env
export CREDHUB_CA_CERT='-----BEGIN CERTIFICATE-----
-----END CERTIFICATE-----
'
export BOSH_CLIENT=client
export BOSH_CLIENT_SECRET=secret
export BOSH_ENVIRONMENT=10.0.0.10
export BOSH_CA_CERT='-----BEGIN CERTIFICATE-----
-----END CERTIFICATE-----
'
export CREDHUB_CLIENT=client
export CREDHUB_SECRET=secret
export CREDHUB_SERVER=https://10.0.0.10:8844

± |main ↑3 ✓| → om --env env.yml bosh-env
export CREDHUB_SERVER=https://10.0.0.10:8844
export CREDHUB_CA_CERT='-----BEGIN CERTIFICATE-----
-----END CERTIFICATE-----
'
export BOSH_CLIENT=client
export BOSH_CLIENT_SECRET=secret
export BOSH_ENVIRONMENT=10.0.0.10
export BOSH_CA_CERT='-----BEGIN CERTIFICATE-----
-----END CERTIFICATE-----
'
export CREDHUB_CLIENT=client
export CREDHUB_SECRET=secret

→ go run main.go --env env.yml bosh-env -b
export BOSH_CLIENT=client
export BOSH_CLIENT_SECRET=secret
export BOSH_ENVIRONMENT=10.0.0.10
export BOSH_CA_CERT='-----BEGIN CERTIFICATE-----
-----END CERTIFICATE-----
'

→ go run main.go --env env.yml bosh-env -c
export CREDHUB_CLIENT=client
export CREDHUB_SECRET=secret
export CREDHUB_SERVER=https://10.0.0.10:8844
export CREDHUB_CA_CERT='-----BEGIN CERTIFICATE-----
-----END CERTIFICATE-----
'

→ go run main.go --env env.yml bosh-env --unset --bosh
unset BOSH_ALL_PROXY
unset CREDHUB_PROXY
unset BOSH_CLIENT
unset BOSH_CLIENT_SECRET
unset BOSH_ENVIRONMENT
unset BOSH_CA_CERT

→ go run main.go --env env.yml bosh-env --unset --credhub
unset BOSH_ALL_PROXY
unset CREDHUB_PROXY
unset CREDHUB_CLIENT
unset CREDHUB_SECRET
unset CREDHUB_SERVER
unset CREDHUB_CA_CERT

→ go run main.go --env env.yml bosh-env --unset
unset BOSH_ALL_PROXY
unset CREDHUB_PROXY
unset BOSH_CLIENT
unset BOSH_CLIENT_SECRET
unset BOSH_ENVIRONMENT
unset BOSH_CA_CERT
unset CREDHUB_CLIENT
unset CREDHUB_SECRET
unset CREDHUB_SERVER
unset CREDHUB_CA_CERT

I have a couple of minor concerns I'm going to look into a little more.
I noticed that --unset unsets the values CREDHUB_PROXY and BOSH_ALL_PROXY,
but we don't appear to actually set those values when we run a regular bosh env
(also visible in the first test run):

→ eval $(om bosh-env ~/workspace/platform-automation-deployments/reference-gcp/env.yml)
2020/10/28 07:43:18 could not execute "bosh-env": could not make api request to director credentials endpoint: target flag is required. Run `om help` for more info.

→ eval $(om --env env.yml bosh-env)

→ env | grep CRED
CREDHUB_SECRET=secret
CREDHUB_CLIENT=client
CREDHUB_SERVER=https://10.0.0.10:8844
CREDHUB_CA_CERT=cert

→ env | grep BOSH
BOSH_CLIENT=client
BOSH_ENVIRONMENT=10.0.0.10
BOSH_CLIENT_SECRET=secret
BOSH_CA_CERT=cert

What are the values of CREDHUB_PROXY and BOSH_ALL_PROXY used for?
are they being secretly set somewhere,
so we should allow them to be unset?

@kcboyle
Copy link
Contributor

kcboyle commented Oct 28, 2020

Ah, I'm just not using an ssh key, so that value wasn't set.

@kcboyle
Copy link
Contributor

kcboyle commented Oct 28, 2020

If you're using the ssh-private-key, the variables BOSH_ALL_PROXY and CREDHUB_PROXY seem like they'd be required for accessing bosh and credhub, respectively.

It looks like --unset will always unset those two vars, regardless of whether --bosh or --credhub is set. This seems like a problem. @iplay88keys did you look into the behavior with --ssh-private-key?

@@ -91,6 +92,10 @@ func (be BoshEnvironment) Execute(args []string) error {
}

variables := make(map[string]string)
unsetVariables := []string{
"BOSH_ALL_PROXY",
Copy link
Contributor

Choose a reason for hiding this comment

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

See comment in Conversation. This seems like a potential problem if you are only unsetting credhub or bosh vars.

Copy link
Author

Choose a reason for hiding this comment

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

I'll respond in the conversation about them.

@iplay88keys
Copy link
Author

iplay88keys commented Oct 28, 2020

When I was thinking through the user experience, It seemed unlikely the user would want to provide a ssh key when unsetting the variables. I moved them out of the ssh key flag check in order to do that, but I didn't think through the case you mentioned where it unsets both no matter what. I should probably just wrap them in the flag checks, but I wanted to see what you thought. Do you think it should behave the same way that bosh-env currently works and require a ssh key to remove those two environment variables (BOSH_ALL_PROXY and CREDHUB_PROXY)? Or should it just respect the -b/--bosh and -c/--credhub flags?

@kcboyle
Copy link
Contributor

kcboyle commented Oct 28, 2020

Just respect the --bosh and --credhub flags. That seems like an easier place for them to be, and I feel like requiring the ssh key could produce some funky and/or unexpected behavior for folks using --unset

@iplay88keys
Copy link
Author

Ok, I made the change and ran the tests!

@kcboyle
Copy link
Contributor

kcboyle commented Oct 28, 2020

looks good to me! Thanks for the quick fixes. merging, and will update the changelog

→ go run main.go --env env.yml bosh-env --unset --credhub
unset CREDHUB_CLIENT
unset CREDHUB_SECRET
unset CREDHUB_SERVER
unset CREDHUB_CA_CERT
unset CREDHUB_PROXY

→ go run main.go --env env.yml bosh-env --unset --bosh
unset BOSH_CLIENT
unset BOSH_CLIENT_SECRET
unset BOSH_ENVIRONMENT
unset BOSH_CA_CERT
unset BOSH_ALL_PROXY

→ go run main.go --env env.yml bosh-env --unset
unset BOSH_CLIENT
unset BOSH_CLIENT_SECRET
unset BOSH_ENVIRONMENT
unset BOSH_CA_CERT
unset BOSH_ALL_PROXY
unset CREDHUB_CLIENT
unset CREDHUB_SECRET
unset CREDHUB_SERVER
unset CREDHUB_CA_CERT
unset CREDHUB_PROXY

@kcboyle kcboyle merged commit bd49646 into pivotal-cf:main Oct 28, 2020
@iplay88keys iplay88keys deleted the bosh-env-unset branch October 28, 2020 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

om bosh-env should be able to un-set vars
3 participants