-
Notifications
You must be signed in to change notification settings - Fork 18
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
feat: expose rbacMode from moduleConfig #1347
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Case Wylie <[email protected]>
Signed-off-by: Case Wylie <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1347 +/- ##
==========================================
+ Coverage 79.36% 79.43% +0.06%
==========================================
Files 38 39 +1
Lines 1793 1799 +6
Branches 363 391 +28
==========================================
+ Hits 1423 1429 +6
+ Misses 368 341 -27
- Partials 2 29 +27
|
Signed-off-by: Case Wylie <[email protected]>
Signed-off-by: Case Wylie <[email protected]>
Signed-off-by: Case Wylie <[email protected]>
Signed-off-by: Case Wylie <[email protected]>
Signed-off-by: Case Wylie <[email protected]>
Signed-off-by: Case Wylie <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Back to you!
@@ -127,6 +127,7 @@ Below are the available configurations through `package.json`. | |||
|------------------|----------------------------------------|---------------------------------| | |||
| `uuid` | Unique identifier for the module | `hub-operator` | | |||
| `onError` | Behavior of the webhook failure policy | `reject`, `ignore` | | |||
| `rbacMode` | Configures module to build binding RBAC with principal of least privilege | `scoped`, `admin` | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest this move down in the table to live right next to the rbac
key. It feels kind of lonely all by itself up here.
It'd also be useful if the description for this mentioned (however you like) that rbacMode: scoped
would pull in the rbac
block of config... or maybe the reverse (the rbac
description including that it's only invoked when rbackMode: scoped
is set)... or maybe both!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't scoped
mode do more than just pull in the rbac
block of config?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, im moving it down.
To try and tie in your comments, scoped only builds the cluster role with the principal of least priv and is decoupled whether there is an rbac block or not.
RBAC block however does nothing unless you are in scoped mode. I think i addressed this in the update that I am about to push. LMK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the relationship between rbacMode
and pepr-build-wasm.ts
..? I would have never thought to look for our RBAC mode tests in a -wasm
file. 🤔 Are they bound together in some way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't (apparently) comment on lines of code that weren't changed, but... just saw that line 15 says "dst folder" -- should probably say something like "/dist folder" instead (since that's what'll actually be there).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some reason the rbac tests were in the -wasm file when I started working on the rbac mode stuff. I have no idea why they were put there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
line 15 only makes a folder. This probably should be live in an it()
block. I will move this below the where the module is being built.
rbacMode is being set through the CLI in pepr-build.ts
, we need a module to build so this is the other option.
journey/pepr-build-wasm.ts
Outdated
execSync(`npx pepr build -r gchr.io/defenseunicorns --rbac-mode scoped -o ${outputDir}`, { | ||
it("should successfully build the Pepr project with arguments and rbacMode scoped", async () => { | ||
// Set rbacMode in the Pepr Module Config of the package.json. | ||
await addScopedRbacMode(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sort of ambivalent about this await addScopedRbacMode();
line living here -- it's changes the state of the entire Pepr module (in that it changes the source of what get's built / asserted on) and yet it's kind of hidden down here inside just-another-test-case.
Doesn't seem like it's hurting anything right now -- at least not as the tests are currently laid out -- but would suggest moving any global state change stuff like this up into a beforeAll()
block (or something) to both 1) call more attention to it, and 2) future-proof the test file against the case that more assertions get added and/or someone tries to "reorder" the it()
blocks.
src/lib/helpers.ts
Outdated
@@ -394,3 +394,19 @@ export function replaceString(str: string, stringA: string, stringB: string) { | |||
const regExp = new RegExp(escapedStringA, "g"); | |||
return str.replace(regExp, stringB); | |||
} | |||
|
|||
// determineRbacMode determines the RBAC mode to use based on the cli and the module's config | |||
export function determineRbacMode(opts: { rbacMode?: string }, cfg: { pepr: { rbacMode?: string } }): string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem like a function that's generalized enough to need to live in a helpers
collection. It also only appears to be used in one place (the build.ts
file).
Would there be anything stopping you from moving this helper back into build.ts
and just using it directly? Much more cohesive over there & would help prevent the helpers.ts
file from becoming a junk drawer for single-use funcs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It first lived there but I realized that we cannot unit test it there. Would the unit test still live in helpers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After ruminating on this issue, considering many alternatives I came to the conclusion to create a folder cli-helpers/build.ts
.
Adding a coverageIgnorePath in the jest config had adverse affects. Babel warnings occurred in the console and the coverage dropped by ~15%, more strictly defined paths would have been necessary as well as an adjustment of other configs to suppress the babel warning or increase a threshold.
[BABEL] Note: The code generator has deoptimised the styling of /Users/cmwylie19/not-pepr/pepr/node_modules/@kubernetes/client-node/dist/gen/types/ObjectParamAPI.js as it exceeds the max of 500KB.
[BABEL] Note: The code generator has deoptimised the styling of /Users/cmwylie19/not-pepr/pepr/node_modules/@kubernetes/client-node/dist/gen/types/ObservableAPI.js as it exceeds the max of 500KB.
[BABEL] Note: The code generator has deoptimised the styling of /Users/cmwylie19/not-pepr/pepr/node_modules/@kubernetes/client-node/dist/gen/apis/AppsV1Api.js as it exceeds the max of 500KB.
Separate jest configs caused validation warnings
● Validation Warning:
Unknown option "collectCoverage" with value true was found.
This is probably a typing mistake. Fixing it will remove this message.
Configuration Documentation:
https://jestjs.io/docs/configuration
We would have needed to run jest from node through node_modules and add an --experimental-vm-modules
from our test.
This approach seems to satisfy the requirement and and provide us room to grow in the future.
Signed-off-by: Case Wylie <[email protected]>
Signed-off-by: Case Wylie <[email protected]>
Description
Allows user to define rbacMode in module config enabling building from infra-as-code practices
End to End Test: ❌ (Journey✅)
Changes in this PR include:
package.json
to test the feature in journey testjourney/resources/values.yaml
for assertionRelated Issue
Fixes #1327
Relates to #
Type of change
Checklist before merging