search

google / knative-gcp

GCP event implementations to use with Knative Eventing.
https://github.com/knative/eventing
Apache License 2.0
160 stars 74 forks source link

Update error message verification in Auth Check #2111

Closed grac3gao-zz closed 3 years ago

grac3gao-zz commented 3 years ago

Fixes #

Proposed Changes

Release Note

Docs

knative-prow-robot commented 3 years ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: grac3gao

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files: - ~~[OWNERS](https://github.com/google/knative-gcp/blob/master/OWNERS)~~ [grac3gao] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
zhongduo commented 3 years ago

I am not sure I fully understand the whole message.

  1. What's the original message that make both the old and new code work?
  2. What's the message that works with only new code but not old code?
grac3gao-zz commented 3 years ago

The full error message we want for authentication check: MountVolume.SetUp failed for volume \"google-cloud-key\" : secret \"google-cloud-key\" not found The full error message we don't want (a k8s internal issue, not an authentication configuration problem): MountVolume.SetUp failed for volume \"google-cloud-key\" : failed to sync secret cache: timed out waiting for the condition

old key word: MountVolume.SetUp failed for volume \"google-cloud-key\" new key word: secret \"google-cloud-key\" not found

Why not use the whole sentence to check MountVolume.SetUp failed for volume \"google-cloud-key\" : secret \"google-cloud-key\" not found ? I found there is a white space right after MountVolume.SetUp failed for volume \"google-cloud-key\", before : in the event message. In other event message, this white space doesn't appear. I guess it is a kind of small typo in the original k8s codebase. In case they might delete this white space, and break our check, I only include half of the sentence to be the key words. Also, before verifying the event message, we filter in k8s events only for specific Pod object and for warning type, so it is unlikely that there might be other event message has the same keyword for this Pod object.

zhongduo commented 3 years ago

Thanks for clarification, this makes sense to me. And I will vote for partial matching too. /lgtm