[kong] add support for watch-namespace #79

wopol · 2020-03-19T21:59:20Z

Adding support for watch-namespace in chart

CLAassistant · 2020-03-19T21:59:28Z

All committers have signed the CLA.

hbagdi · 2020-03-20T02:34:55Z

Can you change this to instead inject arbitrary CLI arguments to the controller?
Something along the lines of taking in an array of strings and passing it verbatim to the controller.

We need to do this to make it easy enough for folks to override other properties via CLI flags.

wopol · 2020-03-20T17:05:01Z

Yes, i can do this. Did you mean something like that?

ingressController:
  installCRDs: false
  ingressClass: myClass
  rbac:
    create: false
  serviceAccount:
    create: false
    name: sa-name
  args:
    - --watch-namespace=namespace-name
    - --other-arg=value

And one more question. What with hardcoded arguments in kong.controller-container function like admin-tls-skip-verify, --admission-webhook-listen etc. Should these arguments stay there?

hbagdi · 2020-03-20T17:14:07Z

Should these arguments stay there?

No. Let's move them over to the values.yaml file as well so that a user can change them, if needed.

wopol · 2020-03-20T18:07:01Z

Should these arguments stay there?

No. Let's move them over to the values.yaml file as well so that a user can change them, if needed.

Now this is a little bit more complicated, but we have all default arguments and we can replace them when we define the same argument in args Map.

hbagdi · 2020-03-20T18:41:16Z

Now this is a little bit more complicated, but we have all default arguments and we can replace them when we define the same argument in args Map.

In values.yaml, we can simply add and keep this simple:

ingressController
  args:
  - --flag=value1
  - --bolean-flag
  - --flag2=value2

Users can inject any flag and edit the default as well.

wopol · 2020-03-20T18:55:38Z

Now this is a little bit more complicated, but we have all default arguments and we can replace them when we define the same argument in args Map.

In values.yaml, we can simply add and keep this simple:
ingressController
  args:
  - --flag=value1
  - --bolean-flag
  - --flag2=value2
Users can inject any flag and edit the default as well.

I prefer simple solution but what if someone use arguments like that:

ingressController:
  ingressClass: other-class
  args:
    - --ingress-class=class-name
    - --publish-service=name

Which values should have higher priority for --ingress-class ingressController.ingressClass or ingressController.args[0] ?
Which values should have higher priority for --publish-service ingressController.args[1] or {{ .Release.Namespace }}/{{ template "kong.fullname" . }}-proxy

hbagdi · 2020-03-20T19:00:46Z

Which values should have higher priority for --ingress-class ingressController.ingressClass or ingressController.args[0] ?

Specifying via args should have a higher priority.

Some background:
We do want to add arg based approach, but we will recommend using environment variables for all configuration changes, especially to these ones that are already supported by environment variables.

Only logging based variables are not yet supported by environment variables, which is why we want to merge this PR in.
Flag/env should take precedence over specifying the value directly using a chart variable, like ingressClass.

wopol · 2020-03-20T19:19:57Z

Only logging based variables are not yet supported by environment variables, which is why we want to merge this PR in.
Flag/env should take precedence over specifying the value directly using a chart variable, like ingressClass.

This will be more difficulty and less readable to check if in args array exist some key when not exist generate default e.g election-id=kong-ingress-controller-leader-{{ .Values.ingressController.ingressClass }}
I will try to do it.

hbagdi · 2020-03-20T20:19:55Z

charts/kong/templates/_helpers.tpl

  {{- else }}
  - --kong-url=http://localhost:{{ .Values.admin.containerPort }}
  {{- end }}
  {{- if .Values.ingressController.admissionWebhook.enabled }}
  - --admission-webhook-listen=0.0.0.0:{{ .Values.ingressController.admissionWebhook.port }}
  {{- end }}
+
+


Please remove these line jumps.

hbagdi · 2020-03-20T20:21:36Z

This looks good to me since going forward, users should add CLI args only for logging related settings.
The existing CLI flags will be moved over to environment variables after controller 0.8 is released, and we already have good amount of code on how to set precedence there.
@rainest thoughts?

rainest · 2020-03-20T22:02:50Z

@hbagdi you mean migrate kong.ingressController.env to something like kong.env's dict-based deduplication system and handle all defaults via environment variables? That seems reasonable.

Post-0.8 is there any reason you'd still use CLI arguments over their equivalent variable? It almost seems like it'd make more sense to wait and have users use environment variables only, since we wouldn't have any means of avoiding conflicts between envvar/CLI settings.

hbagdi · 2020-03-20T22:53:53Z

dict-based deduplication system and handle all defaults via environment variables?

Yes.

Post-0.8 is there any reason you'd still use CLI arguments over their equivalent variable? It almost seems like it'd make more sense to wait and have users use environment variables only, since we wouldn't have any means of avoiding conflicts between envvar/CLI settings.

Yes, because Kong/kubernetes-ingress-controller#506.
Hence, the comment on logging CLI args only. Everything else will move to env vars.
We should move logging CLI args to env vars as well but that's not a priority (for me at least).

rainest

Ah, okay, I thought 0.8 was going to add support also.

hbagdi

