Update proxy features #45979
Merged
Update proxy features #45979
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mcbattirola
force-pushed
the
mcbattirola/update-proxy-features
branch
from
7ef795d to
b610f4c
Compare
zmb3
reviewed
Aug 28, 2024
mcbattirola
force-pushed
the
mcbattirola/update-proxy-features
branch
from
a5561d3 to
0b30b7b
Compare
…tirola/update-proxy-features
…tirola/update-proxy-features
…tirola/update-proxy-features
mcbattirola
force-pushed
the
mcbattirola/update-proxy-features
branch
from
08a8cf0 to
5c8233e
Compare
…tirola/update-proxy-features
|
The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with |
mcbattirola
added
the
no-changelog
rosstimothy
approved these changes
Sep 30, 2024
public-teleport-github-review-bot
bot
removed request for
r0mant,
ryanclark,
kiosion and
rudream
…ational/teleport into mcbattirola/update-proxy-features
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
…tirola/update-proxy-features
|
@mcbattirola See the table below for backport results.
|
mcbattirola
added a commit
that referenced
this pull request
Oct 31, 2024
* Add feature watcher * Add test * Update godocs, fix typos, rename SupportEntitlementsCompatibility to BackfillFeatures * Godocs; rename start and stop functions * Use `Feature` prefix in var names instead of `License` * Fix lint * Fix TestGetWebConfig_LegacyFeatureLimits * Fix TestGetWebConfig_WithEntitlements * Fix tests and lint * Remove sync.Once * Add jitter * Remove Ping call from getUserContext * Move entitlements test to entitlements package * Apply suggestions from code review: godoc and tests improvement. Co-authored-by: Zac Bergquist <[email protected]> * Improve tests * Shadow `t` in EventuallyWithT closure to avoid mistakes * Improve startFeatureWatcher godoc * Log features on update * Log features on update * Avoid race condition on test * Improve TODO comment Co-authored-by: rosstimothy <[email protected]> * Use handler config context * Start feature watcher in NewHandler * Improve startFeatureWatcher godocs * Add TODO to unexport SetClusterFeatures * Remove featureWatcherReady * Use require in require.EventuallyWithT in cases where an error is not expected * Use return of assert.NoError` to return early on require.EventuallyWithT --------- Co-authored-by: Zac Bergquist <[email protected]> Co-authored-by: rosstimothy <[email protected]>
github-merge-queue bot
pushed a commit
that referenced
this pull request
Nov 4, 2024
* Update proxy features (#45979) * Add feature watcher * Add test * Update godocs, fix typos, rename SupportEntitlementsCompatibility to BackfillFeatures * Godocs; rename start and stop functions * Use `Feature` prefix in var names instead of `License` * Fix lint * Fix TestGetWebConfig_LegacyFeatureLimits * Fix TestGetWebConfig_WithEntitlements * Fix tests and lint * Remove sync.Once * Add jitter * Remove Ping call from getUserContext * Move entitlements test to entitlements package * Apply suggestions from code review: godoc and tests improvement. Co-authored-by: Zac Bergquist <[email protected]> * Improve tests * Shadow `t` in EventuallyWithT closure to avoid mistakes * Improve startFeatureWatcher godoc * Log features on update * Log features on update * Avoid race condition on test * Improve TODO comment Co-authored-by: rosstimothy <[email protected]> * Use handler config context * Start feature watcher in NewHandler * Improve startFeatureWatcher godocs * Add TODO to unexport SetClusterFeatures * Remove featureWatcherReady * Use require in require.EventuallyWithT in cases where an error is not expected * Use return of assert.NoError` to return early on require.EventuallyWithT --------- Co-authored-by: Zac Bergquist <[email protected]> Co-authored-by: rosstimothy <[email protected]> * Export ClusterFeatures * Merge fix * Fix tests --------- Co-authored-by: Zac Bergquist <[email protected]> Co-authored-by: rosstimothy <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The proxy service only reads features at startup, leading to potential stale features if they change after the initial load. Some code paths (e.g., SCIM web handlers) check features during requests, which means proxies must be restarted when features change.
This PR introduces a
startFeatureWatchermethod in the web handler. The watcher periodically pings the auth server (every 5 to 10 minutes) to fetch and update features, ensuring they remain up-to-date without restarting the proxy.I chose to add the watcher within the handler rather than
service.TeleportProcessto minimize the impact on other services. This avoids unintended side effects, as the proxy handler is currently the only component requiring dynamic feature updates. If other services need this in the future, we can consider moving the watcher to a higher-level component. Let me know what you think.Closes #39161