refactor: set defaults in Deployment, else k8s sets them for you, creating infinite reconciliation loop

[qicz]

Which issue(s) this PR fixes:
fixes the envoy proxy resources update bug, the resourceProvider's provides resources have more different fields from applied.

Fixes

• Proxy always terminating #1590

[codecov]

Codecov Report

Merging #1594 (ff25ed3) into main (89d16a6) will decrease coverage by 0.09%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1594      +/-   ##
==========================================
- Coverage   64.90%   64.82%   -0.09%     
==========================================
  Files          83       83              
  Lines       11912    11924      +12     
==========================================
- Hits         7732     7730       -2     
- Misses       3701     3711      +10     
- Partials      479      483       +4     

Files Changed	Coverage Δ
...ternal/infrastructure/kubernetes/proxy/resource.go	93.22% <100.00%> (+0.21%)	⬆️
...frastructure/kubernetes/proxy/resource_provider.go	86.39% <100.00%> (+0.16%)	⬆️
...al/infrastructure/kubernetes/ratelimit/resource.go	96.21% <100.00%> (+0.04%)	⬆️
...tructure/kubernetes/ratelimit/resource_provider.go	97.16% <100.00%> (+0.04%)	⬆️

... and 2 files with indirect coverage changes

[arkodg]

@qicz can you please elaborate on the bug ?

[qicz]

@qicz can you please elaborate on the bug ?

the Controller watch all Resource, when some Resource changed the r.InfraIR will Store new Infra

gateway/internal/gatewayapi/runner/runner.go

Lines 91 to 97 in 4e6786a

	for key, val := range result.InfraIR {
	if err := val.Validate(); err != nil {
	r.Logger.Error(err, "unable to validate infra ir, skipped sending it")
	} else {
	r.InfraIR.Store(key, val)
	newKeys = append(newKeys, key)
	}

and the r.InfraIR will Subscribe the change and do CreateOrUpdateProxyInfra

gateway/internal/infrastructure/runner/runner.go

Lines 56 to 75 in 4e6786a

	func (r *Runner) subscribeToProxyInfraIR(ctx context.Context) {
	// Subscribe to resources
	message.HandleSubscription(r.InfraIR.Subscribe(ctx),
	func(update message.Update[string, *ir.Infra]) {
	val := update.Value
	if update.Delete {
	if err := r.mgr.DeleteProxyInfra(ctx, val); err != nil {
	r.Logger.Error(err, "failed to delete infra")
	}
	} else {
	// Manage the proxy infra.
	if err := r.mgr.CreateOrUpdateProxyInfra(ctx, val); err != nil {
	r.Logger.Error(err, "failed to create new infra")
	}
	}
	},
	)
	r.Logger.Info("infra subscriber shutting down")
	}

gateway/internal/infrastructure/kubernetes/infra_resource.go

Lines 63 to 78 in 4e6786a

	func (i *Infra) createOrUpdateDeployment(ctx context.Context, r ResourceRender) error {
	deployment, err := r.Deployment()
	if err != nil {
	return err
	}
	current := &appsv1.Deployment{}
	key := types.NamespacedName{
	Namespace: deployment.Namespace,
	Name: deployment.Name,
	}
	return i.Client.CreateOrUpdate(ctx, key, current, deployment, func() bool {
	return !reflect.DeepEqual(deployment.Spec, current.Spec)
	})
	}

when creating the deployment, some filed will fill default values like DefaultMode 420, DeprecatedServiceAccount the sa name, RevisionHistoryLimit &10, ProgressDeadlineSeconds &600, and the HPA will change the Replicas when occurring the AutoScaler. we should align these fields to ensure the updateChecker() works right, otherwise will update the deployment all the time, and the more pods terminating

gateway/internal/infrastructure/kubernetes/infra_resource.go

Lines 75 to 84 in 328a7dc

	return i.Client.CreateOrUpdate(ctx, key, current, deployment, func() bool {
	// applied to k8s the "DeprecatedServiceAccount" will fill it.
	deployment.Spec.Template.Spec.DeprecatedServiceAccount = current.Spec.Template.Spec.DeprecatedServiceAccount
	// applied to k8s the "SecurityContext" will fill it with default settings.
	deployment.Spec.Template.Spec.SecurityContext = current.Spec.Template.Spec.SecurityContext
	// adapter the hpa updating.
	deployment.Spec.Replicas = current.Spec.Replicas
	return !reflect.DeepEqual(deployment.Spec, current.Spec)
	})
	}

gateway/internal/infrastructure/kubernetes/infra_client.go

Lines 39 to 45 in 328a7dc

	if updateChecker() {
	specific.SetUID(current.GetUID())
	if err := cli.Client.Update(ctx, specific); err != nil {
	return errors.Wrap(err, "for Update")
	}
	}
	}

[qicz]

maybe we should watch the Resource by per controller like the past version @arkodg

[qicz]

@zirain the e2e needs review, sometime works fine on v1.27.0, or v1.25.8 or v1.26.3 but can not works fine all.

[zirain]

@zirain the e2e needs review, sometime works fine on v1.27.0, or v1.25.8 or v1.26.3 but can not works fine all.

see #1599

[arkodg]

should the logic be reverted back to update only the fields that EG cares about instead of updating entire object ?
or should we skip comparison of some fields (that are changed by other controllers) ?

[qicz]

should the logic be reverted back to update only the fields that EG cares about instead of updating entire object ? or should we skip comparison of some fields (that are changed by other controllers) ?

maybe needs a controller for envoyproxy, then we just needs compare enovyproxy.

At present, we only compare the fields that EG cares about or ignore some fields for subsequent feature development, which is easy to cause comparative logic omissions.

At the same time, when the HPA is triggered, it will also be affected, for example, the original number of replicas is 2, the HPA is changed to 3, at this time we modify the envoyproxy, changing it to 4 will not take effect. We needs compare the hard deployment and k8s current deployment replicas when hard > current fire updating.

[qicz]

@arkodg ptal, has been updated ref offline communication

[arkodg]

LGTM thanks !

[zirain]

cc @arkodg need cherrypick to release-0.5?

[zirain]

/wait #1714

[Xunzhuo]

Good catch and thanks for digging into the problem : )

[Xunzhuo]

https://github.com/envoyproxy/gateway/actions/runs/5675991681/job/15382049734
cc @zirain

[zirain]

have no idea about the fail, maybe something underlying changed?