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.

Sign up for GitHub

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

Jump to bottom

Relax config check when Discovery is enabled #5722

Closed
tsandall opened this issue Mar 3, 2023 · 5 comments · Fixed by #6702
Closed

Relax config check when Discovery is enabled #5722

tsandall opened this issue Mar 3, 2023 · 5 comments · Fixed by #6702
Labels
feature-request runtime usability

Comments

@tsandall
Copy link
Member

tsandall commented Mar 3, 2023 •
edited
Loading

Currently OPA does not allow users to supply plugin config when Discovery is enabled. The reason for this was to prevent OPAs being deployed that could not be controlled through discovery. In some cases though we've found that the user deploying OPA must be able to make local overrides even when discovery is in place, e.g., because the system serving the discovery config is unaware of all options available in OPA.

To address this we can relax the configuration check when discovery is enabled so that the bootstrap configuration can contain plugin configs. In case of conflicts, the bootstrap configuration for plugins should win. These local configuration overrides from the bootstrap configuration could be included in the Status API messages so that management systems can at least visibility into the local overrides.

For example, to enable console decision logging when an opa-config.yaml relies on discovery, the user would just need to:

opa run -c opa-config.yaml --set decision_logs.console=true
@ashutosh-narkar ashutosh-narkar self-assigned this Mar 4, 2023
@anderseknert
Copy link
Member

Related: #5070

