Adding payload codec pipeline #224

calum-stripe · 2023-03-21T01:12:59Z

Overview

Adds the payload codec pipeline described here https://docs.temporal.io/security#payload-codec.

This adds a new configuration option called payload_codec which always runs after the payload has been serialized (and before deserialization) when sent to (and received from) the server.

Currently, all payloads go through the data converter but only non search attributes payloads should go through payload codecs (otherwise if the payload is transformed into something the server can not read e.g. compressed or encrypted, then the server will reject it because it can not insert non datetime strings for example into the es index)

Test Plan

# unit test
bundle exec rspec spec/

# integration test
cd examples
(terminal 1) ./bin/worker
(terminal 2) USE_ENCRYPTION=1 ./bin/worker
(terminal 3) USE_ERROR_SERIALIZATION_V2=1 ./bin/worker
(terminal 4) bundle exec rspec spec/integration

…nal-with-start Change how signal_with_start is exposed

…nal-with-start Update method signature in temporal test fixture

…nal-with-start Update our FailWorkflowTask logic's call to ErrorHandler.handle

…e fiber

…ner/woflo-482 Workflow await condition in context wait_for

…ntTarget objects

…ner/woflo-482-fix Fix EventTarget comparison with wildcard

…ner/m1-fixes Set Ruby 2.7 to be consistent with pay-server

…ins_schedule_to_start Turn off schedule_to_start activity timeout by default

…ral-ruby into upgrade-latest

…oner-WOFLO-1235-timeout-history Treat missing history submessage when getting workflow history as timeout

…kins_schedule Dynamic Workflows

…oner-WOFLO-1216 Support terminate-if-running workflow ID reuse policy

…upstream

…oner-latest-upstream Merge latest Coinbase upstream

bergundy · 2023-03-21T03:04:39Z

In the official SDKs search attributes do not go through the user configurable data converter, might want to do the same here.

calum-stripe · 2023-03-21T14:56:06Z

In the official SDKs search attributes do not go through the user configurable data converter, might want to do the same here.

this is true, I wonder if anyone has built a data converter for their search attributes though that by removing this and just using JSON would break their existing setup? Should we bump the version just incase? @bergundy

jeffschoner-stripe

Is it possible to update the existing encryption test to use this new pipeline? I think it's better suited to your solution rather than switching out the converter like it does today.

jeffschoner-stripe · 2023-03-22T19:58:06Z

lib/temporal/concerns/payloads.rb


+      def from_search_attribute_payload(payload_map)
+        # skips the payload_codec step because search attributes don't use this pipeline
+        payload_map.map { |key, value| [key, payload_converter.from_payload(value)] }.to_h


The abstraction is seeping through here a bit. Should this be named something like from_payload_map_without_codec? The call on search attributes would then be made outside of this file which is specific to performing various kinds of encoding.

jeffschoner-stripe · 2023-03-22T19:59:58Z

lib/temporal/concerns/payloads.rb

        Temporal.configuration.converter
      end
+
+      def payload_codec


A comment at the top of this file or on this method explaining the difference between payload converters and payload codecs would be worthwhile. Or actually probably in configuration.rb where a user would see it.

added to the configuration.rb, let me know what you think

jeffschoner-stripe · 2023-03-22T20:02:39Z

lib/temporal/connection/converter/codec/base.rb

+          def encodes(payloads)
+            return nil if payloads.nil?
+
+            Temporalio::Api::Common::V1::Payloads.new(
+              payloads: payloads.payloads.map(&method(:encode))
+            )
+          end
+
+          def decodes(payloads)
+            return nil if payloads.nil?
+
+            Temporalio::Api::Common::V1::Payloads.new(
+              payloads: payloads.payloads.map(&method(:decode))
+            )
+          end


Would these be overridden by someone extending this class? Why not move these into payloads.rb?

they can be, I just was following the same pattern as in payloads.rb which seems to be define the same thing (from_payloads and to_payloads with logic then from_payload and to_payload are empty and required)

You probably are going to only override encode or decode but you have the option to override all for if the use case needs it.

jeffschoner-stripe · 2023-03-22T20:06:23Z

lib/temporal/concerns/payloads.rb

+        payload_codec.encode(payload)
+      end
+
+      def to_search_attribute_payload(data)


same thing here in the other direction

jeffschoner-stripe · 2023-03-22T20:08:01Z

lib/temporal/connection/converter/codec/chain.rb

+        # the codecs are applied last to first meaning the earlier encodings wrap the later ones.
+        # When decoding, the codecs are applied first to last to reverse the effect.
+        class Chain < Base
+          def initialize(payload_codecs:)


nit: This might be more ergonomic with a splat or non-keyword argument instead

i agree, i was just keeping the same pattern as

temporal-ruby/lib/temporal/connection/converter/composite.rb

Line 10 in 378630a

def initialize(payload_converters:)

, what do you think?

jeffschoner-stripe · 2023-03-22T20:09:35Z

spec/unit/lib/temporal/connection/converter/codec/base_spec.rb

@@ -0,0 +1,76 @@
+require 'temporal/connection/converter/codec/chain'
+
+describe Temporal::Connection::Converter::Codec::Base do


Testing the base class seems strange to me since no one is expected to use this directly. It will always be extended, right? This might be a further indication that these non-abstract methods should simply be moved into payloads.rb instead.

hmm, i don't disagree, just was following the pattern that was already in place for from_payloads and to_payloads. What do you think? Im easy either way.

if we do remove, ill remove the unit tests

