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
Changes from 15 commits
4d8e4b7
f3b679c
58b9e48
599cb13
e5de42f
3f6f35a
699a42f
218be79
8d3c9c4
f349cdc
ed23d8f
0861548
eb31885
32363cd
5997a19
ae08173
c283e73
2c689b1
58a309d
f719de2
4cda2b3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 ( | ||
udpaTypedStructType = "type.googleapis.com/udpa.type.v1.TypedStruct" | ||
xdsTypedStructType = "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 { | ||
|
@@ -60,22 +66,33 @@ func buildLogger(loggerConfig *v3rbacpb.RBAC_AuditLoggingOptions_AuditLoggerConf | |
|
||
func getCustomConfig(config *anypb.Any) (json.RawMessage, string, error) { | ||
switch config.GetTypeUrl() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would something like this work instead and be a bit simpler? m, err := config.UnmarshalNew()
if err != nil { return json.RawMessage(""), "", err }
switch m.(type) {
case *v1xdsudpatypepb.TypedStruct:
return convertCustomConfig(m.TypeUrl, m.Value)
case *v3xdsxdstypepb.TypedStruct:
return convertCustomConfig(m.TypeUrl, m.Value)
case *v3auditloggerstreampb.StdoutAuditLog:
return convertStdoutConfig(m)
} (This also means you don't need strings for the type names.) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like that a lot more, changing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. PTAL |
||
case udpaTypedStuctType: | ||
case udpaTypedStructType: | ||
typedStruct := &v1xdsudpatypepb.TypedStruct{} | ||
if err := config.UnmarshalTo(typedStruct); err != nil { | ||
return nil, "", fmt.Errorf("failed to unmarshal resource: %v", err) | ||
} | ||
return convertCustomConfig(typedStruct.TypeUrl, typedStruct.Value) | ||
case xdsTypedStuctType: | ||
case xdsTypedStructType: | ||
typedStruct := &v3xdsxdstypepb.TypedStruct{} | ||
if err := config.UnmarshalTo(typedStruct); err != nil { | ||
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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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" | ||
) | ||
|
||
|
@@ -110,5 +112,56 @@ 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 { | ||
_, ok := logger.(*stdout.Logger) | ||
if !ok { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These can be merged into 1: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changed the structure of the test so this is no longer there at all |
||
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 | ||
} |
There was a problem hiding this comment.
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?There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 @rocksporeThere was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 -
grpc-go/authz/audit/stdout/stdout_logger.go
Lines 34 to 38 in a6e1acf
@rockspore please check me here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
Yes, it can come through here through
NewStatic