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

feat(vault): Create vault resources in GCP that are unique to the cluster #2289

Merged
merged 5 commits into from Nov 26, 2018

Conversation

@agentgonzo
Copy link
Member

agentgonzo commented Nov 20, 2018

Submitter checklist

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

Description

This prevents two different vaults in two different clusters using the
same GCP project having naming clashes for buckets/service-accounts/keys

Special notes for the reviewer(s)

Which issue this PR fixes

fixes #2286

@agentgonzo agentgonzo requested review from ccojocar and garethjevans Nov 20, 2018

Show resolved Hide resolved pkg/vault/vault.go
Show resolved Hide resolved pkg/cloud/gke/gcloud.go Outdated
Show resolved Hide resolved pkg/cloud/gke/gcloud.go Outdated

@jenkins-x-bot jenkins-x-bot requested review from mikecirioli and rajdavies Nov 20, 2018

@agentgonzo agentgonzo force-pushed the agentgonzo:vault-uniqueness branch from aa0bfbe to d7b6021 Nov 20, 2018

feat(vault): Create vault resources in GCP that are unique to the clu…
…ster

This prevents two different vaults in two different clusters using the
same GCP project having naming clashes for buckets/service-accounts/keys

@agentgonzo agentgonzo force-pushed the agentgonzo:vault-uniqueness branch from d7b6021 to 1c76411 Nov 20, 2018

@agentgonzo

This comment has been minimized.

Copy link
Member

agentgonzo commented Nov 21, 2018

/test bdd
previous build (4) seemed to pass

@agentgonzo

This comment has been minimized.

Copy link
Member

agentgonzo commented Nov 21, 2018

/retest

@agentgonzo

This comment has been minimized.

Copy link
Member

agentgonzo commented Nov 21, 2018

/test bdd

@@ -14,3 +17,43 @@ func TestGetRegionFromZone(t *testing.T) {
r = GetRegionFromZone("uswest1-d")
assert.Equal(t, r, "uswest1")
}

func TestGetSimplifiedClusterName(t *testing.T) {
simpleName := GetSimplifiedClusterName("gke_jenkinsx-dev_europe-west1-b_my-cluster-name")

This comment has been minimized.

@jtnord

jtnord Nov 22, 2018

Member

t.Parallel() ?

}

func TestShortClusterName(t *testing.T) {
kuber := kube_test.NewMockKuber()

This comment has been minimized.

@jtnord

jtnord Nov 22, 2018

Member

t.Parallel() ?

}

func TestClusterName(t *testing.T) {
kuber := kube_test.NewMockKuber()

This comment has been minimized.

@jtnord

jtnord Nov 22, 2018

Member

t.Parallel() ?

if err != nil {
return err
}
log.Infof("Current Cluster: %s\n", util.ColorInfo(clusterName))

This comment has been minimized.

@jtnord

jtnord Nov 22, 2018

Member

:sadpanda: that this is new code and is using the jx/log package and not logrus :-(

This comment has been minimized.

@agentgonzo

agentgonzo Nov 23, 2018

Member

is the jx.log package deprecated? I didn't know that. If so, I'll chaneg this to logrus

This comment has been minimized.

@agentgonzo

agentgonzo Nov 23, 2018

Member

OK, not a trivial thing as just changing to logrus.Infof gives:
INFO[0014] Creating GCP service account for Vault backend rather than the string, which is ugly for user output. Will reconsider for a future PR

config := &kmsConfig{
keyring: fmt.Sprintf("%s-keyring", vaultName),
key: fmt.Sprintf("%s-key", vaultName),
keyring: gke.VaultKeyringName(vaultName, clusterName),

This comment has been minimized.

@jtnord

jtnord Nov 22, 2018

Member

should this be tied to GKE here? others that use GKE have GKE in the function name so it is obvious

This comment has been minimized.

@agentgonzo

agentgonzo Nov 23, 2018

Member

So all of these required a fairly hefty refactor moving all these methods into the /cloud/gke package under the new subpackage of /cloud/gke/vault, but that's done

client, _, err := o.KubeClient()
if err != nil {
return "", errors.Wrap(err, "creating kubernetes client")
}

serviceAccountName := fmt.Sprintf("%s-auth-sa", vaultName)
serviceAccountName := gke.VaultAuthServiceAccountName(vaultName, clusterName)

This comment has been minimized.

@jtnord

jtnord Nov 22, 2018

Member

should this be tied to GKE here? others that use GKE have GKE in the function name so it is obvious

@@ -364,3 +370,7 @@ func (o *CreateVaultOptions) exposeVault(vaultService string) error {
options.Services = []string{vaultService}
return options.Run()
}

func (o *CreateVaultOptions) clusterName() (string, error) {
return gke.ShortClusterName(o.Kube())

This comment has been minimized.

@jtnord

jtnord Nov 22, 2018

Member

should this be tied to GKE here? others that use GKE have GKE in the function name so it is obvious

@@ -162,14 +166,19 @@ func (o *DeleteVaultOptions) removeGCPResources(vaultName string) error {
o.GKEZone = zone
}

sa := gke.VaultServiceAccountName(vaultName)
clusterName, err := gke.ShortClusterName(o.Kube())

This comment has been minimized.

@jtnord

jtnord Nov 22, 2018

Member

should this be tied to GKE here? others that use GKE have GKE in the function name so it is obvious

return err
}

sa := gke.VaultServiceAccountName(vaultName, clusterName)

This comment has been minimized.

@jtnord

jtnord Nov 22, 2018

Member

should this be tied to GKE here? others that use GKE have GKE in the function name so it is obvious

}

// CreateBucket Creates a bucket in GKE to store the backend (encrypted) data for vault
func CreateBucket(vaultName, clusterName, projectId, zone string) (string, error) {

This comment has been minimized.

@houndci-bot

houndci-bot Nov 23, 2018

Collaborator

func parameter projectId should be projectID

}

// CreateGCPServiceAccount creates a service account in GCP for the vault service
func CreateGCPServiceAccount(kubeClient kubernetes.Interface, vaultName, namespace, clusterName, projectId string) (string, error) {

This comment has been minimized.

@houndci-bot

houndci-bot Nov 23, 2018

Collaborator

func parameter projectId should be projectID

// If they are generic enough and needed elsewhere, we can move them up one level to more generic GCP methods.

// CreateKmsConfig creates a KMS config for the GKE Vault
func CreateKmsConfig(vaultName, clusterName, projectId string) (*KmsConfig, error) {

This comment has been minimized.

@houndci-bot

houndci-bot Nov 23, 2018

Collaborator

func parameter projectId should be projectID

}
)

type KmsConfig struct {

This comment has been minimized.

@houndci-bot

houndci-bot Nov 23, 2018

Collaborator

exported type KmsConfig should have comment or be unexported

)

var (
ServiceAccountRoles = []string{"roles/storage.objectAdmin",

This comment has been minimized.

@houndci-bot

houndci-bot Nov 23, 2018

Collaborator

exported var ServiceAccountRoles should have comment or be unexported

@jenkins-x-bot jenkins-x-bot added the lgtm label Nov 26, 2018

@jenkins-x-bot jenkins-x-bot added approved and removed lgtm labels Nov 26, 2018

@ccojocar

This comment has been minimized.

Copy link
Member

ccojocar commented Nov 26, 2018

/test bdd
/lgtm

@jenkins-x-bot jenkins-x-bot added the lgtm label Nov 26, 2018

@jenkins-x-bot

This comment has been minimized.

Copy link
Contributor

jenkins-x-bot commented Nov 26, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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:

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 712d9ed into jenkins-x:master Nov 26, 2018

3 checks passed

Hound 6 violations found.
serverless-jenkins succeeded
tide In merge pool.
Details

@agentgonzo agentgonzo deleted the agentgonzo:vault-uniqueness branch Nov 26, 2018

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