feat(GraphQL): Allow more control over custom logic header names

[vardhanapoorv]

Add support for declaring headers with custom name like -
secretHeaders: ["Authorization:Github-Api-Token"] where "Authorization" is send as the header name to GitHub and "Github-Api-Token" is used internally.

---

This change is 

[pawanrawal]

Reviewable status: 0 of 3 files reviewed, 2 unresolved discussions (waiting on @MichaelJCompton, @pawanrawal, and @vardhanapoorv)

---

graphql/schema/rules.go, line 1530 at r1 (raw file):

			for _, h := range secretHeaders.Children {
				key := strings.Split(h.Value.Raw, ":")
				if len(key) != 2 {

If length is 1, then we should do this but if its >2, that should ne an error.

---

graphql/schema/rules.go, line 1535 at r1 (raw file):

				// We try and fetch the value from the stored secrets.
				val := secrets[key[0]]
				headers.Add(key[1], string(val))

Isn't the order reverse here, i.e. we are supposed to send key[0] i.e say Authorization fetch key[1] from the secrets?

[vardhanapoorv]

Reviewable status: 0 of 3 files reviewed, 2 unresolved discussions (waiting on @MichaelJCompton and @pawanrawal)

---

graphql/schema/rules.go, line 1530 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

If length is 1, then we should do this but if its >2, that should ne an error.

Added this. But I think we should also add similar validation for "forward-headers" in "rules.go" and also this particular block only gets triggered when "skipIntrospection" flag is false rather it should get run every time (just the validation part) as a validation like it happens for other fields in custom directive block. So we get the error while submitting the schema itself.

---

graphql/schema/rules.go, line 1535 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Isn't the order reverse here, i.e. we are supposed to send key[0] i.e say Authorization fetch key[1] from the secrets?

Yes, done.

[pawanrawal]

This is looking pretty good, can we add an integration test similar to ForwardHeaders test in custom_logic_test.go?

Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @MichaelJCompton and @vardhanapoorv)

---

graphql/schema/rules.go, line 1530 at r1 (raw file):

Previously, vardhanapoorv (Apoorv Vardhan) wrote…

Added this. But I think we should also add similar validation for "forward-headers" in "rules.go" and also this particular block only gets triggered when "skipIntrospection" flag is false rather it should get run every time (just the validation part) as a validation like it happens for other fields in custom directive block. So we get the error while submitting the schema itself.

Yeah, I think that is a good idea. Do you want to create an issue for that and tackle it later?

[vardhanapoorv]

Done

Reviewable status: 0 of 5 files reviewed, 1 unresolved discussion (waiting on @MichaelJCompton and @pawanrawal)

---

graphql/schema/rules.go, line 1530 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Yeah, I think that is a good idea. Do you want to create an issue for that and tackle it later?

Yes, sounds good.

[MichaelJCompton]

I wonder if we need to just precompute the splitting that seems to be being done on each request.

Reviewable status: 0 of 5 files reviewed, 5 unresolved discussions (waiting on @MichaelJCompton, @pawanrawal, and @vardhanapoorv)

---

graphql/e2e/custom_logic/custom_logic_test.go, line 206 at r2 (raw file):

				 url: "http://mock:8888/verifyCustomNameHeaders",
				 method: "GET",
				 forwardHeaders: ["X-App-Token:App", "X-User-Id"],

Do forward headers have to be present in the actual request?

Just confirm for me the rules about what we send on and when.

---

graphql/e2e/custom_logic/custom_logic_test.go, line 207 at r2 (raw file):

				 method: "GET",
				 forwardHeaders: ["X-App-Token:App", "X-User-Id"],
				 secretHeaders: ["Authorization:Github-Api-Token", "X-App-Token"]

so does this work in introspection? E.g. that I could give a secret git token that I use for introspection and then the user supplies the actual token for queries.

and are we thus always sending on the secret headers?

---

graphql/schema/schemagen.go, line 266 at r2 (raw file):

				key = []string{h.Value.Raw, h.Value.Raw}
			} else if len(key) > 2 {
				continue

Is this actually an error case? Or is that caught elsewhere?

---

graphql/schema/wrappers.go, line 871 at r2 (raw file):

Quoted 6 lines of code…

			key := strings.Split(h.Value.Raw, ":")
			if len(key) == 1 {
				key = []string{h.Value.Raw, h.Value.Raw}
			} else if len(key) > 2 {
				continue
			}

This repetition seems not quite right. Why not set up the header once and for all when we parse the schema at that point you can catch any error and then not have to repeat this patern?

[vardhanapoorv]

Reviewable status: 0 of 5 files reviewed, 5 unresolved discussions (waiting on @MichaelJCompton and @pawanrawal)

---

graphql/e2e/custom_logic/custom_logic_test.go, line 206 at r2 (raw file):

Previously, MichaelJCompton (Michael Compton) wrote…

Do forward headers have to be present in the actual request?

Just confirm for me the rules about what we send on and when.

Yes forward headers are sent with the request. So forward headers are always send and we get the value from there and send to the remote endpoint. Secret headers value is specified in schema itself and that we send that to remote endpoint. If we have a secret header and also a forward header with same name, the value of forward header (if part of request) overrides the secret header value specified in schema.

---

graphql/e2e/custom_logic/custom_logic_test.go, line 207 at r2 (raw file):

Previously, MichaelJCompton (Michael Compton) wrote…

so does this work in introspection? E.g. that I could give a secret git token that I use for introspection and then the user supplies the actual token for queries.

and are we thus always sending on the secret headers?

Secret header value is read from schema only, but if like in this case the forward headers also has that same header so that will override the value coming from secret header in schema.

---

graphql/schema/schemagen.go, line 266 at r2 (raw file):

Previously, MichaelJCompton (Michael Compton) wrote…

Is this actually an error case? Or is that caught elsewhere?

So yes we plan to make it part of rules.go (created a ticket for that) and then we don't need the error checking here.

---

graphql/schema/wrappers.go, line 871 at r2 (raw file):

Previously, MichaelJCompton (Michael Compton) wrote…

			key := strings.Split(h.Value.Raw, ":")
			if len(key) == 1 {
				key = []string{h.Value.Raw, h.Value.Raw}
			} else if len(key) > 2 {
				continue
			}

This repetition seems not quite right. Why not set up the header once and for all when we parse the schema at that point you can catch any error and then not have to repeat this patern?

Yes same as above, validation in rules.go, then here we just add the headers and can remove the other things.

[MichaelJCompton]

merge master and change the title for the new format.

also put up an jira ticket for precomputing custom logic headers and body, etc.

Reviewable status: 0 of 5 files reviewed, 5 unresolved discussions (waiting on @MichaelJCompton and @pawanrawal)

[MichaelJCompton]

Reviewable status: 0 of 5 files reviewed, 5 unresolved discussions (waiting on @MichaelJCompton and @pawanrawal)