(not a duplicate, but if I'm reading things right, fixing this would likely fix that too, so adding here for tracking)

@mjungsbluth
Copy link
Contributor

PR #5314 already did some work on this and left it to the plugins what can be overridden (my assumption was that some things should just not be overridable but that might be wrong). It uses the same merge values method that is used by the command line‘s —set args if I remember correctly

@ashutosh-narkar ashutosh-narkar added usability distribution Issues related to the bundle plugin labels Mar 7, 2023
@ashutosh-narkar ashutosh-narkar mentioned this issue Mar 8, 2023
Closed
@ashutosh-narkar ashutosh-narkar added runtime and removed distribution Issues related to the bundle plugin labels Mar 8, 2023
@johanfylling johanfylling removed their assignment Mar 23, 2023
@mjungsbluth
Copy link
Contributor

Since this is removed from planning and we are hit by this: Is there agreement on doing it as described?

  • Overriding of any configuration is possible with local bootstrap config (or transitively —set)
  • The overrides are added as a delta in the status API

The linked PR does no. 1 but is more specific, i.e some conditions would need to be removed.

What is missing is essentially the delta tracking and publishing this via status API

@tsandall
Copy link
Member Author

@mjungsbluth we've mostly seen this request come from Styra DAS users, however, since Styra DAS now supports the ability to override the discovery config (via the API) we decided to deprioritize the work in OPA. Here is a link to the Styra DAS docs that show how to override the discovery config: https://docs.styra.com/das/policies/policy-organization/systems/opa-discovery#add-or-override-a-value-in-discovery-configuration.

@mjungsbluth
Copy link
Contributor

@tsandall understood and I have seen that change which mitigates the problem for now.
There is however an aspect of split accountability in this as well as outlined in #5070 where the team that owns the config (SRE or some cloud infrastructure department) can be very different / far away from an IAM engineering department that owns Control Plane + central policies.

ashutosh-narkar added a commit to ashutosh-narkar/opa that referenced this issue Apr 18, 2024
@ashutosh-narkar
Relax configuration check when Discovery is enabled
200cc52 
Previously if Discovery was enabled, other features like bundle downloading and status reporting could not be configured manually.
The reason for this was to prevent OPAs being deployed that could not be controlled through discovery. It's possible that
the system serving the discovered config is unaware of all options locally available in OPA. Hence, we relax the configuration
check when discovery is enabled so that the bootstrap configuration can contain plugin configurations. In case of conflicts,
the bootstrap configuration for plugins wins. These local configuration overrides from the bootstrap configuration are included
in the Status API messages so that management systems can get visibility into the local overrides.

**In general, the bootstrap configuration overrides the discovered configuration.** Previously this was not the case for all
configuration fields. For example, if the discovered configuration changes the `labels` section, only labels that are
additional compared to the bootstrap configuration are used, all other changes are ignored. This implies labels in the
bootstrap configuration override those in the discovered configuration. But for fields such as `default_decision`, `default_authorization_decision`,
`nd_builtin_cache`, the discovered configuration would override the bootstrap configuration. Now the behavior is more consistent
for the entire configuration and helps to avoid accidental configuration errors.

Fixes: open-policy-agent#5722

Signed-off-by: Ashutosh Narkar <anarkar4387@gmail.com>
@ashutosh-narkar ashutosh-narkar mentioned this issue Apr 18, 2024
Merged
ashutosh-narkar added a commit to ashutosh-narkar/opa that referenced this issue Apr 22, 2024
@ashutosh-narkar
Relax configuration check when Discovery is enabled
58ff60e 
Previously if Discovery was enabled, other features like bundle downloading and status reporting could not be configured manually.
The reason for this was to prevent OPAs being deployed that could not be controlled through discovery. It's possible that
the system serving the discovered config is unaware of all options locally available in OPA. Hence, we relax the configuration
check when discovery is enabled so that the bootstrap configuration can contain plugin configurations. In case of conflicts,
the bootstrap configuration for plugins wins. These local configuration overrides from the bootstrap configuration are included
in the Status API messages so that management systems can get visibility into the local overrides.

**In general, the bootstrap configuration overrides the discovered configuration.** Previously this was not the case for all
configuration fields. For example, if the discovered configuration changes the `labels` section, only labels that are
additional compared to the bootstrap configuration are used, all other changes are ignored. This implies labels in the
bootstrap configuration override those in the discovered configuration. But for fields such as `default_decision`, `default_authorization_decision`,
`nd_builtin_cache`, the discovered configuration would override the bootstrap configuration. Now the behavior is more consistent
for the entire configuration and helps to avoid accidental configuration errors.

Fixes: open-policy-agent#5722

Signed-off-by: Ashutosh Narkar <anarkar4387@gmail.com>
ashutosh-narkar added a commit to ashutosh-narkar/opa that referenced this issue Apr 23, 2024
@ashutosh-narkar
Relax configuration check when Discovery is enabled
c5cd23e 
Previously if Discovery was enabled, other features like bundle downloading and status reporting could not be configured manually.
The reason for this was to prevent OPAs being deployed that could not be controlled through discovery. It's possible that
the system serving the discovered config is unaware of all options locally available in OPA. Hence, we relax the configuration
check when discovery is enabled so that the bootstrap configuration can contain plugin configurations. In case of conflicts,
the bootstrap configuration for plugins wins. These local configuration overrides from the bootstrap configuration are included
in the Status API messages so that management systems can get visibility into the local overrides.

**In general, the bootstrap configuration overrides the discovered configuration.** Previously this was not the case for all
configuration fields. For example, if the discovered configuration changes the `labels` section, only labels that are
additional compared to the bootstrap configuration are used, all other changes are ignored. This implies labels in the
bootstrap configuration override those in the discovered configuration. But for fields such as `default_decision`, `default_authorization_decision`,
`nd_builtin_cache`, the discovered configuration would override the bootstrap configuration. Now the behavior is more consistent
for the entire configuration and helps to avoid accidental configuration errors.

Fixes: open-policy-agent#5722

Signed-off-by: Ashutosh Narkar <anarkar4387@gmail.com>
ashutosh-narkar added a commit that referenced this issue Apr 23, 2024
@ashutosh-narkar
Relax configuration check when Discovery is enabled
44fa8ad 
Previously if Discovery was enabled, other features like bundle downloading and status reporting could not be configured manually.
The reason for this was to prevent OPAs being deployed that could not be controlled through discovery. It's possible that
the system serving the discovered config is unaware of all options locally available in OPA. Hence, we relax the configuration
check when discovery is enabled so that the bootstrap configuration can contain plugin configurations. In case of conflicts,
the bootstrap configuration for plugins wins. These local configuration overrides from the bootstrap configuration are included
in the Status API messages so that management systems can get visibility into the local overrides.

**In general, the bootstrap configuration overrides the discovered configuration.** Previously this was not the case for all
configuration fields. For example, if the discovered configuration changes the `labels` section, only labels that are
additional compared to the bootstrap configuration are used, all other changes are ignored. This implies labels in the
bootstrap configuration override those in the discovered configuration. But for fields such as `default_decision`, `default_authorization_decision`,
`nd_builtin_cache`, the discovered configuration would override the bootstrap configuration. Now the behavior is more consistent
for the entire configuration and helps to avoid accidental configuration errors.

Fixes: #5722

Signed-off-by: Ashutosh Narkar <anarkar4387@gmail.com>
@BrewTestBot BrewTestBot mentioned this issue Apr 25, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
feature-request runtime usability
Projects
Open Policy Agent
Status: Done
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Relax configuration check when Discovery is enabled ashutosh-narkar/opa
5 participants
@tsandall @anderseknert @mjungsbluth @ashutosh-narkar @johanfylling