Add a basic ghost package #3381

yuwenma · 2022-07-17T20:38:42Z

This package is originated from helm chart bitnami/ghost, which uses MariaDB.
By default, the ghost helmchart does not have the ghost, nor does it enable ingress, volumePermissions, networkPolicy, prometheus.

When rendering the ghost helm chart, we include the ghost host (Deployment and PersistentVolumeClaim), as well as the other services (NetworkPolicy Ingress PrometheusRule).

Follow up work is tracked in #3379

droot

Some quick comments. Feel free to merge it and address it as a follow up PR.

package-examples/ghost/Kptfile

package-examples/ghost/ghost/Kptfile

package-examples/ghost/ghost/deployment-example-ghost.yaml

droot · 2022-07-18T17:42:01Z

package-examples/ghost/ghost/setlabels.yaml

+    config.kubernetes.io/local-config: "true"
+labels:
+  app.kubernetes.io/name: ghost
+  app.kubernetes.io/instance: example


I think this label is interesting. We want this to be coming from package-context.data.name. Is this wired currently ? if not, we should take this out ? because example will not represent an instance in our solution.

It's not wired. The first user that tried to use kpt for an application ran into this. You provided an ApplyReplacements solution on slack.

Yeah, I was wondering if the current package wires using the ApplyReplacements or Starlark.

It is not wired. My plan for this package is to start from helm charts to discover as many feature gaps as possible. So this package contains some helm assumptions like configMap to pass in INI file , secret in auto roll deployment, secret reference and usage in livenessProbe and readiness probe.

For this specific value, helm assumes the namespace is "default" (which is convenience because users can install the charts without dealing w/ namespace), and this instance is the helm release name which is used to "differentiate releases for different app". I'm wondering what can be a good equivalent filter on kpt side.

On the kpt side, the deployment package name is similar to the release name.

package-examples/ghost/mariadb/configmap-example-mariadb.yaml

droot · 2022-07-18T17:44:01Z

package-examples/ghost/mariadb/package-context.yaml

+  annotations:
+    config.kubernetes.io/local-config: "true"
+data:
+  name: example


so variant constructor pattern isn't support well (or tested :)) for nested packages, so will be interesting to see how this plays out.

A user in slack has encountered similar issues using Config Connector subpackages (e.g., CloudSQL).

package-examples/ghost/mariadb/secret-example-mariadb.yaml

package-examples/ghost/mariadb/serviceaccount-example-mariadb.yaml

bgrant0607 · 2022-07-18T17:47:44Z

package-examples/ghost/ghost/deployment-example-ghost.yaml

+          preferredDuringSchedulingIgnoredDuringExecution:
+            - podAffinityTerm:
+                namespaces:
+                  - "example"


Does set-namespace know about this field?

set-namespace misses this field and also misses the podAffinityTerm.namespaceSelector, which means kustomize does not handle this field either.
I need to do a sanity check across 1.24 core/v1/types.go

package-examples/ghost/ghost/deployment-example-ghost.yaml

package-examples/ghost/ghost/secret-example-ghost.yaml

package-examples/ghost/package-context.yaml

bgrant0607 · 2022-07-18T19:52:27Z

package-examples/ghost/ghost/setlabels.yaml

+  name: ghost
+  annotations:
+    config.kubernetes.io/local-config: "true"
+labels:


😞 no spec. I think all custom functionConfig type do not follow the spec status rule. The argument is that those resources won't be deployed to the cluster so it is not a strict spec/status. Skipping spec saves some manual editing.

I'm more happy to add the spec to functionConfig, because it does help answering questions like "what fields & resources a user could be interested in modifying" https://kubernetes.slack.com/archives/C0155NSPJSZ/p1657561360040189

This KRM format has at least 4-6 lines of boilerplate even without the spec line (apiVersion, kind, metadata, name, annotations), so we'll want to figure out how to make it easy to generate, such as via the UI and/or a CLI with autocomplete.

Or generate it given the function name, if we have a catalog that can map between functions and input types.

I really like the idea of autogenerating function config for users by only requiring users to provide the function name. Even from my own experience, writing a function config is a big bottleneck to use KRM functions.

Put this as a feature request in #3379

droot

We discussed this offline and I captured the notes here.

package-examples/ghost/ghost/deployment-example-ghost.yaml

package-examples/ghost/ghost/Kptfile

package-examples/ghost/ghost/deployment-example-ghost.yaml

package-examples/ghost/ghost/secret-example-ghost.yaml

droot · 2022-07-18T21:12:08Z

package-examples/ghost/ghost/setlabels.yaml

+    config.kubernetes.io/local-config: "true"
+labels:
+  app.kubernetes.io/name: ghost
+  app.kubernetes.io/instance: example


Yeah, I was wondering if the current package wires using the ApplyReplacements or Starlark.

package-examples/ghost/package-context.yaml

yuwenma

Package updated, please take another look @bgrant0607 @droot
Some key changes:

Removed secrets and the references (env in pod template, checksum/secret annotation, livenessProbe).
Added CRD for PrometheusRule under crds, this should be the only CRD needs to install.
Dropped name prefix example- and updated the file names.
Dropped the app.kubernetes.io/instance label, I leave a question about the kpt equivalence.
Updated README to guide users get, hydrate and deploy the package.

package-examples/ghost/ghost/deployment-example-ghost.yaml

package-examples/ghost/ghost/secret-example-ghost.yaml

package-examples/ghost/mariadb/configmap-example-mariadb.yaml

package-examples/ghost/mariadb/secret-example-mariadb.yaml

package-examples/ghost/mariadb/serviceaccount-example-mariadb.yaml

yuwenma · 2022-07-18T23:29:25Z

package-examples/ghost/ghost/setlabels.yaml

+    config.kubernetes.io/local-config: "true"
+labels:
+  app.kubernetes.io/name: ghost
+  app.kubernetes.io/instance: example


It is not wired. My plan for this package is to start from helm charts to discover as many feature gaps as possible. So this package contains some helm assumptions like configMap to pass in INI file , secret in auto roll deployment, secret reference and usage in livenessProbe and readiness probe.

For this specific value, helm assumes the namespace is "default" (which is convenience because users can install the charts without dealing w/ namespace), and this instance is the helm release name which is used to "differentiate releases for different app". I'm wondering what can be a good equivalent filter on kpt side.

package-examples/ghost/package-context.yaml

bgrant0607 · 2022-07-19T03:09:36Z

Thanks. LGTM if the application works.

droot

I have a minor comment, but it is good to go.

droot · 2022-07-19T03:10:36Z

package-examples/ghost/README.md

+```bash
+export NAMESPACE=<YOUR NAMESPACE>
+# make sure the namespace is correct and exists. Otherwise, create the namespace
+kubectl create namespace ${NAMESPACE}


why can't we add namespace to the package itself ?

If you used helm chart to get the manifests, you can use --create-namespace arg to add it.

That will make the workflow truly declarative.

The ability to create a namespace would imply that the app team had cluster admin privilege. It also would bring in the need for the flexibility of the namespace provisioning scenario we've been working on: resource quota, limit range, network policy, etc.

We should have an example namespace blueprint for this, which could be used instead of kubectl create namespace.

The ability to create a namespace would imply that the app team had cluster admin privilege. It also would bring in the need for the flexibility of the namespace provisioning scenario we've been working on: resource quota, limit range, network policy, etc.

Aah.. completely missed that. Good pt.

* Add ghost basic package * update ghost pacakge * remove set-labels

Add ghost basic package

136f777

yuwenma requested review from droot, justinsb, mengqiy, mortent and natasha41575 as code owners July 17, 2022 20:38

droot reviewed Jul 18, 2022

View reviewed changes

bgrant0607 reviewed Jul 18, 2022

View reviewed changes

package-examples/ghost/ghost/deployment-example-ghost.yaml Outdated Show resolved Hide resolved

bgrant0607 reviewed Jul 18, 2022

View reviewed changes

package-examples/ghost/ghost/deployment-example-ghost.yaml Outdated Show resolved Hide resolved

bgrant0607 reviewed Jul 18, 2022

View reviewed changes

package-examples/ghost/ghost/secret-example-ghost.yaml Outdated Show resolved Hide resolved

bgrant0607 reviewed Jul 18, 2022

View reviewed changes

package-examples/ghost/package-context.yaml Show resolved Hide resolved

bgrant0607 reviewed Jul 18, 2022

View reviewed changes

droot reviewed Jul 18, 2022

View reviewed changes

yuwenma force-pushed the ghost branch 3 times, most recently from 11448ee to 2040d0d Compare July 19, 2022 00:11

yuwenma commented Jul 19, 2022

View reviewed changes

yuwenma force-pushed the ghost branch from 2040d0d to 026bc4c Compare July 19, 2022 00:13

droot approved these changes Jul 19, 2022

View reviewed changes

update ghost pacakge

97d8831

yuwenma force-pushed the ghost branch from 026bc4c to 97d8831 Compare July 19, 2022 17:27

remove set-labels

f87cdef

yuwenma merged commit 940e3bb into kptdev:main Jul 19, 2022

chunglu-chou pushed a commit to chunglu-chou/kpt that referenced this pull request Aug 20, 2022

Add a basic ghost package (kptdev#3381)

86dca9e

* Add ghost basic package * update ghost pacakge * remove set-labels

Add a basic ghost package #3381

Add a basic ghost package #3381

Uh oh!

Conversation

yuwenma commented Jul 17, 2022

Uh oh!

droot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yuwenma Jul 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

droot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yuwenma left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bgrant0607 commented Jul 19, 2022

Uh oh!

droot left a comment

yuwenma Jul 18, 2022 •

edited

Loading