weixin_39771301
weixin_39771301
2020-12-01 22:45

Fix potentially ignored error from validation checks

Closes https://github.com/kiali/kiali/issues/1786

If i'm correct, the error resulting from validation checks was ignored. Now at least it's logged. If I'm correct having an error might result in empty validations object.

该提问来源于开源项目:kiali/kiali

  • 点赞
  • 写回答
  • 关注问题
  • 收藏
  • 复制链接分享
  • 邀请回答

7条回答

  • weixin_39771301 weixin_39771301 5月前

    I've edited my commit, what do you think of this?

    点赞 评论 复制链接分享
  • weixin_39771301 weixin_39771301 5月前

    Soooo this is now "fixed" as the error is now really coming down to the UI. We can explain now what happened here #1786 . BUT the funny thing is... it's certainly not what we want! Now a call to service details returns 500 internal server error (hence I can't see any service) because of this, on my setup: clusterrbacconfigs.rbac.istio.io is forbidden: User "system:serviceaccount:istio-system:kiali-service-account" cannot list clusterrbacconfigs.rbac.istio.io at the cluster scope: no RBAC policy matched

    It should be a warning, but it should not be something necessary that prevents seeing any service. I don't know if you have a suggestion on how to fix that.

    点赞 评论 复制链接分享
  • weixin_39771301 weixin_39771301 5月前

    Put differently: what should we do with errors during validation?

    • Just log, without informing frontend?
    • Inform frontend but without a code 500 to still allow service display
    • Inform frontend with code 500 (but I think it's bad especially if the errors during validation are something related to the user setup - which isn't even incorrect, like in my case here)
    • Or maybe it's just a problem with that particular "error" that should not be defined as an formal error
    点赞 评论 复制链接分享
  • weixin_39622988 weixin_39622988 5月前

    backend errors should be moved to UI but there are a couple of exceptions where we may want to put just a warning.

    I'm interested in that, because, clusterrbacconfigs should be empty when kiali doesn't have cluster rights.

    Perhaps due the error you are fixing here, it was missed in one of my previous PRs for that.

    I can take a look to confirm what should send.

    点赞 评论 复制链接分享
  • weixin_39771301 weixin_39771301 5月前

    just investigating a little bit more: this error was not triggered from GetMeshPolicies, but from fetchAuthorizationDetails (where there's no similar code to return empty data)

    点赞 评论 复制链接分享
  • weixin_39771301 weixin_39771301 5月前

    take a look on my last commit please : I'm checking explicitly if errors are "forbidden", in which case we log silently ; else error is sent to channel. Doing this both for 'Get[Service]MeshPolicies' as before, plus 'GetAuthorizationDetails' which is where it was failing for me.

    点赞 评论 复制链接分享
  • weixin_39622988 weixin_39622988 5月前

    Yes, the GetAuthorizationDetails is the offending one. It should have a similar logic as GetClusterRbacConfigs which shouldn't propagate fail but just send a warning.

    I'd suggest to unify the logic in both, I mean if we have a check() method to validate the error, worth to use it in both places or viceversa, just send a warning when that method fails.

    点赞 评论 复制链接分享

相关推荐