Skip to content

✨ wire up ServiceAccount based caching layer - #1074

Merged
tmshort merged 2 commits into
operator-framework:mainfrom
everettraven:feature/wire-sa-cache
Jul 22, 2024
Merged

✨ wire up ServiceAccount based caching layer#1074
tmshort merged 2 commits into
operator-framework:mainfrom
everettraven:feature/wire-sa-cache

Conversation

@everettraven

@everettraveneverettraven commented Jul 19, 2024

Copy link
Copy Markdown
Contributor

Description

  • Updates the RestConfigMapper to use a new TokenTripper that wraps a http.RoundTripper to always set the Authorization header to the referenced ServiceAccount
  • Updates the E2E ServiceAccounts introduced in ✨ Wire up Service Account #1038 to use a scoped set of permissions instead of giving it cluster-admin equivalent permissions (used bind + escalate for RBAC to keep the SA permissions as succinct as possible. Open to changing this if desired)
  • Updates the ClusterExtensionReconciler to have an exported field that can be used to configure the contentmanager.Watcher that should be used to establish watches on the ClusterExtension managed objects
  • Updates the contentmanager.Instance.Watch() method to create watches with the same configurations as existed in the ClusterExtensionReconciler.reconcile() method previously
  • Adds an E2E test to verify that when a change is made to managed objects (in the test case we delete a managed object), that the managed object triggers reconciliation and changes are reverted (in the test case we ensure the managed object is recreated)

resolves#975
resolves#983

Reviewer Checklist

  • API Go Documentation
  • Tests: Unit Tests (and E2E Tests, if appropriate)
  • Comprehensive Commit Messages
  • Links to related GitHub Issue(s)

@everettraven
everettraven requested a review from a team as a code ownerJuly 19, 2024 18:27
@netlify

netlifyBot commented Jul 19, 2024

Copy link
Copy Markdown

Deploy Preview for olmv1 ready!

NameLink
🔨 Latest commit586046f
🔍 Latest deploy loghttps://app.netlify.com/sites/olmv1/deploys/669e71fa5220dd0008608ebf
😎 Deploy Previewhttps://deploy-preview-1074--olmv1.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@codecov

codecovBot commented Jul 19, 2024

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 65.62500% with 11 lines in your changes missing coverage. Please review.

Project coverage is 72.82%. Comparing base (58c5776) to head (586046f).

FilesPatch %Lines
internal/authentication/tripper.go61.53%3 Missing and 2 partials ⚠️
internal/authentication/tokengetter.go0.00%4 Missing ⚠️
internal/contentmanager/contentmanager.go83.33%1 Missing ⚠️
...nternal/controllers/clusterextension_controller.go66.66%0 Missing and 1 partial ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #1074 +/- ##
==========================================
- Coverage 72.93% 72.82% -0.11% 
==========================================
Files 31 32 +1 Lines 1862 1866 +4 ==========================================
+ Hits 1358 1359 +1 - Misses 366 368 +2 - Partials 138 139 +1 
FlagCoverage Δ
e2e56.80% <65.62%> (+<0.01%)⬆️
unit44.31% <9.37%> (-0.10%)⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Comment threadinternal/authentication/tripper.go
@everettraven
everettravenforce-pushed the feature/wire-sa-cache branch from 941737c to 274df7eCompareJuly 22, 2024 14:20
@openshift-merge-robotopenshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 22, 2024
@everettraven
everettravenforce-pushed the feature/wire-sa-cache branch from 274df7e to b2dc337CompareJuly 22, 2024 14:23
@openshift-merge-robotopenshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 22, 2024
Signed-off-by: everettraven <everettraven@gmail.com>
@everettraven
everettravenforce-pushed the feature/wire-sa-cache branch from b2dc337 to 5e51907CompareJuly 22, 2024 14:30
Signed-off-by: everettraven <everettraven@gmail.com>

@skattojuskattoju left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci

Copy link
Copy Markdown

@skattoju: changing LGTM is restricted to collaborators

Details

In response to this:

/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

Comment threadcmd/manager/main.go
acg, err := action.NewWrappedActionClientGetter(cfgGetter,
helmclient.WithFailureRollbacks(false),
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: change in whitespace

},
})).
Build(r)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: whitespace-only change

@tmshorttmshort left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

/lgtm
Just small wicked minor nits.

@openshift-ciopenshift-ciBot added the lgtm Indicates that a PR is ready to be merged. label Jul 22, 2024
@tmshort
tmshort added this pull request to the merge queue Jul 22, 2024
Merged via the queue into operator-framework:main with commit b73ea5cJul 22, 2024
perdasilva pushed a commit to LalatenduMohanty/operator-controller that referenced this pull request Aug 13, 2024
* wire up serviceaccount based caching layer
Signed-off-by: everettraven <everettraven@gmail.com>
* remove body close
Signed-off-by: everettraven <everettraven@gmail.com>
---------
Signed-off-by: everettraven <everettraven@gmail.com>
perdasilva pushed a commit to kevinrizza/operator-controller that referenced this pull request Aug 13, 2024
* wire up serviceaccount based caching layer
Signed-off-by: everettraven <everettraven@gmail.com>
* remove body close
Signed-off-by: everettraven <everettraven@gmail.com>
---------
Signed-off-by: everettraven <everettraven@gmail.com>
@skattojuskattoju mentioned this pull request Sep 25, 2024
4 tasks
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtmIndicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Reconciliation of cluster extension content not working on main branch Wire up dynamic caching layer to manage installed content

4 participants

@everettraven@tmshort@skattoju@openshift-merge-robot