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

feature: support helm chart #668

Merged
merged 1 commit into from Sep 8, 2021

Conversation

jrkeen
Copy link
Member

@jrkeen jrkeen commented Aug 27, 2021

What type of PR is this?
/kind feature

What this PR does / why we need it:
Support deploy by helm. #652

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

@karmada-bot karmada-bot added the kind/feature Categorizes issue or PR as related to a new feature. label Aug 27, 2021
@karmada-bot
Copy link
Collaborator

Welcome @jrkeen! It looks like this is your first PR to karmada-io/karmada 🎉

@karmada-bot karmada-bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Aug 27, 2021
@pidb
Copy link
Member

pidb commented Aug 27, 2021

cc @RainbowMango

Copy link
Member

@RainbowMango RainbowMango left a comment

Choose a reason for hiding this comment

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

Thanks!!! I'll review and test it ASAP!
Also, cc @lfbear @mrlihanbo who might interest in it.

artifacts/chart/Chart.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

@pigletfly pigletfly left a comment

Choose a reason for hiding this comment

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

The karmada installation using helm should support two scenarios: with karmada-apiserver and without.And also should take cert-manager for CA injection into consideration.

artifacts/chart/_crds/cluster.karmada.io_clusters.yaml Outdated Show resolved Hide resolved
apiVersion: admissionregistration.k8s.io/v1
kind: MutatingWebhookConfiguration
metadata:
name: mutating-config
Copy link
Contributor

Choose a reason for hiding this comment

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

should we provide a option if user uses cert-manager for CA injection ?

Copy link
Member Author

Choose a reason for hiding this comment

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

More options will be provided in the future.

@@ -0,0 +1,127 @@
{{- if and (eq .Values.etcd.mode "internal") (eq .Values.installMode "host")}}
apiVersion: apps/v1
Copy link
Contributor

Choose a reason for hiding this comment

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

if users are not using karmada-apiserver,then etcd should be optional.

artifacts/chart/templates/karmada_agent.yaml Outdated Show resolved Hide resolved
artifacts/chart/templates/karmada_apiserver.yaml Outdated Show resolved Hide resolved
@RainbowMango
Copy link
Member

RainbowMango commented Aug 30, 2021

@jrkeen
Can you give some descriptions about how to use the chart? (For other reviewers to try it on).

[edit]:
When I tried to install the chart on my kind cluster, I met an error Error: Chart.yaml file is missing

# helm install karmada -n karmada-system --create-namespace .
Error: Chart.yaml file is missing
-bash-4.2# pwd
/root/go/src/github.com/karmada-io/karmada
-bash-4.2# kubectl config get-contexts 
CURRENT   NAME            CLUSTER         AUTHINFO        NAMESPACE
*         kind-helmdemo   kind-helmdemo   kind-helmdemo   
-bash-4.2# 

Did I miss something?

@jrkeen
Copy link
Member Author

jrkeen commented Aug 30, 2021

@jrkeen
Can you give some descriptions about how to use the chart? (For other reviewers to try it on).

[edit]:
When I tried to install the chart on my kind cluster, I met an error Error: Chart.yaml file is missing

# helm install karmada -n karmada-system --create-namespace .
Error: Chart.yaml file is missing
-bash-4.2# pwd
/root/go/src/github.com/karmada-io/karmada
-bash-4.2# kubectl config get-contexts 
CURRENT   NAME            CLUSTER         AUTHINFO        NAMESPACE
*         kind-helmdemo   kind-helmdemo   kind-helmdemo   
-bash-4.2# 

Did I miss something?

First of all, edit values as your needs.

## use host mode.
installMode: "host"
certs:
  ## @param certs.mode "auto" and "custom" are provided
  ## "auto" means auto generate certificate
  ## "custom" means use user certificate
  mode: auto
  auto:
    ## @param certs.auto.expiry expiry of the certificate
    expiry: 43800h
    ## @param certs.auto.hosts hosts of the certificate
    hosts: [
      "kubernetes.default.svc",
      "*.etcd.karmada-system.svc.cluster.local",
      "*.karmada-system.svc.cluster.local",
      "*.karmada-system.svc",
      "localhost",
      "127.0.0.1"
    ]
  custom:
    ## server-ca.crt
    caCrt: |
      -----BEGIN CERTIFICATE-----
      xxxxx
      -----END CERTIFICATE-----
    ## karmada.crt
    crt: |
      -----BEGIN CERTIFICATE-----
      xxxxx
      -----END CERTIFICATE-----
    ## karmada.key
    key: |
      -----BEGIN RSA PRIVATE KEY-----
      xxxxx
      -----END RSA PRIVATE KEY-----
-----------------------------------------------------------------
## use agent mode.
installMode: "agent"
agent:
  clusterName: "cluster name for your member cluster"
  kubeconfig:
    ## server-ca.crt
    caCrt: |
      -----BEGIN CERTIFICATE-----
      xxxxx
      -----END CERTIFICATE-----
    ## karmada.crt
    crt: |
      -----BEGIN CERTIFICATE-----
      xxxxx
      -----END CERTIFICATE-----
    ## karmada.key
    key: |
      -----BEGIN RSA PRIVATE KEY-----
      xxxxx
      -----END RSA PRIVATE KEY-----
    ## karmada apiserver
    server: "https://karmada-apiserver.karmada-system.svc.cluster.local"

Then, execute the installation command.

