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

Conversation

gtcooke94
Copy link
Contributor

@gtcooke94 gtcooke94 commented May 18, 2023

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

@gtcooke94 gtcooke94 requested a review from rockspore May 18, 2023 15:50
}
return nil, "", fmt.Errorf("custom config not implemented for type [%v]", config.GetTypeUrl())
}

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@rockspore rockspore left a comment

Choose a reason for hiding this comment

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

LGTM

@gtcooke94 gtcooke94 requested review from easwars and dfawley May 19, 2023 15:51
Copy link
Member

@arvindbr8 arvindbr8 left a comment

Choose a reason for hiding this comment

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

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

Comment on lines 35 to 37
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"
Copy link
Member

Choose a reason for hiding this comment

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

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

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 immediately seeing any typos, where are they?

Copy link
Member

Choose a reason for hiding this comment

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

udpaTypedStuctType -> udpaTypedStructType

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah thank you! Fixed

internal/xds/rbac/converter.go Show resolved Hide resolved
internal/xds/rbac/converter.go Outdated Show resolved Hide resolved
@@ -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
Contributor

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
Contributor

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
Contributor

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
Contributor

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

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

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
Contributor

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

@gtcooke94 gtcooke94 requested a review from dfawley May 24, 2023 14:40
@@ -60,22 +66,33 @@ func buildLogger(loggerConfig *v3rbacpb.RBAC_AuditLoggingOptions_AuditLoggerConf

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

Choose a reason for hiding this comment

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

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 like that a lot more, changing

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

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

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.

internal/xds/rbac/converter_test.go Show resolved Hide resolved
Comment on lines 149 to 150
_, ok := logger.(*stdout.Logger)
if !ok {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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

@gtcooke94 gtcooke94 requested a review from dfawley May 24, 2023 17:07
@@ -169,3 +167,18 @@ func createStdoutPb(t *testing.T) *anypb.Any {
}
return customConfig
}

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

Choose a reason for hiding this comment

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

I think you might be able to re-use

func MarshalAny(m proto.Message) *anypb.Any {
instead to simplify a little?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed

@dfawley dfawley changed the title [authz] Stdout Logger built in parsing xds: support built-in Stdout audit logger type May 24, 2023
@dfawley dfawley removed their assignment May 24, 2023
@gtcooke94 gtcooke94 added this to the 1.56 Release milestone May 25, 2023
}
return customConfig

return testutils.MarshalAny(pb)
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I get it, changed

@gtcooke94 gtcooke94 merged commit f19266c into grpc:master May 25, 2023
11 checks passed
@gtcooke94 gtcooke94 deleted the XdsRbacAuditLogging branch May 25, 2023 17:25
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants