Configure instances on gatewayConfig

[sarahalsmiller]

Changes proposed in this PR:

• Add optional deploy object to gatewayClassConfiguration that allows users

How I've tested this PR:

-Created a kind cluster and installed CRD and helmchart locally

• need to test actually creating a gateway and making sure the instances scale appropriately

How I expect reviewers to test this PR:

• Added builder unit test that makes sure the deployment gets built with the correct number of replicas set
• Maybe need an end to end test to capture the bug I'm seeing where it doesn't actually set the replicas appropriately

Checklist:

• [ X ] Tests added
• [ X ] CHANGELOG entry added

  Run make changelog-entry for guidance in authoring a changelog entry, and
  commit the resulting file, which should have a name matching your PR number.
  Entries should use imperative present tense (e.g. Add support for...)

[andrewstucki]

Biggest change is the need to not reset the replica count if changed out of band. Ideally what we'd do is pass in the count for an existing deployment (if it exists) and use it. If no deployment exists, then do the defaultInstances/in-code defaults.

Also, is this PR going to also incorporate the arguments for min/max?

[andrewstucki]

ideally this would take a parameter to not overwrite the instance count if it's modified by the user (via something like a kubectl scale operation)

[sarahalsmiller]

Would the operation flow then be something like, check current current spec for override replicas value, check gateway class config, check default?

[andrewstucki]

So, it'd be something like query the deployment for the gateway, if it exists, then use the replica count on it, if not, then set it via this method. Taking a look at the code, there's actually an even easier way to go about this then passing it in on deployment construction (what I'm recommending above).

Most of this is already abstracted and could be handled via adding in some replica overwriting code here:

consul-api-gateway/pkg/apis/v1alpha1/types.go

Line 196 in 1c099ab

func MergeDeployment(a, b *appsv1.Deployment) *appsv1.Deployment {

Which gets used as part of the "create or update" code here:

consul-api-gateway/internal/k8s/reconciler/gateway.go

Lines 471 to 475 in 1c099ab

	deployment := g.deploymentBuilder.Build()
	mutated := deployment.DeepCopy()
	if updated, err := g.client.CreateOrUpdateDeployment(ctx, mutated, func() error {
	mutated = apigwv1alpha1.MergeDeployment(deployment, mutated)
	return g.client.SetControllerOwnership(g.gateway, mutated)

The second argument b in the MergeDeployment function argument should be the already existing deployment, so you'll just want to keep its replica count from getting wiped out by the b.Spec.Template = a.Spec.Template operation.

[sarahalsmiller]

It looks like that might already be getting preserved since it looks like replicas is a layer above template. Might need you to double check on that though.

[andrewstucki]

Ah, interesting, yeah, that would make sense, that's definitely not intended. We should probably audit what isn't handled by this merge operation apart from metadata (which was IIRC the intent of adding this create/update code that bypasses things like metadata).

Think best way to double check this and also make sure we're always allowing for manual modification is to probably add an e2e test for this.

[sarahalsmiller]

Added two e2e tests, one for normal replication and one for testing to make sure we can modify the replica number manually. Let me know your thoughts.

[sarahalsmiller]

Biggest change is the need to not reset the replica count if changed out of band. Ideally what we'd do is pass in the count for an existing deployment (if it exists) and use it. If no deployment exists, then do the defaultInstances/in-code defaults.

I'll see if I can get that working. Is that why the replicas is always set to one on create no matter what?

Also, is this PR going to also incorporate the arguments for min/max?

Nathan said that we were going to breaking out the min/max into a different ticket, so likely not this PR.

[nathancoleman]

Looking good! One big question about why pre-existing code is merging a deployment onto a copy of itself 🤔 . That's probs for Andrew though.

[nathancoleman]

Loving your distinction between require and assert ❤️

Changes addressed