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

Introduced shipperctl admin clusters apply for setting up and updating cluster configuration #45

Merged
merged 1 commit into from Nov 23, 2018

Conversation

Projects
None yet
4 participants
@parhamdoustdar
Copy link
Contributor

parhamdoustdar commented Nov 19, 2018

This pull request introduces the shipperctl command line utility. For now, it only has the shipperctl clusters apply command, which accepts a cluster configuration and uses it to set up management and application clusters.

Other things I did that had nothing to do with the original intent of this branch were to upgrade the version of dep we're using to manage our dependencies, and to exclude all the errors beginning with vendor from the gometalinter output. These were done to make our CI pipeline pass.

@parhamdoustdar parhamdoustdar requested a review from kanatohodets Nov 19, 2018

@ksurent
Copy link
Contributor

ksurent left a comment

A bunch of files need to have their imports formatted (e.g. cmd/shipperctl/configurator/cluster.go but there are more). The invocation should be goimports -local github.com/bookingcom/shipper.

Show resolved Hide resolved cmd/shipperctl/configurator/errors.go Outdated
Show resolved Hide resolved pkg/crds/numbers.go
Show resolved Hide resolved cmd/shipperctl/cmd/apply.go Outdated
Show resolved Hide resolved cmd/shipperctl/cmd/apply.go Outdated

@parhamdoustdar parhamdoustdar force-pushed the shipperctl-admin-join-clusters branch from 1d94cbf to b39a1dd Nov 20, 2018

Show resolved Hide resolved .travis.yml