This reverts commit 145290c, reversing changes made to d8a42d8.

nagl-stripe and others added 30 commits October 27, 2021 10:54

Revert signal_with_start (before taking a different approach)

d8a5df7

Add signal arguments to start_workflow (to support signal_with_start)

06626ad

Merge memo changes

ac66062

Address PR feedback

0aec214

Merge pull request coinbase#46 from stripe-private-oss-forks/nagl-sig…

50ab4da

…nal-with-start Change how signal_with_start is exposed

Update method signature in temporal test fixture

be0510c

Add detail to a few error messages

bbc6d89

Merge pull request coinbase#48 from stripe-private-oss-forks/nagl-sig…

73120be

…nal-with-start Update method signature in temporal test fixture

Update our FailWorkflowTask logic's call to ErrorHandler.handle

22006e3

Merge pull request coinbase#49 from stripe-private-oss-forks/nagl-sig…

6e9624e

…nal-with-start Update our FailWorkflowTask logic's call to ErrorHandler.handle

Workflow await

55253af

Make dispatch more generic

399f9b8

Fix race condition

b461af9

Merge await into wait_for

c76042a

Update sample workflow to use wait_for, rename to WaitForWorkflow

7310062

Reorganize and extend wait_for tests

256f472

Check for completed futures before setting dispatcher and yielding th…

9c776c5

…e fiber

Extend wait_for to take multiple futures and a condition block

3c5b77a

Merge pull request coinbase#47 from stripe-private-oss-forks/jeffscho…

dc74dae

…ner/woflo-482 Workflow await condition in context wait_for

Differentiate TARGET_WILDCARD and WILDCARD, allow comparison with Eve…

51f6c9b

…ntTarget objects

Merge pull request coinbase#50 from stripe-private-oss-forks/jeffscho…

a280807

…ner/woflo-482-fix Fix EventTarget comparison with wildcard

Use Ruby 2.7 to be consistent with pay-server

7290de3

Merge pull request coinbase#51 from stripe-private-oss-forks/jeffscho…

68de5bf

…ner/m1-fixes Set Ruby 2.7 to be consistent with pay-server

Turn off schedule_to_start activity timeout by default

acd5220

Merge pull request coinbase#52 from stripe-private-oss-forks/drewhosk…

62137ba

…ins_schedule_to_start Turn off schedule_to_start activity timeout by default

Refactor metadata generation

0bc8a62

Make task queue available on workflow metadata, add example test

391cd1f

Expose workflow start time metadata

a16f856

Add memos

36b45a6

Merge branch 'memos' from https://github.com/jeffschoner-stripe/tempo…

a7ca88a

…ral-ruby into upgrade-latest

jeffschoner-stripe and others added 16 commits February 17, 2023 13:42

Merge pull request coinbase#123 from stripe-private-oss-forks/jeffsch…

015bc07

…oner-WOFLO-1235-timeout-history Treat missing history submessage when getting workflow history as timeout

Dynamic Workflows

153a877

Add terminate-if-running workflow id reuse policy mappings

08e4ef1

Add integration test for terminate-if-running

bf3be07

Move misplaced test

9ef5d82

Feedback

2cd974e

Merge pull request coinbase#124 from stripe-private-oss-forks/drewhos…

154a068

…kins_schedule Dynamic Workflows

Use terminate-if-running as policy for both invocations

77ab242

Merge pull request coinbase#125 from stripe-private-oss-forks/jeffsch…

6297ac0

…oner-WOFLO-1216 Support terminate-if-running workflow ID reuse policy

Merge remote-tracking branch 'origin/master' into jeffschoner-latest-…

58e17da

…upstream

Merge pull request coinbase#126 from stripe-private-oss-forks/jeffsch…

5f2e396

…oner-latest-upstream Merge latest Coinbase upstream

added payload codecs

9e8fadb

added search attribute payload methods

47556d8

fixed tests

8eaf5e7

added codec tests

cea8ad2

added chain tests

9eec6af

jeffschoner-stripe reviewed Mar 22, 2023

View reviewed changes

calum-stripe added 7 commits March 22, 2023 21:46

updated nits

d8a42d8

Merge branch 'master' into calum/adding-payload-codecs

145290c

Revert "Merge branch 'master' into calum/adding-payload-codecs"

6ca7458

This reverts commit 145290c, reversing changes made to d8a42d8.

Merge branch 'public-master' into calum/adding-payload-codecs

1d90708

fixed tests

2d6a92b

fixed

5f02284

examples/lib/crypt_payload_codec.rb

1aa6ba1

jeffschoner-stripe approved these changes Apr 6, 2023

View reviewed changes

DeRauk approved these changes Apr 7, 2023

View reviewed changes

DeRauk merged commit 21252c4 into coinbase:master Apr 7, 2023

jeffschoner mentioned this pull request Apr 11, 2023

Fix broken encryption specs, update Temporal API proto to 1.20 #233

Merged

		@@ -0,0 +1,76 @@
		require 'temporal/connection/converter/codec/chain'

		describe Temporal::Connection::Converter::Codec::Base do

Adding payload codec pipeline #224

Adding payload codec pipeline #224

Uh oh!

Conversation

calum-stripe commented Mar 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

Test Plan

Uh oh!

bergundy commented Mar 21, 2023

Uh oh!

calum-stripe commented Mar 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jeffschoner-stripe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

11 participants

calum-stripe commented Mar 21, 2023 •

edited

Loading

calum-stripe commented Mar 21, 2023 •

edited

Loading