This is looking good and we would like to merge it in.
Please also add a test case to an existing test (don't add another test file as it puts pressure on the CI).
Add a harmless argument like --ingress-class=test-kong or something like that. A logging arg would be even better. Full list here: https://github.com/Kong/kubernetes-ingress-controller/blob/master/docs/references/cli-arguments.md.

Thank you for pushing through this even though the purpose of the PR changed significantly.

hbagdi · 2020-03-20T22:54:34Z

charts/kong/README.md

  - [Prerequisites](#prerequisites-1)
    - [Kong Enterprise License](#kong-enterprise-license)
    - [Kong Enterprise Docker registry access](#kong-enterprise-docker-registry-access)
  - [Service location hints](#service-location-hints)
  - [RBAC](#rbac)
  - [Sessions](#sessions)
  - [Email/SMTP](#emailsmtp)
- [Changelog](https://github.com/Kong/charts/blob/master/charts/kong/CHANGELOG.md)
 - [Seeking help](#seeking-help)


Please remove changes to this table.

hbagdi · 2020-03-20T22:55:01Z

charts/kong/README.md

@@ -276,6 +283,7 @@ section of `values.yaml` file:
 | installCRDs                        | Create CRDs. Regardless of value of this, Helm v3+ will install the CRDs if those are not present already. Use `--skip-crds` with `helm install` if you want to skip CRD creation. | true |
 | env                                | Specify Kong Ingress Controller configuration via environment variables               |                                                                              |
 | ingressClass                       | The ingress-class value for controller                                                | kong                                                                         |
+| args                               | Map of ingress-controller cli arguments                                               | {}                                                                           |


The description is outdated. It should be an array and not a map.

hbagdi · 2020-03-20T22:56:33Z

charts/kong/values.yaml

@@ -191,6 +191,7 @@ ingressController:
  image:
    repository: kong-docker-kubernetes-ingress-controller.bintray.io/kong-ingress-controller
    tag: 0.7.1
+  args:


Please initialize with an empty array [] to set the user expectation that this is an array.

hbagdi · 2020-04-22T23:43:25Z

@wopol Thank you for your contribution. Please fill in the contributor form to claim your Kong swag: https://github.com/Kong/kong/blob/master/CONTRIBUTING.md#contributor-t-shirt.

[kong] add support for watch-namespace

ce21222

wopol force-pushed the watch-namespace-setting branch from ecd0dcc to ce21222 Compare March 19, 2020 22:01

hbagdi added the changes requested label Mar 20, 2020

wopol force-pushed the watch-namespace-setting branch from 954f718 to 10c0a80 Compare March 20, 2020 18:08

wopol force-pushed the watch-namespace-setting branch from bd471c3 to db2e769 Compare March 20, 2020 19:43

[kong] ingress-controller cli arguments configuration

9833117

wopol force-pushed the watch-namespace-setting branch from db2e769 to 9833117 Compare March 20, 2020 19:47

hbagdi reviewed Mar 20, 2020

View reviewed changes

hbagdi requested a review from rainest March 20, 2020 20:20

[kong] remove unnecessary empty lines

fc982e7

hbagdi added HOLD/DO NOT MERGE and removed changes requested labels Mar 20, 2020

rainest previously approved these changes Mar 20, 2020

View reviewed changes

hbagdi requested changes Mar 20, 2020

View reviewed changes

hbagdi added changes requested and removed HOLD/DO NOT MERGE labels Mar 20, 2020

[kong] fix README.md, use args in test case

0a3e22e

wopol dismissed rainest’s stale review via 0a3e22e March 21, 2020 07:41

wopol requested a review from hbagdi March 21, 2020 11:55

hbagdi approved these changes Mar 23, 2020

View reviewed changes

hbagdi merged commit 48f67a5 into Kong:master Mar 23, 2020

hbagdi mentioned this pull request Mar 23, 2020

[kong] add support for adding CLI args to Ingress Controller #85

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[kong] add support for watch-namespace #79

[kong] add support for watch-namespace #79

wopol commented Mar 19, 2020

CLAassistant commented Mar 19, 2020 •

edited

Loading

hbagdi commented Mar 20, 2020

wopol commented Mar 20, 2020

hbagdi commented Mar 20, 2020

wopol commented Mar 20, 2020

hbagdi commented Mar 20, 2020

wopol commented Mar 20, 2020

hbagdi commented Mar 20, 2020

wopol commented Mar 20, 2020 •

edited

Loading

hbagdi Mar 20, 2020

wopol Mar 20, 2020

hbagdi commented Mar 20, 2020

rainest commented Mar 20, 2020

hbagdi commented Mar 20, 2020

rainest left a comment

hbagdi left a comment

hbagdi Mar 20, 2020

hbagdi Mar 20, 2020

hbagdi Mar 20, 2020

hbagdi commented Apr 22, 2020

[kong] add support for watch-namespace #79

[kong] add support for watch-namespace #79

Conversation

wopol commented Mar 19, 2020

CLAassistant commented Mar 19, 2020 • edited Loading

hbagdi commented Mar 20, 2020

wopol commented Mar 20, 2020

hbagdi commented Mar 20, 2020

wopol commented Mar 20, 2020

hbagdi commented Mar 20, 2020

wopol commented Mar 20, 2020

hbagdi commented Mar 20, 2020

wopol commented Mar 20, 2020 • edited Loading

hbagdi Mar 20, 2020

Choose a reason for hiding this comment

wopol Mar 20, 2020

Choose a reason for hiding this comment

hbagdi commented Mar 20, 2020

rainest commented Mar 20, 2020

hbagdi commented Mar 20, 2020

rainest left a comment

Choose a reason for hiding this comment

hbagdi left a comment

Choose a reason for hiding this comment

hbagdi Mar 20, 2020

Choose a reason for hiding this comment

hbagdi Mar 20, 2020

Choose a reason for hiding this comment

hbagdi Mar 20, 2020

Choose a reason for hiding this comment

hbagdi commented Apr 22, 2020

CLAassistant commented Mar 19, 2020 •

edited

Loading

wopol commented Mar 20, 2020 •

edited

Loading