Adds helm chart for heptio/ark #3795

domcar · 2018-02-21T11:10:22Z

Premise

Helm cannot handle properly CRD becauses it has a validation mechanism that checks the installation before the CRD are actually created,
hence each resource that uses a CRD cannot be validated because the CRD doesn't exist yet!

Solution

The solution here is to create CRD via helm chart, and only after (using a post-install) to install the resources with a container.
The container has the only job to execute a kubectl create -f filename and create the resources.

At the same time the resources created with the hook are completely transparent to Helm, that is, when you delete the
chart those resources remain there. Hence we need a second hook for deleting them

k8s-ci-robot · 2018-02-21T11:10:26Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: helpdesk@rt.linuxfoundation.org

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

domcar · 2018-02-21T11:59:56Z

CLA signed

unguiculus · 2018-02-21T15:24:52Z

/assign

domcar · 2018-02-22T07:53:21Z

/assign @unguiculus

unguiculus

Thanks for the PR. Before I continue reviewing it, please make sure you follow our review guidelines and apply best practices. In general we want separate files per resources (with an exception for RBAC stuff).

https://github.com/kubernetes/charts/blob/master/REVIEW_GUIDELINES.md
https://github.com/kubernetes/helm/blob/master/docs/chart_best_practices/rbac.md

unguiculus · 2018-02-22T14:46:25Z

incubator/ark-server/README.md

+## ConfigMap customization
+Since we want to have a customizable chart it's important that the configmap is a template and not a static file.
+To do this we add the keyword `tpl` when reading the file
+- {{ (tpl (.Files.Glob "static/*").AsConfig .) | indent 2 }}


Why don't you just add it to the templates folder? It doesn't make sense to me to have it in a static folder when it is not static.

For the reason I explained in the Premise Helm cannot handle properly CRD becauses it has a validation mechanism that checks the installation before the CRD are actually created, hence each resource that uses a CRD cannot be validated because the CRD doesn't exist yet!.

If you move the file in templates you get the error Error: apiVersion "ark.heptio.com/v1" in ark-server/templates/01-config-deploy.yaml is not available.

But I agree that the name static is not appropriate

unguiculus · 2018-02-22T14:46:40Z

incubator/ark-server/README.md

+
+### Heptio Secret
+Ark server needs a IAM service accoutn in order to run, if you don't have it you must create it: 
+https://github.com/heptio/ark/blob/v0.6.0/docs/cloud-provider-specifics.md#gcp


unguiculus · 2018-02-22T14:46:48Z

incubator/ark-server/README.md

+
+### Bucket and Project name
+Please change bucket and project/region name in the values.yaml file
+See here for possible values: https://github.com/heptio/ark/blob/v0.6.0/docs/cloud-provider-specifics.md


unguiculus · 2018-02-22T14:48:12Z

incubator/ark-server/static/01-config-deploy.yaml

+
+---
+apiVersion: apps/v1beta1
+kind: Deployment


Why is this not in templates?

I moved it to templates

svyotov · 2018-05-24T12:00:58Z

stable/ark/templates/deployment.yaml

+            - name: plugins
+              mountPath: /plugins
+          {{- if (or (eq .Values.configuration.cloudprovider "aws") (eq .Values.configuration.cloudprovider "gcp")) }}
+            - name: cloud-credentials


The name in my opinion should be a configurable variable.

svyotov · 2018-05-25T09:46:30Z

stable/ark/templates/hook-delete.yaml

+        - name: delete-ark-config
+          image: {{ required "A docker image with kubectl" .Values.kubectl.image.repository }}/{{ required "A docker image with kubectl" .Values.kubectl.image.tag }}
+          imagePullPolicy: Always
+          command: ["kubectl", "delete", "-f", "/tmp/"]


If the image has entry point kubectl or command kubectl, you can pass just the arguments, without the kubectl such as here: https://hub.docker.com/r/lachlanevenson/k8s-kubectl/ imo.

unguiculus · 2018-06-07T04:31:43Z

I'll probably have some time to more thoroughly test and review the PR today. If you don't mind, I'm going to help fix things.

domcar · 2018-06-07T06:35:09Z

@unguiculus you're welcome to help :)

unguiculus · 2018-06-07T15:27:38Z

I made quite a few changes. I'll continue with this next week and hope to push something then.

unguiculus · 2018-06-11T09:06:37Z

/ok-to-test

unguiculus · 2018-06-11T09:06:56Z

I pushed a few changes:

Add option to create or use existing secret.
Create the config resource directly in the configmap without using a separate template.
Expose the complete config directly via values removing any cloud provider specifics there. The only place where cloud provider specific stuff is still needed is in the deployment. The config resource configuration maps directly now.
Disable deployment and hooks if no provider and bucket are set in order to make CI happy. Ark won't start up without configuration but we cannot configure it properly for CI.
Add config table to readme.

I tested it successfully on my GKE cluster.

unguiculus · 2018-06-11T13:05:06Z

@domcar @svyotov @sortigoza @ncdc PTAL

domcar · 2018-06-13T12:59:56Z

@unguiculus I'm fine with the changes and I could successully deploy using an existing secret. But it doesn't work when putting the secrets into .Values.credentials.secretContents. Did you successfully test it? If so can you please tell me how you put the secret into the values.yaml ?

unguiculus · 2018-06-14T05:54:47Z

@domcar I tested it again. It works with limitations. Note that base64 encoding is done by the chart. You need to specify values in plain text.

A few examples:

credentials:
  secretContents:
    AZURE_SUBSCRIPTION_ID: ...
    AZURE_TENANT_ID: ...
    AZURE_RESOURCE_GROUP: ...
    AZURE_CLIENT_ID: ...
    AZURE_CLIENT_SECRET: ...
    AZURE_STORAGE_ACCOUNT_ID: ...
    AZURE_STORAGE_KEY: ...

--set credentials.secretContents.AZURE_SUBSCRIPTION_ID=my-id \
--set credentials.secretContents.AZURE_TENANT_ID=my-id

credentials:
  secretContents:
    cloud: |
      {
        "type": "service_account",
        "project_id": "my-project",
        "private_key_id": "123456789",
        "private_key": "-----BEGIN PRIVATE KEY-----\n...\n-----END PRIVATE KEY-----\n",
        "client_email": "service@my-project.iam.gserviceaccount.com",
        "client_id": "123456789",
        "auth_uri": "https://accounts.google.com/o/oauth2/auth",
        "token_uri": "https://accounts.google.com/o/oauth2/token",
        "auth_provider_x509_cert_url": "https://www.googleapis.com/oauth2/v1/certs",
        "client_x509_cert_url": "https://www.googleapis.com/robot/v1/metadata/x509/service%40my-project.iam.gserviceaccount.com"
      }

I didn't manage to pass the GCP service account key from the last example on the commandline. Some escaping would probably be necessary. I got wrong type for value; expected string; got []interface {}.

domcar · 2018-06-14T06:20:27Z

@unguiculus ok thanks, I managed to deploy on GCP with the secret in the values.yaml and it looks like everything is working

unguiculus · 2018-06-14T07:34:21Z

@domcar Can we merge it?

domcar · 2018-06-14T07:42:23Z

yes :)

unguiculus · 2018-06-14T08:48:41Z

/lgtm

k8s-ci-robot · 2018-06-14T08:48:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: domcar, unguiculus

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [unguiculus]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

* first commit * changes SA name * adds correct rbac rules * renames files; add more customizable vars * updates readme * adds notes * removes config values * changes email in chart * updates readme * test changes author * test change email * test change email * adds prerequisites in readme * fixes typo * adds AWS * updates to version 0.7 * updates version in chart * adds repo source; removes unnecessary values * moves deployment to templates * renames folder * updaates to 0.7.1 * creates ark sa in helpers; separates files according to object type * updates version in chart * adds home to chart * updates to v0.7.1 * modifies chart according to best practices * removes blank line * adds delete backup hook to solve issue crd backup not deleting * adjusts indentation, renames file with using hyphene * moves folder to stable * remove unnecessary test condition * Update to v0.8.1; Add support for Azure * Update readme * Add annotation for kube2iam * Add image for hooks. Update readme * Rename serviceaccount * Use Get instead of Glob * Remove namespace * Add PullPolicy; Modify readme * Rename Chart ark * Add standard labels to resources * Add customizable tolertion and nodeselector * Add missing labels; Use image with tag * Implement suggestion * Various updates * Add missing if block around delete hook

* first commit * changes SA name * adds correct rbac rules * renames files; add more customizable vars * updates readme * adds notes * removes config values * changes email in chart * updates readme * test changes author * test change email * test change email * adds prerequisites in readme * fixes typo * adds AWS * updates to version 0.7 * updates version in chart * adds repo source; removes unnecessary values * moves deployment to templates * renames folder * updaates to 0.7.1 * creates ark sa in helpers; separates files according to object type * updates version in chart * adds home to chart * updates to v0.7.1 * modifies chart according to best practices * removes blank line * adds delete backup hook to solve issue crd backup not deleting * adjusts indentation, renames file with using hyphene * moves folder to stable * remove unnecessary test condition * Update to v0.8.1; Add support for Azure * Update readme * Add annotation for kube2iam * Add image for hooks. Update readme * Rename serviceaccount * Use Get instead of Glob * Remove namespace * Add PullPolicy; Modify readme * Rename Chart ark * Add standard labels to resources * Add customizable tolertion and nodeselector * Add missing labels; Use image with tag * Implement suggestion * Various updates * Add missing if block around delete hook Signed-off-by: voron <av@arilot.com>

domcar added 6 commits February 21, 2018 10:47

first commit

65ac2e3

changes SA name

f675cb1

adds correct rbac rules

5aa0747

renames files; add more customizable vars

14ea5ff

updates readme

536c688

adds notes

4ead002

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 21, 2018

domcar added 2 commits February 21, 2018 12:24

removes config values

3f0aad2

changes email in chart

d5858f5

domcar and others added 4 commits February 21, 2018 13:05

updates readme

b65504c

test changes author

bf9b51c

test change email

adf608d

test change email

e4d1e82

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Feb 21, 2018

adds prerequisites in readme

9373293

k8s-ci-robot assigned unguiculus Feb 21, 2018

fixes typo

d80dd98

domcar added 4 commits February 22, 2018 12:56

adds AWS

44d8acd

updates to version 0.7

669ce07

updates version in chart

f47b895

adds repo source; removes unnecessary values

e85c4f0

unguiculus suggested changes Feb 22, 2018

View reviewed changes

moves deployment to templates

4802657

svyotov reviewed May 24, 2018

View reviewed changes

svyotov reviewed May 25, 2018

View reviewed changes

Implement suggestion

7df92c4

unguiculus mentioned this pull request Jun 11, 2018

Replace Config CRD vmware-tanzu/velero#538

Closed

unguiculus approved these changes Jun 11, 2018

View reviewed changes

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 11, 2018

unguiculus added 3 commits June 11, 2018 11:17

Various updates

6cd1fd9

Merge remote-tracking branch 'upstream/master' into feature/ark

d6bb35b

Add missing if block around delete hook

6901ca2

k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 14, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 14, 2018

k8s-ci-robot merged commit a49bf36 into helm:master Jun 14, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds helm chart for heptio/ark #3795

Adds helm chart for heptio/ark #3795

domcar commented Feb 21, 2018 •

edited

Loading

k8s-ci-robot commented Feb 21, 2018

domcar commented Feb 21, 2018

unguiculus commented Feb 21, 2018

domcar commented Feb 22, 2018

unguiculus left a comment

unguiculus Feb 22, 2018

domcar Feb 23, 2018

unguiculus Feb 22, 2018

unguiculus Feb 22, 2018

unguiculus Feb 22, 2018

domcar Feb 23, 2018

svyotov May 24, 2018

svyotov May 25, 2018

unguiculus commented Jun 7, 2018

domcar commented Jun 7, 2018

unguiculus commented Jun 7, 2018

unguiculus commented Jun 11, 2018

unguiculus commented Jun 11, 2018

unguiculus commented Jun 11, 2018

domcar commented Jun 13, 2018

unguiculus commented Jun 14, 2018 •

edited

Loading

domcar commented Jun 14, 2018

unguiculus commented Jun 14, 2018

domcar commented Jun 14, 2018

unguiculus commented Jun 14, 2018

k8s-ci-robot commented Jun 14, 2018

Adds helm chart for heptio/ark #3795

Adds helm chart for heptio/ark #3795

Conversation

domcar commented Feb 21, 2018 • edited Loading

Premise

Solution

k8s-ci-robot commented Feb 21, 2018

domcar commented Feb 21, 2018

unguiculus commented Feb 21, 2018

domcar commented Feb 22, 2018

unguiculus 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

unguiculus commented Jun 7, 2018

domcar commented Jun 7, 2018

unguiculus commented Jun 7, 2018

unguiculus commented Jun 11, 2018

unguiculus commented Jun 11, 2018

unguiculus commented Jun 11, 2018

domcar commented Jun 13, 2018

unguiculus commented Jun 14, 2018 • edited Loading

domcar commented Jun 14, 2018

unguiculus commented Jun 14, 2018

domcar commented Jun 14, 2018

unguiculus commented Jun 14, 2018

k8s-ci-robot commented Jun 14, 2018

domcar commented Feb 21, 2018 •

edited

Loading

unguiculus commented Jun 14, 2018 •

edited

Loading