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

add openshift resources to chart #125

Merged
merged 4 commits into from
Oct 18, 2024
Merged

Conversation

jonmosco
Copy link
Contributor

Motivation

Running LocalStack on OpenShift requires specific Security Context Constraints (SCC) to be applied to ensure proper deployment and operation.

Changes

  • Added conditional logic in the Helm chart templates to include OpenShift-specific configurations when deploying on an OpenShift cluster.
  • The Route template now includes a condition that checks if both .Values.openshift and .Values.route.enabled are set to true before applying the Route.
  • The RoleBinding template was updated to conditionally apply OpenShift-specific settings.
  • Updated the values.yaml to include new parameters for controlling OpenShift-specific behavior (openshift: true and route.enabled: true).

This change ensures that the Helm chart can be more flexibly deployed across different environments, applying OpenShift-specific configurations only when necessary.

- Added OpenShift resources to the chart.
- Introduced a conditional for the OpenShift route.
- Included port in the route values.
- Added a note to describe how to retrieve the route URL.
Copy link
Member

@alexrashed alexrashed left a comment

Choose a reason for hiding this comment

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

Hi @jonmosco!
Thanks for your contribution!
But I think this needs a bit more discussion before we can move forward...
Imho OpenShift (or any other derivatives of K8s) should not be first-class-citizens in this chart itself, but we should definitely facilitate deploying the chart in these orchestrators.
I think the best way moving forward would be to:

  • Implement the possibility to set arbitrary SCCs in this chart
    • Not all users would like to use the anyuid SCC and most parts of LocalStack do not require root permissions.
  • Afterwards you can use the Chart as subchart which deploys LocalStack with the proper roles (by setting the config of this chart / the subchart in our scenario) and the route (from your own chart).

What do you think about that?

@jonmosco
Copy link
Contributor Author

jonmosco commented Aug 20, 2024

Thank you for the feedback @alexrashed, appreciate the insights.

I agree that making OpenShift (other K8s derivatives) first-class citizens in the chart might not be necessary.

  • Implementing the option to set arbitrary SCCs would provide the flexibility needed while keeping the chart generic.
  • Using this Chart as a subchart to set the proper roles seems like a viable approach.

I think this is a good idea, and I'm interested in helping expand the documentation for OpenShift. I believe the community would benefit from the helm approach, in addition to the DevSpaces method described here.

@alexrashed
Copy link
Member

Perfect, so that basically means that we just need a new section in the values.yaml which allows defining arbitrary SCC roles, which is then used in the template for the roles.yaml.
Do you want to change your PR accordingly? Maybe also add a section to the README on how to use this chart in OpenShift?

@jonmosco
Copy link
Contributor Author

Yes, I will update the PR to include these changes, and I will also add a section to the README on how to use this chart in OpenShift.

Thanks for the suggestion!

@alexrashed
Copy link
Member

@jonmosco Coming back to this PR, do you need any help with implementing the changes we discussed? :)

@jonmosco
Copy link
Contributor Author

jonmosco commented Oct 8, 2024

Not yet; just had a few things to work on and hopefully, will have a PR soon. Thank you!

Copy link
Member

@alexrashed alexrashed left a comment

Choose a reason for hiding this comment

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

Thanks a lot for addressing the comment! I think it would be great if we could generalize it even a bit more (see the comment). What do you think?

Comment on lines 22 to 27
{{- if .Values.scc }}
- apiGroups: ["security.openshift.io"]
resources: ["securitycontextconstraints"]
resourceNames: ["anyuid"]
resourceNames:
{{- range .Values.scc.resourceNames }}
- {{ . | quote }}
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be great if we could generalize this even a bit more by allowing arbitrary configs here, similar to the extraEnvVars here:

{{- if .Values.extraEnvVars }}
{{- include "common.tplvalues.render" (dict "value" .Values.extraEnvVars "context" $) | nindent 12 }}
{{- end }}

Something like extraRoles would allow us to enable the usage in OpenShift, while not committing to a specific derivate.
Waht do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good to me! I will work on these changes. Thanks for the feedback.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just so I understand, if the SCCs were to be created without OpenShift, do you mean something like Pod Security Policies, Pod Security Admission, or custom roles that basically mirror what the SCC would provide?

Copy link
Member

Choose a reason for hiding this comment

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

I would really just "move the value template to one level higher", meaning that you would not set the resource names for a pre-defined / hard coded rule, but that you can add arbitrary complete rule entries.

So basically your value file would look something like this:

role:
  extraRules:
    - apiGroups: ["security.openshift.io"]
      resources: ["securitycontextconstraints"]
      resourceNames: ["anyuid"]
      verbs: ["use"]

We already do the same thing with the extraEnvVars linked above (where we even have template evaluation of the entries), so it should be fairly straight forward to implement.

And in the whole chart there would be no mention of OpenShift (because the chart would stay derivate-agnostic).
But it would still be great to have it explained how it works for OpenShift in the readme (thanks for the push on the docs)!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I think i'm getting closer to what you are describing. Let me know if you like this diff.

Copy link
Member

@alexrashed alexrashed left a comment

Choose a reason for hiding this comment

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

I feel like there was a bit of back-and-forth with the discussion, and your previous commit unfortunately didn't really produce a valid roles.yml.
I hope it's okay, but since I was already at it / testing it, I just adjusted your solution to what I meant / we discussed.
You can see in the commit (78a5552) that it's actually less complex (only 1 line) but also allows template substitution in the values (with the common.tplvalues.render feature).

Thanks a lot for your contribution, I'll directly merge the PR and create a new release.

@alexrashed alexrashed merged commit c3520d7 into localstack:main Oct 18, 2024
2 checks passed
@jonmosco
Copy link
Contributor Author

This looks good to me! Thanks for the explanations and for merging this!

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.

2 participants