Adding middleware to inject Auth token for internal requests to frontend #4364

iamrodrigo · 2021-08-14T08:15:20Z

What changed?

Why?
For https://docs.google.com/document/d/1a-Xo9FSCLubYlIQHB_eX5j4Yv2GDv_82J94At2dAL3o/edit#

How did you test it?
Local test:

docker-compose -f docker/dev/cassandra-esv7-kafka.yml up

and start server with

./cadence-server --zone oauth start

then register a domain and start two workflows

$cadence --do sample d re
Domain sample successfully registered.

then use batch to terminate the two workflows

qlong@~/cadence:
(adding-middleware-for-frontend) $cadence --do sample wf start --wt wftype --et 100 --tl tasklist
Started Workflow Id: d202b1ef-69ec-4025-ac05-3b24ff58e7b9, run Id: 3d2a7f80-e046-4ba7-ad8b-4ae86c6033bf
qlong@~/cadence:
(adding-middleware-for-frontend) $cadence --do sample wf start --wt wftype --et 100
qlong@~/cadence:
(adding-middleware-for-frontend) $cadence --do sample wf start --wt wftype --et 100000 --tl abc
Started Workflow Id: 8873df2a-4bb7-4ff5-aa63-be089ddafbca, run Id: da1757bb-19bb-4e8e-86a0-3f415ca6a111

qlong@~/cadence:
(adding-middleware-for-frontend) $cadence --do sample wf list --op
  WORKFLOW TYPE |             WORKFLOW ID              |                RUN ID                | TASK LIST | START TIME | EXECUTION TIME
  wftype        | 8873df2a-4bb7-4ff5-aa63-be089ddafbca | da1757bb-19bb-4e8e-86a0-3f415ca6a111 | abc       | 14:50:47   | 14:50:47
  wftype        | d202b1ef-69ec-4025-ac05-3b24ff58e7b9 | 3d2a7f80-e046-4ba7-ad8b-4ae86c6033bf | tasklist  | 14:50:30   | 14:50:30

qlong@~/cadence:
(adding-middleware-for-frontend) $cadence --do sample wf batch start -q "WorkflowType='wftype'" --re test --bt terminate
This batch job will be operating on 2 workflows.
Please confirm[Yes/No]:yes
{
  "jobID": "da1b9b01-790f-4051-8ccc-8aa360aed5ee",
  "msg": "batch job is started"
}

and confirm it succeeded

qlong@~/cadence:
(adding-middleware-for-frontend) $cadence --do sample wf batch desc --jid da1b9b01-790f-4051-8ccc-8aa360aed5ee
{
  "msg": "batch job is finished successfully"
}

Potential risks

Release notes

Documentation Changes

common/config/config.go

longquanzheng · 2021-08-15T19:07:12Z

cmd/server/cadence/server.go

@@ -220,8 +221,13 @@ func (s *server) startService() common.Daemon {
 		}
 	}

-	dispatcher, err := params.DispatcherProvider.Get(common.FrontendServiceName, s.cfg.PublicClient.HostPort)
+	// will return empty array if not enabled
+	privateKey, err := s.cfg.Authorization.OAuthAuthorizer.GetPrivateKey()


like the comment, use clusterMetadata.clusterInfomation..AuthorizationProvider

You may need to create a new wrapper to create AuthProvider based on the config as it will be used in clientBean.go

client/clientBean.go

iamrodrigo · 2021-08-17T08:59:33Z

go.mod

@@ -91,3 +91,5 @@ replace github.com/apache/thrift => github.com/apache/thrift v0.0.0-201612212036

 // until new version is released we need to pick up https://github.com/yarpc/yarpc-go/pull/2047
 replace go.uber.org/yarpc => go.uber.org/yarpc v1.52.1-0.20210303193224-b2caa40d56b6
+
+replace go.uber.org/cadence v0.17.1-0.20210806184645-7c70757e7c7f => /home/rv/go/src/go.uber.org/cadence


AuthorizationProvider is currently internal interface. I have a diff to expose it, in the meantime I'm pointing to local. Reminder to myself to delete this before final review

common/config/config.go

iamrodrigo · 2021-08-17T09:03:44Z

client/clientBean.go

@@ -256,7 +273,7 @@ func NewDNSYarpcDispatcherProvider(logger log.Logger, interval time.Duration) Di
 	}
 }

