Fix invalid revision in IOP

[shamsher31]

To fix #24819

# iop.yaml
apiVersion: install.istio.io/v1alpha1
kind: IstioOperator
metadata:
  namespace: istio-system
spec:
  profile: default
  revision: 18  # All of these values will fail:  18, 1.8, 1.8.0, "18", "1.8", "1.8.0"

$ ./out/linux_amd64/istioctl install -f iop.yaml --set hub=gcr.io/istio-testing -d manifests/

Error: failed to get profile, namespace or enabled components: failed to read profile:
invalid revision specified: 18. revision must be a string eg: "1-9-0"

Related:
#28745
#28778

[shamsher31]

/test integ-helm-tests_istio

[shamsher31]

/test integ-security-k8s-tests_istio

[shamsher31]

@howardjohn PTAL.

[su225]

@shamsher31 - I think #24819 is more generic than fixing bad revision. So I think we should still keep it open even if this PR is merged. Probably we should convert it to an umbrella issue to track these kind of stuff?

[shamsher31]

@esnible PTAL and provide your feedback on this.

[esnible]

I like the way this is headed.

@howardjohn has a good point that it is strange to validate inside UnmarshalIOP(). My first idea was that UnmarshallIOP should change non-String Revision to a string, but that the revision itself should be validated elsewhere.

However, this might be too much flexibility. Perhaps UnmarshallIOP() should reject non-strings. It is unclear if the message when a non-string YAML arrives should be "revision must be a string" or the better message you have that requires calling validate inside unmarshal. For Istio developers rejecting non-string is the most helpful, as it keeps the unmarshal method generic. Users might prefer to get a good error message the first time, rather than a generic message. Whichever approach you take, make sure the call to validate the version is inside the function that validates the IOP even if it is already called when unmarshalling.

[shamsher31]

Perhaps UnmarshallIOP() should reject non-strings.

For Istio developers rejecting non-string is the most helpful, as it keeps the unmarshal method generic

Agree. Updated to check just non-string revision in UnmarshallIOP().

but that the revision itself should be validated elsewhere.

make sure the call to validate the version is inside the function that validates the IOP even if it is already called when unmarshalling.

Invalid string revision is checked in the validation step here #28745

Users might prefer to get a good error message the first time, rather than a generic message.

$ ./out/linux_amd64/istioctl install -f iop.yaml --set hub=gcr.io/istio-testing -d manifests/

Error: failed to get profile, namespace or enabled components: failed to read profile:
invalid revision specified: false. revision must be a string eg: "1-9-0"

@esnible Please take another look.

[shamsher31]

@esnible PTAL.

[howardjohn]

Not sure if the code has changed or I am just repeating myself, but I still don't think this is right.

First, this method is extremely inefficient. First we take a yaml string, then marshal to a map, then back to yaml, then marshal to a map again, then marshal to IstioOperator. This feels very wrong and contributes to massive memory allocations (it may seem like speed doesn't matter much since its a one-shot CLI tool, and it doesn't, but the amount of allocations we do is so high it makes our unit tests noticeably slow)

Second, why is revision special? Don't we have the same problem with every other field?

[esnible]

We had problems in the past with IOPs with creation timestamps. (e.g. istioctl/pkg/install/pre-check.go, istioctl/pkg/verifier/verifier.go and already in operator/pkg/validate/common.go.). However, I have retested by removing the clearing of creation time from istioctl x precheck and I see no problems. Let's stop introducing the clearing of Time fields until we can identify a way to reproduce.

[shamsher31]

@howardjohn @esnible Does it make sense to merge this PR or we are ok with invalid revision in IOP?

[esnible]

@shamsher31 I suggest we merge this. I approved it. @howardjohn What do you think?

[howardjohn]

I will repeat my comment:

@shamsher31 I understand why you put it here, but it doesn't feel correct to me. However, I also do not know a better place, as I don't work on this code base often.

Maybe @ostromart or @richardwxn can give a better review, I don't feel comfortable approving since I am not familiar and it isn't obviously correct to me. Thanks

[shamsher31]

@ostromart @richardwxn WDYT about this fix?

[shamsher31]

@istio/wg-environments-maintainers ping.

[ostromart]

my initial reaction to the code is are we sure this is conceptually the right fix?
afaict the issue with some characters in revision is orthogonal to what a field unmarshals to. these issues should be solved separately. the issue of illegal character stems from revision being used in some k8s resource fields where some chars are illegal per k8s naming rules. that used to be a problem, not sure if it is anymore but that's the first thing to sort out.
once we are clear on what chars are legal/illegal, we should type the field in IstioOperator appropriately so that it unmarshals whatever inputs we support (can use intorstring or interface types and make this change in istio/api). then, validate the unmarshaled value only to ensure it is legal wrt k8s naming (the first step).

[shamsher31]

Closing as this is not a valid solution.