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

fix(helm): make helm service port configurable to port 80 #3013

Closed
wants to merge 1 commit into from

Conversation

dbirks
Copy link

@dbirks dbirks commented Oct 12, 2022

Description

This makes the Service port able to be configured to a normally privileged port like port 80.

To do so, I'm pinning the containerPort to 4954 to match what trivy server runs on by default, and am making the service.port to not be "overloaded" to be the container port as well.

I took a quick look at other charts like prometheus and grafana, to see if they allow containerPort to be configurable in the values.yml, but it looks like they don't, so I'm thinking it's not so common. I can't think of a use-case for configuring it at least, since all traffic to the Pod comes from the Service. I can make it configurable if you would like though.

To test locally:

Set up a test cluster:

kind create cluster
# and to clean up after the test:
# kind delete cluster

Before

Confirm that setting the Service port to 80 causes the pod to crash:

git checkout main
helm install trivy-main ./helm/trivy --set service.port=80
kubectl logs trivy-main-0

image

After

Try the same from this branch, and confirm that pod isn't crashing:

gh pr checkout 3013
helm install trivy-patched ./helm/trivy --set service.port=80
kubectl logs trivy-patched-0

Screenshot from 2022-10-12 16-39-47

And confirm that we can access the Service on port 80:

kubectl run -it --rm --image alpine test
# in the test alpine pod now...
apk add curl
curl -i http://trivy-patched/healthz

image

Related issues

Checklist

  • I've read the guidelines for contributing to this repository.
  • I've followed the conventions in the PR title.
  • I've added tests that prove my fix is effective or that my feature works.
  • I've updated the documentation with the relevant information (if needed).
  • I've added usage information (if the PR introduces new options)
  • I've included a "before" and "after" example to the description (if the PR is a user interface change).

@dbirks dbirks requested a review from krol3 as a code owner October 12, 2022 21:00
@CLAassistant
Copy link

CLAassistant commented Oct 12, 2022

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@knqyf263
Copy link
Collaborator

@krol3 Do you have a chance to review this PR?

@chen-keinan
Copy link
Contributor

@dbirks sorry coming late on this , could you please make the port configurable (as you suggested) with default: 4954 I think its the best not to have hardcoded values

@github-actions
Copy link

This PR is stale because it has been labeled with inactivity.

@github-actions github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and will be auto-closed. label Feb 11, 2023
@github-actions github-actions bot closed this Mar 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lifecycle/stale Denotes an issue or PR has remained open with no activity and will be auto-closed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat(helm): configure service port independent of container port
4 participants