# cd /root/go/src/github.com/karmada-io/karmada/artifacts/chart
# helm install karmada/karmada-agent -n karmada-system --create-namespace .

Tip: Manually generate certificates.

# openssl req -x509 -sha256 -new -nodes -days 365 -newkey rsa:2048 -keyout "server-ca.key" -out "server-ca.crt" -subj "/C=xx/ST=x/L=x/O=x/OU=x/CN=ca/emailAddress=x/"
# echo '{"signing":{"default":{"expiry":"43800h","usages":["signing","key encipherment","client auth","server auth"]}}}' > "server-ca-config.json"
# echo '{"CN":"system:admin","hosts":["kubernetes.default.svc","*.etcd.karmada-system.svc.cluster.local","*.karmada-system.svc.cluster.local","*.karmada-system.svc","localhost","127.0.0.1"],"names":[{"O":"system:masters"}],"key":{"algo":"rsa","size":2048}}' | cfssl gencert -ca=server-ca.crt -ca-key=server-ca.key -config=server-ca-config.json - | cfssljson -bare karmada
# mv "karmada-key.pem" "karmada.key"
# mv "karmada.pem" "karmada.crt"

@RainbowMango
Copy link
Member

@jrkeen What's the progress now?
Can we install the chart by helm install karmada -n karmada-system --create-namespace . now?

@jrkeen
Copy link
Member Author

jrkeen commented Sep 6, 2021

@jrkeen What's the progress now?
Can we install the chart by helm install karmada -n karmada-system --create-namespace . now?

Yes, try it. #668 (comment)

@RainbowMango
Copy link
Member

Does this chart support a kind cluster? Or should I install it under a sub-directory of the repo?

# kubectl config get-contexts 
CURRENT   NAME       CLUSTER         AUTHINFO        NAMESPACE
*         helmdemo   kind-helmdemo   kind-helmdemo
# pwd
/root/go/src/github.com/karmada-io/karmada
# helm install karmada -n karmada-system --create-namespace .
Error: Chart.yaml file is missing
# helm version
version.BuildInfo{Version:"v3.6.3", GitCommit:"d506314abfb5d21419df8c7e7e68012379db2354", GitTreeState:"clean", GoVersion:"go1.16.5"}

@jrkeen
Copy link
Member Author

jrkeen commented Sep 6, 2021

Does this chart support a kind cluster? Or should I install it under a sub-directory of the repo?

# kubectl config get-contexts 
CURRENT   NAME       CLUSTER         AUTHINFO        NAMESPACE
*         helmdemo   kind-helmdemo   kind-helmdemo
# pwd
/root/go/src/github.com/karmada-io/karmada
# helm install karmada -n karmada-system --create-namespace .
Error: Chart.yaml file is missing
# helm version
version.BuildInfo{Version:"v3.6.3", GitCommit:"d506314abfb5d21419df8c7e7e68012379db2354", GitTreeState:"clean", GoVersion:"go1.16.5"}
# cd /root/go/src/github.com/karmada-io/karmada/artifacts/chart
# helm install karmada -n karmada-system --create-namespace .

or

# helm install karmada -n karmada-system --create-namespace /root/go/src/github.com/karmada-io/karmada/artifacts/chart

@jrkeen
Copy link
Member Author

jrkeen commented Sep 6, 2021

cc @RainbowMango

Copy link
Member

@RainbowMango RainbowMango 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. Only some nits.

In addition:

  1. Please sync the crd from master again, since we just updated a little bit.
  2. Please tidy your commits.

Thanks~~

charts/README.md Outdated
$ helm install karmada -n karmada-system --create-namespace .
```

If the certificate is `automatically generated`, need to get kubeconfig from the cluster:
Copy link
Member

Choose a reason for hiding this comment

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

What do you mean If? I don't see any description of customizing the certificate.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's my mistake, both modes can be used to obtain kubeconfig in this way, I need to modify the doc. By the way, users can edit values as needs.
image

charts/README.md Outdated
To install the chart with the release name `karmada` in namespace `karmada-system`:

```console
$ helm install karmada -n karmada-system --create-namespace .
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$ helm install karmada -n karmada-system --create-namespace .
$ helm install karmada -n karmada-system --create-namespace ./charts

Does this make sense?

charts/README.md Show resolved Hide resolved
Signed-off-by: jrkeen <jrkeen@hotmail.com>
@jrkeen
Copy link
Member Author

jrkeen commented Sep 7, 2021

All done @RainbowMango

@RainbowMango
Copy link
Member

LGTM. @mrlihanbo @lfbear Would you like to take a look?

@lfbear
Copy link
Member

lfbear commented Sep 8, 2021

I am not good at the helm, I just wonder how to keep in sync the files in charts/_crds/ and charts/templates/ with files in artifacts/deploy, that can not be processed by manual.

@RainbowMango
Copy link
Member

I am not good at the helm, I just wonder how to keep in sync the files in charts/_crds/ and charts/templates/ with files in artifacts/deploy, that can not be processed by manual.

Good question. I'll be done by following PR.
The CRDs should be generated and verified automatically.

@RainbowMango
Copy link
Member

/lgtm
/approve

@karmada-bot karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Sep 8, 2021
@RainbowMango
Copy link
Member

Many thanks to @algebra2k @jrkeen and all of the guys who are reviewing this PR.

The chart means a lot to the project and It might be not so perfect yet, but it will be a good start to iterate it.

@RainbowMango RainbowMango added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 8, 2021
@karmada-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: RainbowMango

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:

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants