[ZK filter] Remove EnvoyException on data plane

[Winbobob]

Commit Message: [ZK filter] Remove EnvoyException on data plane
Additional Description: The Envoy style guideline suggests "Exceptions are allowed on the control plane, though now discouraged in new code. Adding exceptions is disallowed on the data plane", details in #10878 and #27412. This diff tries to remove EnvoyException on the ZK proxy filter data plane and paves path for per-opcode decoder error metrics.
Risk Level: Low
Testing: Unit tests
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A

[repokitteh-read-only]

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #30438 was opened by Winbobob.

see: more, trace.

[Winbobob]

/retest

[Winbobob]

/retest

[Winbobob]

Hi @JuniorHsu, please review whenever you have a chance, thanks!

[JuniorHsu]

i need more time to deliberate on the macro but put an immediate comment first

[JuniorHsu]

let's try to reduce the number of macro

if macros are zk specific, probably introduce a enum

enum class ReportPolicy {
 // comment 
 None, 
 // comment
 DecodeError
}

then ReportPolicy could be a parameter of macro
let's make ENVOY_LOG always happen in debug level if it applies

[JuniorHsu]

could we just return status then we can save a pair of macros?

[Winbobob]

If the function return type is absl::Status, we could return status directly.
If the function return type is absl::StatusOr<T>, we cannot return status directly, so I make it return absl::InvalidArgumentError instead.

[Winbobob]

let's try to reduce the number of macro

if macros are zk specific, probably introduce a enum

enum class ReportPolicy {
 // comment 
 None, 
 // comment
 DecodeError
}

then ReportPolicy could be a parameter of macro let's make ENVOY_LOG always happen in debug level if it applies

I tried, but had some issues.

After adding the above enum, I converted the marcos sth like below

#define ABSL_STATUS_RETURN_IF_STATUS_NOT_OK(status, report_policy)                                 \
  if (!status.ok()) {                                                                              \
    if (report_policy == ReportPolicy::DecodeError) {                                              \
      callbacks_.onDecodeError();                                                                  \
    }                                                                                              \
    return status;                                                                                 \
  }

#define RETURN_INVALID_ARG_ERR_IF_STATUS_NOT_OK(status, report_policy, message)                    \
  if (!status.ok()) {                                                                              \
    if (report_policy == ReportPolicy::DecodeError) {                                              \
      callbacks_.onDecodeError();                                                                  \
    }                                                                                              \
    return absl::InvalidArgumentError(message);                                                    \
  }

utils.cc also needs to access these marcos, but it cannot identify callbacks_, which defines in decode.h.
Since decode.h already imported utils.h, if I try to make utils.h import decode.h, there will be circular dependency.

[JuniorHsu]

let's try to reduce the number of macro
if macros are zk specific, probably introduce a enum

enum class ReportPolicy {
 // comment 
 None, 
 // comment
 DecodeError
}

then ReportPolicy could be a parameter of macro let's make ENVOY_LOG always happen in debug level if it applies

I tried, but had some issues.

After adding the above enum, I converted the marcos sth like below

#define ABSL_STATUS_RETURN_IF_STATUS_NOT_OK(status, report_policy)                                 \
  if (!status.ok()) {                                                                              \
    if (report_policy == ReportPolicy::DecodeError) {                                              \
      callbacks_.onDecodeError();                                                                  \
    }                                                                                              \
    return status;                                                                                 \
  }

#define RETURN_INVALID_ARG_ERR_IF_STATUS_NOT_OK(status, report_policy, message)                    \
  if (!status.ok()) {                                                                              \
    if (report_policy == ReportPolicy::DecodeError) {                                              \
      callbacks_.onDecodeError();                                                                  \
    }                                                                                              \
    return absl::InvalidArgumentError(message);                                                    \
  }

utils.cc also needs to access these marcos, but it cannot identify callbacks_, which defines in decode.h. Since decode.h already imported utils.h, if I try to make utils.h import decode.h, there will be circular dependency.

I was thinking

#define RETURN_INVALID_ARG_ERR_IF_STATUS_NOT_OK(status, action)  \
  if (!status.ok()) {                      \
    action;                                \
    return status;                         \
  }

but seems to introduce more repetition.

Let's not keep the dedicate macro for onDecodeError for now. Thanks for investigating on this

[Winbobob]

You mean "let's keep ..."?

[JuniorHsu]

keep ..."?

Yes 🤦

[Winbobob]

The PR is ready for review again, cc @JuniorHsu , thanks!

[JuniorHsu]

I'm fine with this approach. Over to maintainers' round @mattklein123