Conversation
c7c444c to
4154e68
Compare
9bdf0fd to
a48bdf9
Compare
zmb3
left a comment
There was a problem hiding this comment.
I didn't get all the way through the review, but I've noticed that there are some things that make this PR hard to understand.
It looks like we used to have a "feature" for TAG/Policy/AccessGraph, and then we switched to entitlements, but we also kept the feature for compatibility.
This means there are a bunch of places where we're checking both the feature and the entitlement.
Now we're adding a third thing to check (an additional entitlement). That's a lot of combinations to test and verify that everything is working correctly.
Entitlements have been out for long enough now that we should be able to simplify this by removing the features so the entitlement becomes a single source of truth.
What do you think?
| // If the policy feature is disabled in the license, return a disabled response. if cloud, return the response to allow demo mode enabling | ||
| if !modules.GetModules().Features().GetEntitlement(entitlements.Policy).Enabled && !modules.GetModules().Features().AccessGraph && !modules.GetModules().Features().Cloud { | ||
| if !modules.GetModules().Features().GetEntitlement(entitlements.AccessGraph).Enabled && !modules.GetModules().Features().AccessGraph && !modules.GetModules().Features().Cloud { |
There was a problem hiding this comment.
- I would update the comment here since it talks about the [deprecated] policy feature.
- This if statement is also pretty hard to follow. I wonder if we can make it so that we can only check the
AccessGraphentitlement here, and have the code that sets features handle the compatibility.
There was a problem hiding this comment.
Unfortunately, right now the entitlement and feature serve somewhat unique purposes. The AccessGraph entitlement ensures the customer has access to the feature while the AccessGraph feature ensures the same AND that the TAG resource was connected (see code). I had a draft branch to decouple that feature but it's a bit more involved and not on the critical path for this PR.
|
Moving to draft while PR is rebased |
be45c44 to
7265e5f
Compare
eed9a9b to
be4ca92
Compare
d52cf07 to
0d28b8d
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0d28b8d6f7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // If the policy feature is disabled in the license, return a disabled response. if cloud, return the response to allow demo mode enabling | ||
| if !modules.GetModules().Features().GetEntitlement(entitlements.Policy).Enabled && !modules.GetModules().Features().AccessGraph && !modules.GetModules().Features().Cloud { | ||
| // If the access graph feature is disabled in the license, return a disabled response. if cloud, return the response to allow demo mode enabling | ||
| if !modules.GetModules().Features().GetEntitlement(entitlements.AccessGraph).Enabled && !modules.GetModules().Features().AccessGraph && !modules.GetModules().Features().Cloud { |
There was a problem hiding this comment.
Keep Policy fallback in Access Graph authz gates
This gate now ignores the legacy Policy entitlement, so a cluster with Policy=true, AccessGraph=false, and Features.AccessGraph=false will be treated as unlicensed and denied (GetClusterAccessGraphConfig returns disabled, and update/upsert/reset paths now return AccessDenied). That is a regression during the deprecation window because Policy is still carried in entitlements and other paths still treat it as valid compatibility input.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Features.AccessGraph will be updated to be enabled if either AccessGraph or Policy entitlement is enabled in e.#7898.
AccessGraph entitlementsis also enabled if either AccessGraph or Policy is enabled for self-hosted licenses (GetSelfHostedLicenseFeatures) and legacy licenses (setLegacyLogic) in the same file.
For cloud, no fallback is needed as it always sets AccessGraph and Policy to the same value here
0d28b8d to
115ab82
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 115ab82766
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Summary
This PR substitutes the now deprecated Policy entitlement for the new Access Graph entitlement. It also swaps out all usages in Teleport of Policy for Access Graph.
Note: The Access Graph entitlement has a structure of
{Enabled: bool, Limit: num}while Policy only had{Enabled: bool}. While this PR is a 1:1 swap that only passes through theEnabledflag, theLimitvalue may want to be used in the future, 0 indicating a dedicated instance and 1 indicating a shared instance.https://github.com/gravitational/teleport.e/pull/7898 will be similar to this PR
Merge order of PRs:
Supports https://github.com/gravitational/cloud/issues/16130
Manual Test Plan
Test Environment
I deployed a dev build with these changes alone to cloud staging (
erik-ag-partial) and verified that toggling Access Graph from the SC UI provides and removes access in the product. Though you still need to roll the pods to see the expected UI.Test Cases
Local Entitlement Testing
Prep
Test
AccessGraphentitlement independentlyEnabled
onthe Access Graph feature for the tenant in the SC UI/web/accessgraph), and verify you see the following error:Policywas not enabled andAccessGraphwas set toenabled:truein the responseDisabled
offAccess Graph in the SC UIPolicywas not enabled andAccessGraphentitlement was not enabled in the responseTest
Policyentitlement independently for backward compatibility (simulating old licenses)Enabled
onAccess Graph in the SC UI/web/accessgraph), and verify you see the same connection error againAccessGraphwas not enabled andPolicywas set toenabled:truein the responseDisabled
offAccess Graph in the SC UIAccessGraphwas not enabled andPolicywas not enabled in the response