xds: support built-in Stdout audit logger type by gtcooke94 · Pull Request #6298 · grpc/grpc-go

gtcooke94 · 2023-05-18T15:49:34Z

This PR adds the functionality to parse and build the known StdoutLogger that we include as an implemented AuditLogger.

I added a Name const to identify this logger, matching the LoadBalancer design. I believe this is needed. I also exported stdout.Logger so that we could check the type in the test, though I'm thinking there might be a better way to do this. I'm open to suggestions on a better way to test.

RELEASE NOTES:

xds: add the functionality to parse and build the known StdoutLogger through the XDS path

rockspore · 2023-05-18T20:01:37Z

 }

+func convertStdoutConfig(config *v3auditloggersstreampb.StdoutAuditLog) (json.RawMessage, string, error) {
+	json, err := json.Marshal(config)


Instead of calling this, you could simply return {}.

Discussed offline, we don't want to simply ignore the input config and return {}, though it would be fine with the current impl

rockspore

LGTM

arvindbr8

Easwar is out of office for now. I've taken a pass at it.

arvindbr8 · 2023-05-23T16:30:53Z

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


Suggested change

const udpaTypedStuctType = "type.googleapis.com/udpa.type.v1.TypedStruct"

const xdsTypedStuctType = "type.googleapis.com/xds.type.v3.TypedStruct"

const stdoutType = "type.googleapis.com/envoy.extensions.rbac.audit_loggers.stream.v3.StdoutAuditLog"

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"

)

pls. While you are at there, might be helpful to fix typos

I'm not immediately seeing any typos, where are they?

udpaTypedStuctType -> udpaTypedStructType

Ah thank you! Fixed

dfawley · 2023-05-23T20:52:07Z

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

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


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

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.

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

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?

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

func init() {

audit.RegisterLoggerBuilder(&loggerBuilder{

goLogger: log.New(os.Stdout, "", 0),

})

}

@rockspore please check me here

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.

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.

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.

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?

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

dfawley · 2023-05-23T20:52:56Z

+				t.Fatalf("expected success. got error: %v", err)
+			} else {
+				switch test.name {
+				case "stdout logger":


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?

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

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

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.

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

dfawley · 2023-05-24T15:44:09Z

@@ -60,22 +66,33 @@ func buildLogger(loggerConfig *v3rbacpb.RBAC_AuditLoggingOptions_AuditLoggerConf

 func getCustomConfig(config *anypb.Any) (json.RawMessage, string, error) {
 	switch config.GetTypeUrl() {


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.)

I like that a lot more, changing

dfawley · 2023-05-24T15:45:49Z

+				t.Fatalf("expected success. got error: %v", err)
+			} else {
+				switch test.name {
+				case "stdout logger":


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.

dfawley · 2023-05-24T16:27:49Z

+				_, ok := logger.(*stdout.Logger)
+				if !ok {


These can be merged into 1: if _, ok := logger.(*stdout.Logger); !ok {

Changed the structure of the test so this is no longer there at all

dfawley · 2023-05-24T22:21:58Z

 }
+
+// Builds a config with a nonsensical type in the anypb.Any.
+func createUnsupportedPb(t *testing.T) *anypb.Any {


I think you might be able to re-use

grpc-go/internal/testutils/marshal_any.go

Line 30 in 59134c3

func MarshalAny(m proto.Message) *anypb.Any {

instead to simplify a little?

dfawley · 2023-05-25T15:03:16Z

-	}
-	return customConfig
-
+	return testutils.MarshalAny(pb)


I think you missed the point. Remove createUnsupportedPb and replace it with testutils.MarshalAny(&v3rbacpb.RBAC_AuditLoggingOptions{}). But this is fine too if you want, just remove the t parameter to this function since it's effectively unused.

Ah I get it, changed

gtcooke94 added 3 commits May 18, 2023 15:40

Add test for building stdout logger

4d8e4b7

Cleanup

f3b679c

%s/steam/stream/

58b9e48

gtcooke94 requested a review from rockspore May 18, 2023 15:50

rockspore reviewed May 18, 2023

View reviewed changes

Comment thread internal/xds/rbac/converter_test.go

gtcooke94 added 7 commits May 18, 2023 17:36

go mod tidy compat=1.17

599cb13

Add test with same name but generic config

e5de42f

mod tidy

3f6f35a

vet

699a42f

mod tidy

218be79

mod tidy

8d3c9c4

mod tidy

f349cdc

rockspore reviewed May 18, 2023

View reviewed changes

Try different marshaler

ed23d8f

gtcooke94 added the Type: Security A bug or other problem affecting security label May 18, 2023

gtcooke94 requested a review from rockspore May 19, 2023 14:17

rockspore approved these changes May 19, 2023

View reviewed changes

gtcooke94 requested review from dfawley and easwars May 19, 2023 15:51

arvindbr8 suggested changes May 23, 2023

View reviewed changes

arvindbr8 assigned gtcooke94 May 23, 2023

arvindbr8 added the Status: Requires Reporter Clarification label May 23, 2023

gtcooke94 added 2 commits May 23, 2023 17:14

Address comment

0861548

Address comment

eb31885

gtcooke94 assigned arvindbr8 May 23, 2023

gtcooke94 requested a review from arvindbr8 May 23, 2023 20:07

arvindbr8 removed the Status: Requires Reporter Clarification label May 23, 2023

arvindbr8 assigned dfawley and unassigned arvindbr8 May 23, 2023

arvindbr8 approved these changes May 23, 2023

View reviewed changes

dfawley reviewed May 23, 2023

View reviewed changes

gtcooke94 added 2 commits May 23, 2023 21:04

Cleanup test case

32363cd

typo Stuct -> Struct

5997a19

gtcooke94 requested a review from dfawley May 24, 2023 14:40

dfawley reviewed May 24, 2023

View reviewed changes

gtcooke94 added 3 commits May 24, 2023 17:04

Address PR comments - cleanup getCustomConfig and testing types

ae08173

unexport stdout logger

c283e73

Test cleanup

2c689b1

gtcooke94 requested a review from dfawley May 24, 2023 17:07

typo

58a309d

dfawley reviewed May 24, 2023

View reviewed changes

dfawley approved these changes May 24, 2023

View reviewed changes

dfawley changed the title ~~[authz] Stdout Logger built in parsing~~ xds: support built-in Stdout audit logger type May 24, 2023

dfawley removed their assignment May 24, 2023

simplify helper fn

f719de2

gtcooke94 added this to the 1.56 Release milestone May 25, 2023

dfawley reviewed May 25, 2023

View reviewed changes

use MarshalAny testutil

4cda2b3

gtcooke94 merged commit f19266c into grpc:master May 25, 2023

gtcooke94 deleted the XdsRbacAuditLogging branch May 25, 2023 17:25

github-actions Bot locked as resolved and limited conversation to collaborators Nov 22, 2023

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

		@@ -60,22 +66,33 @@ func buildLogger(loggerConfig *v3rbacpb.RBAC_AuditLoggingOptions_AuditLoggerConf

		func getCustomConfig(config *anypb.Any) (json.RawMessage, string, error) {
		switch config.GetTypeUrl() {

Conversation

gtcooke94 commented May 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rockspore left a comment

Choose a reason for hiding this comment

Uh oh!

arvindbr8 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rockspore May 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gtcooke94 May 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gtcooke94 commented May 18, 2023 •

edited

Loading

rockspore May 24, 2023 •

edited

Loading

gtcooke94 May 24, 2023 •

edited

Loading