Skip to content
This repository has been archived by the owner on Mar 16, 2024. It is now read-only.

change: move actual permissions to .status + apply IRAs to consumed permissions #2226

Merged
merged 13 commits into from
Oct 16, 2023
Merged

change: move actual permissions to .status + apply IRAs to consumed permissions #2226

merged 13 commits into from
Oct 16, 2023

Conversation

iwilltry42
Copy link
Contributor

@iwilltry42 iwilltry42 commented Oct 6, 2023 •
edited
Loading

This PR introduces quite a few changes around permissions:

  • Some comments and renaming to make permissions stuff clearer
  • The actually granted and authorized permissions for every app will now go to AppInstances' .status.permissions field and contain only those permissions required for this specific AppInstance, not for nested Acorns
  • Permissions are promoted from .spec to .status along with the staged AppImage when all checks (including ImageRoleAuthorizations) pass
  • Permissions consumed from nested Services are added after all that and thus we have a new handler for them that only allows the consuming App to run if it's authorized to hold the permissions consumed from the producing service
    • Those will be merged into the .status.permissions field such that this field reflects truly all permissions given to that AppInstance

Checklist

  • The title of this PR would make a good line in Acorn's Release Note's Changelog
  • The title of this PR ends with a link to the main issue being address in parentheses, like: This is a title (#1216). Here's an example
  • All relevant issues are referenced in the PR description. NOTE: don't use GitHub keywords that auto-close issues
  • Commits follow contributing guidance
  • Automated tests added to cover the changes. If tests couldn't be added, an explanation is provided in the Verification and Testing section
  • Changes to user-facing functionality, API, CLI, and upgrade impacts are clearly called out in PR description
  • PR has at least two approvals before merging (or a reasonable exception, like it's just a docs change)

@iwilltry42
change: adds .Status.Permissions + rename permissions fields for clar…
97690c6 
…ity and add a bunch of comments around permissions

Signed-off-by: Thorsten Klein <[email protected]>
@iwilltry42
change: ira-check on granted permissions for current app-level (not n…
4e11532 
…ested)

Signed-off-by: Thorsten Klein <[email protected]>
@iwilltry42
change: use app.Status.Permissions
e69d091 
Signed-off-by: Thorsten Klein <[email protected]>
@iwilltry42
change: handle consumer permissions
402b650 
Signed-off-by: Thorsten Klein <[email protected]>
@iwilltry42
change: move consumerpermissions to own handler
19f5599 
Signed-off-by: Thorsten Klein <[email protected]>
@iwilltry42
fix: perms weren't applied from status
f77c057 
Signed-off-by: Thorsten Klein <[email protected]>
@iwilltry42
change: adjust image matching for imagerules and add test for IRAs
72da4cf 
- add: test for consumer permissions when IRAs are enabled
- change: account for mixed digest namePatterns
- change: custom imageName parsing in matching logic to avoid docker.io
  auto-prefix

Signed-off-by: Thorsten Klein <[email protected]>
@iwilltry42 iwilltry42 marked this pull request as ready for review October 12, 2023 09:23
@iwilltry42 iwilltry42 changed the title change: adds .Status.Permissions + rename permissions fields for clar… change: move actual permissions to .status + apply IRAs to consumed permissions Oct 12, 2023
g-linville
g-linville reviewed Oct 12, 2023
View reviewed changes
Copy link
Contributor

@g-linville g-linville left a comment

Choose a reason for hiding this comment

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

This is my first pass. Gonna have to wrap my head around this some more, but these are my initial thoughts.

integration/client/imagerules/imageroleauthorizations_test.go Outdated Show resolved Hide resolved
pkg/controller/appdefinition/depends.go Outdated Show resolved Hide resolved
pkg/controller/appdefinition/pullappimage.go Outdated Show resolved Hide resolved
pkg/controller/permissions/consumerpermissions.go Outdated Show resolved Hide resolved
pkg/controller/permissions/consumerpermissions.go Show resolved Hide resolved
pkg/controller/permissions/permissions_check.go Show resolved Hide resolved
@iwilltry42 iwilltry42 force-pushed the fix/enhanced-perms-logic branch from cb6a15f to 1f59494 Compare October 12, 2023 15:51
@iwilltry42 iwilltry42 requested a review from g-linville October 12, 2023 17:13
thedadams
thedadams approved these changes Oct 12, 2023
View reviewed changes
pkg/controller/permissions/permissions_check.go Outdated Show resolved Hide resolved
pkg/controller/permissions/consumerpermissions.go Outdated Show resolved Hide resolved
pkg/controller/permissions/consumerpermissions.go Outdated Show resolved Hide resolved
iwilltry42 and others added 3 commits October 13, 2023 16:56
@iwilltry42 @thedadams
Update pkg/controller/permissions/consumerpermissions.go
ed615d7 
Co-authored-by: Donnie Adams <[email protected]>
Signed-off-by: Thorsten Klein <[email protected]>
@iwilltry42 @thedadams
Update pkg/controller/permissions/permissions_check.go
eb959b0 
Co-authored-by: Donnie Adams <[email protected]>
Signed-off-by: Thorsten Klein <[email protected]>
@iwilltry42
change: remove unnecessary check
d049a5d 
Signed-off-by: Thorsten Klein <[email protected]>
@iwilltry42 iwilltry42 merged commit 14748bc into acorn-io:main Oct 16, 2023
@iwilltry42 iwilltry42 deleted the fix/enhanced-perms-logic branch October 16, 2023 14:33
@iwilltry42 iwilltry42 mentioned this pull request Oct 31, 2023
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@thedadams thedadams thedadams approved these changes

@g-linville g-linville g-linville approved these changes

@ibuildthecloud ibuildthecloud Awaiting requested review from ibuildthecloud

@cjellick cjellick Awaiting requested review from cjellick

@njhale njhale Awaiting requested review from njhale

@tylerslaton tylerslaton Awaiting requested review from tylerslaton

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@iwilltry42 @thedadams @g-linville