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

feat(pod-template): add tor service pod template #18

Merged
merged 5 commits into from
Jul 12, 2022

Conversation

conneryn
Copy link
Contributor

@conneryn conneryn commented Jul 8, 2022

Fixes #17

@bugfest
Copy link
Owner

bugfest commented Jul 10, 2022

Hi @conneryn, thanks for the changes.

I think we could improve future templating features if we use corev1.PodSpec (template.spec) directly instead of copying parts of it. I defined Reources as part of the template (template.resources). Do you like this approach?

proposal at branch https://github.com/bugfest/tor-controller/tree/podspectemplate

@conneryn
Copy link
Contributor Author

conneryn commented Jul 11, 2022

Great, thanks for the feedback!

I like the idea of pulling the resources property out to the top level of the template. I made that change, and also manually applied most of the changes from your commit above (sorry, didn't actually rebase/work from your commit, as was already mid-progress on some of the changes and didn't want to deal with resolving the conflicts).

For the OnionBalancedService, I added a balancerTemplate property with spec/metadata and two separate sub-properties (torResources and balancerResources) for the two containers that get added to the balancer pod (see onionbalancedservice-resources.yaml for an example).
NOTE: I'm not a huge fan of the naming here, so I'm very open to suggestions/alternatives.

I also updated the template specs to be corev1.PodSpec, so a user can set any properties they want.
However, two issues arise from using corev1.PodSpec which I had to introduce "hacky"-ish workarounds for:

  1. PodSpec has containers set as required which causes validation of OnionService objects to fail unless set. This is solved by adding hacks/remove-containers-requirement.sh, which runs during make manifests to remove the requirement from the generated CRDs. This solution is borrowed from here.
  2. PodSpec is large, and adding it to the OnionBalancedService caused the generated CRDs to be very large. This makes it so kubectl apply -f -- fails to apply the CRD with the error "metadata.annotations: Too long: must have at most 262144 bytes" (kubectl apply attempts to add an annotation last-applied-configuration, with the whole contents of the document). Similar discussions can be found here, here, here, and here.
    Current Solution: Don't include descriptions with the CRD (added crd:maxDescLen=0 to make manifests).
    Alternate Option: use kubectl create or replace instead of kubectl apply in the make install phase. If any end-users ever try to manually kubectl apply the CRDS, they would still run into this issue.

@bugfest
Copy link
Owner

bugfest commented Jul 11, 2022

Thanks @conneryn!

  1. No problem on my side. Workaround good to go.
  2. I had the same issue. While the workaround does it job I miss the descriptions. I'm a huge fan of kubectl explain and we'd be forcing the user to go to the spec source to understand each field. Can we use kubectl replace --force -f - instead?

Gonna review the files to check If I missed something but it's looking great so far :D

Makefile Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
Copy link
Owner

@bugfest bugfest left a comment

Choose a reason for hiding this comment

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

just a couple of minor things

@conneryn conneryn marked this pull request as ready for review July 12, 2022 01:42
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.

[REQUEST] Support specifying various PodSpec properties on the OnionService pods
2 participants