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

Support openshift routes #1206

Merged
merged 16 commits into from Dec 12, 2022

Conversation

frzifus
Copy link
Member

@frzifus frzifus commented Oct 26, 2022

Closes #902

@frzifus frzifus force-pushed the support_openshift_routes branch 3 times, most recently from a9c5522 to 5c29ef0 Compare October 27, 2022 09:32
@frzifus frzifus force-pushed the support_openshift_routes branch 2 times, most recently from 69617ea to 7285b8e Compare November 29, 2022 14:31
@frzifus
Copy link
Member Author

frzifus commented Nov 29, 2022

actually iam a bit unhappy with the current solution. Any thoughts how to solve this a bit more elegant @pavolloffay ?
Here i try to add and remove a route reconciler depending on the detected platform.

func (r *OpenTelemetryCollectorReconciler) registerOnChangeTask() {
	// NOTE: At the time the reconciler gets created, the platform type is still unknown.
	// TODO: does it actually work? since config is just a copy?
	r.config.OnChange = func() error {
		r.muTasks.Lock()
		defer r.muTasks.Unlock()
		var (
			routesPos = -1
			otelPos   = -1
		)
		for i, t := range r.tasks {
			// search for route reconciler
			switch t.Name {
			case "opentelemetry":
				otelPos = i
			case "routes":
				routesPos = i
			}
		}
		if otelPos == -1 {
			return fmt.Errorf("missing reconciler task: opentelemetry")
		}
		plt := r.config.Platform()
		// if exists and platform is openshift
		if routesPos == -1 && plt == platform.OpenShift {
			end := r.tasks[otelPos:]
			r.tasks = append([]Task{}, r.tasks[:otelPos]...)
			r.tasks = append(r.tasks, Task{reconcile.Routes, "routes", true})
			r.tasks = append(r.tasks, end...)
		}
		// if exists and platform is not openshift
		if routesPos != -1 && plt != platform.OpenShift {
			r.tasks = append(r.tasks[:routesPos], r.tasks[routesPos+1:]...)
		}

		return r.config.OnChange()
	}
}

@frzifus
Copy link
Member Author

frzifus commented Nov 30, 2022

ok, it seems the OnChange field is actually never used.

Since we need to register some kind of onChange method to add and remove routes from the reconciliation.. I thought about different ways:

  1. the hacky one - here i assume code says more then words :octocat: Since we are not able to access the onChange methods from outside the config, we would register a function, where we register our reconcile callback afterwards.
	var onChange func() error

	cfg := config.New(
		...
		config.WithOnChange(func() error { return onChange() }),
		...
	)

	...

	onChange := otelReonciler.OnChange
  1. Make Config threadsafe and add a RegisterOnChange(f func() error) method to it. Since AutoDetect runs concurrently and while we register something, the internal onChange list could be used we should lock it. The main disadvantage here, is that we would change the passed config to be a ptr in the entire codebase.
  2. Another way would be to build some kind of thread-safe OnChangeHandler. Add change handler to register callbacks #1292

Personally, I would prefer solution 3.

@frzifus frzifus force-pushed the support_openshift_routes branch 3 times, most recently from 8b3bf8a to b1ed67f Compare December 1, 2022 22:33
@frzifus
Copy link
Member Author

frzifus commented Dec 1, 2022

@pavolloffay as discussed in person, an extra structure is introduced to define the route termination.

@frzifus frzifus marked this pull request as ready for review December 1, 2022 22:39
@frzifus frzifus requested a review from a team as a code owner December 1, 2022 22:39
@frzifus frzifus force-pushed the support_openshift_routes branch 7 times, most recently from ba6bb56 to 3aa2834 Compare December 2, 2022 20:04
Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>
Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>
- determine platform
- grant otel controller permission to route api

Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>
Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>
Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>
Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>
Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>
Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>
Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>
@frzifus frzifus requested a review from a team as a code owner December 8, 2022 13:24
@frzifus
Copy link
Member Author

frzifus commented Dec 8, 2022

@open-telemetry/operator-ta-maintainers thats what i touched there: 5e800c8

@frzifus frzifus force-pushed the support_openshift_routes branch 3 times, most recently from bb91937 to 4392692 Compare December 8, 2022 16:09
@frzifus
Copy link
Member Author

frzifus commented Dec 8, 2022

@pavolloffay could you have a look? :)

Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>
```
main.go:238:16: shadow: declaration of "err" shadows declaration at line 230 (govet)
    configBytes, err := yaml.Marshal(configs)
                 ^
```

Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>
Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>
Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>
@pavolloffay pavolloffay merged commit 0cf9da2 into open-telemetry:main Dec 12, 2022
@frzifus frzifus deleted the support_openshift_routes branch December 12, 2022 10:35
ItielOlenick pushed a commit to ItielOlenick/opentelemetry-operator that referenced this pull request May 1, 2024
* split service port from config calculation into its own function

Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>

* register openshift route v1 as valid ingress enum

Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>

* add routes to reconcile loop

- determine platform
- grant otel controller permission to route api

Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>

* add openshift api to go mod

Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>

* add naming method for openshift routes

Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>

* add route reconcile routine

Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>

* add route reconciler if platform changes to openshift

Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>

* move route cr definition into testdata package

Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>

* controllers: verify that route is created

Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>

* crd: move route tls termination settings into extra section

Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>

* fix: share platform state across copied config objects

Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>

* controller: split opentelemetry collector callback

Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>

* tests: add route e2e tests

Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>

* fix govet linting

```
main.go:238:16: shadow: declaration of "err" shadows declaration at line 230 (govet)
    configBytes, err := yaml.Marshal(configs)
                 ^
```

Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>

* add ingress workaround description

Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>

* regenerate

Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>

Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>
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.

Expose collector outside of cluster
4 participants