OSSM-5730: Allow creating 2 cluster-wide SMCPs when one of them is a gateway controller

[jewertow]

This change aims to allow installing SMCP as a gateway controller and another one as a standard service mesh at the same time. What's more, service mesh should be allowed to be cluster-wide, even though the gateway controller is also cluster-wide, because if gateway controller would be pre-installed in the future, then cluster-wide SMCP could not be installed at all. Cluster-wide service mesh still makes sense, because users can narrow its scope using discovery selectors. There is a risk of overwriting istio-ca-root-cert config maps, so I added a validation which makes sure that 2 SMCPs can't be installed with the default config map name. Beside that we should require customizing the GatewayClass name, but it could be implemented in a follow-up.

[openshift-ci]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[jewertow]

/test all

[dgn]

thanks Jacek, especially for the good test coverage! I only had time for a first pass, will give it a more thorough look tomorrow. we don't need to allow multi-tenant gateway-controller modes, which simplifies the logic quite a bit; I left some comments to that extent

[dgn]

same here, check negatives first, then happy path

if newSmcp.IsGatewayController() == currentSmcp.IsGatewayController() {
    return append(allErrors, otherSmcpExists)
}
return append(allErrors, validateOverlappingCaCertConfigMapNames(meta, newSmcp, &smcps, allErrors)...)

[jewertow]

Thanks, done.

[dgn]

how would countGatewayControllers() > 1 ever happen? I'd say if len(smcps.Items) > 1 we will never allow creation of a cluster-wide smcp, just return the error straight away

[jewertow]

You're right. I added a test case, which cannot exist and then I implemented conditions, and I didn't notice that it does not make any sense.

{
        name: "creating cluster-wide gateway controller SMCP when another already exists - expected error (2nd execution)",
        smcp: clusterWideGatewayController,
        existingObjs: []*maistrav2.ServiceMeshControlPlane{
                NewV2SMCPResource("basic", "istio-system-1", clusterWideGatewayController),
                NewV2SMCPResource("basic", "istio-system-2", clusterWideGatewayController2),
        },
        expectedErr: fmt.Errorf("a cluster-scoped SMCP may only be created when no other SMCPs exist"),
},

[jewertow]

I was wrong. We need that test case as well, because 2 gateway-controllers can exist when a user creates: gateway-controller and simple cluster-wide and then changes the 2nd to gateway-controller. That's why more than 1 gateway-controller can be found and in such a case we have to return an error and stop reconciliation.

[jewertow]

Actually, I could simplify this condition and implement this validation in ValidateUpdate, which is called when SMCP mode is changed. That approach would be better, because currently, users will be able to update the SMCP without any error, but the reconciliation will fail. If I implemented this check in ValidateUpdate, users would get validation error.

[dgn]

this whole block can be removed if we check above if !currentSmcp.IsClusterScoped()

[jewertow]

As you can see in another comment, this condition would have to be merged with !currentSmcp.IsClusterScoped().

[dgn]

I suggest starting with the negative cases, so:

	if newSmcp.IsClusterScoped() {
		// an SMCP already exists and new one is created
		if len(smcps.Items) == 1 && smcps.Items[0].UID != meta.GetUID() {
			currentSmcp := smcps.Items[0].Spec
			if !currentSmcp.IsClusterScoped() {
				return append(allErrors, otherSmcpExists)
			}

[jewertow]

This condition is incorrect, because when existing SMCP is multi-tenant we have to check if the new one is cluster-wide gateway controller, so it could be:

	if newSmcp.IsClusterScoped() {
		// an SMCP already exists and new one is created
		if len(smcps.Items) == 1 && smcps.Items[0].UID != meta.GetUID() {
			currentSmcp := smcps.Items[0].Spec
			if !currentSmcp.IsClusterScoped() && !newSmcp.IsGatewayController() {
				return append(allErrors, otherSmcpExists)
			}

[dgn]

this whole if-clause is not needed anymore if you check the negative cases first

[jewertow]

Thanks, done

[dgn]

I'm not sure we really need to cover multitenant meshes- we could probably say they're incompatible with GW-API support. That would simplify things as we don't have to mix cluster-wide with multitenant in the same cluster