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

build: add securityContext and fix rabbitMQ cookie #233

Open
wants to merge 11 commits into
base: dev
Choose a base branch
from

Conversation

viktoriaas
Copy link
Contributor

Details

For details see issue #232

Note
PR includes old commits from my repo. However, they are not relevant anymore and I removed the changes as they were not good. Only commits from 17 March 2022 are relevant.

@viktoriaas viktoriaas requested a review from uniqueg March 21, 2022 11:13
Copy link
Member

@uniqueg uniqueg left a comment

Choose a reason for hiding this comment

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

Thanks @viktoriaas. I would like to see some comment about this being added to the deployment instructions, as I understand that (some of) this is optional. So basically mention what can be enabled, how it can be enabled, and under which circumstances this would be necessary/recommended.

Otherwise it looks fine to me. However, I'm probably not the right person for a more technical review. Maybe @zagganas and @lvarin could have a look? Also to see if this breaks anything on OpenShift.

- /bin/sh
- -c
- |
chmod g-rw /var/lib/rabbitmq/.erlang.cookie;
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this for? why is it necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I already explained in issue #233 . If rabbitMQ deployment is restarted (e.g. due to cluster failure) rabbitMQ cookie has incorrect permissions after restart ( rw-rw---- instead of rw-------). This is solved by chmod in main container before calling rabbitmq.

Copy link
Contributor

Choose a reason for hiding this comment

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

The idea is to put the information about this workaround in the code itself (deployment/templates/rabbitmq/rabbitmq-deployment.yaml). Otherwise no one will ever find this information.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

comment added, is it ok now?

deployment/values.yaml Outdated Show resolved Hide resolved
@viktoriaas
Copy link
Contributor Author

viktoriaas commented Mar 21, 2022

@lvarin @zagganas @uniqueg
I added a section explaining securityContext to README and also created an individual field for mongodb.initContainer.runAsRoot. I understand that there most probably exist clusters with not so strict security policy so there can exist combination where deployment is run under non-root but you want to keep init container as well. Are requested changes resolved?

@uniqueg
Copy link
Member

uniqueg commented May 5, 2022

Looks good to me. @lvarin and/or @zagganas?

@uniqueg uniqueg changed the title Add securityContext and fix rabbitMQ cookie build: add securityContext and fix rabbitMQ cookie Jun 19, 2022
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.

4 participants