feat: Helm chart by Yann-J · Pull Request #1623 · apache/fineract

Yann-J · 2021-02-16T22:02:53Z

Description

This is a proper Helm chart to run it on Kubernetes, as a more flexible alternative to the existing deployment definitions and setup scripts. It uses the bitnami mysql chart as a dependency to install MySQL, so all these values can be overridden.

To install, just run:

# Install mysql chart dependency
helm dependency update

# Install chart
helm upgrade --install --set mysql.auth.rootPassword=xxxx my-fineract helm/fineract

Checklist

Please make sure these boxes are checked before submitting your pull request - thanks!

Write the commit message as per https://github.com/apache/fineract/#pull-requests
Acknowledge that we will not review PRs that are not passing the build ("green") - it is your responsibility to get a proposed PR to pass the build, not primarily the project's maintainers.
Create/update unit or integration tests for verifying the changes made.
Follow coding conventions at https://cwiki.apache.org/confluence/display/FINERACT/Coding+Conventions.
Add required Swagger annotation and update API documentation at fineract-provider/src/main/resources/static/api-docs/apiLive.htm with details of any API changes
Submission is not a "code dump". (Large changes can be made "in repository" via a branch. Ask on the developer mailing list for guidance, if required.)

FYI our guidelines for code reviews are at https://cwiki.apache.org/confluence/display/FINERACT/Code+Review+Guide.

edcable · 2021-03-08T20:17:53Z

@Yann-J It was nice speaking to you the other day. @vidakovic our release manager would love to include this in the forthcoming Fineract 1.5 release - are you able to address the failing build?

vorburger

No Go from me for hard-coded default passwords - there must be a better solution?

More minor: How about move this into kubernetes/helm/ instead of (root) helm/?

Lastly, could you include the how to use doc in the README instead of PR desc?

vorburger · 2021-03-08T20:59:25Z

+    rootPassword: ozIRdODs8Jvs3BzZywgK
+    password: ihcqsCcsX6


These default password here scare me, a lot... people will NOT change these - they never do.

There must a better way to do this, with Helm; what would be the Helm equivalent of what we're doing in https://github.com/apache/fineract/blob/develop/kubernetes/kubectl-startup.sh#L22 with kubectl create secret generic fineract-tenants-db-secret --from-literal=username=root --from-literal=password=$(head /dev/urandom | tr -dc A-Za-z0-9 | head -c 16) ?

I don't know whether the following line would generate a proper random password. You can try this to generate the mysql password @Yann-J

password: {{ randAlphaNum 64 | quote }}

Or else we can point a particular secret name to the get password. But in order to do that we need to create that secret first before installing helm chart
Refer: https://stackoverflow.com/questions/56170052/how-not-to-overwrite-randomly-generated-secrets-in-helm-templates

Hello there, sorry for my delay here!

We can't use randAlphaNum inside the chart because that would cause the value to be re-generated each time we deploy. However there's no problem to keep that value empty and implement a runtime warning to instruct users to provide a value.

I'm not so sure that "people will NOT change these" is really true any more with Helm since it's just so easy to do... but for extra protection it makes sense to force it, I can do that.

Agree with that. I think there was the same issue in mysql deployment in local cluster. I've fixed that. This issue was raised in a previous PR #1583

Yeah... I would argue that the situation is a bit different in a Helm context since overriding parameters is dead simple (helm install --set mysql.auth.password=...), and really part of the Helm workflow, so the resistance is probably a bit lower there...

Anyway my latest commit is addressing this, and provides an error message in case the values aren't provided.

vorburger · 2021-03-08T21:13:52Z

@BLasan have you noticed this PR? It looks like something you could be interested in helping to review... 😄

I was thinking about something when reviewing #1653: That (resource limits) should be done here as well? More importantly, we're now maintaining Kubernetes YAML in 2 places (original raw Kube YAML, and Helm chart templates). That's perhaps not ideal... you guys should figure out how you want to do this going forward? Keep both? Replace the original YAML with the Helm templates, and subsequently remove the originals? Up to you - work together.

