Skip to content

Enforcing allowed features - config #7939

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

Draft
wants to merge 3 commits into
base: dmallare/enforcing-allowed-features-plugins
Choose a base branch
from

Conversation

DMallare
Copy link
Contributor

@DMallare DMallare commented Jul 20, 2025


Complete the checklist (and note appropriate exceptions) before the PR is marked ready-for-review.

  • PR description explains the motivation for the change and relevant context for reviewing
  • PR description links appropriate GitHub/Jira tickets (creating when necessary)
  • Changeset is included for user-facing changes
  • Changes are compatible1
  • Documentation2 completed
  • Performance impact assessed and acceptable
  • Metrics and logs are added3 and documented
  • Tests added and passing4
    • Unit tests
    • Integration tests
    • Manual tests, as necessary

Exceptions

Note any exceptions here

Notes

Footnotes

  1. It may be appropriate to bring upcoming changes to the attention of other (impacted) groups. Please endeavour to do this before seeking PR approval. The mechanism for doing this will vary considerably, so use your judgement as to how and when to do this.

  2. Configuration is an important part of many changes. Where applicable please try to document configuration examples.

  3. A lot of (if not most) features benefit from built-in observability and debug-level logs. Please read this guidance on metrics best-practices.

  4. Tick whichever testing boxes are applicable. If you are adding Manual Tests, please document the manual testing (extensively) in the Exceptions.

@apollo-librarian
Copy link

apollo-librarian bot commented Jul 20, 2025

⚠️ Docs preview not attached to branch

The preview was not built because the PR's base branch dmallare/enforcing-allowed-features-plugins is not in the list of sources.

An Apollo team member can comment one of the following commands to dictate which branch to attach the preview to:

  • !docs set-base-branch 1.x
  • !docs set-base-branch dev

Build ID: 27ab07a639952ed069c2a34e

Copy link
Contributor

@DMallare, please consider creating a changeset entry in /.changesets/. These instructions describe the process and tooling.

@DMallare DMallare mentioned this pull request Jul 20, 2025
10 tasks
@DMallare DMallare changed the base branch from dev to dmallare/enforcing-allowed-features-plugins July 20, 2025 21:16
@DMallare DMallare requested a review from aaronArinder July 21, 2025 15:20
@DMallare DMallare force-pushed the dmallare/enforcing-allowed-features-plugins branch from 1a7bdb9 to 8bb665b Compare July 21, 2025 18:30
@DMallare DMallare force-pushed the enforcing-allowed-features-config branch from 121b1b4 to 329f165 Compare July 21, 2025 18:54
@lrlna lrlna self-requested a review July 22, 2025 11:29
Copy link
Member

@lrlna lrlna left a comment

Choose a reason for hiding this comment

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

I think we need to figure out the data structure / state for allowed_features. It's state change is not super clear. I left a comment on it in the license_enforcement.rs file. That's probably the only thing to nail down across these two PRs. And again, this will need a bunch of tests, especially integration tests. Otherwise, the direction seems good to me.

Comment on lines +431 to +432
// If the license has no allowed_features claim, we're using a pricing plan
// that should have the plugin enabled regardless
Copy link
Member

Choose a reason for hiding this comment

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

A more philosophical question - if the pricing plan should have all plugins enabled, shouldn't their allowed features just have all the features? It's a bit of strange state as it reads for me right now:

  • None - all features are allowed and all plugins should be enabled
  • Some(vec![]) - (empty vec) no features are allowed (?)
  • Some(vec![AllowedFeature::PersistedQueries]) - only persisted queries are allowed

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, your understanding is correct and I see why it looks strange. The reasoning behind what we have currently has to do with the fact that we are only adding the allowed_features claim to developer/standard licenses for now. In the future, I see us adding this to all types of licenses and then an Enterprise license, for example, would have all allowed features in their claim or maybe even something like ENABLE_ALL - we can discuss once a design for that comes up.

With the current work that is being done without adding the allowed_features claim to Enterprise licenses, having None mean no features enabled would disable everything for Enterprise licenses.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants