-
Notifications
You must be signed in to change notification settings - Fork 83
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
add support for openshift security context #2027
Conversation
@pgvishnuram can we create an issue for this with the description of the problem we are trying to solve here. And also tag it with the releases we are planning to ship this to. |
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.
Looks good so far. We need to ensure these are all covered by tests.
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.
lgtm. We should add/update the tests
{{ if .Values.global.openshiftEnabled }} | ||
securityContexts: | ||
pod: | ||
runAsNonRoot: true | ||
{{ end }} |
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.
If this works for openshift, can we just use it as the standard config? Is there any good reason to run it without this setting when we are running in a non-openshift cluster? Same question for all the changes in this file, and most of this PR.
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.
I was thinking on the same lines. We can keep changes agnostic to environoment as the standard config and openshift deployment specific config under the flag
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.
Agreed on Daniels point. I think the biggest shift is switching logging from FluentD to Vector as a default which would probably cause issues to existing customers.
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.
We should reduce the variety between openshift and non-openshift as much as possible. The more we reduce that diff, the less code we have to maintain, and the less things there are that can break, and the less QA testing needs to be done.
pre-commit failing |
Its fixed |
Description
This PR adds support to native openshift installation by allowing customer to disable securityContext and podSecurityContext
Components affected
Related Issues
https://github.com/astronomer/issues/issues/5919
Testing
Do not merge this PR until this text is replaced with details about how these changes were tested.
Merging
Do not merge this PR until it lists which release branches this PR should be merged / cherry-picked into.