Adding message field into AnalysisMessage

[xeviknal]

Related to istio/istio#29445
Related to kiali/kiali#3505

When users have both the status field and the analysis enabled, the message presented is quite limited.
In previous versions, the image there was quite more closer to what we have in the istioctl analysis tool.

This change aims to add the description (made out of the message template) in the status field for each message. For instance:

Instead of having this:

$ k get virtualservices.networking.istio.io details -o yaml | tail -13
        subset: v1
status:
  validationMessages:
  - documentation_url: https://istio.io/v1.8/docs/reference/config/analysis/ist0130/?ref=status-controller
    type:
      code: IST0130

have this:

$ k get virtualservices.networking.istio.io details -o yaml | tail -13
        subset: v1
status:
  validationMessages:
  - documentation_url: https://istio.io/v1.8/docs/reference/config/analysis/ist0130/?ref=status-controller
    description:  VirtualService rule #1 not used (only the last rule can have no matches).
    type:
      code: IST0130

Note that the related issue aims to add the severity into the message. This way, the status will have similar output as the istioctl analyze tool:

$ ./bin/istioctl analyze -n bookinfo
Error [IST0101] (VirtualService httpbin.bookinfo) Referenced gateway not found: "httpbin-gateway-bogus"
Error [IST0101] (VirtualService httpbin.bookinfo) Referenced host not found: "httpbin-gateway"
Warning [IST0130] (VirtualService details.bookinfo) VirtualService rule #1 not used (only the last rule can have no matches).
Error: Analyzers found issues when analyzing namespace: bookinfo.
See https://istio.io/v1.8/docs/reference/config/analysis for more information about causes and resolutions.

This will need a subsequent PR on the istio/istio repo to fill the field.

[istio-policy-bot]

😊 Welcome @xeviknal! This is either your first contribution to the Istio api repo, or it's been
awhile since you've been here.

You can learn more about the Istio working groups, code of conduct, and contributing guidelines
by referring to Contributing to Istio.

Thanks for contributing!

Courtesy of your friendly welcome wagon.

[istio-testing]

Hi @xeviknal. Thanks for your PR.

I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[shamsher31]

/ok-to-test

[xeviknal]

hi @shamsher31,
I am having hard time finding why my make clean gen differs from the ones in the Prow.
I am not well-versed in the protos and generated code. I've make it run by installing the needed tools in the istio/tools repo and using go1.15.6.

Do you mind assisting me fixing this differences?
See the diff file just in case it gives some extra info.

[shamsher31]

@xeviknal Did you run protolock commit --force? I tried locally and it was different than what you committed in this PR.

Please run protolock commit --force and then run make gen.

[xeviknal]

thanks @shamsher31

The two commands changes parts that shouldn't. I see changes in the operator_pb2.py and also in unrelated places within the proto.lock.

In the operator_pb2.py it looks like I am using a different code generator. It looks like the generated code are pretty similar, just using different syntax to generate the same behavior (or really similar). Does this give any hint? Am I using a wrong version of a tool?

[shamsher31]

@esnible can you PTAL?

[shamsher31]

Hey, @xeviknal I created a test PR #1779 and it passes all tests. If you want you can cherry-pick that and raise another PR. or we can close your PR and allow #1779 to review and merge. WDYT?

[google-cla]

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

[google-cla]

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

[xeviknal]

@googlebot I consent.

[google-cla]

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

[shamsher31]

@googlebot I consent

[shamsher31]

Thanks, I will close the test PR #1779.

[xeviknal]

Is something there I can do to move this one forward?

[esnible]

@therealmitchconnors Can you review this please?

[therealmitchconnors]

This looks like a duplication of WeakAnalysisMessage.description, but I'm not certain. @zerobfd and @jasonwzm for details.

[zerobfd]

I think it depends on the intent of this field. The weak schema description is intended to apply to a class of errors. For example, a PodMissingProxy error is always going to have a description like "This error means that a pod is missing a proxy."

If this description is about this specific instance of an error, e.g. "The pod service-ad64f03 is missing a proxy." then it's not a duplicate. If it's going to just describe the class of error, then we should find some way to include the weak schema or a typed error (of which there's still only one it looks like).

[costinm]

Do we have any bugs in validation ? Many of the errors seem to be things that shouldn't have passed validation. For dynamic errors - like pod missing proxy - I don't see how a system would work that dynamically updates status (of how many resources ?) whenever something happens. The Status is supposed to represent the status of that resource - not of other resources ( like other pods or control plane).

…

On Tue, Jan 26, 2021 at 9:44 AM Clay ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In meta/v1alpha1/status.gen.json <#1776 (comment)>: > @@ -78,6 +78,11 @@ "description": "A url pointing to the Istio documentation for this specific error type. Should be of the form `^http(s)?://(preliminary\\.)?istio.io/docs/reference/config/analysis/` <http://istio.io/docs/reference/config/analysis/> Required.", "type": "string", "format": "string" + }, I think it depends on the intent of this field. The weak schema description is intended to apply to a class of errors. For example, a PodMissingProxy error is always going to have a description like "This error means that a pod is missing a proxy." If this description is about this specific *instance* of an error, e.g. "The pod service-ad64f03 is missing a proxy." then it's not a duplicate. If it's going to just describe the *class* of error, then we should find some way to include the weak schema or a typed error (of which there's still only one it looks like). — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#1776 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAUR2QKJ7F7GFE4XPQ4ELLS335ORANCNFSM4UWSK45A> .

[therealmitchconnors]

@costinm you are absolutely right. Validation messages about objects not belonging to Istio (such as pods, deployments, namespaces, etc) are turned off for the in-cluster analysis, but are still accessible when running istioctl analyze.

[istio-testing]

@xeviknal: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[istio-policy-bot]

🚧 This issue or pull request has been closed due to not having had activity from an Istio team member since 2021-01-27. If you feel this issue or pull request deserves attention, please reopen the issue. Please see this wiki page for more information. Thank you for your contributions.

Created by the issue and PR lifecycle manager.