Skip to content

⚠️ helm: use chunking release driver in systemNamespace - #1057

Merged
joelanford merged 1 commit into
operator-framework:mainfrom
joelanford:custom-helm-storage
Aug 2, 2024
Merged

⚠️ helm: use chunking release driver in systemNamespace#1057
joelanford merged 1 commit into
operator-framework:mainfrom
joelanford:custom-helm-storage

Conversation

@joelanford

@joelanfordjoelanford commented Jul 16, 2024

Copy link
Copy Markdown
Member

Description

This PR:

  1. uses a new helm release storage driver from helm-operator-plugins that chunks release data into one or more secrets (this also incidentally means that our client and the Helm CLI client won't mistake each other's release storage for their own).
  2. moves release secret storage back to our system namespace so that we don't leak details of our release bookkeeping to users.
  3. Notes that the new helm-operator-plugins commit also makes it possible to use one service account for release storage and a separate service account for deploying bundle contents. This is necessary if we want to avoid leaking our internal release storage mechanism to users (who would otherwise have to configure RBAC for release storage permission)

Fixes: #923

Reviewer Checklist

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

@joelanford
joelanford requested a review from a team as a code ownerJuly 16, 2024 19:48
@openshift-ciopenshift-ciBot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 16, 2024
@netlify

netlifyBot commented Jul 16, 2024

Copy link
Copy Markdown

Deploy Preview for olmv1 ready!

NameLink
🔨 Latest commit30c827a
🔍 Latest deploy loghttps://app.netlify.com/sites/olmv1/deploys/66ad45d46a16c00008ce39a4
😎 Deploy Previewhttps://deploy-preview-1057--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.

@joelanford
joelanford marked this pull request as draft July 16, 2024 19:48
@joelanfordjoelanford changed the title WIP: helm: use chunking release driver in systemNamespace⚠️ WIP: helm: use chunking release driver in systemNamespaceJul 16, 2024
@codecov

codecovBot commented Jul 16, 2024

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 80.26316% with 15 lines in your changes missing coverage. Please review.

Project coverage is 75.28%. Comparing base (f6a9fad) to head (30c827a).

FilesPatch %Lines
internal/action/storagedriver.go72.22%10 Missing and 5 partials ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #1057 +/- ##
=======================================
Coverage 75.28% 75.28% =======================================
Files 33 35 +2 Lines 1861 1914 +53 =======================================
+ Hits 1401 1441 +40 - Misses 321 330 +9 - Partials 139 143 +4 
FlagCoverage Δ
e2e57.36% <80.26%> (+0.40%)⬆️
unit50.78% <1.31%> (-1.40%)⬇️

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.

@openshift-merge-robotopenshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 18, 2024
@openshift-merge-robotopenshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 18, 2024
@joelanford
joelanfordforce-pushed the custom-helm-storage branch 3 times, most recently from 011246e to e11becaCompareJuly 18, 2024 20:09
@openshift-merge-robotopenshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 19, 2024
@openshift-merge-robotopenshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 19, 2024
@joelanford
joelanfordforce-pushed the custom-helm-storage branch 2 times, most recently from bf02ec0 to 1a7068fCompareJuly 20, 2024 09:57
@openshift-merge-robotopenshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 20, 2024
@openshift-merge-robotopenshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 20, 2024
@openshift-merge-robotopenshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 23, 2024
@openshift-merge-robotopenshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 29, 2024
Comment threadinternal/httputil/httputil.go Outdated
@joelanford
joelanford marked this pull request as ready for review July 29, 2024 15:53
Comment threadconfig/base/rbac/role_binding.yaml Outdated

@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
Looks reasonable to me
(Added a comment later about a TODO though)

@openshift-ciopenshift-ciBot added the lgtm Indicates that a PR is ready to be merged. label Jul 30, 2024
@tmshorttmshort removed the lgtm Indicates that a PR is ready to be merged. label Jul 30, 2024

@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.

OK, I want the TODO in there about the timeout.
I would prefer a release of helm-operator-plugins, failing that, perhaps a TODO.

@tmshort

Copy link
Copy Markdown
Contributor

And... a rebase is needed...

@joelanford

Copy link
Copy Markdown
MemberAuthor

Ok, rebased and reverted the catalog client timeout change, so it is back to 10s. Looks like e2e still passes in GH CI.

I'm also seeing main fail the same as locally for me as this branch, so let's chalk this up as a "me" problem for now.

tmshort
tmshort previously approved these changes Aug 2, 2024

@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

@openshift-ciopenshift-ciBot added the lgtm Indicates that a PR is ready to be merged. label Aug 2, 2024
@tmshort
tmshort added this pull request to the merge queue Aug 2, 2024
@tmshort
tmshort removed this pull request from the merge queue due to a manual request Aug 2, 2024
Signed-off-by: Joe Lanford <joe.lanford@gmail.com>
@openshift-ciopenshift-ciBot removed the lgtm Indicates that a PR is ready to be merged. label Aug 2, 2024

@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

@openshift-ciopenshift-ciBot added the lgtm Indicates that a PR is ready to be merged. label Aug 2, 2024
@joelanford
joelanford enabled auto-merge August 2, 2024 20:49
@joelanford
joelanford added this pull request to the merge queue Aug 2, 2024
Merged via the queue into operator-framework:main with commit cb9ea00Aug 2, 2024
perdasilva pushed a commit to LalatenduMohanty/operator-controller that referenced this pull request Aug 13, 2024
perdasilva pushed a commit to kevinrizza/operator-controller that referenced this pull request Aug 13, 2024
@skattojuskattoju mentioned this pull request Sep 25, 2024
4 tasks
@joelanford
joelanford deleted the custom-helm-storage branch October 22, 2024 13:02
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: data too long issue rendering

4 participants

@joelanford@tmshort@acornett21@openshift-merge-robot