helm configure #7

yorinasub17 · 2019-01-17T18:58:27Z

~~Depends on #8~~ MERGED

This implements the helm grant and helm configure subcommands which can be used to grant and configure a local helm client to access the specified helm server install in the target Kubernetes namespace.

yorinasub17 · 2019-01-17T18:58:42Z

This is ready for RDD review.

yorinasub17 · 2019-01-17T19:03:06Z

README.md

+  - `HELM_HOME`: The helm client home directory where the TLS certs are located.
+  - `TILLER_NAMESPACE`: The namespace where the helm server is installed.
+  - `HELM_TLS_VERIFY`: This will be set to true to enable TLS verification.
+  - `HELM_TLS_ENABLE`: This will be set to true to enable TLS authentication.


This is annoying, but helm doesn't have a concept of a config file. The default way for the user is:

helm --tiller-namespace NAMESPACE --tls --tls-verify

everytime they want to use helm. If they want multiple helm configs, this is even worse:

helm --home HELM_HOME_DIR --tiller-namespace NAMESPACE --tls --tls-verify # or if they want to share home dir but use different certs helm --tiller-namespace NAMESPACE --tls --tls-verify --tls-ca-cert ca.cert.pem --tls-cert helm.cert.pem --tls-key helm.key.pem

So the proposal here is to instead provide an environment file they can source that sets these as environment variables. Then this becomes:

source ~/.helm/env helm

yorinasub17 · 2019-01-18T15:33:15Z

From 1:1 with Jim. Aim for ideal experience:

configure => Output config file
kubergrunt helm => Sets the environment variables to call to helm
Make sure to document what it is doing
Look into adding config file to helm directly
- helm plugin
- slack to ask thoughts
- open github issue

Additional notes:

not everyone has bash chops so env / aliasing may not be great as a default option
Does env sourcing work with windows?

yorinasub17 · 2019-01-22T17:20:23Z

Re: config file - I found a github issue in helm on this topic (the OP is completely unrelated, but the discussion evolved to be about config files) and it looks like this will be a helmv3 feature. In that case, I might just stick with the env var style approach, and update this when helmv3 lands.

PS: I found a way to do this in Powershell and Windows Cmd: https://stackoverflow.com/questions/20077820/how-can-i-source-variables-from-a-bat-file-into-a-powershell-script/20078095#20078095. We could detect the OS as part of the command and create the relevant files.

yorinasub17 · 2019-01-28T21:14:24Z

This will be ready to review once #8 merges

rileykarson

I have a couple nitpicks/comments right now, +-1700 is a large change and I didn't look in enough depth for an approval

rileykarson · 2019-01-29T16:51:55Z

cmd/helm.go

@@ -15,15 +15,21 @@ import (
 )

 var (
+	// Shared configurations
+	tillerNamespaceFlag = cli.StringFlag{
+		Name:  "namespace",


nit: I wonder if tiller-namespace and namespace would be better naming here? I expect to use this namespace for a single item, the tiller server, while many resources will reside in the resource namespace.

rileykarson · 2019-01-29T16:53:41Z

cmd/helm.go

+		Usage: "The name of the RBAC user that should be granted access to Tiller. Pass in multiple times for multiple users.",
+	}
+	grantedServiceAccountsFlag = cli.StringSliceFlag{
+		Name:  "service-account",


Why not rbac-service-account for consistency? It's a little awkward to type, but namespacing all 3 under rbac makes the grouping more clear.

Well, ServiceAccounts are technically not a part of the RBAC system. They are grouped under core API.

But I think that is a minor detail and I agree with you about the namespace grouping so will do that.

adjusted to --rbac-service-account

rileykarson · 2019-01-29T16:54:56Z

cmd/helm.go

+	rbacUsers := cliContext.StringSlice(grantedRbacUsersFlag.Name)
+	serviceAccounts := cliContext.StringSlice(grantedServiceAccountsFlag.Name)
+	if len(rbacGroups) == 0 && len(rbacUsers) == 0 && len(serviceAccounts) == 0 {
+		return entrypoint.NewRequiredArgsError("At least one --rbac-group or --service-account is required")


But not --rbac-user?

Thanks forgot that when I added in rbac user support

rileykarson · 2019-01-29T16:57:47Z

helm/deploy.go

@@ -104,7 +106,7 @@ func Deploy(
 		logger.Errorf("Error deploying Helm server: %s", err)
 		return err
 	}
-	logger.Infof("Successfully deployed helm server in namespace %s with service account %s", namespace, serviceAccount)
+	logger.Infof("Successfully deployed helm server in namespace %s with service account %s", tillerNamespace, serviceAccount)


nit: s/helm server/Tiller here and a couple lines below?

rileykarson · 2019-01-29T16:59:33Z

helm/grant.go

+	return nil
+}
+
+// grantAccessToRBACGroups will grant access to the deployed Tiller server to the provided RBAC groups.


nit: outdated function name in comment

yorinasub17 · 2019-01-31T05:58:38Z

Ok the latest version has been verified to work with gruntwork-io/terraform-kubernetes-helm#9, so I am going to merge this and release.

For reference here is the passing circleci build: https://circleci.com/workflow-run/11f575c4-7d30-4423-a61f-cc0b55bdfc9e?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-checks-link

yorinasub17 commented Jan 17, 2019

View reviewed changes

yorinasub17 mentioned this pull request Jan 17, 2019

helm undeploy #8

Merged

yorinasub17 force-pushed the yori-helm-setup branch from 4ff5244 to 3608039 Compare January 24, 2019 06:26

yorinasub17 changed the title ~~[WIP] helm configure~~ [BLOCKED] helm configure Jan 28, 2019

yorinasub17 force-pushed the yori-helm-setup branch from 600f38f to ffcdabf Compare January 29, 2019 14:38

yorinasub17 changed the title ~~[BLOCKED] helm configure~~ helm configure Jan 29, 2019

yorinasub17 added 12 commits January 29, 2019 06:56

RDD: helm configure command

0f16b4b

Update README.md

52f0e87

WIP: helm configure skeleton

9e8c840

Implement grant command and relevant tests

1447b75

Implement the TLS section of the configure command

cab460a

Adjust names and labels

d068aaa

Implement remaining test

2554ffb

Environment file generation

44533b2

Fix pathing issue

cf9654c

Implement and test configure set namespace

2c807f7

use dot sourcing

22b04ef

Fix bugs

c05bce7

yorinasub17 force-pushed the yori-helm-setup branch from ffcdabf to c05bce7 Compare January 29, 2019 14:56

yorinasub17 added the ready-to-review label Jan 29, 2019

rileykarson reviewed Jan 29, 2019

View reviewed changes

yorinasub17 added 3 commits January 29, 2019 14:33

Address commentS

05e9a7a

Refactor to remove switch cases

32eddd8

Clean up logging

3d36430

yorinasub17 mentioned this pull request Jan 31, 2019

k8s-tiller module gruntwork-io/terraform-kubernetes-helm#9

Closed

yorinasub17 merged commit fc902f1 into yori-helm-tools Jan 31, 2019

yorinasub17 deleted the yori-helm-setup branch January 31, 2019 05:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

helm configure #7

helm configure #7

yorinasub17 commented Jan 17, 2019 •

edited

Loading

yorinasub17 commented Jan 17, 2019 •

edited

Loading

yorinasub17 Jan 17, 2019

yorinasub17 commented Jan 18, 2019 •

edited

Loading

yorinasub17 commented Jan 22, 2019

yorinasub17 commented Jan 28, 2019

rileykarson left a comment

rileykarson Jan 29, 2019

yorinasub17 Jan 29, 2019

rileykarson Jan 29, 2019

yorinasub17 Jan 29, 2019

yorinasub17 Jan 29, 2019

rileykarson Jan 29, 2019

yorinasub17 Jan 29, 2019

yorinasub17 Jan 29, 2019

rileykarson Jan 29, 2019

yorinasub17 Jan 29, 2019

rileykarson Jan 29, 2019

yorinasub17 Jan 29, 2019

yorinasub17 commented Jan 31, 2019

helm configure #7

helm configure #7

Conversation

yorinasub17 commented Jan 17, 2019 • edited Loading

yorinasub17 commented Jan 17, 2019 • edited Loading

Choose a reason for hiding this comment

yorinasub17 commented Jan 18, 2019 • edited Loading

yorinasub17 commented Jan 22, 2019

yorinasub17 commented Jan 28, 2019

rileykarson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yorinasub17 commented Jan 31, 2019

yorinasub17 commented Jan 17, 2019 •

edited

Loading

yorinasub17 commented Jan 17, 2019 •

edited

Loading

yorinasub17 commented Jan 18, 2019 •

edited

Loading