Helm enable to config controller manager & audit port #1438

NissesSenap · 2021-07-14T09:57:39Z

What this PR does / why we need it:
The current port can be taken when running in hostNetwork mode.
To work around this I want to be able to configure the controller-manager port in the helm chart.

Which issue(s) this PR fixes (optional, using fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when the PR gets merged):
Fixes #

Special notes for your reviewer:

The current port can be taken when running in hostNetwork mode Signed-off-by: Edvin Norling <edvin.norling@xenit.se>

Signed-off-by: Edvin Norling <edvin.norling@xenit.se>

NissesSenap · 2021-07-14T12:53:03Z

Remove the config for healthz port due to using a string instead of int.
One solution could to put in to two separate variables, one fort port and one for address.
It's probably solvable with helm some way as well. Would love some help to solve that.

This way we can still easily define the port but not the addr. I think it's a good enough middle way. Signed-off-by: Edvin Norling <edvin.norling@xenit.se>

NissesSenap · 2021-07-14T13:35:55Z

Re-added the healthz probe again and ignored the address feature. For now just adding port support.

NissesSenap · 2021-07-14T14:18:14Z

For some reason that I don't know I can't get the

        - --health-addr=":{{ .Values.controllerManager.healthPort }}"

to work. When I try to run with the default value of 9090 i get:
If I remove the definition of --health-addr at all it works without an issue.

The generated value:

      --health-addr=":9090"

I assume I do some easy stupid error, if someone can point me in the correct direction it will be a quick fix.

{"level":"error","ts":1626271697.9305024,"logger":"setup","msg":"unable to start manager","error":"error listening on \":9090\": listen tcp: address tcp/9090\": unknown port","stacktrace":"github.com/go-logr/zapr.(*zapLogger).Error\n\t/go/src/github.com/open-policy-agent/gatekeeper/vendor/github.com/go-logr/zapr/zapr.go:132\nsigs.k8s.io/controller-runtime/pkg/log.(*DelegatingLogger).Error\n\t/go/src/github.com/open-policy-agent/gatekeeper/vendor/sigs.k8s.io/controller-runtime/pkg/log/deleg.go:144\nmain.main\n\t/go/src/github.com/open-policy-agent/gatekeeper/main.go:171\nruntime.main\n\t/usr/local/go/src/runtime/proc.go:225"}

maxsmythe

Thanks for the PR! WRT port numbers, I think the " quotes may be being included in the string argument, try dropping them?

cmd/build/helmify/kustomization.yaml

Signed-off-by: Edvin Norling <edvin.norling@xenit.se>

Also remove some withespace Signed-off-by: Edvin Norling <edvin.norling@xenit.se>

Update the GKE docs since we use the name webhook-server you don't have to update the svc targetPort. Signed-off-by: Edvin Norling <edvin.norling@xenit.se>

NissesSenap · 2021-08-04T08:24:00Z

config/webhook/service.yaml

-      targetPort: 8443
+      targetPort: webhook-server


Realized I need to change the gatekeeper-webhook-service port.
Instead of changing this using kustomize i changed this in the config file instead and use the name.

Thanks to this it will also simplify the GKE usage since they won't have to update the service since we are using the port name instead of the port number, I have updated the docs accordingly.

maxsmythe

LGTM! Thanks for the PR!

maxsmythe · 2021-08-04T22:59:34Z

config/webhook/service.yaml

-      targetPort: 8443
+      targetPort: webhook-server


codecov-commenter · 2021-08-04T23:28:35Z

Codecov Report

Merging #1438 (b17b24d) into master (5975122) will decrease coverage by 0.07%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1438      +/-   ##
==========================================
- Coverage   52.03%   51.95%   -0.08%     
==========================================
  Files          83       83              
  Lines        7593     7593              
==========================================
- Hits         3951     3945       -6     
- Misses       3263     3265       +2     
- Partials      379      383       +4

