[WIP]: Kong ingress controller #7350

merqurio · 2018-08-25T17:14:52Z

What this PR does / why we need it:
Adds a kong ingress controller deployment to the chart. This enables the management of kong using custom resources.

Which issue this PR fixes:
fixes Kong/kubernetes-ingress-controller/issues/36

Special notes for your reviewer:
It's a first approach, it's working for us, please, I'm open to suggestions and reviews.

It's limited to Kong CE 0.13.X, but support for 0.14.0 was merged a couple of days. @shashiranjan84, can you release a new version of the docker image ?

@hbagdi, I wasn't able to make it work when the kong-admin port is secured, is this the expeceted behaviour ? It makes sense to me, but I wanted to check first:

ingress-controller:
 main.go:144] Get https://localhost:8444/: x509: certificate signed by unknown authority

Thanks !

k8s-ci-robot · 2018-08-25T17:14:55Z

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.

k8s-ci-robot · 2018-08-25T17:15:05Z

Hi @merqurio. Thanks for your PR.

I'm waiting for a helm member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

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.

merqurio · 2018-08-25T17:20:30Z

/check-cla

merqurio · 2018-08-25T17:37:58Z

/check-cla

hbagdi · 2018-08-27T15:04:55Z

@hbagdi, I wasn't able to make it work when the kong-admin port is secured, is this the expeceted behaviour ?

Yes, the ingress controller currently does not take into account any authentication applied on the Admin port of Kong.

hbagdi · 2018-08-27T15:09:51Z

stable/kong/values.yaml

+
+# Ingress Controller:
+# A controller watches for kubernetes resources defined with a
+# CustomResourceDefinition that represent the configuration of a deployment.


Kong Ingress Controller's primary purpose is to satisfy Ingress resources created in k8s.
It uses CRDs for more fine grained control over routing and for Kong specific configuration.

@merqurio Could you update the description to communicate this? Thanks!

Done. Thanks,

hbagdi · 2018-08-27T15:43:30Z

ping @shashiranjan84

shashiranjan84 · 2018-08-27T15:51:12Z

I will try to review it as soon as possible.

hbagdi · 2018-08-27T16:14:56Z

I would like to list out two approaches to do this:

Put the controller into the same chart as Kong as done in this PR.
Create another Chart kong-ingress which adds kong chart as a dependency.

I currently don't have a preference, although (2) will be better in the longer term?
If there's an idiomatic way to do this with Helm, we should do that.

hbagdi · 2018-08-27T16:25:58Z

stable/kong/values.yaml

+      path: "/healthz"
+      port: 10254
+      scheme: HTTP
+    initialDelaySeconds: 30


Can we bump this up to 180 as set for Kong Admin and proxy pods?

Sure, I took that from here, changing to 180.

hbagdi · 2018-08-27T16:27:14Z

stable/kong/values.yaml

+    periodSeconds: 10
+    successThreshold: 1
+    timeoutSeconds: 1
+  readinessProbe:


Can we add a initialDelaySeconds of 120 for readinessProbe as set for Kong pods?

Done, it was missing here, modified to 102, thanks

hbagdi · 2018-08-27T16:28:32Z

stable/kong/templates/service-account-kong.yaml

@@ -0,0 +1,7 @@
+{{- if .Values.ingressController.enabled -}}


Can we name this file controller-service-account?

Done, thanks for the suggestion.

hbagdi · 2018-08-27T16:29:19Z

stable/kong/templates/role-binding-kong-ingress.yaml

@@ -0,0 +1,15 @@
+{{- if .Values.ingressController.enabled -}}


Can we name this file controller-rbac-role-binding since kong is redundant here?

hbagdi · 2018-08-27T16:29:49Z

stable/kong/templates/rbac-kong-ingress.yaml

@@ -0,0 +1,42 @@
+{{- if .Values.ingressController.enabled -}}


Can we name this file controller-rbac-role?

hbagdi · 2018-08-27T16:30:59Z

stable/kong/templates/clusterrole-kong-ingress.yaml

@@ -0,0 +1,63 @@
+{{- if .Values.ingressController.enabled -}}


Can we name this file controller-cluster-role?

hbagdi · 2018-08-27T16:32:41Z

stable/kong/templates/cluster-role-binding-kong-ingress.yaml

@@ -0,0 +1,14 @@
+{{- if .Values.ingressController.enabled -}}


Can we name this file controller-rbac-cluster-role-binding?

hbagdi · 2018-08-27T16:39:22Z

stable/kong/templates/crd-kongconsumers.yaml

@@ -0,0 +1,13 @@
+{{- if .Values.ingressController.enabled -}}


Can we keep the names of the files for all CRDs singular?
Like crd-kongconsumer for this file? Thanks!

hbagdi · 2018-08-27T16:40:14Z

stable/kong/templates/deployment-ingress-controller.yaml

@@ -0,0 +1,119 @@
+{{- if .Values.ingressController.enabled -}}


Can we rename this to controller-deployment?

hbagdi · 2018-08-27T16:58:15Z

stable/kong/templates/deployment-ingress-controller.yaml

+        livenessProbe:
+{{ toYaml .Values.ingressController.livenessProbe | indent 10 }}
+        resources:
+{{ toYaml .Values.ingressController.resources | indent 10 }}


@merqurio Could you explain you've included this Volume mount?

Removed. It came from here I wasn't sure if it was necessary. thanks

hbagdi · 2018-08-27T17:04:59Z

@shashiranjan84
A couple of questions,

Migrations are run as a batch job. What is the behavior with it when during a Kong version upgrade? Will it be ensured that the job is fired once again or is the migration job run only once at the start of this deployment?
We currently use an Ingress rule to expose Kong Proxy and Admin, this sounds a bit weird now, since Kong itself is an Ingress controller, which will satisfy the ingress now. Thoughts?

shashiranjan84 · 2018-08-27T17:18:56Z

@merqurio Before I review this PR I am wondering why can't we have Kong-ingress as separate chart?

merqurio · 2018-08-27T22:28:38Z

Put the controller into the same chart as Kong as done in this PR.

Personally I think this is the best approach, due to the templating nature of Helm, setting the ingress controller as a feature feels more natural to me. nginx is also available as an ingress chart. Of course if you consider that it really should be another chart, I don't mind creating a new chart, I just thought that the kong ingress controller was the most kubernetes friendly approach to kong.

Migrations are run as a batch job. What is the behavior with it when during a Kong version upgrade? Will it be ensured that the job is fired once again or is the migration job run only once at the start of this deployment?

I do like the init-container approach i saw in the kong ingress examples. I just modified that too, as I consider that as a kong maintainer you understand better the lifecycle.

We currently use an Ingress rule to expose Kong Proxy and Admin, this sounds a bit weird now, since Kong itself is an Ingress controller, which will satisfy the ingress now. Thoughts?

I completely agree. I didn't want to be too destructive with the existing chart. @shashiranjan84, you're the main maintainer, thoughts on that ? I will remove them in the meantime.

shashiranjan84 · 2018-08-27T22:56:22Z

@merqurio thanks again for PR. I discussed it with @hbagdi and we planned to keep it separate as both Kong as application and as ingress controller behave in a different way. Matter of fact we were planning to create a new chart for ingress controller last week. So I would request you to send a separate PR with a new chart for Kong ingress controller.

I do like the init-container approach i saw in the kong ingress examples. I just modified that too, as I consider that as a kong maintainer you understand better the lifecycle.

init-container is not the right approach. Goal was to not start multiple kong pods running migration at once and a separate migration job handles it well.

merqurio · 2018-08-27T23:01:07Z

Thanks @shashiranjan84 for the feedback, I'll move things around and push back as a new chart. I prefer to keep the changes on the same PR to make it easier to follow. I will create the chart in the incubator directory.

shashiranjan84 · 2018-08-27T23:05:05Z

Thanks @merqurio. Feel free to drop any question here. Me and @hbagdi will work along with you to get 1st version out.

merqurio · 2018-08-28T07:22:17Z

Problems that have emerged due to the split chart approach:

