feat(GraphQL): Add introspection headers to custom logic

[vardhanapoorv]

Fixes GRAPHQL-510
This PR introduces introspectionHeaders for custom logic in GraphQL which will only be used for making the introspection query to the remote endpoint. Also forwardHeaders and secretHeaders cannot overlap.

This change is 

[MichaelJCompton]

looks good, just needs a few more test cases. Ok for @pawanrawal to approve, once master is ok to merge.

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

---

graphql/e2e/custom_logic/custom_logic_test.go, line 248 at r1 (raw file):

				  method: "POST"
				  forwardHeaders: ["Content-Type"]
				  secretHeaders: ["GITHUB-API-TOKEN"]

should this one be removed now

---

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

				if len(secretKey) > 2 {
					return append(errs, gqlerror.ErrorPosf(errPos,
						"Type %s; Field %s; secretHeaders in @custom directive should be of the form 'remote_headername:local_headername' or just 'headername'"+

also some tests for right header structure in the schema gen tests

---

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

						if fhKey[0] == secretKey[0] {
							return append(errs, gqlerror.ErrorPosf(errPos,
								"Type %s; Field %s; secretHeaders and forwardHeaders in @custom directive cannot have overlapping headers"+

we'll need a test in the error cases of the schema gen test

---

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

				if !ok {
					return append(errs, gqlerror.ErrorPosf(graphql.Position,
						"Type %s; Field %s; introspectionHeaders in @custom directive should use secrets to store the header value"+

say exactly how to put it in

[vardhanapoorv]

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

---

graphql/e2e/custom_logic/custom_logic_test.go, line 248 at r1 (raw file):

Previously, MichaelJCompton (Michael Compton) wrote…

should this one be removed now

Done.

---

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

Previously, MichaelJCompton (Michael Compton) wrote…

say exactly how to put it in

Done.

[vardhanapoorv]

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

---

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

Previously, MichaelJCompton (Michael Compton) wrote…

also some tests for right header structure in the schema gen tests

Done.

---

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

Previously, MichaelJCompton (Michael Compton) wrote…

we'll need a test in the error cases of the schema gen test

Done.

[pawanrawal]

Mostly looking good, got some small questions.

Reviewed 35 of 37 files at r1, 3 of 3 files at r2.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @MichaelJCompton and @vardhanapoorv)

---

graphql/schema/rules.go, line 1638 at r2 (raw file):

				secretKey := strings.Split(h.Value.Raw, ":")
				if len(secretKey) == 1 {
					secretKey = []string{h.Value.Raw, h.Value.Raw}

Are we using the value at index 1?

---

graphql/schema/rules.go, line 1646 at r2 (raw file):

						typ.Name, field.Name, h.Value.Raw))
				}
				if forwardHeaders != nil {

We would have iterated over forwardHeaders above. Why don't we store their names in a map and use that to compare against secret key here?

---

graphql/schema/rules.go, line 1682 at r2 (raw file):

	if graphql != nil && !skip && graphqlOpDef != nil {
		introspectionHeaders := httpArg.Value.Children.ForName("introspectionHeaders")

Again, we should have already iterated over these above so we should store them in a map and read here?

---

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

	if secretHeaders != nil {
		hc.RLock()
		for _, h := range secretHeaders.Children {

This should also be already pre-computed ideally. maybe we'll do it later

---

graphql/schema/wrappers_test.go, line 875 at r2 (raw file):

					url: "http://api:8888/graphql"
					method: "POST"
					forwardHeaders: ["API-Token", "Authorization"]

Can you have another example which gives the header in the format outgoing:header and make sure this duplicate check only applies on the outgoing value and the header can be shared between the two?

[vardhanapoorv]

Reviewable status: 36 of 38 files reviewed, 9 unresolved discussions (waiting on @MichaelJCompton and @pawanrawal)

---

graphql/schema/rules.go, line 1638 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Are we using the value at index 1?

Done.

---

graphql/schema/rules.go, line 1646 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

We would have iterated over forwardHeaders above. Why don't we store their names in a map and use that to compare against secret key here?

Done.

---

graphql/schema/rules.go, line 1682 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Again, we should have already iterated over these above so we should store them in a map and read here?

Done.

---

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

Previously, pawanrawal (Pawan Rawal) wrote…

This should also be already pre-computed ideally. maybe we'll do it later

Yeah along with batching custom logic calls.

---

graphql/schema/wrappers_test.go, line 875 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Can you have another example which gives the header in the format outgoing:header and make sure this duplicate check only applies on the outgoing value and the header can be shared between the two?

Done.

[pawanrawal]

Reviewed 38 of 39 files at r3, 1 of 1 files at r4.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @MichaelJCompton and @vardhanapoorv)

---

graphql/schema/rules.go, line 1684 at r2 (raw file):

		introspectionHeaders := httpArg.Value.Children.ForName("introspectionHeaders")
		headers := http.Header{}
		if introspectionHeaders != nil {

for key, val := range iheaders {

}

---

graphql/schema/wrappers_test.go, line 875 at r2 (raw file):

Previously, vardhanapoorv (Apoorv Vardhan) wrote…

Done.

Did you forget to push this one or am I missing this new case?