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

Fix security context issue for Metal3 in upstream Sylva #68

Merged
merged 3 commits into from
Dec 19, 2023

Conversation

ipetrov117
Copy link
Contributor

This PR directly fixes the problem mentioned in - https://gitlab.com/sylva-projects/sylva-core/-/issues/795

Furthermore it introduces metal3 running with non-root security context by default. This is only for the 0.2.1 version, so that it can land in Sylva ASAP. Testing and PR for the latest Metal3 version will be done as part of another dedicated task.

@ipetrov117 ipetrov117 requested a review from hardys December 19, 2023 09:42
Copy link
Contributor

@Kristian-ZH Kristian-ZH left a comment

Choose a reason for hiding this comment

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

Тhis release will be a bit weird because it extends one of the features in the 0.3.0 release where the security context of the components was introduced but 0.3.0 provides only runAsUser and runAsGroup properties.
This PR extends the security context with more fields and it will be strange that 0.2.1 will be the first release now that introduces a securityContext, then 0.3.0 will cut some of the properties that will probably will be re-aded in a future release...

As this is not a bug in our chart I would suggest to not iterate over a working version but releasing a new one with all the changes in this PR.

Of course, if the changes are really important and urgent, you may not agree with this suggestion

/cc @hardys WDYT?

@Kristian-ZH Kristian-ZH self-requested a review December 19, 2023 10:23
podAnnotations: {}

securityContext:
runAsUser: 11000
Copy link
Contributor

Choose a reason for hiding this comment

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

As the imagePullPolicy in our charts is IfNotPresent, this will not gonna work for existing clusters. Probably the imagePullPolicy could be switched to Always for the current release and reverted back in the future...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMO it is not a very good idea to always pull images. This is a tasking task and might cause additional problems such as laggy internet connection causing errors and so on.

My 2c for the Sylva setup, I think we should keep this as IfNotPresent and in the metal3-suse unit in Sylva provide the image pull policy. Having a default Always pull policy will cause more problems than solutions.

Copy link
Contributor

Choose a reason for hiding this comment

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

In general I agree Always isn't the best solution, but because OBS can build new image revisions without updating the tag (and can't maintain older tags without branching the package) I think it's reasonable for the chart defaults.

We can certainly modify that default for Sylva deployments in the values, and would likely want IfNotPresent in future downstream chart artefacts where we can rely on different image-tagging behavior.

Copy link
Contributor

@Kristian-ZH Kristian-ZH left a comment

Choose a reason for hiding this comment

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

The PR was discussed by us and we decided to go ahead with it as a temporary fix due to time constraints.

@ipetrov117 ipetrov117 merged commit deaac1e into suse-edge:main Dec 19, 2023
2 checks passed
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.

3 participants