Decoupling of Parser

[mflendrich]

This is a refactoring PR for package parser inspired by my work on introducing Ingress v1 support. Its size may look intimidating, but when reviewed commit by commit, it should be easier to wrap one's head around the changes happening.

Before this PR:

• type Parser had 26 member functions but did not hold any state,
• Service, Route, Upstream etc. were mutable types, used as input+output arguments by free functions and methods of Parser
• 55 functions were defined in parser.go (2005 LoC)
• parser_test.go was 6046 LoC 😱
• all member functions of Parser could short-circuit the dependency graph of functions by making direct access to store.Storer.

After this PR:

• all (well, hopefully all) functions that mutate an input argument are now a member function of that argument (not a functional change)
• if a function references a store.Storer, now it does so through an argument. In several places, this PR decouples parsing logic from store.Storer by replacing the store.Storer dependency with an actual batch of data. This is a preparation step for ingress v1: parse ingress class field in Ingress v1 resource #790 where the parsing logic will need to work with multiple presentation types for the same information.
• wrapper types (Service, Route, Upstream, etc.) as well as KongState, including their mutator functions, are moved to individual (smaller) files in package kongstate. The general idea (and long-term direction set forth by this PR) is to decouple Kubernetes presentation from Kong presentation by removing concrete Kubernetes types from package kongstate. After this PR, we're still far from there yet. I plan to remove the concrete Ingress type from package kongstate in Merge branch 'master' into 0.2.x #179.
  • type Plugin is almost decoupled - the kongPluginFromK8SPlugin family of functions perform a clean transformation from one world (k8s entities) to the other (go-kong entities).

Notes to reviewers:

• I recommend reviewing commit by commit.
• I recommend merging without squashing.
• The only intended changes in logic are:
  • some tests that operated on kong.(Service|Route|Backend) now operate on kongstate.(Service|Route|Backend),
  • local variables named log shadow function arguments named log where they add fields to the logger. There's potential for repetition of the same field over and over by multiple functions on the call stack.
  • testing for non-panicking if a function gets nil changed (expected to have no side effects),
  • the new type ingressRules and its merging logic (intended to yield identical results),
  • If there is some other (not obviously correct) change in logic that I missed here, don't hesitate to raise it in code review, because I may have missed a test.

[hbagdi]

One more question: What is the rationale behind moving Consumer and KongState into their own dedicated packages?

[mflendrich]

One more question: What is the rationale behind moving Consumer and KongState into their own dedicated packages?

As discussed offline: it was the first type I moved and I didn't have a clear picture yet as to the target packages shape, so Consumer remained separate. Moved everything from package consumer to package kongstate. In a separate commit so as not to go through a rebasing hell.

[rainest]

This helps a lot with difficulty I've been having with the poor separation between the parser and store, especially with trying to write unit tests for them that aren't covering behavior that's sorta shared between them. Separating out the Kong state in particular should help enabling future work to generate config diffs for troubleshooting.

My one note is that I'd caution against the use of acronyms or shorthand that aren't common elsewhere (e.g. "k8s" versus "Kubernetes" or "tech" versus "technology"). Those are harder to grasp when scanning through or reaching code via some path other than a straight readthrough (e.g. from a grep search or traceback, where I might start from somewhere in the middle rather than the start of a function):

kubernetes-ingress-controller/internal/ingress/controller/parser/ingressrules.go

Lines 58 to 72 in 7fdd505

	if _, ok := ir.SecretNameToSNIs[secretKey]; !ok {
	ir.SecretNameToSNIs[secretKey] = []string{}
	}
	if err == nil {
	service.ClientCertificate = &kong.Certificate{
	ID: kong.String(string(secret.UID)),
	}
	} else {
	log.WithFields(logrus.Fields{
	"secret_name": secretName,
	"secret_namespace": service.K8sService.Namespace,
	}).Errorf("failed to fetch secret: %v", err)
	}
	}
	ir.ServiceNameToServices[key] = service

("ir" versus "rules" or "ingressRules")

kubernetes-ingress-controller/internal/ingress/controller/parser/kongstate/kongstate.go

Line 119 in 7fdd505

err := cons.SetCredential(log, credential.Type, credential.Config)

("cons" versus "consumer")

kubernetes-ingress-controller/internal/ingress/controller/parser/kongstate/kongstate.go

Lines 150 to 159 in 7fdd505

	if ks.Services[i].Routes[j].IsTCP {
	kongIngress, err = getKongIngressFromTCPIngress(s,
	&ks.Services[i].Routes[j].TCPIngress)
	if err != nil {
	log.WithFields(logrus.Fields{
	"tcpingress_name": ks.Services[i].Routes[j].TCPIngress.Name,
	"tcpingress_namespace": ks.Services[i].Routes[j].TCPIngress.Namespace,
	}).Errorf("failed to fetch KongIngress resource for Ingress: %v", err)
	}
	} else {

("ks" versus "state" or "kongState")

If the full names are getting egregiously long (4+ words or whatever as an arbitrary threshold), or it's something we need to use often (of the three, I'd argue "ks" is the most reasonable, since it's both used throughout that code and is an acronym for the main concept in that file), then we need to look at shortening, but otherwise, I'd suggest defaulting to the longer form.

Non-blocking though: I was able to figure out the meaning in a couple seconds of looking back, but it was something where I needed to stop scanning and step back to look around for a bit.

[mflendrich]

If the full names are getting egregiously long (4+ words or whatever as an arbitrary threshold), or it's something we need to use often (of the three, I'd argue "ks" is the most reasonable, since it's both used throughout that code and is an acronym for the main concept in that file), then we need to look at shortening, but otherwise, I'd suggest defaulting to the longer form.

It is a Go idiom to name receivers with a one- or two-letter abbreviation of the receiver type name. I cannot say I wasn't surprised by this as I was switching from C++ to Go the other day.

@rainest is that ok if we follow the convention for our projects, or would you like us to use longer receiver names?
I do prefer the shorter ones (it's easy to read when one gets used to it IMO) but I'll accept that if you have a strong preference for longer ones.

[rainest]

If it's idiomatic I'll note it down as "Go thing for receivers specifically, expect that in Go code in general"; TIL.