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

Security context additions to khchecks #423

Merged
merged 9 commits into from Apr 28, 2020
Merged

Security context additions to khchecks #423

merged 9 commits into from Apr 28, 2020

Conversation

2infinitee
Copy link
Collaborator

Need your thoughts on these changes to the helm charts.

  1. Should user and fsgroup be 999 or 1000. I got a bit confused since there was already a security context value for the user.
  2. Are the security context where they should be.

I added these fields into each of the khchecks pod spec:

  podSpec:
    securityContext:
      runAsUser: {{ .Values.securityContext.runAsUser }}
      fsGroup: {{ .Values.securityContext.fsGroup }}

and in the container spec:

      securityContext:
        runAsUser: {{ .Values.securityContext.runAsUser }}
        fsGroup: {{ .Values.securityContext.fsGroup }}

can be adjusted in the values.yaml file under:

securityContext:
  runAsNonRoot: true
  runAsUser: 999
  fsGroup: 999
  allowPrivilegeEscalation: false
  readOnlyRootFilesystem: true

@2infinitee
Copy link
Collaborator Author

#400 tagging issue

@2infinitee
Copy link
Collaborator Author

@NissesSenap does this look about right to what you were asking for?

@NissesSenap
Copy link
Contributor

I have tried it and it works with a workaround for me.
If you can implement my review comment as well then I won't even need to do a workaround :).

Thanks for checking @2infinitee

2infinitee and others added 4 commits April 22, 2020 12:59
adding in suggested if statement to khcheck-deployment's container spec as well.

Co-Authored-By: Eric Greer <eric.greer@comcast.com>
apart of the if statement

Co-Authored-By: Eric Greer <eric.greer@comcast.com>
commiting eric's suggestion.

Co-Authored-By: Eric Greer <eric.greer@comcast.com>
including description of what enabled does

Co-Authored-By: Eric Greer <eric.greer@comcast.com>
Copy link
Collaborator Author

@2infinitee 2infinitee left a comment

Choose a reason for hiding this comment

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

last bits of changes

Copy link
Collaborator

@jonnydawg jonnydawg left a comment

Choose a reason for hiding this comment

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

Nice!

@integrii integrii merged commit b6ee322 into master Apr 28, 2020
@2infinitee 2infinitee deleted the securityContext branch April 30, 2020 17:19
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

5 participants