Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Audit Logging] Logger and factory APIs in C-Core and C++. #32750

Merged
merged 53 commits into from Apr 26, 2023

Conversation

rockspore
Copy link
Contributor

@rockspore rockspore commented Mar 29, 2023

Audit logging APIs for both built-in loggers and third-party logger implementations.

C-Core and C++ have independent base classes of loggers and factories for users to implement.

@rockspore
Copy link
Contributor Author

@ctiller @markdroth PTAL. Is this what you had in your mind yesterday?

@rockspore rockspore added the release notes: no Indicates if PR should not be in release notes label Mar 29, 2023
namespace experimental {

// The base struct for audit context.
typedef struct CoreAuditContext {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

class AuditContext { -- no need for a typedef

bool authorized() const;

private:
const grpc_core::experimental::CoreAuditContext* core_context_;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd make this a reference


// The base class for audit logger implementations.
// Users are expected to inherit this class and implement the Log() function.
class AuditLogger : public grpc_core::experimental::CoreAuditLogger {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need inheritance here: this should be its own type.

// Users should inherit this class and implement those declared virtual
// funcitons.
class AuditLoggerFactory
: public grpc_core::experimental::CoreAuditLoggerFactory {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No inheritance here.

// This base class for audit logger implementations.
class CoreAuditLogger {
public:
virtual void CoreLog(const CoreAuditContext& audit_context) = 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should just be Log(...), and we'll have an instance that has a unique_ptr<grpc::AuditLogger> log_ that we call log_->Log(...) on.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So it means this class defined in C-Core can have a dependence on the C++ class, right? I initially was about to do this but wasn't sure if it's fine to have such a reversed dependency.

Copy link
Contributor Author

@rockspore rockspore Mar 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh. is the following flow correct?

// C-Core
class AuditLogger {
 public:
  virtual void Log() = 0;
};
// C++
class AuditLogger {
 public:
  virtual void Log() = 0;
};

class AuditLoggerFactory {
 public:
  virtual std::unique_ptr<AuditLogger> CreateAuditLogger() = 0;
  std::unique_ptr<CoreLogger> CreateCoreLogger() {
    return std::make_unique<CoreLogger>(CreateAuditLogger());
  }
};

// non-public API
class CoreLogger : public grpc_core::AuditLogger {
 public:
  CoreLogger(std::unique_ptr<AuditLogger> logger) : logger_(logger) {}
  void Log() { logger_->Log(); }
 private:
  std::unique_ptr<AuditLogger> logger_;
};

@rockspore rockspore requested a review from ctiller April 5, 2023 00:02
@rockspore rockspore marked this pull request as ready for review April 21, 2023 22:45
@rockspore rockspore changed the title [Audit Logging] Logger and factory APIs in C-Core and C++ [Audit Logging] Logger and factory APIs in C-Core and C++. Apr 25, 2023
Copy link
Member

@markdroth markdroth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great!

src/core/lib/security/authorization/audit_logging.h Outdated Show resolved Hide resolved
@rockspore rockspore merged commit dcfc5d6 into grpc:master Apr 26, 2023
63 of 64 checks passed
@rockspore rockspore deleted the audit-log-api branch April 26, 2023 13:44
@copybara-service copybara-service bot added the imported Specifies if the PR has been imported to the internal repository label Apr 26, 2023
XuanWang-Amos pushed a commit to XuanWang-Amos/grpc that referenced this pull request May 1, 2023
Audit logging APIs for both built-in loggers and third-party logger
implementations.

C++ uses using decls referring to C-Core APIs.

---------

Co-authored-by: rockspore <rockspore@users.noreply.github.com>
paulosjca pushed a commit to paulosjca/grpc that referenced this pull request May 4, 2023
Audit logging APIs for both built-in loggers and third-party logger
implementations.

C++ uses using decls referring to C-Core APIs.

---------

Co-authored-by: rockspore <rockspore@users.noreply.github.com>
wanlin31 pushed a commit that referenced this pull request May 18, 2023
Audit logging APIs for both built-in loggers and third-party logger
implementations.

C++ uses using decls referring to C-Core APIs.

---------

Co-authored-by: rockspore <rockspore@users.noreply.github.com>
@yijiem yijiem added release notes: yes Indicates if PR needs to be in release notes and removed release notes: no Indicates if PR should not be in release notes labels May 31, 2023
@erm-g erm-g removed the release notes: yes Indicates if PR needs to be in release notes label Jun 12, 2023
@yijiem yijiem added the release notes: no Indicates if PR should not be in release notes label Jun 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bloat/low imported Specifies if the PR has been imported to the internal repository lang/c++ lang/core per-call-memory/neutral per-channel-memory/neutral release notes: no Indicates if PR should not be in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants