Add manifest generator #3124

charith-elastic · 2020-05-21T09:34:05Z

Adds tooling to generate installation manifests for supported configurations of the operator.

sebgl

This is great. I'm so happy we can rely on Helm internally instead of baking our own template tooling.
I left many comments, but for most of them I'd be ok with follow-up issues if you prefer to merge this early and iterate with more PRs.

sebgl · 2020-05-28T06:27:51Z

hack/manifest-gen/README.md

+
+```shell
+make docker-gen-restricted RESTRICTED_NS=myns | kubectl apply -f -
+```


I am a bit surprised about having those instructions rely on Docker. Do you have in mind docker run as the main way to run the manifest generator for users?
I was expecting a simple go binary that we may eventually release with ECK for various platforms (linux/windows/osx), and this README to mention something like:

Compile from source

NOTE: requires Go installed on your computer.

go build

Quickstart

eck manifests generate --profile=global | kubectl apply -f

It is more likely that users will have Docker installed on their machines rather than Go. Asking them to install Go and compile a binary is not a great user experience. That is one of the reasons why I was keen on the distribution model enabled by my initial proposal.

With the code staying inside the cloud-on-k8s repo, the options as I see them are:

Tell users to build the manifest-gen container image locally

Make the manifest-gen container image available in the public registry

Bundle the manifest-gen binary with the ECK container image

I think I'd be fine with releasing the manifest-gen along with the ECK image, especially if they are versioned together (your option 3)?

However I think we should also consider:

Release the manifest-gen as a standalone binary (windows/osx/linux) in our github releases

I imagine most users would either run it from the ECK Docker image, or from the standalone binary.
I would consider that users who run things from the source code after a git clone can manage installing Go?

I'm fine keeping things as they are if you think it makes more sense for now, and revisit once we settle on the release/distribution mechanism.

+1 to discuss an other release mechanism later (curl, brew install elastic/tap/eckctl, yum install eckctl ...)

sebgl · 2020-05-28T06:29:44Z

hack/manifest-gen/README.md

