Skip to content

Conversation

atoulme
Copy link
Contributor

@atoulme atoulme commented Jul 18, 2025

This PR is a draft showing a direction of being able to deploy the mutating webhook using exclusively static configuration rather than instrumentation CRDs.

There are 3 parts to it:

  • Add the instrumentation CRD to the config struct, so it can be fulfilled.
  • Add an option to disable looking up CRDs for instrumentations, instead relying on what the config says.
  • Add an integration test showing this works end to end - this test runs outside of the current e2e tests and performs the installation step explicitly, which is not possible right now with the existing integration tests.

Copy link
Contributor

github-actions bot commented Jul 18, 2025

E2E Test Results

 33 files  ±0  221 suites  ±0   3h 37m 34s ⏱️ - 17m 1s
 85 tests ±0   85 ✅ +2  0 💤 ±0  0 ❌  - 2 
225 runs  ±0  225 ✅ +2  0 💤 ±0  0 ❌  - 2 

Results for commit bea749c. ± Comparison against base commit a62a458.

♻️ This comment has been updated with latest results.

@atoulme atoulme force-pushed the crds_wip branch 9 times, most recently from e61a981 to bea749c Compare July 21, 2025 06:21
@atoulme atoulme changed the title wip of CRDs as config [manager] Run the mutating webhook with configuration, disabling instrumentation CRDs. Jul 21, 2025
@atoulme atoulme marked this pull request as ready for review July 22, 2025 17:43
@atoulme atoulme requested a review from a team as a code owner July 22, 2025 17:43
)

// InstrumentationSpec defines the desired state of OpenTelemetry SDK and instrumentation.
type InstrumentationSpec struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

can we not reuse the instrumentation spec from the CRD package?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you cannot because you get import cycles.

If we choose to have Config define the instrumentation CRD struct, we'd need to move it, and we no longer would generate it from the CRD.

Copy link
Contributor

Choose a reason for hiding this comment

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

This dependency is mostly incidental though. The CRD itself doesn't depend on the config struct, the webhooks do, and they just happen to be in the same package. There's several things we can do to fix this, like moving webhook implementations to a different package, or having them depend on their own config struct rather than the full operator one.

I would also much rather not define the instrumentation spec twice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it? I assumed the Struct was derived from the CRD generation step. I can look further and break this down into smaller chunks.

@jaronoff97
Copy link
Contributor

I think I'm confused why we need to embed the instrumentations in the config struct if all we care about is disabling/enabling?

@atoulme
Copy link
Contributor Author

atoulme commented Jul 28, 2025

I think I'm confused why we need to embed the instrumentations in the config struct if all we care about is disabling/enabling?

If we disable the CRD, we can still enable autoinstrumentation. We just define everything in config.

@jaronoff97
Copy link
Contributor

jaronoff97 commented Jul 28, 2025

I don't think that makes sense to me – if the CRDs are disabled we shouldn't run autoinstrumentation because we have nothing defined? I don't understand why we want to put the CRDs in the config like this.

@atoulme
Copy link
Contributor Author

atoulme commented Jul 28, 2025

I don't think that makes sense to me – if the CRDs are disabled we shouldn't run autoinstrumentation because we have nothing defined?

No, they are defined in config.

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.

3 participants