From 0792a406873e1eaa5d88f95329d1d01de54ecdd8 Mon Sep 17 00:00:00 2001 From: npolshakova Date: Fri, 14 Jun 2024 14:32:13 -0400 Subject: [PATCH 1/5] initial fixes --- .../v1.18.0-beta1/fix-proxy-status-sync.yaml | 11 +++++++++++ projects/gateway2/deployer/values.go | 11 ++++++----- projects/gateway2/deployer/values_helpers.go | 16 ++++++++-------- .../templates/gateway/proxy-deployment.yaml | 9 ++++----- projects/gateway2/status/status_syncer.go | 11 ++++------- 5 files changed, 33 insertions(+), 25 deletions(-) create mode 100644 changelog/v1.18.0-beta1/fix-proxy-status-sync.yaml diff --git a/changelog/v1.18.0-beta1/fix-proxy-status-sync.yaml b/changelog/v1.18.0-beta1/fix-proxy-status-sync.yaml new file mode 100644 index 00000000000..919959eb30a --- /dev/null +++ b/changelog/v1.18.0-beta1/fix-proxy-status-sync.yaml @@ -0,0 +1,11 @@ +changelog: + - type: FIX + issueLink: https://github.com/solo-io/solo-projects/issues/6304 + resolvesIssue: true + description: >- + Fix statuses being synced properly for k8s gateway resources + - type: FIX + issueLink: https://github.com/solo-io/solo-projects/issues/6107 + resolvesIssue: true + description: >- + Follow up to fix discoveryAddress, istioMetaMeshId and istioMetaClusterId in k8s gateway deployment for Istio integration. diff --git a/projects/gateway2/deployer/values.go b/projects/gateway2/deployer/values.go index 2b21526ebbb..e84e9c8ed33 100644 --- a/projects/gateway2/deployer/values.go +++ b/projects/gateway2/deployer/values.go @@ -95,10 +95,7 @@ type helmAutoscaling struct { } type helmIstio struct { - Enabled *bool `json:"enabled,omitempty"` - IstioDiscoveryAddress *string `json:"istioDiscoveryAddress,omitempty"` - IstioMetaMeshId *string `json:"istioMetaMeshId,omitempty"` - IstioMetaClusterId *string `json:"istioMetaClusterId,omitempty"` + Enabled *bool `json:"enabled,omitempty"` } type helmSdsContainer struct { @@ -115,9 +112,13 @@ type sdsBootstrap struct { type helmIstioContainer struct { Image *helmImage `json:"image,omitempty"` LogLevel *string `json:"logLevel,omitempty"` - // Note: This is set by envoySidecarResources in helm chart + Resources *v1alpha1kube.ResourceRequirements `json:"resources,omitempty"` SecurityContext *extcorev1.SecurityContext `json:"securityContext,omitempty"` + + IstioDiscoveryAddress *string `json:"istioDiscoveryAddress,omitempty"` + IstioMetaMeshId *string `json:"istioMetaMeshId,omitempty"` + IstioMetaClusterId *string `json:"istioMetaClusterId,omitempty"` } type helmServiceAccount struct { diff --git a/projects/gateway2/deployer/values_helpers.go b/projects/gateway2/deployer/values_helpers.go index 5b90b1b8fd7..383bd0b5830 100644 --- a/projects/gateway2/deployer/values_helpers.go +++ b/projects/gateway2/deployer/values_helpers.go @@ -135,10 +135,13 @@ func getIstioContainerValues(istioContainerConfig *v1alpha1.IstioContainer) *hel } return &helmIstioContainer{ - Image: istioImage, - LogLevel: ptr.To(istioContainerConfig.GetLogLevel().GetValue()), - Resources: istioContainerConfig.GetResources(), - SecurityContext: istioContainerConfig.GetSecurityContext(), + Image: istioImage, + LogLevel: ptr.To(istioContainerConfig.GetLogLevel().GetValue()), + Resources: istioContainerConfig.GetResources(), + SecurityContext: istioContainerConfig.GetSecurityContext(), + IstioDiscoveryAddress: ptr.To(istioContainerConfig.GetIstioDiscoveryAddress().GetValue()), + IstioMetaMeshId: ptr.To(istioContainerConfig.GetIstioMetaMeshId().GetValue()), + IstioMetaClusterId: ptr.To(istioContainerConfig.GetIstioMetaClusterId().GetValue()), } } @@ -152,10 +155,7 @@ func getIstioValues(istioValues bootstrap.IstioValues, istioConfig *v1alpha1.Ist } return &helmIstio{ - Enabled: ptr.To(istioValues.IntegrationEnabled), - IstioDiscoveryAddress: ptr.To(istioConfig.GetIstioProxyContainer().GetIstioDiscoveryAddress().GetValue()), - IstioMetaMeshId: ptr.To(istioConfig.GetIstioProxyContainer().GetIstioMetaMeshId().GetValue()), - IstioMetaClusterId: ptr.To(istioConfig.GetIstioProxyContainer().GetIstioMetaClusterId().GetValue()), + Enabled: ptr.To(istioValues.IntegrationEnabled), } } diff --git a/projects/gateway2/helm/gloo-gateway/templates/gateway/proxy-deployment.yaml b/projects/gateway2/helm/gloo-gateway/templates/gateway/proxy-deployment.yaml index 7a26764e8da..9db57334d0d 100644 --- a/projects/gateway2/helm/gloo-gateway/templates/gateway/proxy-deployment.yaml +++ b/projects/gateway2/helm/gloo-gateway/templates/gateway/proxy-deployment.yaml @@ -157,15 +157,14 @@ spec: - name: PILOT_CERT_PROVIDER value: istiod - name: CA_ADDR - value: "istiod.istio-system.svc:15012" # TODO: Configurable istioDiscoveryAddress + value: {{ $gateway.istioContainer.istioDiscoveryAddress }} - name: ISTIO_META_MESH_ID - value: "cluster.local" # TODO: Configurable istioMetaMeshId + value: {{ $gateway.istioContainer.istioMetaMeshId }} - name: ISTIO_META_CLUSTER_ID - value: "Kubernetes" # TODO: Configurable istioMetaClusterId + value: {{ $gateway.istioContainer.istioMetaClusterId }} - name: PROXY_CONFIG - # TODO: Configurable istioDiscoveryAddress value: | - {"discoveryAddress": "istiod.istio-system.svc:15012" } + {"discoveryAddress": {{ $gateway.istioContainer.istioDiscoveryAddress }} } - name: POD_NAME valueFrom: fieldRef: diff --git a/projects/gateway2/status/status_syncer.go b/projects/gateway2/status/status_syncer.go index 5de94e22633..53e667a57d9 100644 --- a/projects/gateway2/status/status_syncer.go +++ b/projects/gateway2/status/status_syncer.go @@ -78,8 +78,8 @@ func (f *statusSyncerFactory) HandleProxyReports(ctx context.Context, proxiesWit defer f.lock.Unlock() proxiesToReport := make(map[int][]translatorutils.ProxyWithReports) + var proxySyncCount int for _, proxyWithReport := range filterProxiesByControllerName(proxiesWithReports) { - var proxySyncCount int // Get the sync iteration that produced the proxy from the proxy metadata if proxyWithReport.Proxy.GetMetadata().GetAnnotations() != nil { if syncId, ok := proxyWithReport.Proxy.GetMetadata().GetAnnotations()[utils.ProxySyncId]; ok { @@ -104,15 +104,12 @@ func (f *statusSyncerFactory) HandleProxyReports(ctx context.Context, proxiesWit if plugins, ok := f.registryPerSync[syncCount]; ok { newStatusSyncer(plugins).applyStatusPlugins(ctx, proxies) } else { - // this should never happen - contextutils.LoggerFrom(ctx).DPanicf("no registry found for proxy sync count %d", syncCount) + // this can happen when a non-proxy resource is reconciled by the gloo proxy reconciler (upstreams, secrets, etc) + contextutils.LoggerFrom(ctx).Debugf("no registry found for proxy sync count %d", syncCount) } + delete(f.registryPerSync, syncCount) } - // reinitialize the registry if there are no more proxies for the sync iteration - if len(f.resyncsPerProxy) == 0 { - f.registryPerSync = make(map[int]*registry.PluginRegistry) - } } type statusSyncer struct { From e1ff3593902d07d8f6aec4acd572abe520a317cf Mon Sep 17 00:00:00 2001 From: npolshakova Date: Mon, 17 Jun 2024 17:09:43 -0400 Subject: [PATCH 2/5] cleanup --- projects/gateway2/status/status_syncer.go | 44 +++++++++++++++---- .../gateway2/status/status_syncer_test.go | 5 ++- 2 files changed, 39 insertions(+), 10 deletions(-) diff --git a/projects/gateway2/status/status_syncer.go b/projects/gateway2/status/status_syncer.go index 53e667a57d9..7119bb5f544 100644 --- a/projects/gateway2/status/status_syncer.go +++ b/projects/gateway2/status/status_syncer.go @@ -42,15 +42,18 @@ type statusSyncerFactory struct { // maps a proxy to the sync iteration that produced it // only the latest sync iteration is stored and used to apply status plugins resyncsPerProxy map[types.NamespacedName]int + // proxies left to sync + resyncsPerIteration map[int][]types.NamespacedName lock *sync.Mutex } func NewStatusSyncerFactory() GatewayStatusSyncer { return &statusSyncerFactory{ - registryPerSync: make(map[int]*registry.PluginRegistry), - resyncsPerProxy: make(map[types.NamespacedName]int), - lock: &sync.Mutex{}, + registryPerSync: make(map[int]*registry.PluginRegistry), + resyncsPerProxy: make(map[types.NamespacedName]int), + resyncsPerIteration: make(map[int][]types.NamespacedName), + lock: &sync.Mutex{}, } } @@ -63,9 +66,15 @@ func (f *statusSyncerFactory) QueueStatusForProxies( f.lock.Lock() defer f.lock.Unlock() + // check all proxies are handled in debugger + f.resyncsPerIteration[totalSyncCount] = make([]types.NamespacedName, 0) + // queue each proxy for a given sync iteration for _, proxy := range proxiesToQueue { + // overwrite the sync count for the proxy with the most recent sync count f.resyncsPerProxy[getProxyNameNamespace(proxy)] = totalSyncCount + + f.resyncsPerIteration[totalSyncCount] = append(f.resyncsPerIteration[totalSyncCount], getProxyNameNamespace(proxy)) } // the plugin registry that produced the proxies is the same for all proxies in a given sync f.registryPerSync[totalSyncCount] = pluginRegistry @@ -91,8 +100,21 @@ func (f *statusSyncerFactory) HandleProxyReports(ctx context.Context, proxiesWit // if the proxySyncCount saved in the statusSyncer for a given proxy is higher (i.e. newer) than the syncCount // on the proxy metadata, then continue because this report iteration is for an older sync which we no longer care about if f.resyncsPerProxy[proxyKey] > proxySyncCount { - // old proxy was garbage collected, expect a future resync + // old proxy was garbage collected, expect a future re-sync + continue + } + + if f.resyncsPerIteration[proxySyncCount] == nil { + // re-sync already happened, nothing to do continue + } else { + updatedList := make([]types.NamespacedName, 0) + for _, proxyNameNs := range f.resyncsPerIteration[proxySyncCount] { + if proxyNameNs != proxyKey { + updatedList = append(updatedList, proxyNameNs) + } + } + f.resyncsPerIteration[proxySyncCount] = updatedList } proxiesToReport[proxySyncCount] = append(proxiesToReport[proxySyncCount], proxyWithReport) @@ -102,14 +124,20 @@ func (f *statusSyncerFactory) HandleProxyReports(ctx context.Context, proxiesWit for syncCount, proxies := range proxiesToReport { if plugins, ok := f.registryPerSync[syncCount]; ok { - newStatusSyncer(plugins).applyStatusPlugins(ctx, proxies) + if plugins != nil { + newStatusSyncer(plugins).applyStatusPlugins(ctx, proxies) + } } else { - // this can happen when a non-proxy resource is reconciled by the gloo proxy reconciler (upstreams, secrets, etc) + // This can happen when a non-proxy resource is reconciled by the gloo proxy reconciler (upstreams, secrets, etc) contextutils.LoggerFrom(ctx).Debugf("no registry found for proxy sync count %d", syncCount) + continue } - delete(f.registryPerSync, syncCount) - } + // If there are no more proxies for the sync iteration, delete the sync count + if len(f.resyncsPerIteration) == 0 { + delete(f.registryPerSync, syncCount) + } + } } type statusSyncer struct { diff --git a/projects/gateway2/status/status_syncer_test.go b/projects/gateway2/status/status_syncer_test.go index 3c1a8f1a07c..4651bc29452 100644 --- a/projects/gateway2/status/status_syncer_test.go +++ b/projects/gateway2/status/status_syncer_test.go @@ -284,7 +284,8 @@ var _ = Describe("Status Syncer", func() { // ensure all proxies are removed from the queue Expect(proxiesMap).To(BeEmpty()) registryMap = syncer.(*statusSyncerFactory).registryPerSync - // ensure registry is cleared for all sync iterations - Expect(registryMap).To(BeEmpty()) + Expect(registryMap).ToNot(BeEmpty()) + // ensure registry is only cleared for processed sync iteration + Expect(registryMap).To(And(HaveKey(123), HaveKey(124))) }) }) From 99259a86bcda02c7d19ee1d825a97c4f898bb548 Mon Sep 17 00:00:00 2001 From: npolshakova Date: Mon, 17 Jun 2024 17:48:08 -0400 Subject: [PATCH 3/5] add log lines --- projects/gateway2/proxy_syncer/proxy_syncer.go | 4 ++-- projects/gateway2/status/status_syncer.go | 10 +++++++--- projects/gateway2/status/status_syncer_test.go | 16 ++++++++-------- 3 files changed, 17 insertions(+), 13 deletions(-) diff --git a/projects/gateway2/proxy_syncer/proxy_syncer.go b/projects/gateway2/proxy_syncer/proxy_syncer.go index 3077af2da31..4855510546c 100644 --- a/projects/gateway2/proxy_syncer/proxy_syncer.go +++ b/projects/gateway2/proxy_syncer/proxy_syncer.go @@ -22,7 +22,7 @@ import ( ) // QueueStatusForProxiesFn queues a list of proxies to be synced and the plugin registry that produced them for a given sync iteration -type QueueStatusForProxiesFn func(proxies gloo_solo_io.ProxyList, pluginRegistry *registry.PluginRegistry, totalSyncCount int) +type QueueStatusForProxiesFn func(ctx context.Context, proxies gloo_solo_io.ProxyList, pluginRegistry *registry.PluginRegistry, totalSyncCount int) // ProxySyncer is responsible for translating Kubernetes Gateway CRs into Gloo Proxies // and syncing the proxyClient with the newly translated proxies. @@ -152,7 +152,7 @@ func (s *ProxySyncer) Start(ctx context.Context) error { TranslatedGateways: translatedGateways, }) - s.queueStatusForProxies(proxies, &pluginRegistry, totalResyncs) + s.queueStatusForProxies(ctx, proxies, &pluginRegistry, totalResyncs) s.syncStatus(ctx, rm, gwl) s.syncRouteStatus(ctx, rm) s.reconcileProxies(ctx, proxies) diff --git a/projects/gateway2/status/status_syncer.go b/projects/gateway2/status/status_syncer.go index 7119bb5f544..7240472cbad 100644 --- a/projects/gateway2/status/status_syncer.go +++ b/projects/gateway2/status/status_syncer.go @@ -26,6 +26,7 @@ var _ proxy_syncer.QueueStatusForProxiesFn = (&statusSyncerFactory{}).QueueStatu // GatewayStatusSyncer is responsible for applying status plugins to Gloo Gateway proxies type GatewayStatusSyncer interface { QueueStatusForProxies( + ctx context.Context, proxiesToQueue v1.ProxyList, pluginRegistry *registry.PluginRegistry, totalSyncCount int, @@ -59,6 +60,7 @@ func NewStatusSyncerFactory() GatewayStatusSyncer { // QueueStatusForProxies queues the proxies to be synced and plugin registry for the given sync iteration func (f *statusSyncerFactory) QueueStatusForProxies( + ctx context.Context, proxiesToQueue v1.ProxyList, pluginRegistry *registry.PluginRegistry, totalSyncCount int, @@ -66,6 +68,8 @@ func (f *statusSyncerFactory) QueueStatusForProxies( f.lock.Lock() defer f.lock.Unlock() + contextutils.LoggerFrom(ctx).Debugf("queueing %v proxies for sync %d", len(proxiesToQueue), totalSyncCount) + // check all proxies are handled in debugger f.resyncsPerIteration[totalSyncCount] = make([]types.NamespacedName, 0) @@ -86,6 +90,8 @@ func (f *statusSyncerFactory) HandleProxyReports(ctx context.Context, proxiesWit f.lock.Lock() defer f.lock.Unlock() + contextutils.LoggerFrom(ctx).Debugf("handling proxy reports for %v proxies", len(proxiesWithReports)) + proxiesToReport := make(map[int][]translatorutils.ProxyWithReports) var proxySyncCount int for _, proxyWithReport := range filterProxiesByControllerName(proxiesWithReports) { @@ -124,9 +130,7 @@ func (f *statusSyncerFactory) HandleProxyReports(ctx context.Context, proxiesWit for syncCount, proxies := range proxiesToReport { if plugins, ok := f.registryPerSync[syncCount]; ok { - if plugins != nil { - newStatusSyncer(plugins).applyStatusPlugins(ctx, proxies) - } + newStatusSyncer(plugins).applyStatusPlugins(ctx, proxies) } else { // This can happen when a non-proxy resource is reconciled by the gloo proxy reconciler (upstreams, secrets, etc) contextutils.LoggerFrom(ctx).Debugf("no registry found for proxy sync count %d", syncCount) diff --git a/projects/gateway2/status/status_syncer_test.go b/projects/gateway2/status/status_syncer_test.go index 4651bc29452..6db49a1c773 100644 --- a/projects/gateway2/status/status_syncer_test.go +++ b/projects/gateway2/status/status_syncer_test.go @@ -57,9 +57,10 @@ var _ = Describe("Status Syncer", func() { proxiesToQueue := v1.ProxyList{proxyOne, proxyTwo} pluginRegistry := ®istry.PluginRegistry{} + ctx := context.Background() // Test QueueStatusForProxies method - syncer.QueueStatusForProxies(proxiesToQueue, pluginRegistry, 123) + syncer.QueueStatusForProxies(ctx, proxiesToQueue, pluginRegistry, 123) // Queue the proxy (this is invoked in the proxy syncer) proxiesMap := syncer.(*statusSyncerFactory).resyncsPerProxy @@ -78,7 +79,6 @@ var _ = Describe("Status Syncer", func() { }, }, } - ctx := context.Background() syncer.HandleProxyReports(ctx, proxyOneWithReports) // Ensure proxy one has been removed from the queue after handling reports, but proxy two is still present @@ -145,9 +145,10 @@ var _ = Describe("Status Syncer", func() { proxiesToQueue := v1.ProxyList{proxyOne, proxyTwo} pluginRegistry := ®istry.PluginRegistry{} + ctx := context.Background() // Test QueueStatusForProxies method - syncer.QueueStatusForProxies(proxiesToQueue, pluginRegistry, 123) + syncer.QueueStatusForProxies(ctx, proxiesToQueue, pluginRegistry, 123) // Queue the proxy (this is invoked in the proxy syncer) proxiesMap := syncer.(*statusSyncerFactory).resyncsPerProxy @@ -173,7 +174,6 @@ var _ = Describe("Status Syncer", func() { }, }, } - ctx := context.Background() syncer.HandleProxyReports(ctx, proxiesWithReports) // Ensure both proxies are removed from the queue after handling reports @@ -235,11 +235,12 @@ var _ = Describe("Status Syncer", func() { proxiesToQueue125 := v1.ProxyList{newProxy} pluginRegistry125 := ®istry.PluginRegistry{} + ctx := context.Background() // Each proxy is queued with a different registry per sync iteration - syncer.QueueStatusForProxies(proxiesToQueue123, pluginRegistry123, 123) - syncer.QueueStatusForProxies(proxiesToQueue124, pluginRegistry124, 124) - syncer.QueueStatusForProxies(proxiesToQueue125, pluginRegistry125, 125) + syncer.QueueStatusForProxies(ctx, proxiesToQueue123, pluginRegistry123, 123) + syncer.QueueStatusForProxies(ctx, proxiesToQueue124, pluginRegistry124, 124) + syncer.QueueStatusForProxies(ctx, proxiesToQueue125, pluginRegistry125, 125) // Queue the proxy (this is invoked in the proxy syncer) proxiesMap := syncer.(*statusSyncerFactory).resyncsPerProxy @@ -258,7 +259,6 @@ var _ = Describe("Status Syncer", func() { }, }, } - ctx := context.Background() syncer.HandleProxyReports(ctx, oldProxiesWithReports) // Ensure only the latest proxy is still present From 47b359dd7d6f863c06aa3e588deb39dcea32d260 Mon Sep 17 00:00:00 2001 From: npolshakova Date: Tue, 18 Jun 2024 11:02:09 -0400 Subject: [PATCH 4/5] pr feedback --- projects/gateway2/status/status_syncer.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/projects/gateway2/status/status_syncer.go b/projects/gateway2/status/status_syncer.go index 7240472cbad..c135369bbb8 100644 --- a/projects/gateway2/status/status_syncer.go +++ b/projects/gateway2/status/status_syncer.go @@ -70,14 +70,15 @@ func (f *statusSyncerFactory) QueueStatusForProxies( contextutils.LoggerFrom(ctx).Debugf("queueing %v proxies for sync %d", len(proxiesToQueue), totalSyncCount) - // check all proxies are handled in debugger - f.resyncsPerIteration[totalSyncCount] = make([]types.NamespacedName, 0) - // queue each proxy for a given sync iteration for _, proxy := range proxiesToQueue { // overwrite the sync count for the proxy with the most recent sync count f.resyncsPerProxy[getProxyNameNamespace(proxy)] = totalSyncCount + // keep track of proxies to check all proxies are handled in debugger + if f.resyncsPerIteration[totalSyncCount] == nil { + f.resyncsPerIteration[totalSyncCount] = make([]types.NamespacedName, 0) + } f.resyncsPerIteration[totalSyncCount] = append(f.resyncsPerIteration[totalSyncCount], getProxyNameNamespace(proxy)) } // the plugin registry that produced the proxies is the same for all proxies in a given sync From b2fbb5c84c211cb41bf40051ac34d6ddcaed1910 Mon Sep 17 00:00:00 2001 From: npolshakova Date: Tue, 18 Jun 2024 16:20:23 -0400 Subject: [PATCH 5/5] pr feedback --- projects/gateway2/status/status_syncer.go | 7 ------- 1 file changed, 7 deletions(-) diff --git a/projects/gateway2/status/status_syncer.go b/projects/gateway2/status/status_syncer.go index c135369bbb8..5402a90622f 100644 --- a/projects/gateway2/status/status_syncer.go +++ b/projects/gateway2/status/status_syncer.go @@ -76,9 +76,6 @@ func (f *statusSyncerFactory) QueueStatusForProxies( f.resyncsPerProxy[getProxyNameNamespace(proxy)] = totalSyncCount // keep track of proxies to check all proxies are handled in debugger - if f.resyncsPerIteration[totalSyncCount] == nil { - f.resyncsPerIteration[totalSyncCount] = make([]types.NamespacedName, 0) - } f.resyncsPerIteration[totalSyncCount] = append(f.resyncsPerIteration[totalSyncCount], getProxyNameNamespace(proxy)) } // the plugin registry that produced the proxies is the same for all proxies in a given sync @@ -132,10 +129,6 @@ func (f *statusSyncerFactory) HandleProxyReports(ctx context.Context, proxiesWit for syncCount, proxies := range proxiesToReport { if plugins, ok := f.registryPerSync[syncCount]; ok { newStatusSyncer(plugins).applyStatusPlugins(ctx, proxies) - } else { - // This can happen when a non-proxy resource is reconciled by the gloo proxy reconciler (upstreams, secrets, etc) - contextutils.LoggerFrom(ctx).Debugf("no registry found for proxy sync count %d", syncCount) - continue } // If there are no more proxies for the sync iteration, delete the sync count