Flag	Coverage Δ
unittests	`51.95% <ø> (-0.08%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/readiness/list.go	`79.41% <0.00%> (-11.77%)`	⬇️
...onstrainttemplate/constrainttemplate_controller.go	`52.02% <0.00%> (-1.91%)`	⬇️
...kg/controller/mutators/assign/assign_controller.go	`47.42% <0.00%> (-1.72%)`	⬇️
pkg/readiness/object_tracker.go	`84.24% <0.00%> (+1.83%)`	⬆️
pkg/watch/replay.go	`81.46% <0.00%> (+2.24%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f33db10...b17b24d. Read the comment docs.

NissesSenap · 2021-08-05T06:04:10Z

Thanks for all your help @maxsmythe do you want me to squash the commits or is it something that you do when you merge?

sozercan · 2021-08-05T16:33:12Z

cmd/build/helmify/kustomize-for-helm.yaml

-            - --port=8443
+            - --port=HELMSUBST_DEPLOYMENT_CONTROLLER_MANAGER_PORT
+            - --health-addr=:HELMSUBST_DEPLOYMENT_CONTROLLER_MANAGER_HEALTH_PORT
+            - --prometheus-port=HELMSUBST_DEPLOYMENT_CONTROLLER_MANAGER_PROMETHEUS_PORT


thoughts about calling this metrics-port? since the exporter can target more than Prometheus

Sounds reasonable, i will update PR when i get to work tomorrow or later tonight.

I guess the main reason why I used prometheusPort is due to that the flag is --prometheus-port but if you change this flag in the future to make it more generic we won't have to change the helm values at least :)
I will just verify the change locally and then I will update the PR.

sozercan · 2021-08-05T16:34:14Z

@NissesSenap no need to squash manually, we'll squash merge when PR merges

Signed-off-by: Edvin Norling <edvin.norling@xenit.se>

NissesSenap · 2021-08-21T06:31:02Z

Anything needed to move this PR forward? Would be great if we can get this merged.
Ping @sozercan @maxsmythe

sozercan

Sorry for the delay. LGTM

NissesSenap · 2021-08-24T11:34:21Z

No worries, thank you

…ent#1438) Co-authored-by: Sertaç Özercan <852750+sozercan@users.noreply.github.com>

Helm enable to config controller manager port

c6d2c89

The current port can be taken when running in hostNetwork mode Signed-off-by: Edvin Norling <edvin.norling@xenit.se>

NissesSenap force-pushed the helm/port-config branch from e3401b0 to c6d2c89 Compare July 14, 2021 09:57

Edvin Norling added 2 commits July 14, 2021 14:29

Additional config of metrics & healthz port

16593c7

Signed-off-by: Edvin Norling <edvin.norling@xenit.se>

Remove health endpoint config

e3c1c7f

Signed-off-by: Edvin Norling <edvin.norling@xenit.se>

NissesSenap force-pushed the helm/port-config branch from 5220174 to e3c1c7f Compare July 14, 2021 12:49

Add healthPort instead of healthAddr

a937587

This way we can still easily define the port but not the addr. I think it's a good enough middle way. Signed-off-by: Edvin Norling <edvin.norling@xenit.se>

maxsmythe requested changes Jul 16, 2021

View reviewed changes

cmd/build/helmify/kustomization.yaml Outdated Show resolved Hide resolved

Remove "" from HEALTH_PORT

077d4e6

Signed-off-by: Edvin Norling <edvin.norling@xenit.se>

NissesSenap force-pushed the helm/port-config branch from eeb5cc6 to 077d4e6 Compare July 19, 2021 11:26

NissesSenap and others added 2 commits August 4, 2021 08:21

Merge branch 'master' into helm/port-config

d4b5712

Delete port usng patchesStrategicMerge

02fdf0b

Signed-off-by: Edvin Norling <edvin.norling@xenit.se>

NissesSenap force-pushed the helm/port-config branch from 1171bb6 to 02fdf0b Compare August 4, 2021 07:50

Edvin Norling added 2 commits August 4, 2021 09:51

Add port definition for audit-deployment

e3d8559

Signed-off-by: Edvin Norling <edvin.norling@xenit.se>

Update docs to match with audit

e94896b

Also remove some withespace Signed-off-by: Edvin Norling <edvin.norling@xenit.se>

NissesSenap changed the title ~~Helm enable to config controller manager port~~ Helm enable to config controller manager & audit port Aug 4, 2021

Change webhook svc from 8443 to webhook-server

6a74339

Update the GKE docs since we use the name webhook-server you don't have to update the svc targetPort. Signed-off-by: Edvin Norling <edvin.norling@xenit.se>

NissesSenap force-pushed the helm/port-config branch from e7a9438 to 6a74339 Compare August 4, 2021 08:21

NissesSenap commented Aug 4, 2021

View reviewed changes

NissesSenap requested a review from maxsmythe August 4, 2021 08:24

maxsmythe approved these changes Aug 4, 2021

View reviewed changes

config/webhook/service.yaml

Comment on lines -10 to +9

targetPort: 8443

targetPort: webhook-server

Copy link

Contributor

maxsmythe Aug 4, 2021

Choose a reason for hiding this comment

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

Perfect!

maxsmythe requested review from sozercan and ritazh August 4, 2021 23:05

Merge branch 'master' into helm/port-config

738d905

Merge branch 'master' into helm/port-config

b17b24d

sozercan reviewed Aug 5, 2021

View reviewed changes

Change from prometheusPort to metricsPort

a79e1f0

Signed-off-by: Edvin Norling <edvin.norling@xenit.se>

NissesSenap requested a review from sozercan August 6, 2021 09:07

Merge branch 'master' into helm/port-config

e7ecdb2

sozercan approved these changes Aug 23, 2021

View reviewed changes

sozercan merged commit 1901725 into open-policy-agent:master Aug 23, 2021

simongottschlag pushed a commit to simongottschlag/gatekeeper that referenced this pull request Aug 26, 2021

Helm enable to config controller manager & audit port (open-policy-ag…

d574059

…ent#1438) Co-authored-by: Sertaç Özercan <852750+sozercan@users.noreply.github.com>

simongottschlag mentioned this pull request Aug 26, 2021

opa-gatekeeper: move back to opa chart repository XenitAB/terraform-modules#381

Closed

NissesSenap deleted the helm/port-config branch September 1, 2021 11:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Helm enable to config controller manager & audit port #1438

Helm enable to config controller manager & audit port #1438

NissesSenap commented Jul 14, 2021

NissesSenap commented Jul 14, 2021

NissesSenap commented Jul 14, 2021

NissesSenap commented Jul 14, 2021

maxsmythe left a comment

NissesSenap Aug 4, 2021

maxsmythe Aug 4, 2021

maxsmythe left a comment

maxsmythe Aug 4, 2021

codecov-commenter commented Aug 4, 2021 •

edited

Loading

NissesSenap commented Aug 5, 2021

sozercan Aug 5, 2021

NissesSenap Aug 5, 2021

NissesSenap Aug 6, 2021

sozercan commented Aug 5, 2021

NissesSenap commented Aug 21, 2021

sozercan left a comment

NissesSenap commented Aug 24, 2021

Helm enable to config controller manager & audit port #1438

Helm enable to config controller manager & audit port #1438

Conversation

NissesSenap commented Jul 14, 2021

NissesSenap commented Jul 14, 2021

NissesSenap commented Jul 14, 2021

NissesSenap commented Jul 14, 2021

maxsmythe left a comment

Choose a reason for hiding this comment

NissesSenap Aug 4, 2021

Choose a reason for hiding this comment

maxsmythe Aug 4, 2021

Choose a reason for hiding this comment

maxsmythe left a comment

Choose a reason for hiding this comment

maxsmythe Aug 4, 2021

Choose a reason for hiding this comment

codecov-commenter commented Aug 4, 2021 • edited Loading

Codecov Report

NissesSenap commented Aug 5, 2021

sozercan Aug 5, 2021

Choose a reason for hiding this comment

NissesSenap Aug 5, 2021

Choose a reason for hiding this comment

NissesSenap Aug 6, 2021

Choose a reason for hiding this comment

sozercan commented Aug 5, 2021

NissesSenap commented Aug 21, 2021

sozercan left a comment

Choose a reason for hiding this comment

NissesSenap commented Aug 24, 2021

codecov-commenter commented Aug 4, 2021 •

edited

Loading