-func (p *dnsDispatcherProvider) Get(serviceName string, address string) (*yarpc.Dispatcher, error) {
+func (p *dnsDispatcherProvider) Get(serviceName string, address string, options *DispatcherOptions) (*yarpc.Dispatcher, error) {


Do I need to run something to get the latest mock for this? Quoting you @longquanzheng

There are two implementation, one is mock. The real one is dnsYarpcDispatcherProvider.

client/clientBean.go

cmd/server/cadence/server.go

longquanzheng · 2021-08-17T22:04:23Z

Also, don't forget to remove the private key from Authorizer config.
https://github.com/uber/cadence/blob/a58b8b9670417ccbf2cd021882e750f95d6fc508/common/config/config.go#L104

I thought we will use it in this PR but we changed our mind to make it separate(which is much better)

iamrodrigo · 2021-08-18T06:01:21Z

host/xdc/elasticsearch_test.go

@@ -18,8 +18,8 @@
 // OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
 // THE SOFTWARE.

-// +build !race
-// +build esintegration
+//go:build !race && esintegration


These file was modified after I ran make bins, not sure if it will break something. Same with the next modification

Did you try “make fmt”?

I specifically made this new changes in a new commit just in case I need to revert :) I ran make fmt and I got make: Nothing to be done for 'fmt'. I reverted the last commit (removing all this changes) and tried make fmt and got the same message. Should I revert them manually?

Seems like a go 1.17 thing: https://golang.org/doc/go1.17#build-lines

Since we're not officially on 1.17... tbh I'm not sure if we should do this or not. And I'm fairly surprised that go fmt does this since our stated version is 1.13, and older versions of go will not maintain them - if we ever change these build conditions and are not running <1.17, go fmt will ignore the new format, and 1.17+ will use the old (unmodified) conditions because it's now preferred.

Are you referring to https://github.com/uber/cadence/blob/38881a8f996ce3230a77b7acc946438fbc532895/Dockerfile#L7

I think we should change it to 1.16.
@noiarek can you try using 1.16? If you use homebrew, there is a command to switch the go version.

Thanks @Groxx !

I meant https://github.com/uber/cadence/blob/master/go.mod#L3 (actually go 1.12 apparently), but that works too / I imagine we build the tests from that dockerfile? If so, it wouldn't detect out-of-sync format directives without upgrading, yeah.

@Groxx I've installed go 1.16.7 and ran make fmt after it and I'm still getting this modifications. Any advice?

common/config/config.go

longquanzheng · 2021-08-18T18:09:17Z

.gen/proto/api/v1/common.pb.go

-				return ErrInvalidLengthCommon
-			}
-			if (iNdEx + skippy) < 0 {
+			if (skippy < 0) || (iNdEx+skippy) < 0 {


It seems like this is due to some version upgrade in the proto tool, so I believe that's fine.
Cc @Groxx

This shouldn't be affected unless the makefile's versioned proto tool was changed, which this diff doesn't do.

The two branches that are merged are identical though, possibly a newer go fmt -s did this?

…adence into adding-middleware-for-frontend

This reverts commit 36e20d8.

coveralls · 2021-08-31T17:34:53Z

Pull Request Test Coverage Report for Build 86bab5c0-970a-4756-abf3-35a46567c138

18 of 56 (32.14%) changed or added relevant lines in 5 files are covered.
39 unchanged lines in 5 files lost coverage.
Overall coverage increased (+0.005%) to 56.39%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
common/authorization/authorizer.go	0	6	0.0%
cmd/server/cadence/server.go	0	11	0.0%
client/clientBean.go	12	33	36.36%

Files with Coverage Reduction	New Missed Lines	%
service/matching/matcher.go	2	91.46%
common/cache/lru.go	3	90.73%
common/persistence/nosql/nosqlplugin/cassandra/workflow.go	3	50.23%
common/persistence/nosql/nosqlplugin/cassandra/workflowUtils.go	12	77.74%
common/persistence/nosql/nosqlplugin/cassandra/workflowParsingUtils.go	19	87.01%

Totals
Change from base Build 46642510-b649-49f0-9d2a-494c9b9af33a:	0.005%
Covered Lines:	79100
Relevant Lines:	140274

💛 - Coveralls

iamrodrigo added 2 commits August 14, 2021 01:13

New struct added to dispatcher

f477bd1

Merge branch 'master' into adding-middleware-for-frontend

4f920bf

longquanzheng reviewed Aug 15, 2021

View reviewed changes

common/config/config.go Outdated Show resolved Hide resolved

longquanzheng reviewed Aug 15, 2021

View reviewed changes

common/config/config.go Outdated Show resolved Hide resolved

longquanzheng reviewed Aug 15, 2021

View reviewed changes

Adding middleware to dispatcher

348a195

iamrodrigo commented Aug 17, 2021

View reviewed changes

iamrodrigo added 2 commits August 17, 2021 02:12

Fixing test

8b39a8a

Merge branch 'master' into adding-middleware-for-frontend

ce6ac31

longquanzheng reviewed Aug 17, 2021

View reviewed changes

client/clientBean.go Show resolved Hide resolved

longquanzheng reviewed Aug 17, 2021

View reviewed changes

cmd/server/cadence/server.go Outdated Show resolved Hide resolved

iamrodrigo added 3 commits August 17, 2021 22:54

moving auth creation client to common function

60e5ebe

Fixing conflict

dcd8197

files modified after running make bins

2a5d7dc

iamrodrigo commented Aug 18, 2021

View reviewed changes

longquanzheng reviewed Aug 18, 2021

View reviewed changes

common/config/config.go Outdated Show resolved Hide resolved

longquanzheng reviewed Aug 18, 2021

View reviewed changes

iamrodrigo added 12 commits August 18, 2021 11:12

bringing back comment

d8f1464

Merge branch 'adding-middleware-for-frontend' of github.com:noiarek/c…

08f03fe

…adence into adding-middleware-for-frontend

Using private key from current cluster and removing private key

2b1c39d

Merge branch 'master' into adding-middleware-for-frontend

3545030

Rolling back changes on gen files

d309a11

fixing import

301a793

Deleting references to privatekey

75e3dcf

Running make bins with go 1.16.7

36e20d8

Revert "Running make bins with go 1.16.7"

cd757b6

This reverts commit 36e20d8.

running make fmt

135fa76

removing test and privatekey path

a9b119d

Updating field in struct to not to use pointer in interface

2e74730

iamrodrigo and others added 7 commits August 27, 2021 08:01

Fixing conflicts

99de104

fixing import

d6fc4f5

Merge branch 'master' into adding-middleware-for-frontend

d5ac6f0

Merge branch 'master' into adding-middleware-for-frontend

b43e960

fix go.sum

236c639

reset .gen

1e0a9c3

fix mock

521254a

longquanzheng changed the title ~~[WIP] Adding middleware to inject token for frontend request~~ Adding middleware to inject Auth token for internal requests to frontend Aug 30, 2021

longquanzheng added 5 commits August 30, 2021 14:16

reset unrelated changes

5e3b073

reset

dc95838

optimize

4122cb2

improve logs

75acde7

fix kafka compose file

1898ce8

longquanzheng approved these changes Aug 30, 2021

View reviewed changes

Merge branch 'master' into adding-middleware-for-frontend

de48b9b

longquanzheng merged commit 5191468 into cadence-workflow:master Aug 30, 2021

longquanzheng mentioned this pull request Sep 1, 2021

cassandra-esv7-kafka.yml is not working #4409

Closed

vytautas-karpavicius mentioned this pull request Sep 29, 2021

OAuth public/private key concerns #4520

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding middleware to inject Auth token for internal requests to frontend #4364

Adding middleware to inject Auth token for internal requests to frontend #4364

iamrodrigo commented Aug 14, 2021 •

edited by longquanzheng

Loading

longquanzheng Aug 15, 2021

iamrodrigo Aug 17, 2021

iamrodrigo Aug 17, 2021

longquanzheng commented Aug 17, 2021

iamrodrigo Aug 18, 2021

longquanzheng Aug 18, 2021 •

edited

Loading

iamrodrigo Aug 18, 2021

Groxx Aug 18, 2021 •

edited

Loading

longquanzheng Aug 18, 2021

Groxx Aug 18, 2021

iamrodrigo Aug 20, 2021

longquanzheng Aug 18, 2021

Groxx Aug 18, 2021

coveralls commented Aug 31, 2021 •

edited

Loading

Adding middleware to inject Auth token for internal requests to frontend #4364

Adding middleware to inject Auth token for internal requests to frontend #4364

Conversation

iamrodrigo commented Aug 14, 2021 • edited by longquanzheng Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

longquanzheng commented Aug 17, 2021

Choose a reason for hiding this comment

longquanzheng Aug 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Groxx Aug 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Aug 31, 2021 • edited Loading

Pull Request Test Coverage Report for Build 86bab5c0-970a-4756-abf3-35a46567c138

💛 - Coveralls

iamrodrigo commented Aug 14, 2021 •

edited by longquanzheng

Loading

longquanzheng Aug 18, 2021 •

edited

Loading

Groxx Aug 18, 2021 •

edited

Loading

coveralls commented Aug 31, 2021 •

edited

Loading