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

operators: make Bazel authoritative while still supporting kubebuilder #2837

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

burgerdev
Copy link
Contributor

@burgerdev burgerdev commented Jan 22, 2024

Context

It's not obvious to the casual contributor how the Constellation Node Operator module interacts with its Helm chart: some resources are duplicated, even with a slight mismatch sometimes, and changing the Kubernetes resources is in the best case toily, but in the worst case might lead to bugs (diff between make deploy vs. what ends up bundled in the CLI).

On a related note: the kubebuilder project version we're using does not seem to be supported anymore for current Kubernetes versions. A scary warning reads:

[Deprecation Notice] This version is deprecated.The `go/v3` cannot scaffold projects using kustomize versions v4x+ and cannot fully support Kubernetes 1.25+.It is recommended to upgrade your project to the latest versions available (go/v4).Please, check the migration guide to learn how to upgrade your project

This PR assumes that we want to keep compatibility with kubebuilder workflows (e.g. make deploy)! If we can drop this requirement, we would not need to migrate project versions and could remove a lot of things in the operators/constellation-node-operator directory.

This PR does not fix the discrepancies between kustomized resources in o/c/config/... and helmified resources in i/c/helm/...! (e.g. the csp value) This is fundamentally hard, because we have diffs on top of the initially generated Helm chart that we would need to backport to kustomize. I suggest to ignore this for now - the embedded code generation directives are imho much more error prone than resource patches in a directory clearly related to kubebuilder Makefiles, and we're fixing the former here. This problem would obviously go away if we were to drop support for kubebuilder dev workflows.

Proposed change(s)

  • Migrate kubebuilder project to go/v4 layout (guide)
  • Add Bazel autogeneration for the generate and manifests targets of the kubebuilder Makefile.
  • Reorganize the Constellation Helm chart to accommodate code generation (rename files, split RBAC into resources).
  • Directly generate RBAC and CRDs into the Helm chart.
    • This introduces a few cosmetic changes that should not alter behaviour of the chart.
    • This also introduces whitespace errors, but that should be fixed upstream.

The commits are suitable for individual review, which might also help understanding the logical relationships better.

Additional info

  • AB#3611 (unblocks removal of the kube-rbac-proxy by making changes to resources safe)

Checklist

  • Add labels (e.g., for changelog category)
  • Is PR title adequate for changelog?
  • Link to Milestone

@burgerdev burgerdev added the no changelog Change won't be listed in release changelog label Jan 22, 2024
@burgerdev burgerdev added this to the v2.15.0 milestone Jan 22, 2024
Copy link

netlify bot commented Jan 22, 2024

Deploy Preview for constellation-docs canceled.

Name Link
🔨 Latest commit cc49510
🔍 Latest deploy log https://app.netlify.com/sites/constellation-docs/deploys/65aeb07a19562f00081c6dcf

@katexochen katexochen removed their request for review January 22, 2024 08:17
@malt3
Copy link
Contributor

malt3 commented Jan 22, 2024

This problem would obviously go away if we were to drop support for kubebuilder dev workflows.

I do see your point regarding overhead for supporting the kubebuilder workflows. The added benefits are quite limited. Before we invest more costs into supporting it: What is your preferred resolution?
I would be fine with either solution. Maybe to add some more background to this: I originally added the skaffolding when I was inexperienced with writing operators. If we were to build a new operator today, we might skip this support entirely.

@burgerdev
Copy link
Contributor Author

This problem would obviously go away if we were to drop support for kubebuilder dev workflows.

I do see your point regarding overhead for supporting the kubebuilder workflows. The added benefits are quite limited. Before we invest more costs into supporting it: What is your preferred resolution? I would be fine with either solution. Maybe to add some more background to this: I originally added the skaffolding when I was inexperienced with writing operators. If we were to build a new operator today, we might skip this support entirely.

What I think is valuable to keep are the CRD and RBAC permission workflows, which is part of the reason why I ported them to Bazel in the first place. I like the idea of stating the permissions you need right next to the location where you use them in code, and nobody should be hand-crafting any CRD yaml :) What I can definitely live without is the Makefile, the config dir etc - if nobody is using this for rapid feedback operator development, I'd say we're better off without that.

@malt3
Copy link
Contributor

malt3 commented Jan 22, 2024

What I can definitely live without is the Makefile, the config dir etc - if nobody is using this for rapid feedback operator development, I'd say we're better off without that.

Sounds good to me. Let's go with that.

This commit adds the controller-gen tool as a Bazel dep and adds the
equivalent of `make manifests` to //:generate.

Rename CRDs in the operator Helm chart to match kubebuilder v4 generated
names and add a generation directive to output CRD resources directly
into the Helm chart. CRDs are not templated, so we don't need to helmify
them.

Split the manager-rbac.yaml into a role.yaml and a rolebinding.yaml, and
add a directive to generate role.yaml from operator sources. We're
losing the labels on the role, but that is only a cosmetic defect.
@burgerdev burgerdev force-pushed the burgerdev/generate-operator-resources branch from 710badb to cc49510 Compare January 22, 2024 18:14
@burgerdev burgerdev removed this from the v2.15.0 milestone Jan 24, 2024
@burgerdev burgerdev marked this pull request as draft January 24, 2024 10:15
@burgerdev burgerdev mentioned this pull request Jan 24, 2024
3 tasks
@malt3 malt3 removed their request for review May 24, 2024 07:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no changelog Change won't be listed in release changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants