Add goldenfile tests to ingress controller #587

lucastt · 2023-02-28T17:10:02Z

The idea of this PR is to improve the confidence of developers when making changes to the ingress controler code.

The new test will fetch k8s yamls and serve it through a fake k8s API, the ingress-controler test should generate a cloud formation template to create all the necessary AWS resources for any ingress change fetched in the fake k8s API, the generated template is then compared to an expected output detailed the golden file "expected.cf".

This PR adds the boilerplate for golden tests in the ingress-controller worker level and adds two simple cases one considering the creation of an ALB and the other considering the creation of a NLB to satisfy an ingress definition in k8s.

There is more information in the body of commit messages, please take a look. :)

aws/adapter.go

aws/fake/asg.go

aws/fake/cf.go

aws/fake/ec2.go

worker.go

worker_test.go

szuecs

A couple of comments, nothing to big in the first read.
The test cases read really well and I am excited to see this happening.

mikkeloscar

Generally this looks great and the actual changes are smaller than assumed when looking at the number of lines changed 😅 👍

I added a few comment on style, that don't have to be resolved right now.

It was very helpful for the review to split the changes in multiple commits. In the future having even separate PRs would probably make it even easier as we could reduce the changes are merge one by one, but now it's fine as one big PR :)

mikkeloscar · 2023-03-30T12:28:58Z

aws/adapter.go

@@ -248,6 +248,17 @@ func NewAdapter(clusterID, newControllerID, vpcID string, debug, disableInstrume
 	return
 }

+// UpdateManifest generates a manifest again based on cluster and VPC information.
+// This method is useful when using custom AWS clients.
+func (a *Adapter) UpdateManifest(clusterID, vpcID string) (*Adapter, error) {


The need for this method suggest to me we are not doing this in the best way as we have both NewAdapter and UpdateManifest calling buildManifest. It looks like an opportunity to restructure this a bit so it's not needed. Doesn't have to be done in this PR, but it just came to my attention when looking at it.

Yeah, I agree, I didn't refactor this at the time because I thought it was too much for a single PR. I believe we could make this change in a following PR.

mikkeloscar · 2023-03-30T12:40:10Z

worker_test.go

+		ec2Responses   fake.Ec2MockOutputs
+		asgResponses   fake.AutoscalingMockOutputs
+		elbv2Responses fake.Elbv2MockOutputs
+		cfResponses    fake.CfMockOutputs


nitpick on the naming:

In Go we should aim to use consistent case for initialisms and acronyms. So it should be EC2 instead of Ec2. Same with ELB and CF: https://github.com/golang/go/wiki/CodeReviewComments#initialisms

Since the package is already called fake the actual types may not need to be called something with Mock. E.g. fake.CFOutputs would be clear IMO.

Number 1 I would generally recommend to follow, 2 is more of a opinion so feel free to ignore that. :)

lucastt · 2023-04-17T14:52:28Z

👍

szuecs · 2023-04-18T05:58:18Z

👍

lucastt · 2023-04-18T08:48:19Z

👍

mikkeloscar · 2023-04-18T09:23:43Z

👍

WIth these mocks extracted I can use them in tests outside AWS package. This will be usedful to create a Goldenfile test in the controller level. Signed-off-by: Lucas Thiesen <lucas.thiesen@zalando.de>

What? This commit adds a method to expose the package global variable roots. This variable enable us to define a custom x509 certificate pool to verify certificates against. Why? This change is necessary to enable the testability of the controller and worker. To test a self signed certificate there is only two options, given the structure of the code: 1 - Add the CA certificate to the mechine or VM running the tests and configuring the OS through env vars to use this CA. 2 - Verify the certificate against a customs certificate pool containing the CA certificate used to generate our server certificate. Analysing these two options it was decided that the scond approach would be better for maintainability and readability. Is there any breaking changes? No. ----- Related issues: - https://github.bus.zalan.do/pg9/issues/issues/340 Reference links: - https://pkg.go.dev/crypto/x509 - https://www.rfc-editor.org/rfc/rfc2459.html Signed-off-by: Lucas Thiesen <lucas.thiesen@zalando.de>

This will enable integration tests using the AWS adapter. Signed-off-by: Lucas Thiesen <lucas.thiesen@zalando.de>

What? Adds tests in the golden file format, where given an ingress definition in the format of a k8s yaml and compare the cloud formation template generated by the controller, based on this k8s yaml and a mocked AWS state, and compares it to the content of a .cf file defined in the test. This should work like an integration test between the different adapters and controller logic. Why? The introduction of these tests will bring more confidence when making changes to the ingress controller which is a critical component of our infrastructure. This also enable us to add new test cases if we catch problems or anomalies increasing confidence that the problem would not happen again. Is there any breaking changes? No. ----- Related issues: - https://github.bus.zalan.do/pg9/issues/issues/340 Signed-off-by: Lucas Thiesen <lucas.thiesen@zalando.de>

Signed-off-by: Lucas Thiesen <lucas.thiesen@zalando.de>

lucastt · 2023-04-18T11:36:36Z

👍

mikkeloscar · 2023-04-18T12:04:46Z

👍

lucastt force-pushed the master branch 3 times, most recently from 626d695 to 9af41af Compare March 7, 2023 11:01

lucastt marked this pull request as ready for review March 7, 2023 11:05

szuecs requested a review from mikkeloscar March 7, 2023 11:26

szuecs self-assigned this Mar 7, 2023

szuecs requested a review from AlexanderYastrebov March 7, 2023 11:27

lucastt force-pushed the master branch from f267ca0 to 77a36e1 Compare March 7, 2023 12:35