Mask rule

[dkiser]

Fixes #2379

This is an initial attempt at enhancing decision log masks by providing a new structured format for mask rules that allow either erasing or modifying input objects. Backwards compatibility is maintained with the original mask rule format.

[dkiser]

This implementation does indeed require the path to exist in the upsert opcode. However, if we want to be able to upsert a non-existing field in the input, we can modify this to support that easily.

[patrick-east]

This looks like a really nice improvement!

[tsandall]

@dkiser thanks for adding the upsert support! I tried it out this morning and it works nicely. We should adjust some of the error handling though IMO.

1. The decision logger should validate as much of the mask instructions as possible in one shot and generate an error if any validation issues are found. Let's make the op validation happen inside newMaskRule. This way if the op is missing or incorrect and OPA cannot tell what to do with it, it will be caught.

2. The decision logger should treat undefined paths consistently for remove and upsert operations. Currently if the upsert specifies an undefined path, e.g., a reference into an array, the decision logger reports an error. I think we should suppress these.

Re: (2), I could imagine some users wanting OPA to fail if the path is undefined. I think we can add support for this later if necesarry, either via config or by extending the instructions.

[tsandall]

@dkiser let us know if you can work on those remaining comments around error handling in the next week or so. If not, we can take on those changes so that this lands in the next release.

[dkiser]

@tsandall I'll find some time over the next couple of days and see about addressing those last two comments. Thanks!

[dkiser]

@dkiser thanks for adding the upsert support! I tried it out this morning and it works nicely. We should adjust some of the error handling though IMO.

1. The decision logger should validate as much of the mask instructions as possible in one shot and generate an error if any validation issues are found. Let's make the op validation happen inside newMaskRule. This way if the op is missing or incorrect and OPA cannot tell what to do with it, it will be caught.
2. The decision logger should treat undefined paths consistently for remove and upsert operations. Currently if the upsert specifies an undefined path, e.g., a reference into an array, the decision logger reports an error. I think we should suppress these.

Re: (2), I could imagine some users wanting OPA to fail if the path is undefined. I think we can add support for this later if necesarry, either via config or by extending the instructions.

@tsandall Re: (2), Do we mean that an upsert to an undefined path for an undefined object type to fail silently? I think we want to keep the ability to upsert to an undefined path for a defined upsert object type, which right now it just a map[string]interface{}?

Do you have an existing unit test case to point to that behavior should be changed on?

[tsandall]

@dkiser you're right. The term undefined is not accurate here. I'm referring to this line: https://github.com/open-policy-agent/opa/pull/2433/files#diff-dee023b2dcf1c8c236bac28d4d4fd7b8R189.

In the upsert implementation, if the path refers to a non-object intermediate value, it will result in an error being logged. OTOH, in the remove implementation, if the path refers to a non-object intermediate value, it's treated as undefined and ignored. The behaviour should be consistent. I suggest we remove that error and simply treat the upsert as undefined in this case.

[dkiser]

@tsandall Can you take a gander at the last commit. I agree with the configurable option for undefined object path handling (silent or some kind of decision log error) and added low level error handling for this in the mask rule that can be used in a later PR at the plugin.go level (left a TODO comment in there).

Let me know if this new commit addresses your concerns. Thanks!

[tsandall]

@dkiser this looks good! Can you please squash your commits and format the message according to these guidelines. Once that's done we can merge this.

[dkiser]

@tsandall Done. Thanks for all the help thus far.

[tsandall]

LGTM