more cleanup #10413

yuval-k · 2024-12-22T02:43:12Z

bring in control plane to gateway
don't start gloo anymore.
use upstream go control plane
remove gloov1 objects from gateway (settings still remain)

Note : now that gloo doesn't start anymore we lose the gloo features that are not the control plane. these come to mind:

stats \ stats page. Gloo uses deprecated opencensus. so this is an opportunity to use otel instead. Stats should be served on their own port.
admin page (for snapshots, pprof, changing log level). I don't recommend using the go-utils admin page as it mixes stats and admin page. In addition i don't think admin page should be in an external dependency. it should be part of this project where it is easy to change.
leader election - we didn't have it before in gateway, but have it even less now. relevant for status writing.
webhook validation
snapshot history (not sure what this one is; might not be needed with the krt and xds snapshots available)
snapshot sanitizers (we can bring these back, though i'm not sure we need to with the changes to envoy over the years)

…atus

don't start gloo anymore. use upstream go control plane remove gloov1 objects from gateway (settings still remain)

…needs to propagate to the listener. also need to make sure we don't override status of other gateway-api controllers

lgadban · 2024-12-23T16:51:22Z

snapshot history (not sure what this one is; might not be needed with the krt and xds snapshots available)

provides GME-style snapshots for debugging etc but is dependent on the gloov1 apiSnapshot schema.
We should replace it with an equivalent solution that is backed by krt and/or xds snapshot (probably both, as we want to capture the input resources which feed krt)

…added gw alpha2 (always add it instead)

lgadban · 2024-12-26T16:15:18Z

projects/gateway2/setup/ggv2setup.go

 	restConfig *rest.Config,
 	uccBuilder krtcollections.UniquelyConnectedClientsBulider,
 	extraPlugins []extensionsplug.Plugin,
+	extraGwClasses []string, // TODO: we can remove this and replace with something that watches all GW classes with our controller name


+1

FWIW, I think the initial point of this was to allow extensions to provide classnames

projects/gloo/pkg/setup/setup.go

lgadban · 2024-12-26T16:24:42Z

projects/gateway2/setup/controlplane.go

+func NewControlPlane(ctx context.Context,
+	bindAddr net.Addr,
+	callbacks xdsserver.Callbacks) (envoycache.SnapshotCache, error) {


style nits: ctx on a new line, return signature on new line; same for NewControlPlaneWithListener

lgadban · 2024-12-26T17:51:20Z

projects/gateway2/setup/ggv2setup.go

@@ -128,23 +146,18 @@ func StartGGv2WithConfig(ctx context.Context,
 	}, krt.WithName("GlooSettingsSingleton"))

 	logger.Info("creating reporter")


Suggested change

logger.Info("creating reporter")

…iServices (kgateway-dev#10413)

lgadban

LGTM, good iteration on cleanup, will continue to be an ongoing process

tests are failing so can reapprove if larger changes are needed to get things passing

yuval-k · 2024-12-31T11:03:50Z

re failing tests:

regression gloo & (old) gateway fail because gloo no longer turned on.
the Kubernetes E2E will need to be fixed one by one, as they have both new gateway and old gloo test, so i figure that can be done in a follow-up.

jenshu · 2025-01-03T16:01:30Z

projects/gateway2/translator/gateway/gateway_translator.go

@@ -1,4 +1,4 @@
-package translator
+package gatewaytranslator


can we name the package gateway (same as directory name) otherwise we will get confused when importing it

jenshu · 2025-01-03T16:01:49Z

projects/gateway2/translator/gateway/gateway_translator_test.go

@@ -1,4 +1,4 @@
-package translator_test
+package gatewaytranslator_test


same comment - gateway_test

jenshu · 2025-01-03T16:06:30Z

projects/gateway2/translator/gateway/translator_case_test.go

@@ -1,4 +1,4 @@
-package translator_test
+package gatewaytranslator_test


Suggested change

package gatewaytranslator_test

package gateway_test

same comment

jenshu · 2025-01-03T17:38:48Z

projects/gateway/pkg/validation/validator.go

@@ -288,8 +289,8 @@ func (v *validator) validateProxiesAndExtensions(ctx context.Context, snapshot *

 	validatingK8sGateway := isK8sGatewayProxy(opts.Resource)
 	if validatingK8sGateway {


this logic seems wrong? why are we translating gloo edge proxies if it's a k8sgateway proxy?

this code will get deleted anyway (its from ggv2 validation where a gateway is translated to a proxy); just didn't want to do this in this PR.

jenshu · 2025-01-03T17:44:55Z

projects/gateway2/extensions2/plugins/destrule/destrule_plugin.go

@@ -26,6 +26,10 @@ const (
 )

 func NewPlugin(ctx context.Context, commoncol *common.CommonCollections) extensionsplug.Plugin {
+	if !commoncol.InitialSettings.Spec.GetGloo().GetIstioOptions().GetEnableIntegration().GetValue() {


can you add a comment here why we return empty plugin if istio integration is enabled?

jenshu · 2025-01-03T18:01:48Z

projects/gateway2/krtcollections/policy_test_utils.go

+
+import "github.com/solo-io/gloo/projects/gateway2/ir"
+
+func (h *RoutesIndex) TEST() []ir.HttpRouteIR {


what is this file for, do we need this?

i can deleted that, i used it temporary for a test

jenshu · 2025-01-03T18:08:21Z

projects/gateway2/proxy_syncer/kube_gw_translator_syncer.go

@@ -33,20 +32,7 @@ func (s *ProxyTranslator) syncXds(
 	// TODO: me may need to copy this to not change krt cache.
 	// TODO: this is also may not be needed now that envoy has
 	// a default initial fetch timeout
-	snap.MakeConsistent()
-	s.xdsCache.SetSnapshot(proxyKey, snap)
+	// snap.MakeConsistent()


do we need to add this back later?

i think not, but we can discuss.
i think we can build the code such that, we no longer need sanitizers

jenshu · 2025-01-03T18:10:49Z

projects/gateway2/proxy_syncer/xdswrapper.go

-	return p.snap.Equal(in.snap)
+	// check that all the versions are the equal
+	for i, r := range p.snap.Resources {
+		if r.Version != in.snap.Resources[i].Version {


are the snapshot resources guaranteed to be sorted?

yes, its an array where the index is the resource type.

jenshu

i'm good with merging so we can iterate

yuval-k added 4 commits December 20, 2024 23:38

empty namespace rejected by api server. make it nil when reporting st…

7b0593c

…atus

bring in control plane to gateway

0ebf0a9

don't start gloo anymore. use upstream go control plane remove gloov1 objects from gateway (settings still remain)

report to parent httproute intead of listner. need to verify if this …

d71c858

…needs to propagate to the listener. also need to make sure we don't override status of other gateway-api controllers

no need for pointer in CallbackFuncs

36ed23d

no need for gloo objects in the scheme, and no need to conditionally …

9aee668

…added gw alpha2 (always add it instead)

lgadban reviewed Dec 26, 2024

View reviewed changes

more cleanups

304b302

tiberiuac pushed a commit to openet/gloo that referenced this pull request Dec 30, 2024

Disabling flaky test TestConfigureTCPRouteBackingDestinationsWithMult…

3bb702e

…iServices (kgateway-dev#10413)

use shared translation logic in tests and in proxy_syncer

a4ae8b6

yuval-k enabled auto-merge (squash) December 30, 2024 16:39

fix test build

38cc38f

lgadban approved these changes Dec 30, 2024

View reviewed changes

jenshu reviewed Jan 3, 2025

View reviewed changes

jenshu approved these changes Jan 3, 2025

View reviewed changes

pr comments

7aa9d56

lgadban approved these changes Jan 3, 2025

View reviewed changes

yuval-k merged commit c3e1651 into main Jan 3, 2025
8 of 17 checks passed

yuval-k deleted the yuval-k/more-cleanup branch January 3, 2025 20:59

jenshu mentioned this pull request Jan 9, 2025

[Epic] Admin page #10416

Open

lgadban mentioned this pull request Jan 14, 2025

audit state of test failures #10447

Closed

timflannagan mentioned this pull request Jan 29, 2025

[Epic] Cleanup Repo #10496

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

more cleanup #10413

more cleanup #10413

yuval-k commented Dec 22, 2024 •

edited

Loading

lgadban commented Dec 23, 2024

lgadban Dec 26, 2024

lgadban Dec 26, 2024

lgadban Dec 26, 2024

lgadban left a comment

yuval-k commented Dec 31, 2024

jenshu Jan 3, 2025

jenshu Jan 3, 2025

jenshu Jan 3, 2025

jenshu Jan 3, 2025

yuval-k Jan 3, 2025

jenshu Jan 3, 2025

jenshu Jan 3, 2025

yuval-k Jan 3, 2025

jenshu Jan 3, 2025

yuval-k Jan 3, 2025

jenshu Jan 3, 2025

yuval-k Jan 3, 2025

jenshu left a comment

		@@ -128,23 +146,18 @@ func StartGGv2WithConfig(ctx context.Context,
		}, krt.WithName("GlooSettingsSingleton"))

		logger.Info("creating reporter")

		@@ -1,4 +1,4 @@
		package translator_test
		package gatewaytranslator_test

		@@ -288,8 +289,8 @@ func (v validator) validateProxiesAndExtensions(ctx context.Context, snapshot

		validatingK8sGateway := isK8sGatewayProxy(opts.Resource)
		if validatingK8sGateway {


		import "github.com/solo-io/gloo/projects/gateway2/ir"

		func (h *RoutesIndex) TEST() []ir.HttpRouteIR {

more cleanup #10413

more cleanup #10413

Conversation

yuval-k commented Dec 22, 2024 • edited Loading

lgadban commented Dec 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgadban left a comment

Choose a reason for hiding this comment

yuval-k commented Dec 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jenshu left a comment

Choose a reason for hiding this comment

yuval-k commented Dec 22, 2024 •

edited

Loading