chamikaramj commented on a change in pull request #15690:
URL: https://github.com/apache/beam/pull/15690#discussion_r728524427
##########
File path: sdks/python/apache_beam/io/external/generate_sequence_test.py
##########
@@ -54,6 +57,44 @@ def test_generate_sequence(self):
else:
raise e
+ @unittest.skipUnless(
+ os.environ.get('EXPANSION_SERVICE_TYPE') != 'Python',
+ 'Java Class Lookup based expansion is not supported by the Python '
+ 'expansion service')
+ def test_generate_sequence_java_class_lookup_payload_builder(self):
+ port = os.environ.get('EXPANSION_PORT')
+ address = 'localhost:%s' % port
+
+ with TestPipeline() as p:
+ payload_builder = JavaClassLookupPayloadBuilder(
+ 'org.apache.beam.sdk.io.GenerateSequence')
+ payload_builder.with_constructor_method('from', 1)
+ payload_builder.add_builder_method('to', 10)
+
+ res = (
+ p
+ | ExternalTransform(None, payload_builder,
expansion_service=address))
+ assert_that(res, equal_to([i for i in range(1, 10)]))
+
+ @unittest.skipUnless(
+ os.environ.get('EXPANSION_SERVICE_TYPE') != 'Python',
+ 'Java Class Lookup based expansion is not supported by the Python '
+ 'expansion service')
+ def test_generate_sequence_java_external_transform(self):
+ port = os.environ.get('EXPANSION_PORT')
+ address = 'localhost:%s' % port
+
+ with TestPipeline() as p:
+ java_transform = JavaExternalTransform(
+ 'org.apache.beam.sdk.io.GenerateSequence', expansion_service=address)
+ # We have to use 'getattr' below since 'from' is a reserved keyword for
+ # Python.
+ getattr(java_transform, 'from')(1)
+ java_transform.to(10)
Review comment:
Seems like latter is not supported yet so changed to former.
##########
File path: sdks/python/apache_beam/io/external/generate_sequence_test.py
##########
@@ -54,6 +57,44 @@ def test_generate_sequence(self):
else:
raise e
+ @unittest.skipUnless(
+ os.environ.get('EXPANSION_SERVICE_TYPE') != 'Python',
Review comment:
I understand it's a bit convoluted but I would like to test to break if
we drop/change EXPANSION_SERVICE_TYPE "Java" in configs instead of the test
suite silently ignoring this test. Added a comment.
##########
File path: sdks/python/apache_beam/io/external/generate_sequence_test.py
##########
@@ -54,6 +57,44 @@ def test_generate_sequence(self):
else:
raise e
+ @unittest.skipUnless(
+ os.environ.get('EXPANSION_SERVICE_TYPE') != 'Python',
+ 'Java Class Lookup based expansion is not supported by the Python '
+ 'expansion service')
+ def test_generate_sequence_java_class_lookup_payload_builder(self):
+ port = os.environ.get('EXPANSION_PORT')
+ address = 'localhost:%s' % port
+
+ with TestPipeline() as p:
+ payload_builder = JavaClassLookupPayloadBuilder(
+ 'org.apache.beam.sdk.io.GenerateSequence')
+ payload_builder.with_constructor_method('from', 1)
+ payload_builder.add_builder_method('to', 10)
+
+ res = (
+ p
+ | ExternalTransform(None, payload_builder,
expansion_service=address))
+ assert_that(res, equal_to([i for i in range(1, 10)]))
+
+ @unittest.skipUnless(
+ os.environ.get('EXPANSION_SERVICE_TYPE') != 'Python',
+ 'Java Class Lookup based expansion is not supported by the Python '
+ 'expansion service')
+ def test_generate_sequence_java_external_transform(self):
+ port = os.environ.get('EXPANSION_PORT')
+ address = 'localhost:%s' % port
+
+ with TestPipeline() as p:
+ java_transform = JavaExternalTransform(
+ 'org.apache.beam.sdk.io.GenerateSequence', expansion_service=address)
+ # We have to use 'getattr' below since 'from' is a reserved keyword for
+ # Python.
+ getattr(java_transform, 'from')(1)
+ java_transform.to(10)
+ res = (p | java_transform)
+
+ assert_that(res, equal_to([i for i in range(1, 10)]))
Review comment:
Done.
##########
File path:
sdks/java/testing/expansion-service/src/test/resources/test_expansion_service_allowlist.yaml
##########
@@ -0,0 +1,26 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+#
+
+version: v1
+allowedClasses:
Review comment:
The allowlist was added since there were significant security concerns
regarding allowing Python to invoke arbitrary Java classes and methods.
See below for context.
https://lists.apache.org/thread.html/ra7d663a619485a45727b29c21eb3334849cfdfb034ef2b1a1d69824a%40%3Cdev.beam.apache.org%3E
I think even with a "* allow list" the security risk will be there since I'm
afraid most users will end up using this due to being the easiest way to
configure.
Also, this main reason for adding Java Class Lookup based expansion was
allowing Python users to use Java cross-language transforms without writing new
Java code. This is still the case even with this allowlist. Also I think this
"yaml" based config file is easy enough for most uses to write and specify when
starting the expansion service (users should already know Java class and
builder/constructor method names since they are required when using the Python
API). I'm also hoping to document this in the Beam docs.
cc: @lukecwik
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
