Add an annotations to inject the API key into the action context. #4284

rabbah · 2019-02-13T21:49:12Z

See #4226. Note that this is a breaking change - actions that are deployed need to be updated to add this annotation.

Closes #4226.

Description

Related issue and scope

I opened an issue to propose and discuss this change (change the default action context to omit api key #4226).

My changes affect the following components

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.

rabbah · 2019-02-13T21:50:03Z

I know there are actions we're deploying that will need to be updated, waiting for travis to tell me which ones.

core/invoker/src/main/scala/org/apache/openwhisk/core/containerpool/ContainerProxy.scala

rabbah · 2019-02-15T03:09:20Z

I will need to update a few API tests which check the API response and now are missing the new annotation.

codecov-io · 2019-02-17T02:55:15Z

Codecov Report

Merging #4284 into master will decrease coverage by 4.6%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #4284      +/-   ##
==========================================
- Coverage   85.57%   80.96%   -4.61%     
==========================================
  Files         168      168              
  Lines        7839     7850      +11     
  Branches      519      528       +9     
==========================================
- Hits         6708     6356     -352     
- Misses       1131     1494     +363

Impacted Files	Coverage Δ
...org/apache/openwhisk/core/controller/Actions.scala	`93.33% <100%> (+0.16%)`	⬆️
...org/apache/openwhisk/core/entity/WhiskAction.scala	`86.9% <100%> (+0.15%)`	⬆️
.../openwhisk/core/containerpool/ContainerProxy.scala	`92.03% <100%> (+0.06%)`	⬆️
.../apache/openwhisk/core/controller/WebActions.scala	`92.33% <100%> (+0.06%)`	⬆️
...a/org/apache/openwhisk/core/entity/Parameter.scala	`95.45% <100%> (ø)`	⬆️
...core/database/cosmosdb/RxObservableImplicits.scala	`0% <0%> (-100%)`	⬇️
...core/database/cosmosdb/CosmosDBArtifactStore.scala	`0% <0%> (-95.18%)`	⬇️
...sk/core/database/cosmosdb/CosmosDBViewMapper.scala	`0% <0%> (-92.6%)`	⬇️
...whisk/core/database/cosmosdb/CosmosDBSupport.scala	`0% <0%> (-84.62%)`	⬇️
...abase/cosmosdb/CosmosDBArtifactStoreProvider.scala	`4% <0%> (-52%)`	⬇️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1c6cc36...4ef54fe. Read the comment docs.

dubee · 2019-02-19T18:55:35Z

core/invoker/src/main/scala/org/apache/openwhisk/core/containerpool/ContainerProxy.scala

+        // if the action requests the api key to be injected into the action context, add it here;
+        // treat a missing annotation as requesting the api key for backward compatibility
+        val authEnvironment = {
+          if (job.action.annotations.isTruthy(WhiskAction.provideApiKeyAnnotationName, valueForNonExistent = true)) {


👍 I second to stay backwards compatible.

dubee · 2019-02-19T18:56:41Z

docs/actions.md

-* `__OW_ACTIVATION_ID` the activation id for this running action instance
-* `__OW_DEADLINE` the approximate time when this action will have consumed its entire duration quota (measured in epoch milliseconds)
+* `__OW_API_HOST` the API host for the OpenWhisk deployment running this action.
+* `__OW_API_KEY` the API key for the subject invoking the action, this key may be a restricted API key. This property is absent unless explicitly [requested](./annotations.md#annotations-for-all-actions).


May want to update the docs explaining the reverse compatibility feature you added.

done in last commit.

mdeuser · 2019-02-19T22:13:19Z

core/controller/src/main/scala/org/apache/openwhisk/core/controller/Actions.scala

+        }
+    } else annotations
+    newAnnotations ++ execAnnotation(exec)
+  }


mhenke1 · 2019-02-21T12:10:36Z

core/invoker/src/main/scala/org/apache/openwhisk/core/containerpool/ContainerProxy.scala

+        val authEnvironment = {
+          if (job.action.annotations.isTruthy(WhiskAction.provideApiKeyAnnotationName, valueForNonExistent = true)) {
+            job.msg.user.authkey.toEnvironment
+          } else JsObject.empty


} else job.msg.user.authkey.toEnvironmentNotSensitive

I do suggest to move the decision what to hide as sensitive information to each GenericAuthKey implementation by adding a new method authkey.toEnvironmentNotSensitive.
For BasicAuthenticationAuthKey this method should return JsObject.empty
Other GenericAuthKey implementations could decide which of the information stored should be made available on request only.

If we stay backwards compatible , I would be willing to implement this change implement in a following PR.

Can you send a comment to the dev list with the proposal. It is reasonable to me. Do you have an example you can attach?

I will address it once this PR is merged. No need to delay.

…d to the action context.

…s to treat a missing key. Default is missing key -> false (preserving behavior). Add test for isTruthy.

Actions that already exist without the annotation will receive the key as they might already expect.

dubee

PG3 3388 ⏳

…ache#4284) * Factor out some annotation names to WhiskAction singleton. * Omit API key unless requested explicitly. * Add an annotation provide-api-key which causes an API key to be passed to the action context. * Modify Parameters.isTruthy to allow for caller to specify how it wants to treat a missing key. Default is missing key -> false (preserving behavior). Add test for isTruthy. * Treat a missing provide-key-annotation as truthy annotation. Actions that already exist without the annotation will receive the key as they might already expect. * Add container proxy test. * Update tests for new system annotation. * Allow pre-existing actions to be exempt from new system annotations. * Update docs.

rabbah force-pushed the keys branch 2 times, most recently from 1b05f74 to f187ddc Compare February 14, 2019 01:02

markusthoemmes reviewed Feb 14, 2019

View reviewed changes

core/invoker/src/main/scala/org/apache/openwhisk/core/containerpool/ContainerProxy.scala Outdated Show resolved Hide resolved

rabbah force-pushed the keys branch 3 times, most recently from 931c291 to 2c0381a Compare February 15, 2019 02:24

rabbah added controller review Review for this PR has been requested and yet needs to be done. security labels Feb 15, 2019

rabbah requested a review from cbickel February 15, 2019 02:27

rabbah force-pushed the keys branch 2 times, most recently from 92cf2ec to 33f4ae8 Compare February 15, 2019 02:33

rabbah force-pushed the keys branch 3 times, most recently from fc3305f to b5daa48 Compare February 17, 2019 02:20

dubee reviewed Feb 19, 2019

View reviewed changes

mdeuser reviewed Feb 19, 2019

View reviewed changes

jthomas mentioned this pull request Feb 20, 2019

Document annotation parameter need to access API key apache/openwhisk-client-js#146

Open

jasonpet mentioned this pull request Feb 21, 2019

Use authKey from action parameters instead of __OW_API_KEY apache/openwhisk-package-kafka#320

Closed

mhenke1 reviewed Feb 21, 2019

View reviewed changes

dubee mentioned this pull request Feb 22, 2019

Use passed API key to create trigger instead of ENV variable apache/openwhisk-package-kafka#321

Merged

rabbah added 3 commits February 26, 2019 08:05

Factor out some annotation names to WhiskAction singleton.

3738fd1

Omit API key unless requested explicitly.

f7a49cd

Add an annotation provide-api-key which causes an API key to be passe…

12cf462

…d to the action context.

rabbah added 6 commits February 26, 2019 08:05

Modify Parameters.isTruthy to allow for caller to specify how it want…

c77d1cf

…s to treat a missing key. Default is missing key -> false (preserving behavior). Add test for isTruthy.

Treat a missing provide-key-annotation as truthy annotation.

7adbee5

Actions that already exist without the annotation will receive the key as they might already expect.

Add container proxy test.

4d89ac4

Update tests for new system annotation.

013eb47

Allow pre-existing actions to be exempt from new system annotations.

e7dce59

Update docs.

4ef54fe

rabbah force-pushed the keys branch from b5daa48 to 4ef54fe Compare February 26, 2019 13:38

dubee reviewed Mar 5, 2019

View reviewed changes

dubee self-assigned this Mar 5, 2019

dubee approved these changes Mar 5, 2019

View reviewed changes

dubee mentioned this pull request Mar 6, 2019

Add API key annotation to tests that rely on Node.js client apache/openwhisk-package-kafka#323

Merged

rabbah added ready and removed review Review for this PR has been requested and yet needs to be done. labels Mar 7, 2019

dubee merged commit fb0bab6 into apache:master Mar 8, 2019

dubee mentioned this pull request Mar 8, 2019

Fix broken tests around API key changes apache/openwhisk-cli#420

Merged

chetanmeh mentioned this pull request Mar 12, 2019

Feature flag support to turn on/off support for provide-api-key annotation #4334

Merged

21 tasks

tardieu mentioned this pull request Mar 15, 2019

Synthesize provide-api-key annotation apache/openwhisk-composer#31

Merged

rabbah deleted the keys branch March 17, 2019 19:52

Add an annotations to inject the API key into the action context. #4284

Add an annotations to inject the API key into the action context. #4284

Uh oh!

Conversation

rabbah commented Feb 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related issue and scope

My changes affect the following components

Types of changes

Checklist:

Uh oh!

rabbah commented Feb 13, 2019

Uh oh!

Uh oh!

rabbah commented Feb 15, 2019

Uh oh!

codecov-io commented Feb 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

dubee Feb 19, 2019

Choose a reason for hiding this comment

Uh oh!

mhenke1 Feb 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dubee Feb 19, 2019

Choose a reason for hiding this comment

Uh oh!

rabbah Feb 26, 2019

Choose a reason for hiding this comment

Uh oh!

mdeuser Feb 19, 2019

Choose a reason for hiding this comment

Uh oh!

mhenke1 Feb 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rabbah Feb 21, 2019

Choose a reason for hiding this comment

Uh oh!

mhenke1 Feb 26, 2019

Choose a reason for hiding this comment

Uh oh!

rabbah Mar 5, 2019

Choose a reason for hiding this comment

Uh oh!

dubee left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

rabbah commented Feb 13, 2019 •

edited

Loading

codecov-io commented Feb 17, 2019 •

edited

Loading

mhenke1 Feb 21, 2019 •

edited

Loading

mhenke1 Feb 21, 2019 •

edited

Loading