There is no support for recursive dependency solving for requirements (at least yet), so deploying Postgres or Cassandra is totally up to the user in charts where kong-ingress is a dependency.
I took advanvantage that template names are global to override some values in the kong chart (db related oned). Feels nasty.
I still don't like this approach from a users perspective. To trigger a migration I have to modify a value for a subchart in the parent. This is suboptimal for complex charts, where kong-ingress itself might be a dependency, adding more complexity.
There are many kong-ingress values defined at the subchart level. It doesn't make much sense to override so many values.

I really think the one chart solution is much more elegant and simpler to maintain. Please, think about complex charts where the ingress will be used.

A couple of questions:

Why does the proxy/admin have service of type NodePort ?

merqurio · 2018-08-28T07:34:35Z

For some reason even if kong.postgresql.enabled: false is set as default or set manually the postgresql chart is deployed anyway. Not sure why. Tested on minikube and kubernetes testing cluster.

merqurio · 2018-08-28T07:38:13Z

Thanks @shashiranjan84 and @hbagdi for the support, I really appreciate @Kong presence to make this chart reliable and focused in the long term.

hbagdi · 2018-08-30T21:28:37Z

Hello @merqurio,

@shashiranjan84 and I had a discussion on this and we've agreed on keeping the ingress controller chart as part of the kong chart. We apologize for the extra work you had to do around separating things out.

Could you revert it to the original PR you had?
We will review it, test it and then work together to get this merged in.

Thank you for taking the initiative to make this possible!

k8s-ci-robot · 2018-09-03T06:01:16Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: merqurio
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: unguiculus

If they are not already assigned, you can assign the PR to them by writing /assign @unguiculus in a comment when ready.

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

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

RTodorov · 2018-10-19T17:44:10Z

Hello guys! Great work with this chart, looking forward to getting this very important improvement merged.

Once this PR is merged, I'd like to add support to customized ingress class name, via - '--ingress-class=kong', as described here.

Do you think it would make sense? If yes, I could work on that PR.

My use case is that I'll have a normal nginx-ingress serving internal stuff (like prometheus, grafana etc) and Kong serving my microservices endpoints.

Thanks for your input and hard work on this PR!

hbagdi · 2018-10-19T17:51:29Z

@RTodorov Sounds good, thank you!

hbagdi · 2018-10-22T16:55:17Z

@merqurio Could you merge and resolve conflicts and add docs? Let's wrap this up soon to avoid staleness.

…troller

Signed-off-by: Gabriel de Maeztu <gabriel.maeztu@gmail.com>

…troller

hbagdi · 2018-10-25T16:27:56Z

@cpanato @mattfarina @davidkarlsen Could you guys /ok-to-test?

hbagdi · 2018-10-25T18:20:37Z

stable/kong/Chart.yaml

@@ -10,5 +10,5 @@ maintainers:
 name: kong
 sources:
 - https://github.com/Kong/kong
-version: 0.5.0
+version: 0.5.1


Let's bump this up to 0.6.0 since there are other PRs on this charts which might cause conflicts and I want to minimize back and forth on this PR now.

hbagdi · 2018-10-25T18:21:52Z

stable/kong/README.md

+| Parameter                      | Description                                                                      | Default             |
+| ------------------------------ | -------------------------------------------------------------------------------- | ------------------- |
+| image.repository               | Kong image                                                                       | `kong`              |
+| image.tag                      | Kong image version                                                               | `0.14.0`            |


Can we keep this to 0.14.1, I think this could came in here due to a rebase/merge from master.

hbagdi · 2018-10-25T18:28:23Z

stable/kong/README.md

+kong run the following command:
+
+```bash
+helm install . \


This seems like it's being used during development but end users shouldn't really need to do it.
Should it rather be helm install stable/kong?

hbagdi · 2018-10-25T18:41:38Z

stable/kong/values.yaml

@@ -97,7 +97,7 @@ livenessProbe:
    path: "/status"
    port: admin
    scheme: HTTPS
-  initialDelaySeconds: 30
+  initialDelaySeconds: 180


2 and 3 minutes are extremely high.
@shashiranjan84 has shown concern around this change in one of this past review.

hbagdi · 2018-10-30T15:55:33Z

@merqurio Gentle nudge for this. We're at the last mile I believe!

cpanato · 2018-11-01T12:21:36Z

@merqurio please rebase and also sign the DCO

hbagdi · 2018-11-07T17:19:27Z

/hold
This has moved to #9020. Please close this PR.
Thank you for contributing this @merqurio. I'll be making some changes and merging it soon.

cpanato · 2018-11-08T14:26:09Z

/hold cancel
/close

hbagdi · 2018-11-13T16:50:32Z

/close

hbagdi · 2018-11-13T16:50:50Z

/close

k8s-ci-robot requested review from linki and unguiculus August 25, 2018 17:14

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Aug 25, 2018

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

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 Aug 25, 2018

merqurio mentioned this pull request Aug 25, 2018

<Feature Request> Helm Chart for Kong Ingress Controller Kong/kubernetes-ingress-controller#36

Closed

hbagdi reviewed Aug 27, 2018

View reviewed changes

mattfarina added the Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). label Aug 27, 2018

merqurio changed the title ~~Kong ingress controller~~ [WIP]: Kong ingress controller Aug 28, 2018

Merge branch 'master' of github.com:helm/charts into kong-ingress-con…

65a9e85

…troller

Add labels for a clean delete and admin port SSL

1dd5022

Signed-off-by: Gabriel de Maeztu <gabriel.maeztu@gmail.com>

merqurio added 3 commits October 24, 2018 13:53

some details

debc7c1

Signed-off-by: Gabriel de Maeztu <gabriel.maeztu@gmail.com>

Comment limitations

f64400b

Merge branch 'master' of github.com:helm/charts into kong-ingress-con…

66648a2

…troller

hbagdi suggested changes Oct 25, 2018

View reviewed changes

hbagdi mentioned this pull request Nov 5, 2018

[stable/kong] introduce Kong ingress controller #9020

Merged

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 7, 2018

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 8, 2018

unguiculus closed this Nov 13, 2018

		@@ -0,0 +1,15 @@
		{{- if .Values.ingressController.enabled -}}

		@@ -0,0 +1,42 @@
		{{- if .Values.ingressController.enabled -}}

		@@ -0,0 +1,63 @@
		{{- if .Values.ingressController.enabled -}}

		@@ -0,0 +1,14 @@
		{{- if .Values.ingressController.enabled -}}

		@@ -0,0 +1,13 @@
		{{- if .Values.ingressController.enabled -}}

		@@ -0,0 +1,119 @@
		{{- if .Values.ingressController.enabled -}}

[WIP]: Kong ingress controller #7350

[WIP]: Kong ingress controller #7350

Conversation

merqurio commented Aug 25, 2018

k8s-ci-robot commented Aug 25, 2018

k8s-ci-robot commented Aug 25, 2018

merqurio commented Aug 25, 2018

merqurio commented Aug 25, 2018

hbagdi commented Aug 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hbagdi commented Aug 27, 2018

shashiranjan84 commented Aug 27, 2018

hbagdi commented Aug 27, 2018

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

hbagdi commented Aug 27, 2018

shashiranjan84 commented Aug 27, 2018

merqurio commented Aug 27, 2018

shashiranjan84 commented Aug 27, 2018

merqurio commented Aug 27, 2018

shashiranjan84 commented Aug 27, 2018

merqurio commented Aug 28, 2018

merqurio commented Aug 28, 2018 • edited Loading

merqurio commented Aug 28, 2018

hbagdi commented Aug 30, 2018

k8s-ci-robot commented Sep 3, 2018

RTodorov commented Oct 19, 2018 • edited Loading

hbagdi commented Oct 19, 2018

hbagdi commented Oct 22, 2018

hbagdi commented Oct 25, 2018

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

hbagdi commented Oct 30, 2018

cpanato commented Nov 1, 2018

hbagdi commented Nov 7, 2018

cpanato commented Nov 8, 2018

hbagdi commented Nov 13, 2018

hbagdi commented Nov 13, 2018

merqurio commented Aug 28, 2018 •

edited

Loading

RTodorov commented Oct 19, 2018 •

edited

Loading