+Cluster-wide installation with access to all namespaces:
+
+```shell
+make docker-gen-global | kubectl apply -f -


It feels like a bug to me that we need to go through a Makefile + a shell script to run the command. vs. just invoking the CLI with the right args (feels like the CLI is too complicated to be used directly?)

The script is simply there to update the chart with the latest CRDs and version. People can use the CLI directly if they wish but I think the main audience for the tool is non-developers who would prefer a simpler command to run rather than a complicated compilation + run recipe.

the main audience for the tool is non-developers

But in that case they would probably run either form a released docker image or released binary directly, not from cloning the github repo and running a make command?

sebgl · 2020-05-28T06:43:01Z

hack/manifest-gen/README.md

@@ -0,0 +1,44 @@
+ECK Manifest Generator


Longer term, I'm thinking about this as a general-purpose CLI tool for ECK, that we may release alongside ECK itself (same versioning), for people to use as a standalone binary or a kubectl plugin.

Potential use cases:

eck manifest $args # printout yaml manifests eck install # apply yaml eck license apply my/license.json # apply an Enterprise license eck license trial start # start an enterprise trial license eck elasticsearch my-cluster stop-manage # pause reconciliations for a cluster eck reattach-volumes mycluster.yml # re-create a cluster with existing PVs eck elasticsearch my-cluster replace-node my-cluster-data-0 # safe-remove a Pod for rescheduling on another host

(not sure about eck vs. for example eckctl to match our ecctl tool)

As such I would imagine this tool to live in cmd/cli (a real thing we maintain, not a "hack").

But of course this goes way beyond the scope of that PR, I'm totally fine with moving towards that goal later on.
Curious about what the rest of team thinks long-term @elastic/cloud-k8s.

I agree with the general direction you are outlining @sebgl
But does it change much for this PR, I don't think so.

That was my original proposal too :)

hack/manifest-gen/assets/charts/eck/.helmignore

sebgl · 2020-05-28T06:48:34Z

hack/manifest-gen/assets/charts/eck/profile-restricted.yaml

@@ -0,0 +1,7 @@
+config:


I'm not fond of restricted and global.
restricted sounds like a security thing to me (restricted Pod). global is a bit vague.

What do you think about cluster-wide and namespaced?

namespaced is pretty obtuse compared to restricted IMO.

@elastic/cloud-k8s curious what others think here? restricted feels very misleading to me but that could be because english isn't my 1st language.

I'm -1 on global and +1 on cluster-wide. I'm not sure about a namespaced as it doesn't map well to k8s usage where it means belonging to a single namespace. For us it's one or more namespaces.

Should we really care if we can make it an implementation detail and hide this wording to the user:

> # Generate and install a "global", "cluster-wide" operator > make | kubectl apply -f -

> # Generate and install an operator which only manages some namespaces > make MANAGED_NAMESPACES="foo,bar" | kubectl apply -f -

I'm not sure there is a right wording, OLM has installation mode types, but as long as it is not exposed to the user we can change our mind later.

by the way it not clear what the default target actually does

Great point @barkbay! Either we have managed namespaces, either we don't (default), which seems good enough to switch on what yaml is required. So we could remove the profile variable entirely?

I think global and restricted are OK names in my ears. But I am no native speaker. The question whether we need a profile feature which as I understand it also controls whether we have a webhook or not and sets up the managed namespaces at the moment is almost separate from that.

Right now it seems that we can predicate that decision indeed on if and how many managed namespace the user defines. But I wonder if some form of configuration preset will be useful once this thing generates additional config /RBAC for Beats for example (we can of course add it then when we need it)

I think the different OLM install mode types are pretty clear what they do so could see us adopting them, but I think we can leave that as something for later

As discussed out of band today it might be worth merging this in as is and opening a separate issue to discuss this as a refinement

Although currently the profiles seems like installation modes, they are more than that. The idea is to provide a set of defaults than can then be individually customised. They also serve as documentation/examples for what can be achieved with the operator. For example, someone might want to install the operator to manage just a single namespace but with the webhook. Without profiles, that person would need to read and understand the whole Helm chart and construct a complicated command line with the "right" combination of parameters. However, with a profile, all that person has to do is to look at the profile definition that closely matches what they want and simply override the flag that disables the webhook.

sebgl · 2020-05-28T07:20:57Z

hack/manifest-gen/main.go

+
+	cmd := &cobra.Command{
+		Use:           "generate",
+		Short:         "Generate ECK manifests",


We're a bit inconsistent in using manifests vs. manifest. Not sure what's best? Let's try to settle on one.

I guess it's the awkwardness of using the plural form for names. manifests-gen doesn't sound right -- hence why sometimes manifest is used in places.

sebgl · 2020-05-28T07:35:02Z

hack/manifest-gen/main.go

+		log.Fatalf("Failed to locate assets: %v", err)
+	}
+
+	cmd.PersistentFlags().StringVar(&sourceFlag, "source", sourceDir, "Source directory")


would it be more explicit to call it charts?

hack/manifest-gen/main.go

sebgl · 2020-05-28T07:38:20Z

hack/manifest-gen/manifest-gen.sh

+usage() {
+    echo "Usage: $0 [-u | -g <args>]"
+    echo "    '-u'"
+    echo "         Update the chart and exit"


it's immediately clear to me what "update the chart" means, can we clarify a bit?

sebgl · 2020-05-28T07:41:57Z

hack/manifest-gen/internal/manifest.go

+}
+
+// Generate produces a manifest for installing ECK using an embedded Helm chart.
+func Generate(opts *GenerateFlags) error {


It'd be nice to have a few comments explaining how this relies on helm internally and differs (or not?) from what helm would normally do if you invoke it directly.

barkbay · 2020-06-09T07:12:18Z

hack/manifest-gen/assets/charts/eck/Chart.yaml

+
+# This is the version number of the application being deployed. This version number should be
+# incremented each time you make changes to the application.
+appVersion: 1.2.0


IIUC this field should be kept in sync with https://github.com/elastic/cloud-on-k8s/blob/master/VERSION ?

It is: https://github.com/charith-elastic/cloud-on-k8s/blob/6f3f2bee22fff6742d29b9d5e5b0685cad3a6186/hack/manifest-gen/manifest-gen.sh#L14

barkbay · 2020-06-09T07:20:15Z

hack/manifest-gen/assets/charts/eck/values.yaml

+  # Namespace to install the operator to.
+  namespace: elastic-system
+  # Version of the operator. Should be set to the desired container image tag.
+  version: 1.2.0


Could we not reuse {{ .Chart.AppVersion }} ?

That makes it tricky to test unreleased images. With an explicit setting, I can test a locally built snapshot by passing something like --set=operator.version=1.2.0-48b3bc4e --set=operator.image.repository=docker.elastic.co/eck-dev/eck-operator-cell

I see there's this related issue about allowing to set appversion without editing the chart yaml helm/helm#8194

hack/manifest-gen/Dockerfile

hack/manifest-gen/Makefile

david-kow · 2020-06-12T07:57:53Z

hack/manifest-gen/manifest-gen.sh

+
+    cp -f "$ALL_CRDS" "${CHART_DIR}/crds/all-crds.yaml"
+    sed -i -E "s#version: [0-9]+\.[0-9]+\.[0-9]+#version: $VERSION#" "${CHART_DIR}/values.yaml"
+    sed -i -E "s#appVersion: [0-9]+\.[0-9]+\.[0-9]+#appVersion: $VERSION#" "${CHART_DIR}/Chart.yaml"


This ends up with the below on MacOSX.

$ ls assets/charts/eck/Chart* assets/charts/eck/Chart.yaml assets/charts/eck/Chart.yaml-E $ ls assets/charts/eck/values* assets/charts/eck/values.yaml assets/charts/eck/values.yaml-E

Interesting... looks like -i is consuming -E as the backup suffix. It shouldn't do that though. What is the version of sed you have on your machine?

Can't check the version unfortunately. Apparently it's FreeBSD sed from 2005. For .ci/Makefile we've ended up with sed ... > tmp && mv tmp ....

As far as I know in place edits that work on both gnu sed and freebsd aren't possible without something like that Makefile david linked

hack/manifest-gen/main.go

Co-authored-by: Thibault Richard <thbkrkr@users.noreply.github.com>

pebrc

LGTM! I am in favour of merging as is and addressing any outstanding issue in follow up PRs.

charith-elastic · 2020-06-23T12:17:54Z

Jenkins test this please

charith-elastic added >feature Adds or discusses adding a feature to the product v1.2.0 labels May 21, 2020

sebgl reviewed May 28, 2020

View reviewed changes

pebrc added the release-highlight Candidate for the ECK release highlight summary label Jun 4, 2020

barkbay reviewed Jun 9, 2020

View reviewed changes

charith-elastic force-pushed the manifest-generator branch from 6f3f2be to 1e1e9c1 Compare June 10, 2020 11:27

thbkrkr reviewed Jun 11, 2020

View reviewed changes

hack/manifest-gen/Dockerfile Outdated Show resolved Hide resolved

hack/manifest-gen/Makefile Outdated Show resolved Hide resolved

hack/manifest-gen/Makefile Outdated Show resolved Hide resolved

hack/manifest-gen/Makefile Outdated Show resolved Hide resolved

david-kow reviewed Jun 12, 2020

View reviewed changes

hack/manifest-gen/main.go Outdated Show resolved Hide resolved

charith-elastic force-pushed the manifest-generator branch 2 times, most recently from 5e378ed to c72d161 Compare June 18, 2020 10:18

charith-elastic and others added 14 commits June 23, 2020 09:11

Add manifest generator

592854b

Update hack/manifest-gen/Makefile

6befb1e

Co-authored-by: Thibault Richard <thbkrkr@users.noreply.github.com>

Update hack/manifest-gen/Makefile

ddfb12a

Co-authored-by: Thibault Richard <thbkrkr@users.noreply.github.com>

Update hack/manifest-gen/Makefile

8e9f9ef

Co-authored-by: Thibault Richard <thbkrkr@users.noreply.github.com>

Update hack/manifest-gen/Dockerfile

8d69274

Co-authored-by: Thibault Richard <thbkrkr@users.noreply.github.com>

Add licence header + fix rebase artifacts

f9502f2

Deal with GNU and BSD sed differences

9c03455

Use updated CRDs

c90901f

Add Docker push target

bc90c47

Rebase

997b21f

rename createClusterResources

678f616

rename webhook certs secret

a5d38f6

Moar comments

8dd608f

Rebase and regenerate

baf608a

charith-elastic force-pushed the manifest-generator branch from e9f7030 to baf608a Compare June 23, 2020 08:12

pebrc approved these changes Jun 23, 2020

View reviewed changes

charith-elastic merged commit 6e915e3 into elastic:master Jun 23, 2020

charith-elastic removed the >feature Adds or discusses adding a feature to the product label Jun 25, 2020

charith-elastic added the >enhancement Enhancement of existing functionality label Jun 25, 2020

This was referenced Jul 1, 2020

Create an Helm chart for this #938

Closed

Release manifest generator #3372

Closed

Add manifest generator #3124

Add manifest generator #3124

Conversation

charith-elastic commented May 21, 2020

sebgl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Compile from source

Quickstart

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

sebgl May 29, 2020 • edited Loading

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

pebrc Jun 9, 2020 • edited Loading

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

Choose a reason for hiding this comment

david-kow Jun 12, 2020 • edited Loading

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

pebrc left a comment

Choose a reason for hiding this comment

charith-elastic commented Jun 23, 2020

sebgl May 29, 2020 •

edited

Loading

pebrc Jun 9, 2020 •

edited

Loading

david-kow Jun 12, 2020 •

edited

Loading