xds: support built-in Stdout audit logger type #6298
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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,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": | ||
|
There was a problem hiding this comment. 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? There was a problem hiding this comment. 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. There was a problem hiding this comment. 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 There was a problem hiding this comment.
I mean you should be able to use There was a problem hiding this comment. 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? |
||
| _, 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 | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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.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= | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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.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= | ||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
_loggersuffix? 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