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

Save secrets to vault instead of local files #2276

Merged
merged 18 commits into from Nov 28, 2018

Conversation

@agentgonzo
Copy link
Member

agentgonzo commented Nov 16, 2018

Submitter checklist

  • Change is code complete and matches issue description.
  • Change is covered by existing or new tests.

Description

Save secret data to a vault installation rather than local config files

Special notes for the reviewer(s)

Summary of the changes:

  • Extracting an interface for AuthConfigService that can be backed by either a Vault or the existing file-system
  • Installing a system vault (named jx-vault in the jx namespace) on a install when using either the --vault or --gitops flags
  • Using the new Vault-backed AuthConfigService to save secrets if the above is true

I'm not sure what will break (and thus what needs subsequently 'fixing') as a result of this if secrets are stored in vault rather than on the file-system so could do with pointers as to which parts need updating after as a result of this PR. I don't plan to merge it leaving 'broken' parts of the workflow.

Which issue this PR fixes

fixes #

Show resolved Hide resolved pkg/vault/jxvaulter.go
Show resolved Hide resolved pkg/jenkins/utils.go Outdated
Show resolved Hide resolved pkg/jenkins/utils.go Outdated
Show resolved Hide resolved pkg/auth/vault_based_config_saver.go Outdated
Show resolved Hide resolved pkg/auth/types.go Outdated
Show resolved Hide resolved pkg/auth/interfaces.go Outdated
Show resolved Hide resolved pkg/auth/file_based_config_saver.go Outdated
Show resolved Hide resolved pkg/auth/file_based_config_saver.go Outdated
Show resolved Hide resolved pkg/auth/auth_config_service.go Outdated
Show resolved Hide resolved pkg/auth/auth_config_service.go Outdated

@jenkins-x-bot jenkins-x-bot added size/XL and removed size/L labels Nov 16, 2018

Show resolved Hide resolved pkg/jx/cmd/common_issues.go
Show resolved Hide resolved pkg/jx/cmd/common_auth.go
Show resolved Hide resolved pkg/jx/cmd/common_auth.go
Show resolved Hide resolved pkg/jx/cmd/common_chat.go
Show resolved Hide resolved pkg/jx/cmd/common_addon.go
Show resolved Hide resolved pkg/gits/provider.go
Show resolved Hide resolved pkg/gits/provider.go
Show resolved Hide resolved pkg/gits/git_repo.go
Show resolved Hide resolved pkg/gits/git_repo.go
Show resolved Hide resolved pkg/auth/file_based_config_saver.go Outdated
Show resolved Hide resolved pkg/vault/constants.go
vault, err := f.GetSystemVault()
v := auth.NewVaultBasedAuthConfigService(configName, vault)
return v, err
} else {

This comment has been minimized.

@houndci-bot

houndci-bot Nov 19, 2018

Collaborator

if block ends with a return statement, so drop this else and outdent its block

feat(vault): Using a system vault if necessary to store/retrieve secrets
This will fail if there is no system vault installed.

@agentgonzo agentgonzo force-pushed the agentgonzo:vault-install branch from 187b98d to a7585e7 Nov 19, 2018

@agentgonzo agentgonzo force-pushed the agentgonzo:vault-install branch from a7585e7 to a9e64dc Nov 19, 2018

@agentgonzo

This comment has been minimized.

Copy link
Member

agentgonzo commented Nov 19, 2018

/test bdd

@agentgonzo

This comment has been minimized.

Copy link
Member

agentgonzo commented Nov 19, 2018

/retest

1 similar comment
@jstrachan

This comment has been minimized.

Copy link
Member

jstrachan commented Nov 19, 2018

/retest

@agentgonzo

This comment has been minimized.

Copy link
Member

agentgonzo commented Nov 19, 2018

/meow

@agentgonzo

This comment has been minimized.

Copy link
Member

agentgonzo commented Nov 19, 2018

@jstrachan - even though I can't trigger the test build for this, it's in a state where I would appreciate your eyes over it.

@rawlingsj

This comment has been minimized.

Copy link
Member

rawlingsj commented Nov 19, 2018

/test bdd

@agentgonzo

This comment has been minimized.

Copy link
Member

agentgonzo commented Nov 19, 2018

/meow

@agentgonzo

This comment has been minimized.

Copy link
Member

agentgonzo commented Nov 19, 2018

Looks like the bot responds to @rawlingsj but not me :-(

@rawlingsj

This comment has been minimized.

Copy link
Member

rawlingsj commented Nov 19, 2018

Looks like the bot responds to @rawlingsj but not me :-(

there was an issue earlier that I saw and everything seems back to normal now, if you trigger with the magic comment again it should work. Let me know on slack if not.

@agentgonzo

This comment has been minimized.

Copy link
Member

agentgonzo commented Nov 20, 2018

/meow

@agentgonzo

This comment has been minimized.

Copy link
Member

agentgonzo commented Nov 20, 2018

/retest

1 similar comment
@agentgonzo

This comment has been minimized.

Copy link
Member

agentgonzo commented Nov 20, 2018

/retest

@amuniz

amuniz approved these changes Nov 20, 2018

}

config.CurrentServer = url
return s.saver.SaveConfig(s.config)

This comment has been minimized.

@amuniz

amuniz Nov 20, 2018

Member

NIT: This call looks quite weird to me. s already has everything to do the job (which is s.saver and s.config), so something like s.SaveConfig() seems more natural.

This comment has been minimized.

@agentgonzo

agentgonzo Nov 27, 2018

Member

it looks weird to me too, but adding a wrapper method doesn't appear to be the go way

@jenkins-x-bot jenkins-x-bot removed the lgtm label Nov 27, 2018

@jenkins-x-bot

This comment has been minimized.

Copy link
Contributor

jenkins-x-bot commented Nov 27, 2018

New changes are detected. LGTM label has been removed.

@agentgonzo agentgonzo force-pushed the agentgonzo:vault-install branch from 9e07177 to 6c7e4e6 Nov 27, 2018

Merge remote-tracking branch 'origin/master' into vault-install
# Conflicts:
#	pkg/gits/provider.go
#	pkg/jx/cmd/create_vault.go
#	pkg/tests/helpers.go

@agentgonzo agentgonzo force-pushed the agentgonzo:vault-install branch from 5ef9bcd to 7aa32c3 Nov 27, 2018

@ccojocar

This comment has been minimized.

Copy link
Member

ccojocar commented Nov 28, 2018

/retest

@agentgonzo

This comment has been minimized.

Copy link
Member

agentgonzo commented Nov 28, 2018

/test bdd

test(gke): do not run in parallel the tests which mock the same method
It seems that pegomock has a concurrency issue in this case, which leads
to random failures.

@agentgonzo agentgonzo force-pushed the agentgonzo:vault-install branch from 0d396f1 to ac6b0c6 Nov 28, 2018

@ccojocar

This comment has been minimized.

Copy link
Member

ccojocar commented Nov 28, 2018

/retest

@agentgonzo agentgonzo force-pushed the agentgonzo:vault-install branch from 550a094 to 51e520c Nov 28, 2018

@ccojocar

This comment has been minimized.

Copy link
Member

ccojocar commented Nov 28, 2018

/retest

vault(vault): Disable storing secrets in vault for --gitops for now
This should be reenabled once we can consume the secrets properly.

@agentgonzo agentgonzo force-pushed the agentgonzo:vault-install branch from 416d0ee to 07db653 Nov 28, 2018

@ccojocar

This comment has been minimized.

Copy link
Member

ccojocar commented Nov 28, 2018

/hold cancel

@ccojocar

This comment has been minimized.

Copy link
Member

ccojocar commented Nov 28, 2018

/test bdd

@agentgonzo

This comment has been minimized.

Copy link
Member

agentgonzo commented Nov 28, 2018

/retest

@agentgonzo

This comment has been minimized.

Copy link
Member

agentgonzo commented Nov 28, 2018

/test bdd

@agentgonzo agentgonzo added the lgtm label Nov 28, 2018

@agentgonzo

This comment has been minimized.

Copy link
Member

agentgonzo commented Nov 28, 2018

/lgtm

@jenkins-x-bot

This comment has been minimized.

Copy link
Contributor

jenkins-x-bot commented Nov 28, 2018

@agentgonzo: you cannot LGTM your own PR.

In response to this:

/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@jenkins-x-bot

This comment has been minimized.

Copy link
Contributor

jenkins-x-bot commented Nov 28, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: agentgonzo, amuniz, ccojocar

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [agentgonzo,amuniz,ccojocar]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@jenkins-x-bot jenkins-x-bot merged commit fc96f50 into jenkins-x:master Nov 28, 2018

2 of 3 checks passed

tide Not mergeable. Needs updatebot label.
Details
Hound 17 violations found.
serverless-jenkins succeeded

@agentgonzo agentgonzo deleted the agentgonzo:vault-install branch Nov 29, 2018

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