S3 environment_credentials option confusing

[anderseknert]

As documented the option

services[_].credentials.s3_signing.environment_credentials

expects a value of type "{}" without any further explanation of what it's meant to contain. From looking at the code it seems this option is used as a boolean, where the presence of an object activates the environment credentials option, so making the option expect a boolean would make sense IMO. Another problem with the current option is that it only works when provided in a config file - as a command line option this breaks:

 $ opa run -s '--set=services.authz.credentials.s3_signing.environment_credentials={}'
error: config error: json: cannot unmarshal array into Go struct field awsSigningAuthPlugin.credentials.s3_signing.environment_credentials of type rest.awsEnvironmentCredentialService

Expected Behavior

services[_].credentials.s3_signing.environment_credentials is enabled if true is provided

Actual Behavior

services[_].credentials.s3_signing.environment_credentials is enabled if {} is provided

[patrick-east]

Agreed that it is confusing, I have seen a few example configs where people have put environment variables and stuff into it. If we can change the docs to more explicitly say it needs to literally be {} and not just a like object type or whatever, that might help quite a bit.

Another problem with the current option is that it only works when provided in a config file - as a command line option this breaks:

You'll have to use opa run -s '--set=services.authz.credentials.s3_signing.environment_credentials=null' as noted in https://www.openpolicyagent.org/docs/latest/configuration/#empty-objects

[anderseknert]

Thanks for the pointer on using =null 👍 Unfortunately doesn't change much as for the ergonomics of using {} or null to mean "yes, please" 😄 I did a little POC on fixing this yesterday, and by customizing the JSON marshaller we could change this to be a boolean option while preserving the {} option for backwards compatibility. WDYT?

If we keep it as it is, I agree that we should at least update the docs.

[tsandall]

What if the parser for --set options was changed to support default values? E.g., if the value is omitted, it would default to {}. I think this would be preferable to extending each config object w/ custom marshaling logic.

cc @patrick-east

[anderseknert]

Each config object? This is the only one I'm aware of where an empty object masquerades as a boolean true. To me it makes sense that if an option is boolean in nature, it should also be boolean in config. The current {} value seems like an internal implementation detail of Go (empty object mapping to empty struct) leaking out to end users.

Custom marshaling isn't strictly necessary either - that was only an attempt (poor one perhaps) to have the config option changed to be a boolean without breaking existing configurations.

[anderseknert]

Having default values for --set is a good one regardless of this specific config option though 👍

[tsandall]

We run into this with custom plugins and possibly elsewhere. @patrick-east added a section in the docs because it was common enough. I think that making the default {} would cover the most common errors. You would still get an error if you tried to --set a scalar field w/o providing a value, but that seems reasonable to me. Perhaps the error handling could be improved to mention that a value is required in that case.

[Sulbigar]

@tsandall @anderseknert Are we sure setting it to null works?
I tried and I get the below error.
{"level":"error","msg":"Bundle load failed: request failed: a AWS credential service must be specified when S3 signing is enabled","name":"authz","plugin":"bundle","time":"2021-09-30T13:41:44-04:00"}
The command I ran is
opa run -s -a 0.0.0.0:8191 --set "services.local.url=https://sulbi_bucket.s3.amazonaws.com" --set "services.local.credentials.s3_signing.environment_credentials=null" --set "bundles.authz.service=local" --set "bundles.authz.resource=sulbi/roots/bundle_1.tar.gz" --set "bundles.authz.polling.min_delay_seconds=5" --set "bundles.authz.polling.max_delay_seconds=10" -l debug --set "decision_logs.console=true"

The version of OPA is 0.32.0

[anderseknert]

I am sure that it used to work, yes. However, trying it now with the example you provided I get the same error. Moving the same options into a config file works, so this is either a regression, or the option truly is confusing :) I'll file a new bug ticket. Thanks for reporting!

[stale]

This issue has been automatically marked as inactive because it has not had any activity in the last 30 days.

[stale]

This issue has been automatically marked as inactive because it has not had any activity in the last 30 days.

[anderseknert]

I don't think there's much to be done here, so will close this. If others find this confusing they'll hopefully be able to find this issue.