- 
                Notifications
    You must be signed in to change notification settings 
- Fork 1.8k
Feat: Add HPA & PDB Helm Chart Support #13391 #13512
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
base: dev
Are you sure you want to change the base?
Conversation
59e977a    to
    6ad7153      
    Compare
  
    6ad7153    to
    a12e4c1      
    Compare
  
    b402cbc    to
    8916e76      
    Compare
  
    - Add PodDisruptionBudget for Django pods - Add HorizontalPodAutoscaler for Django pods - Add PodDisruptionBudget for Celery Beat pods - Add HorizontalPodAutoscaler for Celery Beat pods - All resources default to disabled (enabled: false) - Configurable via values.yaml Fixes DefectDojo#13391
8916e76    to
    feeec15      
    Compare
  
    | We should also strongly consider adding  | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fully agree with @rossops's comments
| This pull request introduces an uncontrolled configuration injection risk: the Celery worker PodDisruptionBudget Helm template renders  
Uncontrolled Configuration Injection in  | 
| Vulnerability | Uncontrolled Configuration Injection | 
|---|---|
| Description | The Helm chart for the Celery worker Pod Disruption Budget (PDB) uses toYamlon.Values.celery.worker.podDisruptionBudget. Thevalues.schema.jsonfor this object does not explicitly setadditionalProperties: false, which means that any arbitrary key-value pairs provided by a user in the Helm values forcelery.worker.podDisruptionBudgetwill be rendered directly into the Kubernetes PodDisruptionBudgetspec. This allows an attacker with control over Helm values to inject arbitrary fields into the PDB spec, potentially leading to a denial of service for cluster operations by making the PDB too restrictive (e.g.,maxUnavailable: 0) or negating its purpose by making it too permissive. | 
django-DefectDojo/helm/defectdojo/templates/celery-worker-pdb.yaml
Lines 30 to 31 in 98da46a
| {{ toYaml (omit .Values.celery.worker.podDisruptionBudget "enabled" ) | indent 2 }} | |
| {{- end }} | 
All finding details can be found in the DryRun Security Dashboard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of small questions/suggestions (see comments).
Plus, can you add a description to https://github.com/DefectDojo/django-DefectDojo/blob/dev/helm/defectdojo/Chart.yaml#L37 (it will fix the linter).
The rest looks good to me.
| averageUtilization: {{ . }} | ||
| type: Utilization | ||
| {{- end }} | ||
| {{- if .Values.celery.worker.autoscaling.autoscaleBehavior }} | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any reason not to use with here?
| type: Utilization | ||
| {{- end }} | ||
| {{- if .Values.celery.worker.autoscaling.autoscaleBehavior }} | ||
| behavior: {{ toYaml .Values.celery.worker.autoscaling.autoscaleBehavior | nindent 4 }} | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shorter version: ....autoscaling.behavior might be better
| averageUtilization: {{ . }} | ||
| type: Utilization | ||
| {{- end }} | ||
| {{- if .Values.django.autoscaling.autoscaleBehavior }} | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same (with)
| type: Utilization | ||
| {{- end }} | ||
| {{- if .Values.django.autoscaling.autoscaleBehavior }} | ||
| behavior: {{ toYaml .Values.django.autoscaling.autoscaleBehavior | nindent 4 }} | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same (....autoscaling.behavior)
Fixes: #13391
We don't want to waste your time, so if you're unsure whether your hypothetical enhancement meets the criteria for approval, please file an issue to get pre-approval before beginning work on a PR.
Learn more here: https://github.com/DefectDojo/django-DefectDojo/blob/master/readme-docs/CONTRIBUTING.md#submission-pre-approval
Description
Describe the feature / bug fix implemented by this PR.
If this is a new parser, the parser guide may be worth (re)reading.
Test results
Ideally you extend the test suite in
tests/anddojo/unitteststo cover the changed in this PR.Alternatively, describe what you have and haven't tested.
Documentation
Please update any documentation when needed in the documentation folder)
Checklist
This checklist is for your information.
dev.dev.bugfixbranch.Extra information
Please clear everything below when submitting your pull request, it's here purely for your information.
Moderators: Labels currently accepted for PRs:
Contributors: Git Tips
Rebase on dev branch
If the dev branch has changed since you started working on it, please rebase your work after the current dev.
On your working branch
mybranch:In case of conflict:
When everything's fine on your local branch, force push to your
myOriginremote:To cancel everything:
Squashing commits
pickbyfixupon the commits you want squashed outpickbyrewordon the first commit if you want to change the commit messageForce push to your
myOriginremote: