feat: add fast-cgi worker #71

johnduro · 2019-07-10T09:58:46Z

Here is a first implementation of fast-cgi, don't hesitate to ask for modification if you think it would be better/more consistent in an other way.

I did not touch nor add any test yet, i prefer you validate this draft before and i ll add them in another commit.

Check carefully the error handling as i'm not totally sure it is ok.

Should i add validation of the configuration ? For executable for example. And where do you see it ?

corvus-ch

Hi @johnduro,

Thanks fro your contribution. I am OK with most of it, keeping in mind that this is a work in progress. In general, I miss comments on the public functions. I have also left comments on a few things.

There is one topic we need to discuss and find an agreement. What shall be the format when sending the data to FCGI. I had planned to keep the body and the metadata separate from each other in order to prevent any strange and hard to control encoding issues. I see three options:

Send the metadata separated as headers/environment variables. I have not checked if this even doable.
Have body and metadata send together in one JSON document. In that case the body needs to be encoded with base64 to prevent issues.
Make use multipart mime types and send the meta data as JSON document in one part and the body in a second one.

I would prefer to no longer maintain one format. The all in one JSON document is a bad candidate as it might be a lot of overhead for simpler use cases. The separation would allow to cover both simple and complex use cases without the need of using multiple formats which required to change core concepts on the implementation.

What do you think?

Regards
Christian

corvus-ch · 2019-07-28T10:11:23Z

worker/factory/factory.go

@@ -0,0 +1,28 @@
+package factory


I would place NewProcess directly in the worker package.

i tried to do that but then worker package must import fastcgi and exec package and these two implement worker making a circular dependency forbidden by go

corvus-ch · 2019-07-28T10:16:00Z

worker/factory/factory.go

+)
+
+func NewProcess(config *config.Config, logger logr.Logger) (worker.Process, error) {
+	if config.Worker() == "executable" {


Use constants for the possible worker values.

corvus-ch · 2019-07-28T10:17:11Z

worker/factory/factory.go

+)
+
+func NewProcess(config *config.Config, logger logr.Logger) (worker.Process, error) {
+	if config.Worker() == "executable" {


Personally I would prefer a switch/case construct here.

corvus-ch · 2019-07-28T10:19:47Z

main.go

-	} else {
-		// TODO Make reject/requeue conditions configurable.
-		p = exec.New(e, []int{}, c.Bool("output"), ll)
+	// TODO Make reject/requeue conditions configurable.


This comment needs to be kept with the call of exec.New.

corvus-ch · 2019-07-28T10:28:15Z

worker/fastcgi/fastcgi.go

+	}
+
+	p.PayloadMaker = p.newPayload
+	if config.Include() == true {


I would like to get rid of maintaining multiple payload formats. See #59. I would prefer to have the meta data submitted as headers/environment variables and keep the body on its own.

corvus-ch · 2019-07-28T10:28:52Z

worker/fastcgi/fastcgi.go

+	fcgi, err := fcgiclient.Dial(p.Network, p.Address)
+
+	if err != nil {
+		p.Log.Info("Failed to initiate connexion to fast-cgi.")


Keep this as one single error message.

corvus-ch · 2019-07-28T10:39:23Z

worker/fastcgi/fastcgi.go

+			Exchange: attributes.Exchange(),
+			RoutingKey: attributes.RoutingKey(),
+		},
+		Body: buffer.Bytes(),


If the body consists of arbitrary binary data, this is likely to yield unexpected results if not broken JSON documents. See https://stackoverflow.com/questions/1443158/binary-data-in-json-string-something-better-than-base64.

Making use of a multipart mime type could also be an option.

johnduro · 2019-08-09T13:14:15Z

Hey @corvus-ch,

Thank you for your review, i'll try to work on it next week.

For the sending format i tried to reproduce the way the data were sent previously
but i see what you mean and i think too it would be better to have a single format for the body.

I think sending metadata through header/fast-cgi param would be my first choice
but i have to check to doability as it is not obvious the lib will permit it.
If this way works i'll send it as a json (encoded in base64 ? not mandatory i think) in a key rmq_metadata.
Sending the json document like now with include would be my third and fallback choice.
Sending 2 documents is doable with the lib so if the first does not work i'll go for this one.

Ok for your others comments, i'll treat them soon.

Regards

corvus-ch · 2019-08-12T19:46:57Z

If this way works i'll send it as a json (encoded in base64 ? not mandatory i think) in a key rmq_metadata.

Why not sending them as multiple headers? One per meta data field? No need for JSON encode and decode. The worker code can just pick the values he is interested in and ignore the others.

johnduro · 2019-08-13T08:18:02Z

Some meta might be arrays so we will have to choose a format anyway for them and, from my point of view, i would prefer to do only one decode and maybe validation on a single json document than several.

johnduro · 2019-09-16T17:00:51Z

Hey @corvus-ch ,

I updated the PR (i'll to be more responsive for the next steps !) with your review, tell me if it is ok and right after i'll start the tests.

For the payload i was able to pass the metadata through a json inside the header HTTP_RMQ_METADATA in the consumer which transform into rmq-metadata at the arrival.

Tell me if things are missing or not the way you'd want them.

johnduro force-pushed the fastcgi-implementation branch from 841db52 to 6a30d6c Compare July 10, 2019 09:59

corvus-ch reviewed Jul 28, 2019

View reviewed changes

feat: add fast-cgi worker

aa7a5aa

johnduro force-pushed the fastcgi-implementation branch from 6a30d6c to aa7a5aa Compare September 16, 2019 16:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add fast-cgi worker #71

feat: add fast-cgi worker #71

johnduro commented Jul 10, 2019

corvus-ch left a comment

corvus-ch Jul 28, 2019

johnduro Sep 16, 2019

corvus-ch Jul 28, 2019

corvus-ch Jul 28, 2019

corvus-ch Jul 28, 2019

corvus-ch Jul 28, 2019

corvus-ch Jul 28, 2019

corvus-ch Jul 28, 2019

johnduro commented Aug 9, 2019

corvus-ch commented Aug 12, 2019

johnduro commented Aug 13, 2019

johnduro commented Sep 16, 2019

feat: add fast-cgi worker #71

Are you sure you want to change the base?

feat: add fast-cgi worker #71

Conversation

johnduro commented Jul 10, 2019

corvus-ch left a 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

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

johnduro commented Aug 9, 2019

corvus-ch commented Aug 12, 2019

johnduro commented Aug 13, 2019

johnduro commented Sep 16, 2019