IstioControlPlaneSpec Version validation pattern too wide

[Tanemahuta]

Describe the bug
The regex validating the version of IstioControlPlaneSpec.Version is too wide. The . should be escaped.

Steps to reproduce the issue:
Deploy operator chart version 2.0.5. Add a IstioControlPlane with

spec:
   version: 12

Expected behavior
Validation fails in the k8s API.

[Laci21]

Hi @Tanemahuta,

Thanks for the PR! Could you please elaborate on what was your issue? What was the exact IstioControlPlane CR that you used and what issue exactly did you get? Because we are not aware of such issue right now.

Having said that your PR might make sense anyway, we'll review it, we just would like to understand the root issue as well.

[Tanemahuta]

Sure. I tried to deploy the operator (chart 2.0.6) and a controlplane cr.
Could only set version to "1." due to the validation pattern.
Controller failed validation due to version not starting with 1.11. I don't think that this was intentional by the code and the descriptors.

[Laci21]

You should be able to deploy this: https://github.com/banzaicloud/istio-operator/blob/release-1.11/config/samples/servicemesh_v1alpha1_istiocontrolplane.yaml#L6

[Laci21]

So it only needs to be started with 1. because of this pattern: https://github.com/banzaicloud/istio-operator/blob/release-1.11/config/crd/bases/istio-operator-crds.gen.yaml#L6382 (note the ^ at the beginning).

[Tanemahuta]

I have tried that, but the pattern is matched to the full string. Please believe me, i have tried to use 1.11, but the k8s API threw a validation error. I modified the CRD locally and: kazam, it worked.

[waynz0r]

@Tanemahuta,

what was the error the apiserver threw? The version field is a string, so 1.11 won't work without quotes like 1.11.5 would for example, but that has nothing to do with the regex pattern.

[Tanemahuta]

@Laci21 and @waynz0r,
First, I have to excuse, the additional description of the issue was misleading and partially wrong.

It was my editor throwing an invalid error when validating the CR against the CRD (seems like the regex was matched against the complete string instead of finding a single match).

When I deployed the spec with version: "1.12", the controller yielded an error (as intended by you, I suppose).

(Sorry, exhausting week for me, obviously)

I still have two requests:

1. the regex is not quite doing what you want it to do. It would also match a 12 as a version, which is probably not what you had in mind.
2. it would be rather nice, if the controller would add a warning instead of erroring when encountering a minor version, which has not been tested yet (in your case >1.11), since the users of your operator would have to wait for you to release a new version of the operator, and they'd be stuck to the minor for the time being.

My solutions:

1. I have modified this issue and the corresponding PR, I hope you do not mind.
2. I could add a new issue and create a new PR, if you would want it. But since this is controller behavior, you probably want to discuss this in the team.

Cheers,
Christian

[Laci21]

Hi Christian,

Let me answer to your requests:

1. Actually, our intention in the helm chart is exactly to match 1.12, 1.13 or whatever else (matching this regex^1.) as well. This is because when we release a new helm chart from our istio operator, which will support Istio 1.12 it should be usable with any other older Istio 1.x version as well. We don't break backward compatibility and because of that we don't have to maintain different helm chart versions for different Istio operator versions. You can always use the latest istio operator helm chart and when we will have support for Istio 1.12, you should be able to install Istio 1.11 and Istio 1.12 as well with the latest operator. That's why we deliberately allow to match e.g. 1.12.
2. When we will have support for Istio 1.12, we'll release an image sth like v2.12.0 and update the regex in the controller code to match ^1.12 for those versions. So 1.11 controller will only reconcile ICP CR's with version 1.11 and 1.12 controllers will only reconcile 1.12 ICP CR's and so on. This is again deliberate and we throw an error so that it is clear to the user that unmatched versions cannot be reconciled. Otherwise cross reconciliations might cause issues e.g. during Istio control plane canary upgrades.

Summing up, everything is deliberate as it is today. Let us know if you have questions, issues with any of the approaches, we are open to suggestions.

Laszlo

[Laci21]

Also, the IstioControlPlane CRD is backward compatible as well, which is why the version is used as ^1..

[Tanemahuta]

@Laci21 , please read the issue description again. The dot in the regex needs to be escaped, that's what I modified my PR to do.

[Laci21]

My bad, you are completely right, that is indeed a bug, thanks for catching and fixing it!

[Laci21]

Fixed in #809.