Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Execute Only for Shared Actions #4935

Merged
merged 8 commits into from
Aug 25, 2020
Merged

Execute Only for Shared Actions #4935

merged 8 commits into from
Aug 25, 2020

Conversation

bkemburu
Copy link
Contributor

Execute Only For Shared Actions/Packages

Shared packages/bindings of shared packages and actions within shared packages/bindings. currently allows both READ and EXECUTE operations for all users of the system. The config option supports EXECUTE-only on shared packages such that the actions within the package are able to be EXECUTED but not READ by users who do not own the namespace. I wrote tests in the PackageApiTests and PackageActionApiTests that verify the functionality of EXECUTE-only

Description

Related issue and scope

  • I opened an issue to propose and discuss this change (#????)

My changes affect the following components

  • API
  • Controller
  • Message Bus (e.g., Kafka)
  • Loadbalancer
  • Invoker
  • Intrinsic actions (e.g., sequences, conductors)
  • Data stores (e.g., CouchDB)
  • Tests
  • Deployment
  • CLI
  • General tooling
  • Documentation

Types of changes

  • Bug fix (generally a non-breaking change which closes an issue).
  • Enhancement or new feature (adds new functionality).
  • Breaking change (a bug fix or enhancement which changes existing behavior).

Checklist:

  • I signed an Apache CLA.
  • I reviewed the style guides and followed the recommendations (Travis CI will check :).
  • I added tests to cover my changes.
  • My changes require further changes to the documentation.
  • I updated the documentation where necessary.

@bkemburu bkemburu changed the title Kemburu/execute only backup Execute Only for Shared Actions Jul 27, 2020
@style95
Copy link
Member

style95 commented Jul 28, 2020

@bkemburu Thank you for the contribution.
This is a good feature and I have wanted this.

But I hope we can generalize this issue into a permission management.
There are some discussions in the existing thread about Unix-like permission.
#3629
#4058

rabbah
rabbah reviewed Jul 28, 2020
View reviewed changes
Copy link
Member

@rabbah rabbah left a comment

Choose a reason for hiding this comment

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

Thanks for this contribution. As @style95 noted this is a long desired feature.

I have provided some comments on code style. You'll need to also apply our project code formatting or Travis will fail.

core/controller/src/main/scala/org/apache/openwhisk/core/controller/Actions.scala Outdated
val value = entityName.path
terminate(StatusCode.int2StatusCode(403), s"GET not permitted for '$value' since it's an action in a shared package")
} else {
code match {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: I'd suggest the simpler/cleaner if (code) { } else { }.

@bkemburu bkemburu force-pushed the kemburu/execute-only-backup branch 2 times, most recently from 28def2b to aa845f0 Compare August 6, 2020 13:21
@bkemburu
Update application.conf
38880d9 
Changed Flag to shared-packages-execute-only with default value set to false.
@bkemburu bkemburu force-pushed the kemburu/execute-only-backup branch from aa845f0 to 38880d9 Compare August 6, 2020 16:04
@bkemburu bkemburu requested a review from rabbah August 6, 2020 18:49
tysonnorris
tysonnorris reviewed Aug 7, 2020
View reviewed changes
core/controller/src/main/scala/org/apache/openwhisk/core/controller/Packages.scala Outdated

/** Here's where I check package execute only case with package binding. */
val packagePath = wp.namespace.asString
val bindingPath = wp.binding.iterator.next().toString
Copy link
Contributor

Choose a reason for hiding this comment

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

is wp.binding.iterator guaranteed to not be empty?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, because this code falls within a binding case so binding has to hold some value.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah not enough context shown in the diff; then I think you can replace wp.binding.iterator.next().toString with b.toString to make this more clear

@bkemburu bkemburu force-pushed the kemburu/execute-only-backup branch from 9d9db52 to 13c7f50 Compare August 12, 2020 17:55
@bkemburu bkemburu force-pushed the kemburu/execute-only-backup branch from 13c7f50 to 3b0338a Compare August 12, 2020 17:57
tysonnorris added a commit to adobe-apiplatform/incubator-openwhisk that referenced this pull request Aug 17, 2020
@tysonnorris
manually merge vesh's pr apache#4935 to manually merge to runtime-prs…
f11f6b2 
…-v84
tysonnorris
tysonnorris approved these changes Aug 25, 2020
View reviewed changes
Copy link
Contributor

@tysonnorris tysonnorris left a comment

Choose a reason for hiding this comment

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

LGTM

@tysonnorris tysonnorris merged commit e255126 into apache:master Aug 25, 2020
@rabbah
Copy link
Member

rabbah commented Aug 27, 2020

thanks for the contribution @bkemburu 🎉

This was referenced Aug 27, 2020
Closed
Closed
tysonnorris pushed a commit to adobe-apiplatform/incubator-openwhisk that referenced this pull request Nov 10, 2020
@bkemburu @tysonnorris
Execute Only for Shared Actions (apache#4935)
31902c9 
* Initial Commit of Execute Only
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tysonnorris tysonnorris tysonnorris approved these changes

@rabbah rabbah Awaiting requested review from rabbah

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

Successfully merging this pull request may close these issues.
4 participants
@bkemburu @style95 @rabbah @tysonnorris