BLasan · 2021-03-09T03:30:18Z

@BLasan have you noticed this PR? It looks like something you could be interested in helping to review...

I was thinking about something when reviewing #1653: That (resource limits) should be done here as well? More importantly, we're now maintaining Kubernetes YAML in 2 places (original raw Kube YAML, and Helm chart templates). That's perhaps not ideal... you guys should figure out how you want to do this going forward? Keep both? Replace the original YAML with the Helm templates, and subsequently remove the originals? Up to you - work together.

It would be great if we could add resource limits in order to run the image using minimum number of resources in the cluster. (Cluster resources will not be over eaten by then) If someone wants to run the product in k8s without cloning it, they could use the helm charts directly (after releasing this) What if we could move these helm implementations to a new repository and maintain it there? Don't know whether this is a good idea. But we could maintain this and can do releases separately.

BLasan · 2021-03-09T10:05:46Z

+          ports:
+            - containerPort: 8080
+              name: http
+          readinessProbe:


You can use separate paths to check liveness and readiness probes. We have added configurations in to spring boot. See #1649

BLasan · 2021-03-09T10:29:50Z

+    rootPassword: ozIRdODs8Jvs3BzZywgK
+    password: ihcqsCcsX6


I don't know whether the following line would generate a proper random password. You can try this to generate the mysql password @Yann-J

password: {{ randAlphaNum 64 | quote }}

Or else we can point a particular secret name to the get password. But in order to do that we need to create that secret first before installing helm chart
Refer: https://stackoverflow.com/questions/56170052/how-not-to-overwrite-randomly-generated-secrets-in-helm-templates

BLasan · 2021-03-16T16:05:07Z

+kind: Secret
+apiVersion: v1
+metadata:
+  name: {{ .Release.Name }}


Use {{ .Release.Name }}-secret.

Actually, it's already Kind = Secret so I think that might be a bit redundant...

Yuh, but you have used the prefix {{ .Release.Name }} for other resources. So instead of having just the {{ .Release.Name }} I would suggest to use {{ .Release.Name }}-secret as the secret name. That'll be something meaningful. But it's not a requirement with high priority. Have you done a release for this implementation? Hope this works fine :) The configurations you've done look good to me

BLasan · 2021-03-16T16:06:14Z

+          image: "{{ .Values.fineractServer.image.name }}:{{ .Values.fineractServer.image.tag }}"
+          envFrom:
+            - secretRef:
+                name: {{ .Release.Name | quote }}


Use the secret name (this is bit confusing)

Yann-J · 2021-03-16T16:19:38Z

OK, I've made an update that addresses most comments above (I think), in particular avoid setting default passwords, and raising an error if they're empty.

Some tuning along the way, in particular all extra variables (Secret or not) are now evaluated as templates - don't know if that's useful here but it doesn't hurt, and I've seen many cases where it is. They're also all supporting multiline values. Again, not sure if it's super useful here but you never know.

Yann-J · 2021-03-17T04:43:42Z

@BLasan have you noticed this PR? It looks like something you could be interested in helping to review... 😄

I was thinking about something when reviewing #1653: That (resource limits) should be done here as well? More importantly, we're now maintaining Kubernetes YAML in 2 places (original raw Kube YAML, and Helm chart templates). That's perhaps not ideal... you guys should figure out how you want to do this going forward? Keep both? Replace the original YAML with the Helm templates, and subsequently remove the originals? Up to you - work together.

Regarding the scripts vs Helm, I would say that Helm is probably the de-facto standard nowadays for deploying to k8s, but not everyone will have it, so it could make sense to keep supporting both.

However, I would say that in the long run, once it's quite stable, the Helm chart probably should be in a separate repo so it can be versioned independently from the application, and then published into https://artifacthub.io/

Yann-J · 2021-03-17T04:50:12Z

Note that there would be a lot more we could do... in particular the next item on my list is to implement a helm test hook, as well as making the mysql dependency optional, in case you already have an external instance running.

This is really only a quickstart...

BLasan · 2021-03-17T05:09:39Z

Note that there would be a lot more we could do... in particular the next item on my list is to implement a helm test hook, as well as making the mysql dependency optional, in case you already have an external instance running.

This is really only a quickstart...

@BLasan have you noticed this PR? It looks like something you could be interested in helping to review... smile
I was thinking about something when reviewing #1653: That (resource limits) should be done here as well? More importantly, we're now maintaining Kubernetes YAML in 2 places (original raw Kube YAML, and Helm chart templates). That's perhaps not ideal... you guys should figure out how you want to do this going forward? Keep both? Replace the original YAML with the Helm templates, and subsequently remove the originals? Up to you - work together.

Regarding the scripts vs Helm, I would say that Helm is probably the de-facto standard nowadays for deploying to k8s, but not everyone will have it, so it could make sense to keep supporting both.

However, I would say that in the long run, once it's quite stable, the Helm chart probably should be in a separate repo so it can be versioned independently from the application, and then published into https://artifacthub.io/

+1 @vorburger I think this is ideal as we can maintain the helm chart implmentations separately and do releases accordingly

BLasan · 2021-03-17T05:12:24Z

Note that there would be a lot more we could do... in particular the next item on my list is to implement a helm test hook, as well as making the mysql dependency optional, in case you already have an external instance running.

This is really only a quickstart...

Great. Shall we wait until @vorburger and @edcable 's reviews ?

github-actions · 2021-04-17T00:14:27Z

This pull request seems to be stale. Are you still planning to work on it? We will automatically close it in 30 days.

Yann-J · 2021-04-19T06:21:32Z

Is there anything else you folks need from me on this one?

awasum · 2021-04-19T07:09:27Z

@Yann-J There are some Apache Rat failures in the build on Travis: https://travis-ci.com/github/apache/fineract/builds/220288308

You could either add the licenses or ignore the files. After that @vorburger may give his go ahead or more feedback...

github-actions · 2021-05-20T00:51:40Z

This pull request seems to be stale. Are you still planning to work on it? We will automatically close it in 30 days.

Yann Jouanique added 3 commits February 16, 2021 23:56

Helm chart

806b20a

Fix namespace, added ingress, livenessprobes

e57b9b8

Helm chart tweak

f6122da

vorburger requested changes Mar 8, 2021

View reviewed changes

BLasan requested changes Mar 9, 2021

View reviewed changes

BLasan requested changes Mar 16, 2021

View reviewed changes

Some Helm chart corrections

741df92

README for Helm

730d4ab

Yann-J requested a review from vorburger March 17, 2021 04:48

github-actions Bot added the stale label Apr 17, 2021

github-actions Bot removed the stale label Apr 20, 2021

github-actions Bot added the stale label May 20, 2021

github-actions Bot closed this Jun 20, 2021

Uh oh!

Conversation

Yann-J commented Feb 16, 2021

Description

Checklist

Uh oh!

edcable commented Mar 8, 2021

Uh oh!

vorburger left a comment

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!

Choose a reason for hiding this comment

Uh oh!

vorburger commented Mar 8, 2021

Uh oh!

BLasan commented Mar 9, 2021

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!

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!

Yann-J commented Mar 16, 2021

Uh oh!

Yann-J commented Mar 17, 2021

Uh oh!

Yann-J commented Mar 17, 2021

Uh oh!

BLasan commented Mar 17, 2021

Uh oh!

BLasan commented Mar 17, 2021

Uh oh!

github-actions Bot commented Apr 17, 2021

Uh oh!

Yann-J commented Apr 19, 2021

Uh oh!

awasum commented Apr 19, 2021

Uh oh!

github-actions Bot commented May 20, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants