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

Move otel operator CRDs to templates #1175

Merged
merged 6 commits into from
May 9, 2024

Conversation

swiatekm
Copy link
Contributor

@swiatekm swiatekm commented May 8, 2024

Partially resolves #1167. See the issue for more details. I ended up patching both the webhook name and the certificate name in addition to the namespace in the CRD.

I verified the upgrade procedure manually.

@swiatekm swiatekm requested a review from Allex1 as a code owner May 8, 2024 11:18
@swiatekm swiatekm requested a review from a team May 8, 2024 11:18
@swiatekm swiatekm force-pushed the crds-to-templates branch from 8b655b9 to 9d0f99d Compare May 8, 2024 11:35
@swiatekm
Copy link
Contributor Author

swiatekm commented May 8, 2024

Looks like the failing test is flaky, it passes locally for me.

Copy link
Member

@TylerHelmuth TylerHelmuth left a comment

Choose a reason for hiding this comment

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

Similar to how managing the CRDs via the crd directory is optional I think installing the CRDs via the template should be optional as well. We should add a new section to the values.yaml that allows controlling when the templated CRDs are installed.

charts/opentelemetry-operator/UPGRADING.md Show resolved Hide resolved
@TylerHelmuth
Copy link
Member

You will need to bump the minor version of the chart and regenerate the examples via make generate-examples CHARTS=opentelemetry-operator.

@swiatekm swiatekm force-pushed the crds-to-templates branch from 9d0f99d to bbc87cd Compare May 8, 2024 14:39
@swiatekm swiatekm requested a review from TylerHelmuth May 8, 2024 14:59
@swiatekm
Copy link
Contributor Author

swiatekm commented May 8, 2024

Similar to how managing the CRDs via the crd directory is optional I think installing the CRDs via the template should be optional as well. We should add a new section to the values.yaml that allows controlling when the templated CRDs are installed.

Does a top-level installCrds option sound good? Or do we want something more elaborate?

@TylerHelmuth
Copy link
Member

Since we're allowing templating, it feels possible someone will want to set a specific value, so I think a section called crds or installCRDs with an enabled field is good.

Copy link
Member

@dmitryax dmitryax left a comment

Choose a reason for hiding this comment

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

LGTM

@jaronoff97
Copy link
Contributor

+1 to tyler's recommendation

@swiatekm swiatekm force-pushed the crds-to-templates branch from a0f5519 to 413977d Compare May 8, 2024 17:42
@swiatekm swiatekm requested a review from jaronoff97 May 8, 2024 17:42
@swiatekm
Copy link
Contributor Author

swiatekm commented May 8, 2024

Added a crds section to the values file, with a create: true option. This makes it consistent with other create-stuff options that currently exist.

It's increasingly annoying to automatically convert CRDs published by the operator into the form the Helm Chart wants. I added a make function to make this easier, but at some point we may want to look for a more holistic solution here.

@TylerHelmuth can you have another look? Also @pavolloffay.

Copy link
Contributor

@jaronoff97 jaronoff97 left a comment

Choose a reason for hiding this comment

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

🎉

Makefile Outdated Show resolved Hide resolved
@swiatekm swiatekm force-pushed the crds-to-templates branch from 4c80273 to fe7e411 Compare May 8, 2024 19:12
@swiatekm
Copy link
Contributor Author

swiatekm commented May 8, 2024

I think something I'm missing here is that this change only works with cert-manager enabled. I should inject the caBundle directly when using a Secret. Please don't merge this until then, I'll get to it tomorrow morning.

On that note, we should really run the e2e tests with cert-manager disabled.

@TylerHelmuth
Copy link
Member

Currently the chart has a cert-manager requirement by default, but supports other options. We definitely need the CRDs to work without the cert manager like they were before. I'm ok with the CI continuing to use cert manager for now since that is our default requirement

@swiatekm
Copy link
Contributor Author

swiatekm commented May 9, 2024

Thinking about it a bit more, that change isn't necessary in this PR, as there's no conversion webhooks defined, and the CRDs don't need a caBundle. So we can merge this as-is and solve this problem in the actual upgrade do 0.99.0.

@swiatekm swiatekm force-pushed the crds-to-templates branch from 6ec7a7b to 5a8651f Compare May 9, 2024 11:02
@swiatekm swiatekm force-pushed the crds-to-templates branch from 5a8651f to 0aa80fc Compare May 9, 2024 11:42
@swiatekm
Copy link
Contributor Author

swiatekm commented May 9, 2024

I've added running e2e tests without certmanager to the CI. We can remove it later if you want, but I'd like to have confidence everything works right in both cases for the CRD migration.

@swiatekm swiatekm force-pushed the crds-to-templates branch from 0aa80fc to 93bb8e6 Compare May 9, 2024 13:02
@TylerHelmuth TylerHelmuth merged commit 89e420f into open-telemetry:main May 9, 2024
4 checks passed
TylerHelmuth added a commit to TylerHelmuth/opentelemetry-helm-charts that referenced this pull request May 9, 2024
* Move otel operator CRDs to templates

* Allow CRD creation to be disabled

* Fix webhook cert annotations when cert-manager disabled

* Run operator tests without cert-manager

* Add README note about CRD creation

* Apply suggestions from code review

---------

Co-authored-by: Tyler Helmuth <[email protected]>
@swiatekm swiatekm deleted the crds-to-templates branch May 10, 2024 09:06
12ushan pushed a commit to giffgaff/opentelemetry-helm-charts that referenced this pull request Jul 22, 2024
* Move otel operator CRDs to templates

* Allow CRD creation to be disabled

* Fix webhook cert annotations when cert-manager disabled

* Run operator tests without cert-manager

* Add README note about CRD creation

* Apply suggestions from code review

---------

Co-authored-by: Tyler Helmuth <[email protected]>
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.

Collector CRD 0.99.0 requires templated conversion webhook
4 participants