From d94c3757b92ecbedfb88eab8d303a8304831f229 Mon Sep 17 00:00:00 2001 From: David Jumani Date: Tue, 11 Feb 2025 14:19:49 -0500 Subject: [PATCH 1/4] [1.18] fix: gwp does not respect image variant (#10604) --- .../v1.18.7/fix-gwp-fips-distroless.yaml | 5 ++ install/helm/gloo/templates/_gg-helpers.tpl | 8 +-- install/helm/gloo/templates/_helpers.tpl | 66 ++++++++++++------- install/test/k8sgateway_test.go | 51 ++++++++++++++ 4 files changed, 101 insertions(+), 29 deletions(-) create mode 100644 changelog/v1.18.7/fix-gwp-fips-distroless.yaml diff --git a/changelog/v1.18.7/fix-gwp-fips-distroless.yaml b/changelog/v1.18.7/fix-gwp-fips-distroless.yaml new file mode 100644 index 00000000000..e34746f5d46 --- /dev/null +++ b/changelog/v1.18.7/fix-gwp-fips-distroless.yaml @@ -0,0 +1,5 @@ +changelog: +- type: FIX + issueLink: https://github.com/solo-io/gloo/issues/10602 + resolvesIssue: false + description: Fixes the gateway params image to respect the fips and distroless variants specified by global.image.variant. This only applies to the kubernetes gateway proxy. diff --git a/install/helm/gloo/templates/_gg-helpers.tpl b/install/helm/gloo/templates/_gg-helpers.tpl index 2a5241a76c2..55095a5a401 100644 --- a/install/helm/gloo/templates/_gg-helpers.tpl +++ b/install/helm/gloo/templates/_gg-helpers.tpl @@ -42,18 +42,18 @@ Images valid for the GatewayParameters ref Image api in projects/gateway2/api/v1alpha1/kube/container.proto */}} {{- define "gloo-gateway.gatewayParametersImage" -}} -{{- $image := . -}} +{{ $image := . }} {{- if $image.registry }} registry: {{ $image.registry }} {{- end -}}{{/* if $image.registry */}} {{- if $image.repository }} -repository: {{ $image.repository }} +repository: {{ template "gloo.image.repository" $image }} {{- end -}}{{/* if $image.repository */}} {{- if $image.tag }} -tag: {{ $image.tag }} +tag: {{ template "gloo.image.tag" $image }} {{- end -}}{{/* if $image.tag */}} {{- if $image.digest }} -digest: {{ $image.digest }} +digest: {{ template "gloo.image.digest" $image }} {{- end -}}{{/* if $image.digest */}} {{- if $image.pullPolicy }} pullPolicy: {{ $image.pullPolicy }} diff --git a/install/helm/gloo/templates/_helpers.tpl b/install/helm/gloo/templates/_helpers.tpl index af7bf19e4b8..cfb9454252a 100644 --- a/install/helm/gloo/templates/_helpers.tpl +++ b/install/helm/gloo/templates/_helpers.tpl @@ -25,59 +25,75 @@ ClusterRole {{- end -}} {{- end -}} -{{/* -Construct a container image name from a registry, repository, tag, and digest. -*/}} -{{- define "gloo.image" -}} -{{- $image := printf "%s/%s" .registry .repository -}} - +{{- define "gloo.image.repository" -}} {{- /* for fips or fips-distroless variants: add -fips to the image repo (name) */ -}} +{{- if .repository -}} +{{- $repository := .repository -}} {{- if or .fips (has .variant (list "fips" "fips-distroless")) -}} {{- $fipsSupportedImages := list "gloo-ee" "extauth-ee" "gloo-ee-envoy-wrapper" "rate-limit-ee" "discovery-ee" "sds-ee" -}} {{- if (has .repository $fipsSupportedImages) -}} -{{- $image = printf "%s-fips" $image -}} +{{- $repository = printf "%s-fips" $repository -}} {{- end -}}{{- /* if (has .repository $fipsSupportedImages) */ -}} {{- end -}}{{- /* if or .fips (has .variant (list "fips" "fips-distroless")) */ -}} +{{ $repository }} +{{- end -}}{{- /* if .repository */ -}} +{{- end -}}{{- /* define "gloo.image.repository" */ -}} -{{- /* -add tag, if it exists -*/ -}} +{{- define "gloo.image.tag" -}} {{- if .tag -}} -{{- $image = printf "%s:%s" $image .tag -}} -{{- end -}}{{- /* if .tag */ -}} - +{{- $tag := .tag -}} {{- /* for distroless or fips-distroless variants: add -distroless to the tag */ -}} {{- if and .tag (has .variant (list "distroless" "fips-distroless")) -}} {{- $distrolessSupportedImages := list "gloo" "gloo-envoy-wrapper" "discovery" "sds" "certgen" "kubectl" "access-logger" "ingress" "gloo-ee" "extauth-ee" "gloo-ee-envoy-wrapper" "rate-limit-ee" "discovery-ee" "sds-ee" "observability-ee" "caching-ee" -}} {{- if (has .repository $distrolessSupportedImages) -}} -{{- $image = printf "%s-distroless" $image -}} {{- /* Add distroless suffix to the tag since it contains the same binaries in a different container */ -}} +{{- $tag = printf "%s-distroless" $tag -}} {{- /* Add distroless suffix to the tag since it contains the same binaries in a different container */ -}} {{- end -}}{{- /* if (has .repository $distrolessSupportedImages) */ -}} {{- end -}}{{- /* if and .tag (has .variant (list "distroless" "fips-distroless")) */ -}} +{{ $tag }} +{{- end -}}{{- /* if .tag */ -}} +{{- end -}}{{- /* define "gloo.image.tag" */ -}} -{{- /* -add digest for the chosen variant, if it exists -*/ -}} +{{- define "gloo.image.digest" -}} +{{- $digest := "" -}} {{- if or .fips (eq .variant "fips") -}} {{- if .fipsDigest -}} - {{- $image = printf "%s@%s" $image .fipsDigest -}} + {{- $digest = .fipsDigest -}} {{- end -}}{{- /* if .fipsDigest */ -}} {{- else if eq .variant "distroless" -}} {{- if .distrolessDigest -}} - {{- $image = printf "%s@%s" $image .distrolessDigest -}} + {{- $digest = .distrolessDigest -}} {{- end -}}{{- /* if .distrolessDigest */ -}} {{- else if eq .variant "fips-distroless" -}} {{- if .fipsDistrolessDigest -}} - {{- $image = printf "%s@%s" $image .fipsDistrolessDigest -}} + {{- $digest = .fipsDistrolessDigest -}} {{- end -}}{{- /* if .fipsDistrolessDigest */ -}} {{- else -}} {{- if .digest -}}{{- /* standard image digest */ -}} - {{- $image = printf "%s@%s" $image .digest -}} + {{- $digest = .digest -}} {{- end -}}{{- /* if .digest */ -}} {{- end -}} +{{ $digest }} +{{- end -}}{{- /* define "gloo.image.digest" */ -}} + + +{{/* +Construct a container image name from a registry, repository, tag, and digest. +*/}} +{{- define "gloo.image" -}} +{{- $repository := include "gloo.image.repository" . -}} +{{- $image := printf "%s/%s" .registry $repository -}} +{{- $tag := include "gloo.image.tag" . -}} +{{- if $tag -}} +{{- $image = printf "%s:%s" $image $tag -}} +{{- end -}}{{- /* if .tag */ -}} +{{- $digest := include "gloo.image.digest" . -}} +{{- if $digest -}} +{{- $image = printf "%s@%s" $image $digest -}} +{{- end -}}{{- /* if .digest */ -}} {{ $image }} {{- end -}}{{- /* define "gloo.image" */ -}} @@ -170,7 +186,7 @@ It takes 4 values: .defaults - the default securityContext for the pod or container .globalSec - global security settings, usually from .Values.global.securitySettings .indent - the number of spaces to indent the output. If not set, the output will not be indented. - The indentation argument is necessary because it is possible that no output will be rendered. + The indentation argument is necessary because it is possible that no output will be rendered. If that happens and the caller handles the indentation the result will be a line of whitespace, which gets caught by the whitespace tests Depending upon the value of .values.merge, the securityContext will be merged with the defaults or completely replaced. @@ -234,7 +250,7 @@ It takes 4 values: .podSecurityStandards - podSecurityStandard from values.yaml .globalSec - global security settings, usually from .Values.global.securitySettings .indent - the number of spaces to indent the output. If not set, the output will not be indented. - The indentation argument is necessary because it is possible that no output will be rendered. + The indentation argument is necessary because it is possible that no output will be rendered. If that happens and the caller handles the indentation the result will be a line of whitespace, which gets caught by the whitespace tests If .podSecurityStandards.container.enableRestrictedContainerDefaults is true, the defaults will be set to a restricted set of values. @@ -260,7 +276,7 @@ It takes 4 values: {{- end -}} {{- /* set default seccompProfileType */ -}} -{{- $pss_restricted_defaults := dict +{{- $pss_restricted_defaults := dict "runAsNonRoot" true "capabilities" (dict "drop" (list "ALL")) "allowPrivilegeEscalation" false }} @@ -280,7 +296,7 @@ It takes 4 values: {{- end -}} {{- end -}} {{- /* call general securityContext template */ -}} -{{- include "gloo.securityContext" (dict +{{- include "gloo.securityContext" (dict "values" $values "defaults" $defaults "indent" $indent diff --git a/install/test/k8sgateway_test.go b/install/test/k8sgateway_test.go index fdcb4e2b20c..c2be672a433 100644 --- a/install/test/k8sgateway_test.go +++ b/install/test/k8sgateway_test.go @@ -487,6 +487,57 @@ var _ = Describe("Kubernetes Gateway API integration", func() { }) }) }) + + Context("distroless and fips", func() { + DescribeTable("Uses the correct image for the sds-ee container", func(variant string, expectedImage string) { + extraValueArgs := []string{ + "kubeGateway.gatewayParameters.glooGateway.sdsContainer.image.registry=my-sds-reg", + "kubeGateway.gatewayParameters.glooGateway.sdsContainer.image.tag=my-sds-tag", + "kubeGateway.gatewayParameters.glooGateway.sdsContainer.image.repository=sds-ee", + "global.image.variant=" + variant, + } + valuesArgs = append(valuesArgs, extraValueArgs...) + // Updated values so need to re-render + prepareHelmManifest(namespace, glootestutils.HelmValues{ValuesArgs: valuesArgs}) + + gwp := getDefaultGatewayParameters(testManifest) + gwpKube := gwp.Spec.Kube + Expect(gwpKube).ToNot(BeNil()) + sdsContainer := gwpKube.SdsContainer.Image + image := fmt.Sprintf("%s/%s:%s", *sdsContainer.Registry, *sdsContainer.Repository, *sdsContainer.Tag) + Expect(image).To(Equal(expectedImage)) + }, + Entry("No variant specified", "", "my-sds-reg/sds-ee:my-sds-tag"), + Entry("Standard variant", "standard", "my-sds-reg/sds-ee:my-sds-tag"), + Entry("Fips variant", "fips", "my-sds-reg/sds-ee-fips:my-sds-tag"), + Entry("Distroless variant", "distroless", "my-sds-reg/sds-ee:my-sds-tag-distroless"), + Entry("Fips-Distroless variant", "fips-distroless", "my-sds-reg/sds-ee-fips:my-sds-tag-distroless")) + + DescribeTable("Uses the correct image for the gloo-ee-envoy-wrapper container", func(variant string, expectedImage string) { + extraValueArgs := []string{ + "kubeGateway.gatewayParameters.glooGateway.envoyContainer.image.registry=my-gloo-ee-envoy-wrapper-reg", + "kubeGateway.gatewayParameters.glooGateway.envoyContainer.image.tag=my-gloo-ee-envoy-wrapper-tag", + "kubeGateway.gatewayParameters.glooGateway.envoyContainer.image.repository=gloo-ee-envoy-wrapper", + "global.image.variant=" + variant, + } + valuesArgs = append(valuesArgs, extraValueArgs...) + // Updated values so need to re-render + prepareHelmManifest(namespace, glootestutils.HelmValues{ValuesArgs: valuesArgs}) + + gwp := getDefaultGatewayParameters(testManifest) + gwpKube := gwp.Spec.Kube + Expect(gwpKube).ToNot(BeNil()) + envoyContainer := gwpKube.EnvoyContainer.Image + image := fmt.Sprintf("%s/%s:%s", *envoyContainer.Registry, *envoyContainer.Repository, *envoyContainer.Tag) + Expect(image).To(Equal(expectedImage)) + }, + Entry("No variant specified", "", "my-gloo-ee-envoy-wrapper-reg/gloo-ee-envoy-wrapper:my-gloo-ee-envoy-wrapper-tag"), + Entry("Standard variant", "standard", "my-gloo-ee-envoy-wrapper-reg/gloo-ee-envoy-wrapper:my-gloo-ee-envoy-wrapper-tag"), + Entry("Fips variant", "fips", "my-gloo-ee-envoy-wrapper-reg/gloo-ee-envoy-wrapper-fips:my-gloo-ee-envoy-wrapper-tag"), + Entry("Distroless variant", "distroless", "my-gloo-ee-envoy-wrapper-reg/gloo-ee-envoy-wrapper:my-gloo-ee-envoy-wrapper-tag-distroless"), + Entry("Fips-Distroless variant", "fips-distroless", "my-gloo-ee-envoy-wrapper-reg/gloo-ee-envoy-wrapper-fips:my-gloo-ee-envoy-wrapper-tag-distroless")) + + }) }) When("kube gateway integration is disabled (default)", func() { From 3b1b1b28d34d4fb69b988b85147a3a4a9b71e5ea Mon Sep 17 00:00:00 2001 From: Seth Heidkamp <61526534+sheidkamp@users.noreply.github.com> Date: Mon, 17 Feb 2025 14:51:37 -0500 Subject: [PATCH 2/4] 1.18.x linter cleanup (#10628) --- .golangci.yaml | 21 +++++---------------- changelog/v1.18.8/linter-yaml-changes.yaml | 7 +++++++ 2 files changed, 12 insertions(+), 16 deletions(-) create mode 100644 changelog/v1.18.8/linter-yaml-changes.yaml diff --git a/.golangci.yaml b/.golangci.yaml index 871f3450d11..0512f9316c4 100644 --- a/.golangci.yaml +++ b/.golangci.yaml @@ -79,24 +79,13 @@ run: # If we find that the job is timing out, we can explore ways to make this job run faster, or increase the timeout. timeout: 10m - skip-dirs: - # don't lint ruleguard files - - test/rules - - # don't lint gomock intermediate files - - 'gomock_reflect_\d*' - # output configuration options output: - # Format: colored-line-number|line-number|json|colored-tab|tab|checkstyle|code-climate|junit-xml|github-actions|teamcity - # - # Multiple can be specified by separating them by comma, output can be provided - # for each of them by separating format name and path by colon symbol. - # Output path can be either `stdout`, `stderr` or path to the file to write to. - # Example: "checkstyle:report.xml,json:stdout,colored-line-number" - # - # Default: colored-line-number - format: colored-line-number + # Use default format + # Default: + # formats: + # - format: colored-line-number + # path: stdout # Print lines of code with issue. print-issued-lines: true diff --git a/changelog/v1.18.8/linter-yaml-changes.yaml b/changelog/v1.18.8/linter-yaml-changes.yaml new file mode 100644 index 00000000000..5fae59c6a32 --- /dev/null +++ b/changelog/v1.18.8/linter-yaml-changes.yaml @@ -0,0 +1,7 @@ +changelog: + - type: NON_USER_FACING + resolvesIssue: false + description: >- + Updates for golangci-lint-action breaking changes in `6.5.0` + skipCI-kube-tests:true + skipCI-docs-build:true From 1a6a2d974e70d69759d5e992d1cdbc1503181cdc Mon Sep 17 00:00:00 2001 From: "Ariana W." Date: Mon, 17 Feb 2025 16:07:06 -0500 Subject: [PATCH 3/4] [Backport v1.18.x] Fix glooctl check and TestOneWayServerTlsFailsWithoutOneWayTls test flakes (#10622) Co-authored-by: Ryan Old --- .../v1.18.8/fix_glooctl-check_test-flake.yaml | 10 ++++++ projects/gloo/cli/pkg/cmd/check/gloo_stats.go | 11 ++++-- .../gloo/cli/pkg/cmd/check/gloo_stats_test.go | 7 ++-- projects/gloo/cli/pkg/cmd/check/root.go | 2 +- .../e2e/features/glooctl/check_suite.go | 36 +++++++++++-------- .../e2e/features/server_tls/k8s_suite.go | 3 +- 6 files changed, 47 insertions(+), 22 deletions(-) create mode 100644 changelog/v1.18.8/fix_glooctl-check_test-flake.yaml diff --git a/changelog/v1.18.8/fix_glooctl-check_test-flake.yaml b/changelog/v1.18.8/fix_glooctl-check_test-flake.yaml new file mode 100644 index 00000000000..07bc89d7ab3 --- /dev/null +++ b/changelog/v1.18.8/fix_glooctl-check_test-flake.yaml @@ -0,0 +1,10 @@ +changelog: +- type: NON_USER_FACING + issueLink: https://github.com/solo-io/solo-projects/issues/6619 + description: fixes flaky enterprise glooctl check tests + resolvesIssue: false +- type: NON_USER_FACING + description: | + Adjusted timeout in an effort to reduce failures of these tests. + issueLink: https://github.com/solo-io/solo-projects/issues/7685 + resolvesIssue: false diff --git a/projects/gloo/cli/pkg/cmd/check/gloo_stats.go b/projects/gloo/cli/pkg/cmd/check/gloo_stats.go index 4e20ca19912..b8bd0dff572 100644 --- a/projects/gloo/cli/pkg/cmd/check/gloo_stats.go +++ b/projects/gloo/cli/pkg/cmd/check/gloo_stats.go @@ -34,7 +34,7 @@ var ( customGlooDeploymentName = helpers.GlooDeploymentName ) -func ResourcesSyncedOverXds(stats, deploymentName string) bool { +func ResourcesSyncedOverXds(printer printers.P, stats, deploymentName string) bool { var outOfSyncResources []string metrics := parseMetrics(stats, []string{glooeTotalEntites, glooeInSyncEntities}, deploymentName) for metric, val := range metrics { @@ -51,6 +51,12 @@ func ResourcesSyncedOverXds(stats, deploymentName string) bool { fmt.Println(resourcesOutOfSyncMessage(outOfSyncResources)) return false } + + if len(metrics) == 0 { + printer.AppendStatus("xds metrics", "No xds metrics to check") + } else { + printer.AppendStatus("xds metrics", "OK") + } return true } @@ -69,6 +75,7 @@ func RateLimitIsConnected(stats string) bool { } func checkXdsMetrics(ctx context.Context, printer printers.P, opts *options.Options, deployments *appsv1.DeploymentList) error { + printer.AppendCheck("Checking xds metrics... ") errMessage := "Problem while checking for gloo xds errors" if deployments == nil { fmt.Println("Skipping due to an error in checking deployments") @@ -103,7 +110,7 @@ func checkXdsMetrics(ctx context.Context, printer printers.P, opts *options.Opti return fmt.Errorf(err) } - if !ResourcesSyncedOverXds(stats, customGlooDeploymentName) { + if !ResourcesSyncedOverXds(printer, stats, customGlooDeploymentName) { fmt.Println(errMessage) return fmt.Errorf(errMessage) } diff --git a/projects/gloo/cli/pkg/cmd/check/gloo_stats_test.go b/projects/gloo/cli/pkg/cmd/check/gloo_stats_test.go index 99f5fb20cf0..fbc1f1fdf85 100644 --- a/projects/gloo/cli/pkg/cmd/check/gloo_stats_test.go +++ b/projects/gloo/cli/pkg/cmd/check/gloo_stats_test.go @@ -4,6 +4,7 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" "github.com/solo-io/gloo/projects/gloo/cli/pkg/cmd/check" + "github.com/solo-io/gloo/projects/gloo/cli/pkg/printers" ) var _ = Describe("Gloo Stats", func() { @@ -14,7 +15,7 @@ var _ = Describe("Gloo Stats", func() { Context("check resources in sync", func() { It("returns true when there are no stats", func() { - result := check.ResourcesSyncedOverXds("", deploymentName) + result := check.ResourcesSyncedOverXds(printers.P{}, "", deploymentName) Expect(result).To(BeTrue()) }) @@ -38,7 +39,7 @@ glooe_solo_io_xds_total_entities{resource="type.googleapis.com/envoy.api.v2.List glooe_solo_io_xds_total_entities{resource="type.googleapis.com/envoy.api.v2.RouteConfiguration"} 1 glooe_solo_io_xds_total_entities{resource="type.googleapis.com/glooe.solo.io.RateLimitConfig"} 1 ` - result := check.ResourcesSyncedOverXds(stats, deploymentName) + result := check.ResourcesSyncedOverXds(printers.P{}, stats, deploymentName) Expect(result).To(BeFalse()) }) @@ -59,7 +60,7 @@ glooe_solo_io_xds_total_entities{resource="type.googleapis.com/envoy.api.v2.List glooe_solo_io_xds_total_entities{resource="type.googleapis.com/envoy.api.v2.RouteConfiguration"} 1 glooe_solo_io_xds_total_entities{resource="type.googleapis.com/glooe.solo.io.RateLimitConfig"} 1 ` - result := check.ResourcesSyncedOverXds(stats, deploymentName) + result := check.ResourcesSyncedOverXds(printers.P{}, stats, deploymentName) Expect(result).To(BeTrue()) }) }) diff --git a/projects/gloo/cli/pkg/cmd/check/root.go b/projects/gloo/cli/pkg/cmd/check/root.go index cb89c5065bb..b20c8ffe84d 100644 --- a/projects/gloo/cli/pkg/cmd/check/root.go +++ b/projects/gloo/cli/pkg/cmd/check/root.go @@ -952,7 +952,7 @@ func checkSecrets(ctx context.Context, printer printers.P, _ *options.Options, n client, err := helpers.GetSecretClient(ctx, namespaces) if err != nil { multiErr = multierror.Append(multiErr, err) - printer.AppendStatus("secrets", fmt.Sprintf("%v Errors!", multiErr.Len())) + printer.AppendStatus("Secrets", fmt.Sprintf("%v Errors!", multiErr.Len())) return multiErr } diff --git a/test/kubernetes/e2e/features/glooctl/check_suite.go b/test/kubernetes/e2e/features/glooctl/check_suite.go index 360c9018d05..1e0f7c46aec 100644 --- a/test/kubernetes/e2e/features/glooctl/check_suite.go +++ b/test/kubernetes/e2e/features/glooctl/check_suite.go @@ -12,6 +12,9 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) +// TODO(nfuden): Fix clusterloadassignment issues that forced xds-metrics to be excluded. +// Consider https://github.com/envoyproxy/envoy/issues/7529#issuecomment-1227724217 + var _ e2e.NewSuiteFunc = NewCheckSuite // checkSuite contains the set of tests to validate the behavior of `glooctl check` @@ -23,9 +26,7 @@ type checkSuite struct { testInstallation *e2e.TestInstallation } -// NewChecksuite for glooctl check validation -// TODO(nfuden): Fix clusterloadassignment issues that forced xds-metrics to be excluded. -// Consider https://github.com/envoyproxy/envoy/issues/7529#issuecomment-1227724217 +// NewCheckSuite for glooctl check validation func NewCheckSuite(ctx context.Context, testInst *e2e.TestInstallation) suite.TestingSuite { return &checkSuite{ ctx: ctx, @@ -34,6 +35,7 @@ func NewCheckSuite(ctx context.Context, testInst *e2e.TestInstallation) suite.Te } func (s *checkSuite) TestCheck() { + // exclude xds-metrics check due to Envoy bug: see TODO at top of file output, err := s.testInstallation.Actions.Glooctl().Check(s.ctx, "-n", s.testInstallation.Metadata.InstallNamespace, "-x", "xds-metrics") s.NoError(err) @@ -50,6 +52,7 @@ func (s *checkSuite) TestCheck() { } func (s *checkSuite) TestCheckExclude() { + // exclude xds-metrics check due to Envoy bug: see TODO at top of file for excludeKey, expectedOutput := range checkCommonGlooGatewayOutputByKey { output, err := s.testInstallation.Actions.Glooctl().Check(s.ctx, "-n", s.testInstallation.Metadata.InstallNamespace, "-x", fmt.Sprintf("xds-metrics,%s", excludeKey)) @@ -68,6 +71,7 @@ func (s *checkSuite) TestCheckExclude() { } func (s *checkSuite) TestCheckReadOnly() { + // exclude xds-metrics check due to Envoy bug: see TODO at top of file output, err := s.testInstallation.Actions.Glooctl().Check(s.ctx, "-n", s.testInstallation.Metadata.InstallNamespace, "--read-only", "-x", "xds-metrics") s.NoError(err) @@ -92,11 +96,12 @@ func (s *checkSuite) TestCheckReadOnly() { func (s *checkSuite) TestCheckKubeContext() { // When passing an invalid kube-context, `glooctl check` should succeed _, err := s.testInstallation.Actions.Glooctl().Check(s.ctx, - "-n", s.testInstallation.Metadata.InstallNamespace, "--kube-context", "invalid-context") + "-n", s.testInstallation.Metadata.InstallNamespace, "--kube-context", "invalid-context", "-x", "xds-metrics") s.Error(err) s.Contains(err.Error(), "Could not get kubernetes client: Error retrieving Kubernetes configuration: context \"invalid-context\" does not exist") // When passing the kube-context of the running cluster, `glooctl check` should succeed + // exclude xds-metrics check due to Envoy bug: see TODO at top of file _, err = s.testInstallation.Actions.Glooctl().Check(s.ctx, "-n", s.testInstallation.Metadata.InstallNamespace, "--kube-context", s.testInstallation.ClusterContext.KubeContext, "-x", "xds-metrics") s.NoError(err) @@ -110,7 +115,7 @@ func (s *checkSuite) TestCheckTimeout() { s.NoError(err) _, err = s.testInstallation.Actions.Glooctl().Check(s.ctx, - "-n", s.testInstallation.Metadata.InstallNamespace, + "-n", s.testInstallation.Metadata.InstallNamespace, "-x", "xds-metrics", "-c", shortTimeoutValues.Name()) s.Error(err) s.Contains(err.Error(), "context deadline exceeded") @@ -121,30 +126,28 @@ func (s *checkSuite) TestCheckTimeout() { _, err = normalTimeoutValues.Write([]byte(`checkTimeoutSeconds: 300s`)) s.NoError(err) - // Note: This test does not use `"-x", "xds-metrics"`, so it will require the xds-metrics check to pass with no errors - // In gloo-ee the prometheus metrics must be enabled for the check to pass + // exclude xds-metrics due to Envoy bug: see TODO at top of file _, err = s.testInstallation.Actions.Glooctl().Check(s.ctx, - "-n", s.testInstallation.Metadata.InstallNamespace, + "-n", s.testInstallation.Metadata.InstallNamespace, "-x", "xds-metrics", "-c", normalTimeoutValues.Name()) s.NoError(err) } func (s *checkSuite) TestCheckNamespace() { // namespace does not exist - output, err := s.testInstallation.Actions.Glooctl().Check(s.ctx, "-n", "not-gloo-system") + output, err := s.testInstallation.Actions.Glooctl().Check(s.ctx, "-n", "not-gloo-system", "-x", "xds-metrics") s.Error(err) s.Contains(output, "Could not communicate with kubernetes cluster: namespaces \"not-gloo-system\" not found") // gloo not in namespace - output, err = s.testInstallation.Actions.Glooctl().Check(s.ctx, "-n", "default") + output, err = s.testInstallation.Actions.Glooctl().Check(s.ctx, "-n", "default", "-x", "xds-metrics") s.Error(err) s.Contains(output, "Warning: The provided label selector (gloo) applies to no pods") // pod does not exist - // Note: This test does not use `"-x", "xds-metrics"`, so it will require the xds-metrics check to pass with no errors - // In gloo-ee the prometheus metrics must be enabled for the check to pass + // exclude xds-metrics check due to Envoy bug: see TODO at top of file output, err = s.testInstallation.Actions.Glooctl().Check(s.ctx, - "-n", s.testInstallation.Metadata.InstallNamespace, + "-n", s.testInstallation.Metadata.InstallNamespace, "-x", "xds-metrics", "-p", "not-gloo") s.NoError(err) s.Contains(output, "Warning: The provided label selector (not-gloo) applies to no pods") @@ -152,7 +155,7 @@ func (s *checkSuite) TestCheckNamespace() { // resource namespace does not exist output, err = s.testInstallation.Actions.Glooctl().Check(s.ctx, - "-n", s.testInstallation.Metadata.InstallNamespace, + "-n", s.testInstallation.Metadata.InstallNamespace, "-x", "xds-metrics", "-r", "not-gloo-system") s.Error(err) s.Contains(output, fmt.Sprintf("No namespaces specified are currently being watched (defaulting to '%s' namespace)", s.testInstallation.Metadata.InstallNamespace)) @@ -192,6 +195,7 @@ func (s *checkSuite) TestNoGateways() { Namespace: s.testInstallation.Metadata.InstallNamespace, }, gomega.Equal(0)) + // exclude xds-metrics check due to Envoy bug: see TODO at top of file _, err = s.testInstallation.Actions.Glooctl().Check(s.ctx, "-n", s.testInstallation.Metadata.InstallNamespace, "--kube-context", s.testInstallation.ClusterContext.KubeContext, "-x", "xds-metrics") s.Error(err) @@ -219,6 +223,7 @@ func (s *checkSuite) TestEdgeGatewayScaled() { Namespace: s.testInstallation.Metadata.InstallNamespace, }, gomega.Equal(0)) + // exclude xds-metrics check due to Envoy bug: see TODO at top of file output, err := s.testInstallation.Actions.Glooctl().Check(s.ctx, "-n", s.testInstallation.Metadata.InstallNamespace, "--kube-context", s.testInstallation.ClusterContext.KubeContext, "-x", "xds-metrics") s.NoError(err) @@ -252,9 +257,10 @@ func (s *checkSuite) TestEdgeResourceError() { s.NoError(err) // Run check. This needs to run in eventually to get all errors reported + // exclude xds-metrics check due to Envoy bug: see TODO at top of file gomega.Eventually(func() error { _, err = s.testInstallation.Actions.Glooctl().Check(s.ctx, - "-n", s.testInstallation.Metadata.InstallNamespace, "--kube-context", s.testInstallation.ClusterContext.KubeContext) + "-n", s.testInstallation.Metadata.InstallNamespace, "--kube-context", s.testInstallation.ClusterContext.KubeContext, "-x", "xds-metrics") return err }, time.Minute*2, time.Second*10).Should(gomega.SatisfyAll( gomega.MatchError(gomega.ContainSubstring(fmt.Sprintf("* Found rejected virtual service by '%s': default reject-me-too (Reason: 2 errors occurred:", s.testInstallation.Metadata.InstallNamespace))), diff --git a/test/kubernetes/e2e/features/server_tls/k8s_suite.go b/test/kubernetes/e2e/features/server_tls/k8s_suite.go index 3cad9bd3b73..6f7deeaa8f0 100644 --- a/test/kubernetes/e2e/features/server_tls/k8s_suite.go +++ b/test/kubernetes/e2e/features/server_tls/k8s_suite.go @@ -131,5 +131,6 @@ func (s *k8sServerTlsTestingSuite) assertEventualError(hostHeaderValue string, c Container: "curl", }, append(curlOptions("gloo-proxy-gw", s.ns, hostHeaderValue), curl.WithPath("/status/200")), - code) + code, + time.Minute*2) } From cb6714d336d4a51b7d299355648f5104980b07c2 Mon Sep 17 00:00:00 2001 From: Steven Landow Date: Tue, 18 Feb 2025 12:44:34 -0800 Subject: [PATCH 4/4] [1.18] listener options merge what translator generates #10578 (#10635) --- changelog/v1.18.9/merge-listener-options.yaml | 8 ++++++ .../listener_options_plugin.go | 7 ++++- .../listener_options_plugin_test.go | 16 +++++++++--- projects/gloo/pkg/utils/merge.go | 26 +++++++++++++++++++ 4 files changed, 52 insertions(+), 5 deletions(-) create mode 100644 changelog/v1.18.9/merge-listener-options.yaml diff --git a/changelog/v1.18.9/merge-listener-options.yaml b/changelog/v1.18.9/merge-listener-options.yaml new file mode 100644 index 00000000000..f873258b4ae --- /dev/null +++ b/changelog/v1.18.9/merge-listener-options.yaml @@ -0,0 +1,8 @@ +changelog: + - type: NON_USER_FACING + issueLink: https://github.com/solo-io/solo-projects/issues/7300 + resolvesIssue: true + description: >- + If we generate any ListenerOptions from a Gateway to Proxy + translator, they will no longer be overridden by user + ListenerOptions. diff --git a/projects/gateway2/translator/plugins/listeneroptions/listener_options_plugin.go b/projects/gateway2/translator/plugins/listeneroptions/listener_options_plugin.go index 24fd6401c02..5ff8a686886 100644 --- a/projects/gateway2/translator/plugins/listeneroptions/listener_options_plugin.go +++ b/projects/gateway2/translator/plugins/listeneroptions/listener_options_plugin.go @@ -7,6 +7,7 @@ import ( "github.com/solo-io/gloo/projects/gateway2/translator/plugins" lisquery "github.com/solo-io/gloo/projects/gateway2/translator/plugins/listeneroptions/query" v1 "github.com/solo-io/gloo/projects/gloo/pkg/api/v1" + "github.com/solo-io/gloo/projects/gloo/pkg/utils" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -47,7 +48,11 @@ func (p *plugin) ApplyListenerPlugin( // use the first option (highest in priority) // see for more context: https://github.com/solo-io/solo-projects/issues/6313 optToUse := attachedOptions[0] - outListener.Options = optToUse.Spec.GetOptions() + if outListener.GetOptions() != nil { + outListener.Options, _ = utils.ShallowMergeListenerOptions(outListener.GetOptions(), optToUse.Spec.GetOptions()) + } else { + outListener.Options = optToUse.Spec.GetOptions() + } return nil } diff --git a/projects/gateway2/translator/plugins/listeneroptions/listener_options_plugin_test.go b/projects/gateway2/translator/plugins/listeneroptions/listener_options_plugin_test.go index 569b76b2092..f1ab8a524ec 100644 --- a/projects/gateway2/translator/plugins/listeneroptions/listener_options_plugin_test.go +++ b/projects/gateway2/translator/plugins/listeneroptions/listener_options_plugin_test.go @@ -16,6 +16,7 @@ import ( "github.com/solo-io/gloo/projects/gateway2/translator/testutils" "github.com/solo-io/gloo/projects/gateway2/wellknown" v1 "github.com/solo-io/gloo/projects/gloo/pkg/api/v1" + "github.com/solo-io/gloo/projects/gloo/pkg/api/v1/options/proxy_protocol" corev1 "github.com/solo-io/skv2/pkg/api/core.skv2.solo.io/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/controller-runtime/pkg/client" @@ -48,12 +49,19 @@ var _ = Describe("ListenerOptions Plugin", func() { }, } - outputListener = &v1.Listener{} + outputListener = &v1.Listener{ + Options: &v1.ListenerOptions{ + ProxyProtocol: &proxy_protocol.ProxyProtocol{}, + }, + } expectedOptions = &v1.ListenerOptions{ + // from config PerConnectionBufferLimitBytes: &wrapperspb.UInt32Value{ Value: uint32(419), }, + // base + ProxyProtocol: &proxy_protocol.ProxyProtocol{}, } }) JustBeforeEach(func() { @@ -102,7 +110,7 @@ var _ = Describe("ListenerOptions Plugin", func() { It("does not add buffer limit", func() { err := plugin.ApplyListenerPlugin(ctx, listenerCtx, outputListener) Expect(err).ToNot(HaveOccurred()) - Expect(outputListener.GetOptions()).To(BeNil()) + Expect(outputListener.GetOptions().GetPerConnectionBufferLimitBytes()).To(BeNil()) }) }) @@ -114,11 +122,10 @@ var _ = Describe("ListenerOptions Plugin", func() { It("does not add buffer limit", func() { err := plugin.ApplyListenerPlugin(ctx, listenerCtx, outputListener) Expect(err).ToNot(HaveOccurred()) - Expect(outputListener.GetOptions()).To(BeNil()) + Expect(outputListener.GetOptions().GetPerConnectionBufferLimitBytes()).To(BeNil()) }) }) }) - }) func attachedListenerOption() *solokubev1.ListenerOption { @@ -144,6 +151,7 @@ func attachedListenerOption() *solokubev1.ListenerOption { }, } } + func attachedListenerOptionWithSectionName() *solokubev1.ListenerOption { listOpt := attachedListenerOption() listOpt.Spec.TargetRefs[0].SectionName = &wrapperspb.StringValue{ diff --git a/projects/gloo/pkg/utils/merge.go b/projects/gloo/pkg/utils/merge.go index d129529b12a..39370a0a83c 100644 --- a/projects/gloo/pkg/utils/merge.go +++ b/projects/gloo/pkg/utils/merge.go @@ -80,6 +80,32 @@ func isEmptyValue(v reflect.Value) bool { return false } +// ShallowMergeListenerOptions merges the top-level fields of src into dst. +// The fields in dst that have non-zero values will not be overwritten. +// It performs a shallow merge of top-level fields only. +// It returns a boolean indicating whether any fields in src overwrote dst. +func ShallowMergeListenerOptions(dst, src *v1.ListenerOptions) (*v1.ListenerOptions, bool) { + if src == nil { + return dst, false + } + + if dst == nil { + return src.Clone().(*v1.ListenerOptions), true + } + + dstValue, srcValue := reflect.ValueOf(dst).Elem(), reflect.ValueOf(src).Elem() + + overwrote := false + for i := range dstValue.NumField() { + dstField, srcField := dstValue.Field(i), srcValue.Field(i) + if srcOverride := ShallowMerge(dstField, srcField); srcOverride { + overwrote = true + } + } + + return dst, overwrote +} + // ShallowMergeRouteOptions merges the top-level fields of src into dst. // The fields in dst that have non-zero values will not be overwritten. // It performs a shallow merge of top-level fields only.