You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@openwhisk.apache.org by GitBox <gi...@apache.org> on 2018/02/21 02:52:10 UTC
[GitHub] csantanapr closed pull request #3298: Attempt to resolve action name from conductor to a fully qualified name for added convenience.
csantanapr closed pull request #3298: Attempt to resolve action name from conductor to a fully qualified name for added convenience.
URL: https://github.com/apache/incubator-openwhisk/pull/3298
This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:
As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):
diff --git a/common/scala/src/main/scala/whisk/core/entity/FullyQualifiedEntityName.scala b/common/scala/src/main/scala/whisk/core/entity/FullyQualifiedEntityName.scala
index c52af75284..0abca406e2 100644
--- a/common/scala/src/main/scala/whisk/core/entity/FullyQualifiedEntityName.scala
+++ b/common/scala/src/main/scala/whisk/core/entity/FullyQualifiedEntityName.scala
@@ -96,4 +96,36 @@ protected[core] object FullyQualifiedEntityName extends DefaultJsonProtocol {
case Failure(t) => deserializationError("fully qualified name malformed")
}
}
+
+ /**
+ * Converts the name to a fully qualified name.
+ * There are 3 cases:
+ * - name is not a valid EntityPath => error
+ * - name is a valid single segment with a leading slash => error
+ * - name is a valid single segment without a leading slash => map it to user namespace, default package
+ * - name is a valid multi segment with a leading slash => treat it as fully qualified name (max segments allowed: 3)
+ * - name is a valid multi segment without a leading slash => treat it as package name and resolve it to the user namespace (max segments allowed: 3)
+ *
+ * The last case is ambiguous as '/namespace/action' and 'package/action' will be the same EntityPath value.
+ * The action should use a fully qualified result to avoid the ambiguity.
+ *
+ * @param name name of the action to fully qualify
+ * @param namespace the user namespace for the simple resolution
+ * @return Some(FullyQualifiedName) if the name is valid otherwise None
+ */
+ protected[core] def resolveName(name: JsValue, namespace: EntityName): Option[FullyQualifiedEntityName] = {
+ name match {
+ case v @ JsString(s) =>
+ Try(v.convertTo[EntityPath]).toOption
+ .flatMap { path =>
+ val n = path.segments
+ val leadingSlash = s.startsWith(EntityPath.PATHSEP)
+ if (n < 1 || n > 3 || (leadingSlash && n == 1) || (!leadingSlash && n > 3)) None
+ else if (leadingSlash || n == 3) Some(path)
+ else Some(namespace.toPath.addPath(path))
+ }
+ .map(_.toFullyQualifiedEntityName)
+ case _ => None
+ }
+ }
}
diff --git a/common/scala/src/main/scala/whisk/http/ErrorResponse.scala b/common/scala/src/main/scala/whisk/http/ErrorResponse.scala
index f161618fe1..162fda651f 100644
--- a/common/scala/src/main/scala/whisk/http/ErrorResponse.scala
+++ b/common/scala/src/main/scala/whisk/http/ErrorResponse.scala
@@ -124,9 +124,9 @@ object Messages {
def compositionComponentInvalid(value: JsValue) =
s"Failed to parse action name from json value $value during composition."
def compositionComponentNotFound(name: String) =
- s"""Failed to resolve action with name "$name" during composition."""
+ s"Failed to resolve action with name '$name' during composition."
def compositionComponentNotAccessible(name: String) =
- s"""Failed entitlement check for action with name "$name" during composition."""
+ s"Failed entitlement check for action with name '$name' during composition."
/** Error messages for bad requests where parameters do not conform. */
val parametersNotAllowed = "Request defines parameters that are not allowed (e.g., reserved properties)."
diff --git a/core/controller/src/main/scala/whisk/core/controller/actions/PrimitiveActions.scala b/core/controller/src/main/scala/whisk/core/controller/actions/PrimitiveActions.scala
index 2006a6b7b6..24817f3d69 100644
--- a/core/controller/src/main/scala/whisk/core/controller/actions/PrimitiveActions.scala
+++ b/core/controller/src/main/scala/whisk/core/controller/actions/PrimitiveActions.scala
@@ -28,7 +28,6 @@ import scala.concurrent.duration._
import scala.language.postfixOps
import scala.util.Failure
import scala.util.Success
-import scala.util.Try
import akka.actor.Actor
import akka.actor.ActorRef
import akka.actor.ActorSystem
@@ -355,41 +354,15 @@ protected[actions] trait PrimitiveActions {
// no next action, end composition execution, return to caller
Future.successful(ActivationResponse(activation.response.statusCode, Some(params.getOrElse(result))))
case Some(next) =>
- Try(next.convertTo[EntityPath]) match {
- case Failure(t) =>
- // parsing failure
- Future.successful(ActivationResponse.applicationError(compositionComponentInvalid(next)))
- case Success(_) if session.accounting.components >= actionSequenceLimit =>
- // composition is too long
+ FullyQualifiedEntityName.resolveName(next, user.namespace) match {
+ case Some(fqn) if session.accounting.components < actionSequenceLimit =>
+ tryInvokeNext(user, fqn, params, session)
+
+ case Some(_) => // composition is too long
Future.successful(ActivationResponse.applicationError(compositionIsTooLong))
- case Success(next) =>
- // resolve and invoke next action
- val fqn = (if (next.defaultPackage) EntityPath.DEFAULT.addPath(next) else next)
- .resolveNamespace(user.namespace)
- .toFullyQualifiedEntityName
- val resource = Resource(fqn.path, Collection(Collection.ACTIONS), Some(fqn.name.asString))
- entitlementProvider
- .check(user, Privilege.ACTIVATE, Set(resource), noThrottle = true)
- .flatMap { _ =>
- // successful entitlement check
- WhiskActionMetaData
- .resolveActionAndMergeParameters(entityStore, fqn)
- .flatMap {
- case next =>
- // successful resolution
- invokeComponent(user, action = next, payload = params, session)
- }
- .recover {
- case _ =>
- // resolution failure
- ActivationResponse.applicationError(compositionComponentNotFound(next.asString))
- }
- }
- .recover {
- case _ =>
- // failed entitlement check
- ActivationResponse.applicationError(compositionComponentNotAccessible(next.asString))
- }
+
+ case None => // parsing failure
+ Future.successful(ActivationResponse.applicationError(compositionComponentInvalid(next)))
}
}
}
@@ -397,6 +370,42 @@ protected[actions] trait PrimitiveActions {
}
+ /**
+ * Checks if the user is entitled to invoke the next action and invokes it.
+ *
+ * @param user the subject
+ * @param fqn the name of the action
+ * @param params parameters for the action
+ * @param session the session for the current activation
+ * @return promise for the eventual activation
+ */
+ private def tryInvokeNext(user: Identity, fqn: FullyQualifiedEntityName, params: Option[JsObject], session: Session)(
+ implicit transid: TransactionId): Future[ActivationResponse] = {
+ val resource = Resource(fqn.path, Collection(Collection.ACTIONS), Some(fqn.name.asString))
+ entitlementProvider
+ .check(user, Privilege.ACTIVATE, Set(resource), noThrottle = true)
+ .flatMap { _ =>
+ // successful entitlement check
+ WhiskActionMetaData
+ .resolveActionAndMergeParameters(entityStore, fqn)
+ .flatMap {
+ case next =>
+ // successful resolution
+ invokeComponent(user, action = next, payload = params, session)
+ }
+ .recover {
+ case _ =>
+ // resolution failure
+ ActivationResponse.applicationError(compositionComponentNotFound(fqn.asString))
+ }
+ }
+ .recover {
+ case _ =>
+ // failed entitlement check
+ ActivationResponse.applicationError(compositionComponentNotAccessible(fqn.asString))
+ }
+ }
+
/**
* A method that knows how to handle a component invocation.
*
diff --git a/docs/conductors.md b/docs/conductors.md
index 8ab888f1bf..895efa705b 100644
--- a/docs/conductors.md
+++ b/docs/conductors.md
@@ -235,6 +235,7 @@ If the _action_ field is defined in the output of the conductor action, the runt
- internal error (invocation failure or timeout).
In any of the first three failure scenarios, the conductor action invocation ends with an _application error_ status code and an error message describing the reason for the failure. In the latter, the status code is _internal error_.
+The action name should be a fully qualified name, which is of the form `/namespace/package-name/action-name` or `/namespace/action-name`. Failure to specify a fully qualified name may result in ambiguity or even a parsing error that terminates the activation.
If there is no error, _action_ is invoked on the _params_ dictionary if specified (auto boxed if necessary) or if not on the empty dictionary.
diff --git a/tests/src/test/scala/system/basic/WskBasicNode6Tests.scala b/tests/src/test/scala/system/basic/WskBasicNode6Tests.scala
index a97636f701..f4a04e3a2e 100644
--- a/tests/src/test/scala/system/basic/WskBasicNode6Tests.scala
+++ b/tests/src/test/scala/system/basic/WskBasicNode6Tests.scala
@@ -37,7 +37,7 @@ abstract class WskBasicNode6Tests extends TestHelpers with WskTestHelpers with J
val defaultAction = Some(TestUtils.getTestActionFilename("hello.js"))
lazy val currentNodeJsKind = "nodejs:6"
- behavior of "Runtime $currentNodeJsKind"
+ behavior of s"Runtime $currentNodeJsKind"
it should "Ensure that NodeJS actions can have a non-default entrypoint" in withAssetCleaner(wskprops) {
(wp, assetHelper) =>
diff --git a/tests/src/test/scala/system/basic/WskConductorTests.scala b/tests/src/test/scala/system/basic/WskConductorTests.scala
index 2ff00ffebd..7115104462 100644
--- a/tests/src/test/scala/system/basic/WskConductorTests.scala
+++ b/tests/src/test/scala/system/basic/WskConductorTests.scala
@@ -121,10 +121,12 @@ abstract class WskConductorTests extends TestHelpers with WskTestHelpers with Js
// an undefined action
val undefinedrun = wsk.action.invoke(echo, Map("payload" -> testString.toJson, "action" -> missing.toJson))
+ val namespace = wsk.namespace.whois()
+
withActivation(wsk.activation, undefinedrun) { activation =>
activation.response.status shouldBe "application error"
activation.response.result.get.fields.get("error") shouldBe Some(
- JsString(compositionComponentNotFound(missing)))
+ JsString(compositionComponentNotFound(s"$namespace/$missing")))
checkConductorLogsAndAnnotations(activation, 1) // echo
}
}
diff --git a/tests/src/test/scala/whisk/core/controller/test/ConductorsApiTests.scala b/tests/src/test/scala/whisk/core/controller/test/ConductorsApiTests.scala
index f08d70026b..bacd9124e4 100644
--- a/tests/src/test/scala/whisk/core/controller/test/ConductorsApiTests.scala
+++ b/tests/src/test/scala/whisk/core/controller/test/ConductorsApiTests.scala
@@ -127,7 +127,7 @@ class ConductorsApiTests extends ControllerTestCommon with WhiskActionsApi {
val response = responseAs[JsObject]
response.fields("response").asJsObject.fields("status") shouldBe "application error".toJson
response.fields("response").asJsObject.fields("result") shouldBe JsObject(
- "error" -> compositionComponentNotFound(missing.toString).toJson)
+ "error" -> compositionComponentNotFound(s"$namespace/$missing").toJson)
response.fields("logs").convertTo[JsArray].elements.size shouldBe 1
}
}
@@ -137,7 +137,7 @@ class ConductorsApiTests extends ControllerTestCommon with WhiskActionsApi {
put(entityStore, WhiskAction(namespace, conductor, jsDefault("??"), annotations = Parameters("conductor", "true")))
put(entityStore, WhiskAction(namespace, step, jsDefault("??")))
put(entityStore, WhiskAction(alternateNamespace, step, jsDefault("??"))) // forbidden action
- val forbidden = s"$alternateNamespace/$step" // forbidden action name
+ val forbidden = s"/$alternateNamespace/$step" // forbidden action name
// dynamically invoke step action
Post(
@@ -179,7 +179,7 @@ class ConductorsApiTests extends ControllerTestCommon with WhiskActionsApi {
val response = responseAs[JsObject]
response.fields("response").asJsObject.fields("status") shouldBe "application error".toJson
response.fields("response").asJsObject.fields("result") shouldBe JsObject(
- "error" -> compositionComponentNotAccessible(forbidden).toJson)
+ "error" -> compositionComponentNotAccessible(forbidden.drop(1)).toJson)
response.fields("logs").convertTo[JsArray].elements.size shouldBe 1
}
diff --git a/tests/src/test/scala/whisk/core/entity/test/SchemaTests.scala b/tests/src/test/scala/whisk/core/entity/test/SchemaTests.scala
index 18bea38ce5..b7bfb14c52 100644
--- a/tests/src/test/scala/whisk/core/entity/test/SchemaTests.scala
+++ b/tests/src/test/scala/whisk/core/entity/test/SchemaTests.scala
@@ -322,6 +322,31 @@ class SchemaTests extends FlatSpec with BeforeAndAfter with ExecHelpers with Mat
a[DeserializationException] should be thrownBy FullyQualifiedEntityName.serdesAsDocId.read(names(6))
}
+ it should "resolve names that may or may not be fully qualified" in {
+ FullyQualifiedEntityName.resolveName(JsString("a"), EntityName("ns")) shouldBe Some(
+ EntityPath("ns/a").toFullyQualifiedEntityName)
+
+ FullyQualifiedEntityName.resolveName(JsString("a/b"), EntityName("ns")) shouldBe Some(
+ EntityPath("ns/a/b").toFullyQualifiedEntityName)
+
+ FullyQualifiedEntityName.resolveName(JsString("a/b/c"), EntityName("ns")) shouldBe Some(
+ EntityPath("/a/b/c").toFullyQualifiedEntityName)
+
+ FullyQualifiedEntityName.resolveName(JsString("a/b/c/d"), EntityName("ns")) shouldBe None
+
+ FullyQualifiedEntityName.resolveName(JsString("/a"), EntityName("ns")) shouldBe None
+
+ FullyQualifiedEntityName.resolveName(JsString("/a/b"), EntityName("ns")) shouldBe Some(
+ EntityPath("/a/b").toFullyQualifiedEntityName)
+
+ FullyQualifiedEntityName.resolveName(JsString("/a/b/c"), EntityName("ns")) shouldBe Some(
+ EntityPath("/a/b/c").toFullyQualifiedEntityName)
+
+ FullyQualifiedEntityName.resolveName(JsString("/a/b/c/d"), EntityName("ns")) shouldBe None
+
+ FullyQualifiedEntityName.resolveName(JsString(""), EntityName("ns")) shouldBe None
+ }
+
behavior of "Binding"
it should "desiarilize legacy format" in {
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services