Uploaded image for project: 'Sentry'
  1. Sentry
  2. SENTRY-1546

Generic Policy provides bad error messages for Sentry exceptions

    XMLWordPrintableJSON

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 1.8.0, 2.0.0
    • Fix Version/s: 1.8.0, 2.0.0
    • Component/s: None
    • Labels:

      Description

      I discovered that when you attempt to create a role that already exists the error message you get back from Thrift i just 'Role: foo' which is very confusing.

      The reason is that the SentryStore throws

      SentryAlreadyExistsException("Role: " + trimmedRoleName);

      and the generic policy processor passes the message as is:

        public TCreateSentryRoleResponse create_sentry_role(
            final TCreateSentryRoleRequest request) throws TException {
          Response<Void> respose = requestHandle(new RequestHandler<Void>() {
            @Override
            public Response<Void> handle() throws Exception {
              validateClientVersion(request.getProtocol_version());
              authorize(request.getRequestorUserName(),
                  getRequestorGroups(conf, request.getRequestorUserName()));
              store.createRole(request.getComponent(), request.getRoleName(),
                      request.getRequestorUserName());
              return new Response<Void>(Status.OK());
            }
          });
      ...
      

      The similar thing is happening for other requests and other Sentry-specific exceptions.

      The legacy policy processor does decorate the error a bit:

        public TCreateSentryRoleResponse create_sentry_role(
          TCreateSentryRoleRequest request) throws TException {
          final Timer.Context timerContext = sentryMetrics.createRoleTimer.time();
          TCreateSentryRoleResponse response = new TCreateSentryRoleResponse();
          try {
            validateClientVersion(request.getProtocol_version());
            authorize(request.getRequestorUserName(),
                getRequestorGroups(request.getRequestorUserName()));
            sentryStore.createSentryRole(request.getRoleName());
            response.setStatus(Status.OK());
            notificationHandlerInvoker.create_sentry_role(request, response);
          } catch (SentryAlreadyExistsException e) {
            String msg = "Role: " + request + " already exists.";
            LOGGER.error(msg, e);
            response.setStatus(Status.AlreadyExists(msg, e));
          } catch (SentryAccessDeniedException e) {
            LOGGER.error(e.getMessage(), e);
            response.setStatus(Status.AccessDenied(e.getMessage(), e));
          } catch (SentryThriftAPIMismatchException e) {
            LOGGER.error(e.getMessage(), e);
            response.setStatus(Status.THRIFT_VERSION_MISMATCH(e.getMessage(), e));
          } catch (Exception e) {
            String msg = "Unknown error for request: " + request + ", message: " + e.getMessage();
            LOGGER.error(msg, e);
            response.setStatus(Status.RuntimeError(msg, e));
          } finally {
      ...
      

      I think that it is better to just put the right message in the exception itself and do not decorate it later.

        Attachments

        1. SENTRY-1546.001.patch
          3 kB
          Krishna Kalyan
        2. SENTRY-1546.002.patch
          3 kB
          Krishna Kalyan
        3. SENTRY-1546.003.patch
          10 kB
          Krishna Kalyan
        4. SENTRY-1546.005.patch
          10 kB
          Krishna Kalyan
        5. SENTRY-1546.001-sentry-ha-redesign.patch
          9 kB
          Krishna Kalyan

          Issue Links

            Activity

              People

              • Assignee:
                kkalyan Krishna Kalyan
                Reporter:
                akolb Alex Kolbasov
              • Votes:
                0 Vote for this issue
                Watchers:
                2 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: