replace ports for default ones in kb

[camilamacedo86]

See that we have here:

        options := ctrl.Options{
		MetricsBindAddress: 8383,
		Port:                    9443, 
	}

When the new layout uses:

        options := ctrl.Options{
		MetricsBindAddress: 8080,
		Port:                    9443, 
	}

If we do not change it in the helm-operator then we might need to change the scaffolds. See: https://github.com/kubernetes-sigs/kubebuilder/search?q=8080

[camilamacedo86]

Hi @joelanford,

WDYT?

[joelanford]

If we do not change it in the helm-operator then we might need to change the scaffolds.

It is inevitable that we'll have to change the scaffolds from the default kubebuilder scaffolds. For example:

• we need to insert the --leader-election-id in the scaffold
• depending on our kube-state-metrics decision, we may need to do some extra stuff in the scaffolding to export those metrics on a separate port.
• we need to remove the webhook stuff since that is not supported by the helm-operator.

Therefore, I think maintaining backwards compatibility with existing operators is more important than kubebuilder alignment on this configuration.

[camilamacedo86]

Hi @joelanford,

When I speak over to change the scaffolds I mean about the conifg/templates files. So:

we need to insert the --leader-election-id in the scaffold:

(Not really true)

See that with the PR https://github.com/joelanford/helm-operator/pull/11 we can address the option via to customize the --leader-election-id via docs. What is the use case where the user really need to define the --leader-election-id ? Is it a common scenario? Would it really be a need for the biggest part of the users or it is a more advanced very unique need?

• depending on our kube-state-metrics decision, we may need to do some extra stuff in the scaffolding to export those metrics on a separate port.

IMO in order to keep the simplicity (KISS), we could use the current setup to scape the metrics instead of to export those metrics on a separate port.

• we need to remove the webhook stuff since that is not supported by the helm-operator:

Do not scaffold a template file has not the same complexity to change its content and in the biggest part of the scenarios will not break the backwards compatibility with SDK and KB new layout. So, in POV for the first moment, we should try to avoid replacing the content of the files as always that it is possible.

Therefore, I think maintaining backwards compatibility with existing operators is more important than kubebuilder alignment on this configuration.

Note that operator-sdk init will not use the current ports used by helm-operator here. So, we are not making the things aligned across the project as well.

[joelanford]

What is the use case where the user really need to define the --leader-election-id ?

Anytime leader election is enabled, which is the default for existing helm-operators AND in the kubebuilder go operators. See https://github.com/kubernetes-sigs/kubebuilder/blob/5413199261b87a22f7a30515862397e09f198bdf/pkg/plugin/v2/scaffolds/internal/templates/main.go#L244

We can't do it directly in main.go because that file is effectively shared by ALL helm-based operators.

IMO in order to keep the simplicity (KISS), we could use the current setup to scape the metrics instead of to export those metrics on a separate port.

Possibly, but that would be a breaking change from how we're already doing it in the helm and ansible operators. That IS an option, but one that would need to be discussed/documented.

Think about this from the perspective of an existing user of the helm operator. Ideally, they would be able to change their helm-operator base image to one based on this repo, and everything would still work.

Note that operator-sdk init will not use the current ports used by helm-operator here. So, we are not making the things aligned across the project as well.

Why not? That's how I've got the WIP plugin working now:
https://github.com/joelanford/helm-operator/blob/2d480b5bddf2955dd9591dc9ba690d737dc27ac7/pkg/plugin/v2/scaffolds/internal/templates/metricsauth/auth_proxy_patch.go#L80

[camilamacedo86]

I will track it in the umbrella. closing