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

Local client configure on deploy #14

Merged
merged 7 commits into from
Feb 1, 2019

Conversation

yorinasub17
Copy link
Contributor

This adds additional optional flags to helm deploy so that you can grant and configure the local client in one step as part of the deploy.

This also additionally copies in the helm docs from terraform-kubernetes-helm so that that is available here as well as a reference.

assert.NoError(t, Deploy(kubectlOptions, namespaceName, serviceAccountName, tlsOptions))

// Grant and configure client as a new service account, testing the flow
serviceAccountKubectlOptions := grantAndConfigureClientAsServiceAccount(t, terratestKubectlOptions, kubectlOptions, tlsOptions)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: we still test the same underlying functions, since that is what is used by Deploy.

@@ -0,0 +1,222 @@
# What is Helm?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy paste job

Copy link

@rileykarson rileykarson left a comment

Choose a reason for hiding this comment

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

Generally LGTM, just a couple comments on docs and one question where the code is unintuitive.

@@ -168,16 +170,16 @@ basic helm server, this subcommand contains features such as:
- Tying certificate access to RBAC roles to harden access to the Tiller server.

**Note**: This command does not create `Namespaces` or `ServiceAccounts`, delegating that responsibility to other
systems. Checkout our [terraform-kubernetes-helm module](https://github.com/gruntwork-io/terraform-kubernetes-helm) for
an end to end implementation.
systems.

Choose a reason for hiding this comment

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

Should we give an example of the systems, such as our modules or the operator using kubectl?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes good point. I guess I had this thought that things are changing quickly and fast so I wasn't sure what examples to give here. I'll add a TODO for now, and a github issue so it isn't lost.

README.md Outdated
@@ -194,6 +196,8 @@ This will:
- Store the Certificate Authority private key in a new `Secret` in the `kube-system` namespace.
- Launch Tiller using the generated TLS certificate in the specified `Namespace` with the specified `ServiceAccount`.

You can also optionally grant access to an RBAC entity and configure the local helm client to use that using one of `--rbac-user`, `--rbac-group`, `--rbac-service-account` options.,

Choose a reason for hiding this comment

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

As a reader, it's not immediately clear why I wouldn't want to always include one of these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, you know what? You bring up a good point. I originally had the thought that you wouldn't want to setup the local helm client in a CI job during development, but I realized that you most likely wouldn't do development like that.

return rbacEntity, setEntities, err
}
}
if setEntities != 1 {

Choose a reason for hiding this comment

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

Why do we pass out setEntities and check for 0 there instead of making this check > 1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Simplified this by requiring that you pass in an rbac entity to deploy.

@yorinasub17
Copy link
Contributor Author

UPDATE: I made one more change 900a0e7

Instead of using a label to store the RBAC entity on the client certs, I switched to using an annotation. The difference is that labels are indexed for lookups, while annotations are not. I did this because:

  • You would most likely do a name lookup if you have the rbac entity name instead of a label lookup
  • labels have the same restrictions as a name, which is that you can't have special characters (other than _ and .) but rbac entity names like group and user can (e.g system:master).

@yorinasub17
Copy link
Contributor Author

Merging and releasing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants