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

xds: support built-in Stdout audit logger type #6298

Merged
merged 21 commits into from May 25, 2023
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 8 additions & 5 deletions authz/audit/stdout/stdout_logger.go
Expand Up @@ -31,6 +31,9 @@ import (

var grpcLogger = grpclog.Component("authz-audit")

// Name is the string to identify this logger type in the registry
const Name = "stdout_logger"
Copy link
Member

Choose a reason for hiding this comment

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

Are all loggers going to be registered with a _logger suffix? Isn't this redundant? Is this standardized across languages or no?

Copy link
Contributor

Choose a reason for hiding this comment

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

While I don't have strong preference on whether to keep this suffix, we are using the same name in C++ and it will be the same across languages such that the same authz policy can work everywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was previously hardcoded here

I was just pulling it out to be an exported const so it could be looked up.

I'd be okay to change the name to stdout, what do you think @rockspore

Copy link
Member

Choose a reason for hiding this comment

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

Oh wait..should this just not even be in the registry at all? It seems like it's a built-in type that we support and not something that is supposed to be supported via the registry for generic loggers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe the intention with the built in types is that we place them in the registry for use -

func init() {
audit.RegisterLoggerBuilder(&loggerBuilder{
goLogger: log.New(os.Stdout, "", 0),
})
}

@rockspore please check me here

Copy link
Contributor

@rockspore rockspore May 24, 2023

Choose a reason for hiding this comment

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

I'd be okay to change the name to stdout, what do you think @rockspore

If there is no strong preference, I'm leaning towards keeping it as is so that we don't have to change what's already in C++ and the latest gRFC PR.

Oh wait..should this just not even be in the registry at all?

We use the same registry for built-in loggers. We just pre-register them by importing the pkg here. The registry API probably needs to prevent the built-in types from being overwritten.

Copy link
Member

Choose a reason for hiding this comment

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

We use the same registry for built-in loggers. We just pre-register them by importing the pkg here. The registry API probably needs to prevent the built-in types from being overwritten.

This doesn't sound right to me.

I think the built-in types should be hard-coded and not be present in the registry. IIUC the registry is for the custom types that users can implement. AFAICT there's no need to put the built-in types into the registry. It buys us nothing and seems to have some downsides.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is an important but tangential-to-this-pr point to iron out.

@dfawley how would you feel about keeping this PR going as if the current behavior is the right behavior.
Then, we can resolve this separately, and if we change how the stdout logger is constructed we will update that in a separate PR.
As the audit logging is currently implemented, this is the correct way to do it.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, maybe this is fine. This is still pretty similar to our LB policy design this was based on. The difference here is that the two steps of converting from xds config to local config and then building are so close together that it seems unnecessary to have them be separate. But they're separate because we intend for them to be used through other pathways than xDS, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, we tried to stay similar to the LB policy for consistency

But they're separate because we intend for them to be used through other pathways than xDS, right?

Yes, it can come through here through NewStatic


func init() {
audit.RegisterLoggerBuilder(&loggerBuilder{
goLogger: log.New(os.Stdout, "", 0),
Expand All @@ -46,13 +49,13 @@ type event struct {
Timestamp string `json:"timestamp"` // Time when the audit event is logged via Log method
}

// logger implements the audit.Logger interface by logging to standard output.
type logger struct {
// Logger implements the audit.Logger interface by logging to standard output.
type Logger struct {
goLogger *log.Logger
}

// Log marshals the audit.Event to json and prints it to standard output.
func (l *logger) Log(event *audit.Event) {
func (l *Logger) Log(event *audit.Event) {
jsonContainer := map[string]interface{}{
"grpc_audit_log": convertEvent(event),
}
Expand All @@ -75,14 +78,14 @@ type loggerBuilder struct {
}

func (loggerBuilder) Name() string {
return "stdout_logger"
return Name
}

// Build returns a new instance of the stdout logger.
// Passed in configuration is ignored as the stdout logger does not
// expect any configuration to be provided.
func (lb *loggerBuilder) Build(audit.LoggerConfig) audit.Logger {
return &logger{
return &Logger{
goLogger: lb.goLogger,
}
}
Expand Down
2 changes: 1 addition & 1 deletion examples/go.mod
Expand Up @@ -17,7 +17,7 @@ require (
github.com/census-instrumentation/opencensus-proto v0.4.1 // indirect
github.com/cespare/xxhash/v2 v2.2.0 // indirect
github.com/cncf/udpa/go v0.0.0-20220112060539-c52dc94e7fbe // indirect
github.com/envoyproxy/go-control-plane v0.11.1-0.20230406144219-ba92d50b6596 // indirect
github.com/envoyproxy/go-control-plane v0.11.1-0.20230517004634-d1c5e72e4c41 // indirect
github.com/envoyproxy/protoc-gen-validate v0.10.1 // indirect
golang.org/x/net v0.9.0 // indirect
golang.org/x/sys v0.7.0 // indirect
Expand Down
4 changes: 2 additions & 2 deletions examples/go.sum
Expand Up @@ -636,8 +636,8 @@ github.com/docopt/docopt-go v0.0.0-20180111231733-ee0de3bc6815/go.mod h1:WwZ+bS3
github.com/dustin/go-humanize v1.0.0/go.mod h1:HtrtbFcZ19U5GC7JDqmcUSB87Iq5E25KnS6fMYU6eOk=
github.com/envoyproxy/go-control-plane v0.10.2-0.20220325020618-49ff273808a1/go.mod h1:KJwIaB5Mv44NWtYuAOFCVOjcI94vtpEz2JU/D2v6IjE=
github.com/envoyproxy/go-control-plane v0.10.3/go.mod h1:fJJn/j26vwOu972OllsvAgJJM//w9BV6Fxbg2LuVd34=
github.com/envoyproxy/go-control-plane v0.11.1-0.20230406144219-ba92d50b6596 h1:MDgbDqe1rWfGBa+yCcthuqDSHvXFyenZI1U7f1IbWI8=
github.com/envoyproxy/go-control-plane v0.11.1-0.20230406144219-ba92d50b6596/go.mod h1:84cjSkVxFD9Pi/gvI5AOq5NPhGsmS8oPsJLtCON6eK8=
github.com/envoyproxy/go-control-plane v0.11.1-0.20230517004634-d1c5e72e4c41 h1:TNyxMch3whemmD2xddvlcYav9UR0hUvFeWnMUMSdhHA=
github.com/envoyproxy/go-control-plane v0.11.1-0.20230517004634-d1c5e72e4c41/go.mod h1:84cjSkVxFD9Pi/gvI5AOq5NPhGsmS8oPsJLtCON6eK8=
github.com/envoyproxy/protoc-gen-validate v0.1.0/go.mod h1:iSmxcyjqTsJpI2R4NaDN7+kN2VEUnK/pcBlmesArF7c=
github.com/envoyproxy/protoc-gen-validate v0.6.7/go.mod h1:dyJXwwfPK2VSqiB9Klm1J6romD608Ba7Hij42vrOBCo=
github.com/envoyproxy/protoc-gen-validate v0.9.1/go.mod h1:OKNgG7TCp5pF4d6XftA0++PMirau2/yoOwVac3AbF2w=
Expand Down
2 changes: 1 addition & 1 deletion gcp/observability/go.sum
Expand Up @@ -647,7 +647,7 @@ github.com/docopt/docopt-go v0.0.0-20180111231733-ee0de3bc6815/go.mod h1:WwZ+bS3
github.com/dustin/go-humanize v1.0.0/go.mod h1:HtrtbFcZ19U5GC7JDqmcUSB87Iq5E25KnS6fMYU6eOk=
github.com/envoyproxy/go-control-plane v0.10.2-0.20220325020618-49ff273808a1/go.mod h1:KJwIaB5Mv44NWtYuAOFCVOjcI94vtpEz2JU/D2v6IjE=
github.com/envoyproxy/go-control-plane v0.10.3/go.mod h1:fJJn/j26vwOu972OllsvAgJJM//w9BV6Fxbg2LuVd34=
github.com/envoyproxy/go-control-plane v0.11.1-0.20230406144219-ba92d50b6596/go.mod h1:84cjSkVxFD9Pi/gvI5AOq5NPhGsmS8oPsJLtCON6eK8=
github.com/envoyproxy/go-control-plane v0.11.1-0.20230517004634-d1c5e72e4c41/go.mod h1:84cjSkVxFD9Pi/gvI5AOq5NPhGsmS8oPsJLtCON6eK8=
github.com/envoyproxy/protoc-gen-validate v0.1.0/go.mod h1:iSmxcyjqTsJpI2R4NaDN7+kN2VEUnK/pcBlmesArF7c=
github.com/envoyproxy/protoc-gen-validate v0.6.7/go.mod h1:dyJXwwfPK2VSqiB9Klm1J6romD608Ba7Hij42vrOBCo=
github.com/envoyproxy/protoc-gen-validate v0.9.1/go.mod h1:OKNgG7TCp5pF4d6XftA0++PMirau2/yoOwVac3AbF2w=
Expand Down
2 changes: 1 addition & 1 deletion go.mod
Expand Up @@ -6,7 +6,7 @@ require (
github.com/cespare/xxhash/v2 v2.2.0
github.com/cncf/udpa/go v0.0.0-20220112060539-c52dc94e7fbe
github.com/cncf/xds/go v0.0.0-20230310173818-32f1caf87195
github.com/envoyproxy/go-control-plane v0.11.1-0.20230406144219-ba92d50b6596
github.com/envoyproxy/go-control-plane v0.11.1-0.20230517004634-d1c5e72e4c41
github.com/golang/glog v1.1.0
github.com/golang/protobuf v1.5.3
github.com/google/go-cmp v0.5.9
Expand Down
4 changes: 2 additions & 2 deletions go.sum
Expand Up @@ -17,8 +17,8 @@ github.com/cncf/xds/go v0.0.0-20230310173818-32f1caf87195 h1:58f1tJ1ra+zFINPlwLW
github.com/cncf/xds/go v0.0.0-20230310173818-32f1caf87195/go.mod h1:eXthEFrGJvWHgFFCl3hGmgk+/aYT6PnTQLykKQRLhEs=
github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c=
github.com/envoyproxy/go-control-plane v0.9.0/go.mod h1:YTl/9mNaCwkRvm6d1a2C3ymFceY/DCBVvsKhRF0iEA4=
github.com/envoyproxy/go-control-plane v0.11.1-0.20230406144219-ba92d50b6596 h1:MDgbDqe1rWfGBa+yCcthuqDSHvXFyenZI1U7f1IbWI8=
github.com/envoyproxy/go-control-plane v0.11.1-0.20230406144219-ba92d50b6596/go.mod h1:84cjSkVxFD9Pi/gvI5AOq5NPhGsmS8oPsJLtCON6eK8=
github.com/envoyproxy/go-control-plane v0.11.1-0.20230517004634-d1c5e72e4c41 h1:TNyxMch3whemmD2xddvlcYav9UR0hUvFeWnMUMSdhHA=
github.com/envoyproxy/go-control-plane v0.11.1-0.20230517004634-d1c5e72e4c41/go.mod h1:84cjSkVxFD9Pi/gvI5AOq5NPhGsmS8oPsJLtCON6eK8=
github.com/envoyproxy/protoc-gen-validate v0.1.0/go.mod h1:iSmxcyjqTsJpI2R4NaDN7+kN2VEUnK/pcBlmesArF7c=
github.com/envoyproxy/protoc-gen-validate v0.10.1 h1:c0g45+xCJhdgFGw7a5QAfdS4byAbud7miNWJ1WwEVf8=
github.com/envoyproxy/protoc-gen-validate v0.10.1/go.mod h1:DRjgyB0I43LtJapqN6NiRwroiAU2PaFuvk/vjgh61ss=
Expand Down
21 changes: 19 additions & 2 deletions internal/xds/rbac/converter.go
Expand Up @@ -24,13 +24,19 @@ import (
v1xdsudpatypepb "github.com/cncf/xds/go/udpa/type/v1"
v3xdsxdstypepb "github.com/cncf/xds/go/xds/type/v3"
v3rbacpb "github.com/envoyproxy/go-control-plane/envoy/config/rbac/v3"
v3auditloggersstreampb "github.com/envoyproxy/go-control-plane/envoy/extensions/rbac/audit_loggers/stream/v3"
arvindbr8 marked this conversation as resolved.
Show resolved Hide resolved
"google.golang.org/grpc/authz/audit"
"google.golang.org/grpc/authz/audit/stdout"
"google.golang.org/protobuf/encoding/protojson"
"google.golang.org/protobuf/types/known/anypb"
"google.golang.org/protobuf/types/known/structpb"
)

const udpaTypedStuctType = "type.googleapis.com/udpa.type.v1.TypedStruct"
const xdsTypedStuctType = "type.googleapis.com/xds.type.v3.TypedStruct"
const (
udpaTypedStuctType = "type.googleapis.com/udpa.type.v1.TypedStruct"
xdsTypedStuctType = "type.googleapis.com/xds.type.v3.TypedStruct"
stdoutType = "type.googleapis.com/envoy.extensions.rbac.audit_loggers.stream.v3.StdoutAuditLog"
)

func buildLogger(loggerConfig *v3rbacpb.RBAC_AuditLoggingOptions_AuditLoggerConfig) (audit.Logger, error) {
if loggerConfig.GetAuditLogger().GetTypedConfig() == nil {
Expand Down Expand Up @@ -72,10 +78,21 @@ func getCustomConfig(config *anypb.Any) (json.RawMessage, string, error) {
return nil, "", fmt.Errorf("failed to unmarshal resource: %v", err)
}
return convertCustomConfig(typedStruct.TypeUrl, typedStruct.Value)
case stdoutType:
stdoutLoggerConfig := &v3auditloggersstreampb.StdoutAuditLog{}
if err := config.UnmarshalTo(stdoutLoggerConfig); err != nil {
return nil, "", fmt.Errorf("failed to unmarshal resource: %v", err)
}
return convertStdoutConfig(stdoutLoggerConfig)
}
return nil, "", fmt.Errorf("custom config not implemented for type [%v]", config.GetTypeUrl())
}

func convertStdoutConfig(config *v3auditloggersstreampb.StdoutAuditLog) (json.RawMessage, string, error) {
json, err := protojson.Marshal(config)
return json, stdout.Name, err
}

func convertCustomConfig(typeURL string, s *structpb.Struct) (json.RawMessage, string, error) {
// The gRPC policy name will be the "type name" part of the value of the
// type_url field in the TypedStruct. We get this by using the part after
Expand Down
57 changes: 57 additions & 0 deletions internal/xds/rbac/converter_test.go
Expand Up @@ -22,7 +22,9 @@ import (

v3corepb "github.com/envoyproxy/go-control-plane/envoy/config/core/v3"
v3rbacpb "github.com/envoyproxy/go-control-plane/envoy/config/rbac/v3"
v3auditloggersstreampb "github.com/envoyproxy/go-control-plane/envoy/extensions/rbac/audit_loggers/stream/v3"
"google.golang.org/grpc/authz/audit"
"google.golang.org/grpc/authz/audit/stdout"
"google.golang.org/protobuf/types/known/anypb"
)

Expand Down Expand Up @@ -110,5 +112,60 @@ func (s) TestBuildLoggerErrors(t *testing.T) {

})
}
}

func (s) TestBuildLoggerKnownTypes(t *testing.T) {
tests := []struct {
name string
loggerConfig *v3rbacpb.RBAC_AuditLoggingOptions_AuditLoggerConfig
}{
{
name: "stdout logger",
rockspore marked this conversation as resolved.
Show resolved Hide resolved
loggerConfig: &v3rbacpb.RBAC_AuditLoggingOptions_AuditLoggerConfig{
AuditLogger: &v3corepb.TypedExtensionConfig{
Name: stdout.Name,
TypedConfig: createStdoutPb(t),
},
IsOptional: false,
},
},
{
name: "stdout logger with generic TypedConfig",
loggerConfig: &v3rbacpb.RBAC_AuditLoggingOptions_AuditLoggerConfig{
AuditLogger: &v3corepb.TypedExtensionConfig{
Name: stdout.Name,
TypedConfig: createXDSTypedStruct(t, map[string]interface{}{}, stdout.Name),
},
IsOptional: false,
},
},
}
for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
logger, err := buildLogger(test.loggerConfig)
if err != nil {
t.Fatalf("expected success. got error: %v", err)
dfawley marked this conversation as resolved.
Show resolved Hide resolved
} else {
switch test.name {
case "stdout logger":
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 switching behavior on the testcase name is proper. The name is there for descriptive purposes only. Everything else should be covered by a parameter in the testcase struct.

In this case, if you want to test that the proper logger was built, you should be able to find the logger's type by looking it up in the registry, too, instead of exporting it.

Is this not something that should be tested for the other test case, too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it should be tested for the other test case, only keeping it out was a holdover from when I had it combined with the other test in this file.
Will clean it up

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not 100% clear what you mean by getting the correct logger type from the registry.

But that's also something we can kick down the road because there's only one logger type right now, so I can leave it hard-coded in as it is right now if that is okay with you

Copy link
Member

Choose a reason for hiding this comment

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

I'm not 100% clear what you mean by getting the correct logger type from the registry.

I mean you should be able to use audit.GetLoggerBuilder(<name>).Build() go get the type instead of needing to export the type. The types of the loggers shouldn't need to be exported, as you mentioned in your first PR comment.

Copy link
Contributor Author

@gtcooke94 gtcooke94 May 24, 2023

Choose a reason for hiding this comment

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

Ah okay, so you're suggesting to just build another logger with the known name and use reflect or something like that to pull out the type?
I'll make the change to pull that out so I can unexport the logger

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PTAL

_, ok := logger.(*stdout.Logger)
if !ok {
t.Fatalf("expected logger to be type stdout.Logger but was not")
}
}
}

})
}
}

// Builds stdout config for audit logger proto.
func createStdoutPb(t *testing.T) *anypb.Any {
t.Helper()
pb := &v3auditloggersstreampb.StdoutAuditLog{}
customConfig, err := anypb.New(pb)
if err != nil {
t.Fatalf("createStdoutPb failed during anypb.New: %v", err)
}
return customConfig
}
2 changes: 1 addition & 1 deletion interop/observability/go.sum
Expand Up @@ -648,7 +648,7 @@ github.com/docopt/docopt-go v0.0.0-20180111231733-ee0de3bc6815/go.mod h1:WwZ+bS3
github.com/dustin/go-humanize v1.0.0/go.mod h1:HtrtbFcZ19U5GC7JDqmcUSB87Iq5E25KnS6fMYU6eOk=
github.com/envoyproxy/go-control-plane v0.10.2-0.20220325020618-49ff273808a1/go.mod h1:KJwIaB5Mv44NWtYuAOFCVOjcI94vtpEz2JU/D2v6IjE=
github.com/envoyproxy/go-control-plane v0.10.3/go.mod h1:fJJn/j26vwOu972OllsvAgJJM//w9BV6Fxbg2LuVd34=
github.com/envoyproxy/go-control-plane v0.11.1-0.20230406144219-ba92d50b6596/go.mod h1:84cjSkVxFD9Pi/gvI5AOq5NPhGsmS8oPsJLtCON6eK8=
github.com/envoyproxy/go-control-plane v0.11.1-0.20230517004634-d1c5e72e4c41/go.mod h1:84cjSkVxFD9Pi/gvI5AOq5NPhGsmS8oPsJLtCON6eK8=
github.com/envoyproxy/protoc-gen-validate v0.1.0/go.mod h1:iSmxcyjqTsJpI2R4NaDN7+kN2VEUnK/pcBlmesArF7c=
github.com/envoyproxy/protoc-gen-validate v0.6.7/go.mod h1:dyJXwwfPK2VSqiB9Klm1J6romD608Ba7Hij42vrOBCo=
github.com/envoyproxy/protoc-gen-validate v0.9.1/go.mod h1:OKNgG7TCp5pF4d6XftA0++PMirau2/yoOwVac3AbF2w=
Expand Down
2 changes: 1 addition & 1 deletion stats/opencensus/go.sum
Expand Up @@ -630,7 +630,7 @@ github.com/docopt/docopt-go v0.0.0-20180111231733-ee0de3bc6815/go.mod h1:WwZ+bS3
github.com/dustin/go-humanize v1.0.0/go.mod h1:HtrtbFcZ19U5GC7JDqmcUSB87Iq5E25KnS6fMYU6eOk=
github.com/envoyproxy/go-control-plane v0.10.2-0.20220325020618-49ff273808a1/go.mod h1:KJwIaB5Mv44NWtYuAOFCVOjcI94vtpEz2JU/D2v6IjE=
github.com/envoyproxy/go-control-plane v0.10.3/go.mod h1:fJJn/j26vwOu972OllsvAgJJM//w9BV6Fxbg2LuVd34=
github.com/envoyproxy/go-control-plane v0.11.1-0.20230406144219-ba92d50b6596/go.mod h1:84cjSkVxFD9Pi/gvI5AOq5NPhGsmS8oPsJLtCON6eK8=
github.com/envoyproxy/go-control-plane v0.11.1-0.20230517004634-d1c5e72e4c41/go.mod h1:84cjSkVxFD9Pi/gvI5AOq5NPhGsmS8oPsJLtCON6eK8=
github.com/envoyproxy/protoc-gen-validate v0.1.0/go.mod h1:iSmxcyjqTsJpI2R4NaDN7+kN2VEUnK/pcBlmesArF7c=
github.com/envoyproxy/protoc-gen-validate v0.6.7/go.mod h1:dyJXwwfPK2VSqiB9Klm1J6romD608Ba7Hij42vrOBCo=
github.com/envoyproxy/protoc-gen-validate v0.9.1/go.mod h1:OKNgG7TCp5pF4d6XftA0++PMirau2/yoOwVac3AbF2w=
Expand Down