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

OpenShift Compatibility #63

Closed
wants to merge 15 commits into from
Closed

Conversation

namloc2001
Copy link

@namloc2001 namloc2001 commented Mar 30, 2021

Following changes have been made:

  • Updated securityContext settings to enable OpenShift Restricted SCC compatibility.
  • Added in wait-for-db initContainer to ensure pods don't start before database is ready.
  • Exposed more PostgreSQL keys in values.yaml to simplify deployment parameter setting for OpenShift installs.
  • README updated to explain how to use this chart on OpenShift

davidkarlsen and others added 7 commits March 30, 2021 12:04
Signed-off-by: Matt Colman <namloc2001@yahoo.co.uk>
Updated securityContext settings to enable OpenShift Restricted SCC compatibility. Added in wait-for-db initContainer to ensure pods don't start before database is ready. Exposed more PostgreSQL keys in values.yaml to allow deployment on OpenShift.

Signed-off-by: Matt Colman <namloc2001@yahoo.co.uk>
Signed-off-by: Matt Colman <namloc2001@yahoo.co.uk>
Signed-off-by: Matt Colman <namloc2001@yahoo.co.uk>
Signed-off-by: Matt Colman <namloc2001@yahoo.co.uk>
Signed-off-by: Matt Colman <namloc2001@yahoo.co.uk>
Co-authored-by: David J. M. Karlsen <david@davidkarlsen.com>
Signed-off-by: Matt Colman <namloc2001@yahoo.co.uk>
namloc2001 and others added 2 commits March 30, 2021 12:08
Signed-off-by: Matt Colman <namloc2001@yahoo.co.uk>
Signed-off-by: Matt Colman <namloc2001@yahoo.co.uk>
Signed-off-by: Matt Colman <namloc2001@yahoo.co.uk>
@namloc2001
Copy link
Author

@davidkarlsen the tests are failing for busybox with "failed to create containerd task: OCI runtime create failed: container_linux.go:367: starting container process caused: exec: "[]": executable file not found in $PATH: unknown" however I cannot recreate this locally with Docker or in my OpenShift cluster where it works fine...

@davidkarlsen
Copy link
Collaborator

davidkarlsen commented Mar 30, 2021

there is no need to "expose" postgresql keys in the chart, these are all controllable like in https://github.com/bitnami/charts/blob/master/bitnami/postgresql/values.yaml

GitHub
Helm Charts. Contribute to bitnami/charts development by creating an account on GitHub.

@namloc2001
Copy link
Author

namloc2001 commented Mar 30, 2021

there is no need to "expose" postgresql keys in the chart, these are all controllable like in https://github.com/bitnami/charts/blob/master/bitnami/postgresql/values.yaml

GitHub**bitnami/charts**Helm Charts. Contribute to bitnami/charts development by creating an account on GitHub.

@davidkarlsen, I disagree. If we amend them in the subchart and then update the subcharts over time, our changes will be lost. Exposing these keys is very helpful for deployments as we can then make the consumer aware of the keys we anticipate might need attention and we can control it all from the parent chart. I have seen this done on many charts and would strongly encourage you to please allow these to be here.

@namloc2001
Copy link
Author

Hi @davidkarlsen do you have any view why the tests are failing on this container? And do my changes/suggestions resolve your other queries? Thanks.

@davidkarlsen
Copy link
Collaborator

If we amend them in the subchart and then update the subcharts over time, our changes will be lost. Exposing these keys is very helpful for deployments as we can then make the consumer aware of the keys we anticipate might need attention and we can control it all from the parent chart. I have seen this done on many charts and would strongly encourage you to please allow these to be here.

Duplicating the keys won't solve that. And the version of the sub-chart is locked through the lock-file - having double up definitions won't add value - quite the contradictory. Should the sub-chart change, so will the calling of it have to do as well.

Copy link
Collaborator

@davidkarlsen davidkarlsen left a comment

Choose a reason for hiding this comment

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

See comments, in summary i think:

  • SCC description can be easier one-liner
  • init-containers should be wildcards yaml copies, so they can fit any situation, and independent per deployment
  • securitycontext should be enabled by default as it was (openshift users can opt-in to turn it off)

Signed-off-by: Matt Colman <namloc2001@yahoo.co.uk>
@namloc2001
Copy link
Author

See comments, in summary i think:

* SCC description can be easier one-liner

* init-containers should be wildcards yaml copies, so they can fit any situation, and independent per deployment

* securitycontext should be enabled by default as it was (openshift users can opt-in to turn it off)

Hi @davidkarlsen,

  • SCC rolebinding command as one-liner updated in README
  • init-containers - not sure of best way to achieve this
  • securityContexts are enabled by default, as it was. Exception is frontend.podSecurityContext.enabled as this was commented out in previous version, so I've set it as false to produce some effect.
  • I have removed the postgreSQL keys from values.yaml and instead given them as --set commands in README.

@davidkarlsen
Copy link
Collaborator

When thinking about this, maybe a better solution is to use health-checks? That will in essence do the same as for the database-check, but more generically, like it was in previous versions: https://github.com/evryfs/helm-charts/blob/dependency-track-0.2.4/charts/dependency-track/templates/deployment.yaml#L64

if the DB is not ready (or any other problem for that matter), the app will sit in a restart-loop until the resources are available.

For now the API url can be used as endpoint, while we await more sophisticated ones later on DependencyTrack/dependency-track#1001

WDYT?

@namloc2001
Copy link
Author

namloc2001 commented Apr 6, 2021

When thinking about this, maybe a better solution is to use health-checks? That will in essence do the same as for the database-check, but more generically, like it was in previous versions: https://github.com/evryfs/helm-charts/blob/dependency-track-0.2.4/charts/dependency-track/templates/deployment.yaml#L64

if the DB is not ready (or any other problem for that matter), the app will sit in a restart-loop until the resources are available.

For now the API url can be used as endpoint, while we await more sophisticated ones later on DependencyTrack/dependency-track#1001

WDYT?

Hi @davidkarlsen my idea was to avoid the restart-loop by using the initContainer. Happy for you to take the lead on the decision here, but my preference would be the initContainer. Perhaps if we amend the initContainer values to be something like:

initContainers:
  postgresql:
    enabled: true
  other:
    enabled:true

And then we can call in the relevant initContainer section, such as:

initContainerPostgresql:
  name: wait-for-db
  image:
    repository: busybox
    tag: 1.33.0-uclibc
    pullPolicy: IfNotPresent
  imagePullSecrets: []
  resources:
    limits:
      cpu: 200m
      memory: 500Mi
    requests:
      cpu: 100m
      memory: 200Mi
  command: []

via:

      {{- if or .Values.frontend.initContainers.postgresql.enabled .Values.frontend.initContainers.other.enabled }}
      initContainers:
      {{- with .Values.initContainerPostgresql }}
      {{- toYaml . | nindent 8 }}
      {{- with .Values.initContainerOther }}
      {{- toYaml . | nindent 8 }}

Whereby other and initContainerOther are actual entities set over time as required?

GitHub
OpenSourced Helm charts. Contribute to evryfs/helm-charts development by creating an account on GitHub.

@davidkarlsen
Copy link
Collaborator

davidkarlsen commented Apr 13, 2021

Hi - sorry for the silence - had a lot of stuff going - I think it would make sense to go with health-checks, they are generic and would be helpful anyways. Let's start with that at least - we can always come back and improve in a future version.

@davidkarlsen
Copy link
Collaborator

This was never completed and is obsoleted by #81

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants