Skip to content

Commit 3cf04dd

Browse files
rikatzrikatz
committed
OCPBUGS-55050: Remove resources limits from Gateway API proxy containers
When a Gateway is deployed, Istio will create a deployment containing resources limits set, which is not expected on Openshift components. These resource limits are added as part of the default helm template used by Istio controller when creating the deployments and Pods. An attempt to make Istio/Sail Library to have a default configuration on resource limits is not possible, given the Kubernetes resources struct has omitempty, which means when something is null omit, and when it ommits it considers that a user passing null has 'no opinion' as opposed to 'I want to set it null'. This way, the only way of fixing it is by telling the Gateway customization, via the Gateway Class that we want to enforce the resource to be null, and then when Istio deploys this Gateway instance it will remove the resources.limits field from the proxy pods
1 parent 1a9e496 commit 3cf04dd

6 files changed

Lines changed: 101 additions & 39 deletions

File tree

‎pkg/operator/controller/gatewayclass/controller.go‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,9 @@ const (
8181
// can be specified. This annotation is only intended for use by
8282
// OpenShift developers.
8383
istioVersionOverrideAnnotationKey = "unsupported.do-not-use.openshift.io/istio-version"
84+
// gatewayProxyContainerName is the name of the proxy container
85+
// in gateway deployments managed by Istio.
86+
gatewayProxyContainerName = "istio-proxy"
8487
// sailLibraryFinalizer is added to GatewayClasses using Sail Library installation.
8588
// When a GatewayClass with this finalizer is deleted:
8689
// 1. Sail Library mode: Uninstall Istio if this is the last GatewayClass, then remove finalizer

‎pkg/operator/controller/gatewayclass/controller_test.go‎

Lines changed: 29 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@ import (
44
"context"
55
"encoding/json"
66
"fmt"
7-
"strings"
87
"testing"
98
"time"
109

@@ -184,18 +183,18 @@ func Test_Reconcile(t *testing.T) {
184183
}
185184

186185
gatewayclassesConfig := func(config string, gatewayclasses ...string) json.RawMessage {
187-
return json.RawMessage(fmt.Appendf(nil, `{%s}`, strings.Join(func() []string {
188-
var result []string
189-
190-
for _, name := range gatewayclasses {
191-
result = append(result, fmt.Sprintf(`"%s":%s`, name, config))
192-
}
193-
194-
return result
195-
}(), ",")))
186+
payload := make(map[string]json.RawMessage, len(gatewayclasses))
187+
for _, name := range gatewayclasses {
188+
payload[name] = json.RawMessage(config)
189+
}
190+
b, err := json.Marshal(payload)
191+
if err != nil {
192+
t.Fatalf("an error occurred during json creation for gatewayclassesConfig: %s", err)
193+
}
194+
return b
196195
}
197-
hpaConfig := func(minReplicas int) string {
198-
return fmt.Sprintf(`{"horizontalPodAutoscaler":{"spec":{"maxReplicas":10,"minReplicas":%d}}}`, minReplicas)
196+
gatewayclassConfig := func(minReplicas int) string {
197+
return fmt.Sprintf(`{"deployment":{"spec":{"template":{"spec":{"containers":[{"name":"istio-proxy","resources":{"limits":null}}]}}}},"horizontalPodAutoscaler":{"spec":{"maxReplicas":10,"minReplicas":%d}}}`, minReplicas)
199198
}
200199

201200
istioRevision := func() *sailv1.IstioRevision {
@@ -351,7 +350,7 @@ func Test_Reconcile(t *testing.T) {
351350
},
352351
expectCreate: []client.Object{
353352
subscription("redhat-operators", "stable", "servicemeshoperator3.v3.0.1"),
354-
istio("v1.24.4", false, nil, gatewayclassesConfig(hpaConfig(2), "openshift-default")),
353+
istio("v1.24.4", false, nil, gatewayclassesConfig(gatewayclassConfig(2), "openshift-default")),
355354
manifests.IstiodAllowNetworkPolicy(),
356355
},
357356
expectUpdate: []client.Object{},
@@ -366,7 +365,7 @@ func Test_Reconcile(t *testing.T) {
366365
},
367366
expectCreate: []client.Object{
368367
subscription("redhat-operators", "stable", "servicemeshoperator3.v3.0.1"),
369-
istio("v1.24.4", false, nil, gatewayclassesConfig(hpaConfig(1), "openshift-default")),
368+
istio("v1.24.4", false, nil, gatewayclassesConfig(gatewayclassConfig(1), "openshift-default")),
370369
manifests.IstiodAllowNetworkPolicy(),
371370
},
372371
expectUpdate: []client.Object{},
@@ -390,7 +389,7 @@ func Test_Reconcile(t *testing.T) {
390389
},
391390
expectCreate: []client.Object{
392391
subscription("redhat-operators", "stable", "servicemeshoperator3.v3.0.1"),
393-
istio("v1.24.4", false, nil, gatewayclassesConfig(hpaConfig(2), "openshift-default", "openshift-internal")),
392+
istio("v1.24.4", false, nil, gatewayclassesConfig(gatewayclassConfig(2), "openshift-default", "openshift-internal")),
394393
manifests.IstiodAllowNetworkPolicy(),
395394
},
396395
expectUpdate: []client.Object{},
@@ -406,7 +405,7 @@ func Test_Reconcile(t *testing.T) {
406405
},
407406
expectCreate: []client.Object{
408407
subscription("redhat-operators", "stable", "servicemeshoperator3.v3.0.1"),
409-
istio("v1.24.4", false, expectedProxyConfiguration, gatewayclassesConfig(hpaConfig(2), "openshift-default")),
408+
istio("v1.24.4", false, expectedProxyConfiguration, gatewayclassesConfig(gatewayclassConfig(2), "openshift-default")),
410409
manifests.IstiodAllowNetworkPolicy(),
411410
},
412411
expectUpdate: []client.Object{},
@@ -426,7 +425,7 @@ func Test_Reconcile(t *testing.T) {
426425
},
427426
expectCreate: []client.Object{
428427
subscription("redhat-operators", "stable", "servicemeshoperator3.v3.0.1"),
429-
istio("v1.24.4", true, nil, gatewayclassesConfig(hpaConfig(2), "openshift-default")),
428+
istio("v1.24.4", true, nil, gatewayclassesConfig(gatewayclassConfig(2), "openshift-default")),
430429
manifests.IstiodAllowNetworkPolicy(),
431430
},
432431
expectUpdate: []client.Object{},
@@ -446,7 +445,7 @@ func Test_Reconcile(t *testing.T) {
446445
},
447446
expectCreate: []client.Object{
448447
subscription("redhat-operators", "stable", "servicemeshoperator3.v3.0.1"),
449-
istio("v1.24.4", true, nil, gatewayclassesConfig(hpaConfig(2), "openshift-default")),
448+
istio("v1.24.4", true, nil, gatewayclassesConfig(gatewayclassConfig(2), "openshift-default")),
450449
manifests.IstiodAllowNetworkPolicy(),
451450
},
452451
expectUpdate: []client.Object{},
@@ -463,7 +462,7 @@ func Test_Reconcile(t *testing.T) {
463462
},
464463
expectCreate: []client.Object{
465464
subscription("redhat-operators", "stable", "servicemeshoperator3.v3.0.1"),
466-
istio("v1.24-latest", false, nil, gatewayclassesConfig(hpaConfig(2), "openshift-default")),
465+
istio("v1.24-latest", false, nil, gatewayclassesConfig(gatewayclassConfig(2), "openshift-default")),
467466
manifests.IstiodAllowNetworkPolicy(),
468467
},
469468
expectUpdate: []client.Object{},
@@ -483,7 +482,7 @@ func Test_Reconcile(t *testing.T) {
483482
},
484483
expectCreate: []client.Object{
485484
subscription("foo", "bar", "baz"),
486-
istio("quux", false, nil, gatewayclassesConfig(hpaConfig(2), "openshift-default")),
485+
istio("quux", false, nil, gatewayclassesConfig(gatewayclassConfig(2), "openshift-default")),
487486
manifests.IstiodAllowNetworkPolicy(),
488487
},
489488
expectUpdate: []client.Object{},
@@ -573,11 +572,11 @@ func Test_Reconcile(t *testing.T) {
573572
infraConfig(configv1.HighlyAvailableTopologyMode),
574573
gatewayClass("openshift-default", true, nil, nil, false),
575574
istioCRD(),
576-
istio("v1.24.4", true, nil, gatewayclassesConfig(hpaConfig(2), "openshift-default")),
575+
istio("v1.24.4", true, nil, gatewayclassesConfig(gatewayclassConfig(2), "openshift-default")),
577576
},
578577
expectPatched: []client.Object{},
579578
expectDelete: []client.Object{
580-
istio("v1.24.4", true, nil, gatewayclassesConfig(hpaConfig(2), "openshift-default")),
579+
istio("v1.24.4", true, nil, gatewayclassesConfig(gatewayclassConfig(2), "openshift-default")),
581580
},
582581
expectedResult: reconcile.Result{
583582
RequeueAfter: 5 * time.Second,
@@ -618,7 +617,7 @@ func Test_Reconcile(t *testing.T) {
618617
expectedStatusPatched: []client.Object{
619618
gatewayClass("openshift-default", true, nil, installedConditions(), false),
620619
},
621-
expectedSailLibraryOptions: expectedSailLibraryOptions("v1.24.4", false, nil, gatewayclassesConfig(hpaConfig(2), "openshift-default")),
620+
expectedSailLibraryOptions: expectedSailLibraryOptions("v1.24.4", false, nil, gatewayclassesConfig(gatewayclassConfig(2), "openshift-default")),
622621
},
623622
{
624623
name: "Sail Library: installs Istio (single replica)",
@@ -634,7 +633,7 @@ func Test_Reconcile(t *testing.T) {
634633
expectedStatusPatched: []client.Object{
635634
gatewayClass("openshift-default", true, nil, installedConditions(), false),
636635
},
637-
expectedSailLibraryOptions: expectedSailLibraryOptions("v1.24.4", false, nil, gatewayclassesConfig(hpaConfig(1), "openshift-default")),
636+
expectedSailLibraryOptions: expectedSailLibraryOptions("v1.24.4", false, nil, gatewayclassesConfig(gatewayclassConfig(1), "openshift-default")),
638637
},
639638
{
640639
name: "Sail Library: installs Istio (highly available)",
@@ -650,7 +649,7 @@ func Test_Reconcile(t *testing.T) {
650649
expectedStatusPatched: []client.Object{
651650
gatewayClass("openshift-default", true, nil, installedConditions(), false),
652651
},
653-
expectedSailLibraryOptions: expectedSailLibraryOptions("v1.24.4", false, nil, gatewayclassesConfig(hpaConfig(2), "openshift-default")),
652+
expectedSailLibraryOptions: expectedSailLibraryOptions("v1.24.4", false, nil, gatewayclassesConfig(gatewayclassConfig(2), "openshift-default")),
654653
},
655654
{
656655
name: "Sail Library: installs Istio with system proxy configuration",
@@ -667,7 +666,7 @@ func Test_Reconcile(t *testing.T) {
667666
expectedStatusPatched: []client.Object{
668667
gatewayClass("openshift-default", true, nil, installedConditions(), false),
669668
},
670-
expectedSailLibraryOptions: expectedSailLibraryOptions("v1.24.4", false, expectedProxyConfiguration, gatewayclassesConfig(hpaConfig(2), "openshift-default")),
669+
expectedSailLibraryOptions: expectedSailLibraryOptions("v1.24.4", false, expectedProxyConfiguration, gatewayclassesConfig(gatewayclassConfig(2), "openshift-default")),
671670
},
672671
{
673672
name: "Sail Library: installs Istio for multiple gatewayclasses in single-topology mode with system proxy configuration",
@@ -694,7 +693,7 @@ func Test_Reconcile(t *testing.T) {
694693
expectedStatusPatched: []client.Object{
695694
gatewayClass("openshift-default", true, nil, installedConditions(), false),
696695
},
697-
expectedSailLibraryOptions: expectedSailLibraryOptions("v1.24.4", false, expectedProxyConfiguration, gatewayclassesConfig(hpaConfig(1), "openshift-default", "openshift-internal", "openshift-custom")),
696+
expectedSailLibraryOptions: expectedSailLibraryOptions("v1.24.4", false, expectedProxyConfiguration, gatewayclassesConfig(gatewayclassConfig(1), "openshift-default", "openshift-internal", "openshift-custom")),
698697
},
699698
{
700699
name: "Sail Library: experimental InferencePool CRD",
@@ -715,7 +714,7 @@ func Test_Reconcile(t *testing.T) {
715714
expectedStatusPatched: []client.Object{
716715
gatewayClass("openshift-default", true, nil, installedConditions(), false),
717716
},
718-
expectedSailLibraryOptions: expectedSailLibraryOptions("v1.24.4", true, nil, gatewayclassesConfig(hpaConfig(2), "openshift-default")),
717+
expectedSailLibraryOptions: expectedSailLibraryOptions("v1.24.4", true, nil, gatewayclassesConfig(gatewayclassConfig(2), "openshift-default")),
719718
},
720719
{
721720
name: "Sail Library: stable InferencePool CRD",
@@ -736,7 +735,7 @@ func Test_Reconcile(t *testing.T) {
736735
expectedStatusPatched: []client.Object{
737736
gatewayClass("openshift-default", true, nil, installedConditions(), false),
738737
},
739-
expectedSailLibraryOptions: expectedSailLibraryOptions("v1.24.4", true, nil, gatewayclassesConfig(hpaConfig(2), "openshift-default")),
738+
expectedSailLibraryOptions: expectedSailLibraryOptions("v1.24.4", true, nil, gatewayclassesConfig(gatewayclassConfig(2), "openshift-default")),
740739
},
741740
{
742741
name: "Sail Library: Istio version override",
@@ -756,7 +755,7 @@ func Test_Reconcile(t *testing.T) {
756755
"unsupported.do-not-use.openshift.io/istio-version": "v1.24-latest",
757756
}, installedConditions(), false),
758757
},
759-
expectedSailLibraryOptions: expectedSailLibraryOptions("v1.24-latest", false, nil, gatewayclassesConfig(hpaConfig(2), "openshift-default")),
758+
expectedSailLibraryOptions: expectedSailLibraryOptions("v1.24-latest", false, nil, gatewayclassesConfig(gatewayclassConfig(2), "openshift-default")),
760759
},
761760
{
762761
name: "Sail Library: full removal of last GatewayClass",

‎pkg/operator/controller/gatewayclass/istio_olm.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -237,10 +237,10 @@ func desiredIstio(name types.NamespacedName, ownerRef metav1.OwnerReference, ist
237237
}
238238

239239
if extraConfig.infraConfig != nil {
240-
if hpaConfig, err := buildHorizontalPodAutoscalerConfig(extraConfig.infraConfig, gatewayclasses); err != nil {
240+
if gwClassConfig, err := buildGatewayClassesConfig(extraConfig.infraConfig, gatewayclasses); err != nil {
241241
return nil, err
242242
} else {
243-
istio.Spec.Values.GatewayClasses = hpaConfig
243+
istio.Spec.Values.GatewayClasses = gwClassConfig
244244
}
245245
}
246246
}

‎pkg/operator/controller/gatewayclass/istio_sail_installer.go‎

Lines changed: 26 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -210,10 +210,10 @@ func openshiftValues(enableInferenceExtension bool, operandNamespace string, gat
210210
}
211211
}
212212
if extraConfig.infraConfig != nil {
213-
if hpaConfig, err := buildHorizontalPodAutoscalerConfig(extraConfig.infraConfig, gatewayclasses); err != nil {
214-
return nil, fmt.Errorf("failed to build HPA config: %w", err)
213+
if gwClassConfig, err := buildGatewayClassesConfig(extraConfig.infraConfig, gatewayclasses); err != nil {
214+
return nil, fmt.Errorf("failed to build gateway class config: %w", err)
215215
} else {
216-
val.GatewayClasses = hpaConfig
216+
val.GatewayClasses = gwClassConfig
217217
}
218218
}
219219
}
@@ -244,10 +244,9 @@ func buildProxyMetadata(proxyConfig *configv1.Proxy) map[string]string {
244244
return proxyMetadata
245245
}
246246

247-
// buildHorizontalPodAutoscalerConfig returns Istio configuration for the
248-
// horizontal pod autoscaler given an infrastructure config and a slice of
249-
// gatewayclasses.
250-
func buildHorizontalPodAutoscalerConfig(infraConfig *configv1.Infrastructure, gatewayclasses []gatewayapiv1.GatewayClass) (json.RawMessage, error) {
247+
// buildGatewayClassesConfig returns Istio per-gatewayclass configuration
248+
// overlays given an infrastructure config and a slice of gatewayclasses.
249+
func buildGatewayClassesConfig(infraConfig *configv1.Infrastructure, gatewayclasses []gatewayapiv1.GatewayClass) (json.RawMessage, error) {
251250
const maxReplicas = 10
252251

253252
var minReplicas = 2
@@ -262,6 +261,26 @@ func buildHorizontalPodAutoscalerConfig(infraConfig *configv1.Infrastructure, ga
262261
"maxReplicas": maxReplicas,
263262
},
264263
},
264+
"deployment": map[string]any{
265+
"spec": map[string]any{
266+
"template": map[string]any{
267+
"spec": map[string]any{
268+
"containers": []map[string]any{
269+
{
270+
"name": gatewayProxyContainerName,
271+
// OCPBUGS-55050: Managed cluster should set requests but not limits
272+
// Istio sets resource limits by default, that come from the helm chart.
273+
// This change enforces the resources.limits to be null, so we drop this value
274+
// during the Proxy deployment
275+
"resources": map[string]any{
276+
"limits": nil,
277+
},
278+
},
279+
},
280+
},
281+
},
282+
},
283+
},
265284
}
266285
gatewayclassesConfig := map[string]any{}
267286
for _, gatewayclass := range gatewayclasses {

‎test/e2e/gateway_api_test.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1574,6 +1574,7 @@ func ensureGatewayObjectSuccess(t *testing.T, ns *corev1.Namespace) []string {
15741574
expectedMinReplicas = 1
15751575
}
15761576
assertHorizontalPodAutoscalerEnabled(t, operatorcontroller.DefaultOperandNamespace, testGatewayName, operatorcontroller.OpenShiftDefaultGatewayClassName, expectedMinReplicas)
1577+
assertProxyDeployCustomConfigurations(t, operatorcontroller.DefaultOperandNamespace, testGatewayName, operatorcontroller.OpenShiftDefaultGatewayClassName)
15771578

15781579
t.Log("Making sure the httproute is created and accepted...")
15791580
_, err = assertHttpRouteSuccessful(t, ns.Name, "test-httproute", gateway)

‎test/e2e/util_gatewayapi_test.go‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -811,6 +811,46 @@ func assertHorizontalPodAutoscalerEnabled(t *testing.T, namespaceName, gatewayNa
811811
}
812812
}
813813

814+
// assertProxyDeployCustomConfigurations verifies that the deployment sets
815+
// the right custom configurations on pods.
816+
func assertProxyDeployCustomConfigurations(t *testing.T, namespaceName, gatewayName, gatewayclassName string) {
817+
t.Helper()
818+
819+
deploymentName := types.NamespacedName{
820+
Namespace: namespaceName,
821+
Name: fmt.Sprintf("%s-%s", gatewayName, gatewayclassName),
822+
}
823+
t.Logf("Verifying resource limit is null on the proxy containers of the deployment %s...", deploymentName)
824+
if err := wait.PollUntilContextTimeout(context.Background(), 2*time.Second, 2*time.Minute, false, func(ctx context.Context) (bool, error) {
825+
var dep appsv1.Deployment
826+
if err := kclient.Get(ctx, deploymentName, &dep); err != nil {
827+
t.Logf("Failed to get Deployment %s: %v; retrying...", deploymentName, err)
828+
return false, nil
829+
}
830+
831+
foundIstioProxy := false
832+
for _, c := range dep.Spec.Template.Spec.Containers {
833+
if c.Name == "istio-proxy" {
834+
foundIstioProxy = true
835+
if len(c.Resources.Limits) > 0 {
836+
t.Log("Container 'istio-proxy' has resources.limits, expected no limits set; retrying...")
837+
return false, nil
838+
}
839+
}
840+
}
841+
842+
// We need to be sure that the istio-proxy exists, and that it has no resources.limits.
843+
// If we cannot find istio-proxy, we must try again
844+
if !foundIstioProxy {
845+
t.Log("Container 'istio-proxy' was not found; retrying...")
846+
return false, nil
847+
}
848+
return true, nil
849+
}); err != nil {
850+
t.Errorf("Failed to verify terminationMessagePolicy on deployment %s: %v", deploymentName, err)
851+
}
852+
}
853+
814854
func waitForGatewayListenerCondition(t *testing.T, gatewayName types.NamespacedName, listenerName string, conditions ...metav1.Condition) error {
815855
t.Helper()
816856

0 commit comments

Comments
 (0)