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

Introduce new customization options and restricted config filesystems #92

Merged
merged 11 commits into from
Jan 25, 2024

Conversation

at-platform24
Copy link
Contributor

with some small and some large changes.

A summary is:

  • Bump up from stable-8719 to stable-8960-1
  • Add /config as emptyDir, to support readOnlyRootFilesystem
  • Make defaults config and cont-init scripts customize at deploy time
  • Add nodePorts for Prosody and Web for restricted Kubernetes deployments
  • Add example configurations, and Makefile, to test changes quickly

with some small and some large changes.

A summary is:
- Bump up from stable-8719 to stable-8960-1
- Add /config as emptyDir, to support readOnlyRootFilesystem
- Make defaults config and cont-init scripts customize at deploy time
- Add nodePorts for Prosody and Web for restricted Kubernetes
  deployments
- Add example configurations, and Makefile, to test changes quickly
@at-platform24
Copy link
Contributor Author

@spijet May you have a review please.

@spijet
Copy link
Collaborator

spijet commented Nov 24, 2023

Hello @at-platform24!

Sorry for the huge delay. I've added some comments to the PR, please reply when you have time.

Do I understand it right that if the custom defaults file value is empty or unset, the ConfigMap field gets populated with a stub comment, and there will be no record for that file in volumeMounts?

Also, can you please elaborate on the replicas field for Prosody? Do you want to scale it up or to replace it with an external Prosody instance(s)?

@at-platform24
Copy link
Contributor Author

at-platform24 commented Nov 24, 2023

Do I understand it right that if the custom defaults file value is empty or unset, the ConfigMap field gets populated with a stub comment, and there will be no record for that file in volumeMounts?

That is correct. make template or make templateWithCustomDefaults can be tried to verify the volumenMounts.

 make template > /tmp/jitsi-default.yaml
 make templateWithCustomDefaults > /tmp/jitsi-custom.yaml
 diff /tmp/jitsi-* | tail -30

Do you want to scale it up or to replace it with an external Prosody instance(s)?

Our use case is where we can use the external prosody instance, however, having the full config available from the helm release make external prosody configuration teaks and validation mush simpler, as well as being able to quickly stop/ start different versions during upgrades, having a safe and fast rollback path where run in-cluster prosody.

Thank you for the thoughtful review, and excellent work in maintaining this chart.

@spijet
Copy link
Collaborator

spijet commented Nov 24, 2023

Our use case is where we can use the external prosody instance, however, having the full config available from the helm release make external prosody configuration teaks and validation mush simpler, as well as being able to quickly stop/ start different versions during upgrades, having a safe and fast rollback path where run in-cluster prosody.

Hmm, maybe we could add a new option that disables the provision of Prosody (the StatefulSet, Service and whatnot) while keeping all the ConfigMaps and Secrets intact? I think it might be easier than explaining that the replicas field is not supposed to be used for actually scaling Prosody. :)

@at-platform24
Copy link
Contributor Author

at-platform24 commented Nov 24, 2023

Makes sense.

fixed this, along with adding a comment. However, have not removed full manifest creation since it is useful to know what all is needed by prosody.
and resolved merge conflicts.

PR title is no longer relevant, but I guess that should not be a problem 😄

@spijet
Copy link
Collaborator

spijet commented Nov 24, 2023

Oh, I just realized that I forgot to actually submit the review. 🤦‍♂️

Sorry, I'll update the comments and submit the review in a moment.

Copy link
Collaborator

@spijet spijet left a comment

Choose a reason for hiding this comment

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

Please refer to the in-file comments for details.

values.yaml Outdated Show resolved Hide resolved
values.yaml Show resolved Hide resolved
values.yaml Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@at-platform24
Copy link
Contributor Author

One question about version.
This changeset does not add any breaking changes from previous one, should this be a patch version bump or minor version bump?

@spijet
Copy link
Collaborator

spijet commented Nov 24, 2023

should this be a patch version bump or minor version bump?

Let's do a minor version bump (v1.3.8 -> v1.4.0). This PR adds new functionality to the chart, so it makes sense to bump the minor version, even though there are no breaking changes.

@spijet
Copy link
Collaborator

spijet commented Nov 24, 2023

Also, since the Makefile and the custom configuration examples are not going to be rendered by Helm and applied to the k8s cluster during chart install, it makes sense to add the Makefile and example-configurations directory to the .helmignore file. Can you do that inside the PR, please?

@at-platform24
Copy link
Contributor Author

All changes applied.
Feel free to make any changes needed that you find useful for the broader user community.

There are cases where we may prefer to create common secrets
outside the chart and override values filled from configmaps
or secrets created from this chart.

Common usecases would be Hashicorp Vault/ 1Password operator
integrations.

This commits makes such overrides possible.
@spijet
Copy link
Collaborator

spijet commented Dec 16, 2023

Sorry for the delay. :(
I'll review the revised changes in the coming days and will let you know if everything is OK.

@spijet
Copy link
Collaborator

spijet commented Jan 22, 2024

Once again, sorry for the delay. The Christmas / New Year holidays are a busy period here. 😅
I'll review it now and will let you know if there's anything else we need to edit before merging.

Copy link
Collaborator

@spijet spijet left a comment

Choose a reason for hiding this comment

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

Overall looking good, I hope this review is the last before merging. :D

charts/prosody/templates/statefulset.yaml Outdated Show resolved Hide resolved
charts/prosody/templates/statefulset.yaml Outdated Show resolved Hide resolved
values.yaml Show resolved Hide resolved
@spijet spijet changed the title Chart upgrade for Jitsi stable-8960-1 version Introduce new customization options and restricted config filesystems Jan 22, 2024
@spijet spijet mentioned this pull request Jan 23, 2024
@spijet
Copy link
Collaborator

spijet commented Jan 23, 2024

I just merged #99, so keep an eye out for any possible conflicts and be ready to rebase if needed. 👀

@at-platform24
Copy link
Contributor Author

I just merged #99, so keep an eye out for any possible conflicts and be ready to rebase if needed. 👀

luckily for me no conflicts occurred on merge.
I hope we are ready for finally merging the MR.

@spijet
Copy link
Collaborator

spijet commented Jan 25, 2024

Let's do one final push with the "bundled vs external" Prosody knob and the ConfigMap permissions and I'll merge it right away. :)

@spijet
Copy link
Collaborator

spijet commented Jan 25, 2024

Yep, looking good to me! Merging.

Thank you for your time and once again sorry for the delays!

@spijet spijet merged commit 93f5ee9 into jitsi-contrib:main Jan 25, 2024
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