var applyCmd = &cobra.Command{
Use: "apply",
Short: "Set up management and application clusters to work with Shipper",

This comment has been minimized.

@icanhazbroccoli

icanhazbroccoli Nov 21, 2018

Contributor

I appreciate these descriptions, it gives an idea what the package does. I'm also wondering if it makes any sense to write regular package godoc-friendly comments so the automatically built documentation will include it right away?

Show resolved Hide resolved cmd/shipperctl/configurator/cluster.go
@kanatohodets
Copy link
Contributor

kanatohodets left a comment

I'd like to see slightly more robust validation of the cluster spec section.

For example, a clusters.yaml like the following:

managementClusters:
- name: microk8s
applicationClusters:
- name: microk8s

Yields errors like so:

$ ./shipperctl admin clusters apply -f clusters.yaml
Setting up management cluster microk8s:
Registering or updating custom resource definitions... done
Creating a namespace called shipper-system... already exists. Skipping
Creating a service account called shipper... already exists. Skipping
Creating a ClusterRole called shipper-admin... already exists. Skipping
Creating a ClusterRoleBinding called shipper... already exists. Skipping
Creating a Role called secret-reader in the shipper-system namespace... already exists. Skipping
Creating a RoleBinding called shipper in the shipper-system namespace... already exists. Skipping
Finished setting up cluster microk8s

Setting up application cluster microk8s:
Creating a namespace called shipper-system... already exists. Skipping
Creating a service account called shipper... already exists. Skipping
Creating a ClusterRoleBinding called shipper-cluster-admin... already exists. Skipping
Finished setting up cluster microk8s

Joining management cluster microk8s to application cluster microk8s:
Creating or updating the cluster object for cluster microk8s on the management cluster... Error: Cluster.shipper.booking.com "microk8s" is invalid: []: Invalid value: map[string]interface {}{"metadata":map[string]interface {}{"name":"microk8s", "selfLink":"/apis/shipper.booking.com/v1alpha1/clusters/microk8s", "uid":"4c3996ae-ede0-11e8-b526-e4b97a20b8f0", "resourceVersion":"744943", "generation":1, "creationTimestamp":"2018-11-21T22:53:44Z"}, "spec":map[string]interface {}{"region":"", "apiMaster":"http://192.168.1.14:8080", "scheduler":map[string]interface {}{"unschedulable":false}, "capabilities":interface {}(nil)}, "status":map[string]interface {}{"inService":false}, "kind":"Cluster", "apiVersion":"shipper.booking.com/v1alpha1"}: validation failure list:
spec.capabilities in body must be of type array: "null"
Usage:
  shipperctl admin clusters apply [flags]

Flags:
  -f, --file string                       config file (default "clusters.yaml")
  -h, --help                              help for apply
      --kube-config string                the path to the Kubernetes configuration file (default "~/.kube/config")
  -n, --shipper-system-namespace string   the namespace where Shipper is running (default "shipper-system")

2018/11/21 23:56:42 Cluster.shipper.booking.com "microk8s" is invalid: []: Invalid value: map[string]interface {}{"metadata":map[string]interface {}{"name":"microk8s", "selfLink":"/apis/shipper.booking.com/v1alpha1/clusters/microk8s", "uid":"4c3996ae-ede0-11e8-b526-e4b97a20b8f0", "resourceVersion":"744943", "generation":1, "creationTimestamp":"2018-11-21T22:53:44Z"}, "spec":map[string]interface {}{"region":"", "apiMaster":"http://192.168.1.14:8080", "scheduler":map[string]interface {}{"unschedulable":false}, "capabilities":interface {}(nil)}, "status":map[string]interface {}{"inService":false}, "kind":"Cluster", "apiVersion":"shipper.booking.com/v1alpha1"}: validation failure list:
spec.capabilities in body must be of type array: "null"

I have a few changes that I think would help:

  1. Avoid printing out "usage" for API errors. I think this comment can help -- looks like we need to set cmd.SilenceUsage = true at a certain point.
  2. Avoid printing the error twice. Perhaps the 'progress text' should just say "failed!" and then bubble the error upwards.
  3. Up-front validation of the cluster specs so we can give direct and useful feedback on why they're no good; OpenAPI is a decent fallback, but we can do much better here I think.

Overall I'm super happy about this, just want to brush up a little bit.

Show resolved Hide resolved cmd/shipperctl/configurator/cluster.go Outdated
Show resolved Hide resolved cmd/shipperctl/cmd/apply.go Outdated
@kanatohodets
Copy link
Contributor

kanatohodets left a comment

Tested this with an in-cluster Shipper: it worked first try! We'll have to do some work before we can convert the E2E tests to have an in-cluster Shipper (need a helm repo) but I think that would be a nice way to go eventually: use shipperctl and a Deployment to configure shipper in the E2E setup.

I'm really happy with the state now: kudos on the progress output text, and also on being flexible enough to handle the range of clusters.yaml contents I threw at it (no management cluster, no application clusters, same cluster specified twice, etc.).

Edit: ok, this was a bit misleading, since microk8s runs without RBAC by default. however, after enabling RBAC and verifying that it was in force (broken cluster role binding; shipper getting 'unauthorized' API responses), everything does look good.

shipperctl: first draft of `admin clusters apply`
crds: translate remaining CRDs to structs

shipperctl: create all the CRDs in 'apply'

Fixed bugs discovered in functional testing.

Changed code and custom resource definitions to migrate to v1alpha1.

Updated dep in the Travis configuration

Fixed a linting error.

Attempting to force vendor errors to be excluded.

- Removed the Should* methods on the Cluster configurator, except
  ShouldCopySecret -- kept that one to generate better output
- Formatted go imports
- Fixed the definition of the SecretNotFound error struct
- Changed the output of the command to conform to this new way of
  calling functions and using the already exists error as an
  indication that the object already exists

Turned variable declarations into constants where needed.

Improved outputting errors and fixed the Shipper import name.

- INitialize the list of capabilities to an empty slice if it's nil
- Do a priliminary validation of clusters to generate nicer
- messages. At this point, this is only checking if region is empty.

shipperctl: use shipper.SchemaGroupVersion instead of string

shipperctl: include secrets/events in management clusterrole

This is a bug, since Shipper doesn't need cluster-wide access to Secrets
in the management cluster, but it isn't ready to have scoped access. So,
until we fix that (#52) we don't need code for Roles or RoleBindings,
and just include cluster-wide access to secrets in the ClusterRole.

shipperctl: firmer split between app/management clusters

This commit ensures that management cluster and app cluster setup are
kept distinct, and introduces seperate service accounts for the two.

Furthermore, all RBAC objects are labled with the domain that pertain to
-- management or application. This helps to keep things straight in
a combo cluster, as in development.

@kanatohodets kanatohodets force-pushed the shipperctl-admin-join-clusters branch from f7dabf8 to 166cf5a Nov 23, 2018

@kanatohodets kanatohodets merged commit d7aa70f into master Nov 23, 2018

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