From 6fec5055510f378ae27f6c670137362f46fe528e Mon Sep 17 00:00:00 2001
From: Kate Osborn <k.osborn@f5.com>
Date: Tue, 23 May 2023 09:32:17 -0600
Subject: [PATCH] FIXME Review

Reviewed FIXMEs with team and updated comments.
---
 internal/events/event.go                           |  1 -
 internal/events/handler.go                         |  3 +++
 internal/events/loop.go                            |  2 +-
 internal/manager/manager.go                        |  7 ++-----
 internal/nginx/config/servers.go                   |  4 ++--
 internal/nginx/config/upstreams_template.go        |  1 +
 internal/nginx/config/validation/http_njs_match.go |  4 ++--
 internal/nginx/runtime/manager.go                  |  1 +
 internal/state/change_processor.go                 |  1 -
 internal/state/dataplane/configuration.go          | 11 +++--------
 internal/state/graph/gateway_listener.go           |  2 +-
 internal/state/graph/httproute.go                  | 10 ++--------
 internal/state/secrets/secrets.go                  |  1 +
 13 files changed, 19 insertions(+), 29 deletions(-)

diff --git a/internal/events/event.go b/internal/events/event.go
index 5cade7ce9a..6d0a6717a5 100644
--- a/internal/events/event.go
+++ b/internal/events/event.go
@@ -6,7 +6,6 @@ import (
 )
 
 // EventBatch is a batch of events to be handled at once.
-// FIXME(pleshakov): think about how to avoid using an interface{} here
 type EventBatch []interface{}
 
 // UpsertEvent represents upserting a resource.
diff --git a/internal/events/handler.go b/internal/events/handler.go
index ee7ab749a3..3326b569e3 100644
--- a/internal/events/handler.go
+++ b/internal/events/handler.go
@@ -94,6 +94,7 @@ func (h *EventHandlerImpl) updateNginx(ctx context.Context, conf dataplane.Confi
 	// Write all secrets (nuke and pave).
 	// This will remove all secrets in the secrets directory before writing the requested secrets.
 	// FIXME(kate-osborn): We may want to rethink this approach in the future and write and remove secrets individually.
+	// https://github.com/nginxinc/nginx-kubernetes-gateway/issues/561
 	err := h.cfg.SecretMemoryManager.WriteAllRequestedSecrets()
 	if err != nil {
 		return err
@@ -124,6 +125,7 @@ func (h *EventHandlerImpl) propagateUpsert(e *UpsertEvent) {
 		h.cfg.Processor.CaptureUpsertChange(r)
 	case *apiv1.Secret:
 		// FIXME(kate-osborn): need to handle certificate rotation
+		// https://github.com/nginxinc/nginx-kubernetes-gateway/issues/553
 		h.cfg.SecretStore.Upsert(r)
 	case *discoveryV1.EndpointSlice:
 		h.cfg.Processor.CaptureUpsertChange(r)
@@ -144,6 +146,7 @@ func (h *EventHandlerImpl) propagateDelete(e *DeleteEvent) {
 		h.cfg.Processor.CaptureDeleteChange(e.Type, e.NamespacedName)
 	case *apiv1.Secret:
 		// FIXME(kate-osborn): make sure that affected servers are updated
+		// https://github.com/nginxinc/nginx-kubernetes-gateway/issues/553
 		h.cfg.SecretStore.Delete(e.NamespacedName)
 	case *discoveryV1.EndpointSlice:
 		h.cfg.Processor.CaptureDeleteChange(e.Type, e.NamespacedName)
diff --git a/internal/events/loop.go b/internal/events/loop.go
index 10be12ed5b..ee330080f8 100644
--- a/internal/events/loop.go
+++ b/internal/events/loop.go
@@ -21,6 +21,7 @@ import (
 // (2) A reload can have side-effects for the data plane traffic.
 // FIXME(pleshakov): better document the side effects and how to prevent and mitigate them.
 // So when the EventLoop have 100 saved events, it is better to process them at once rather than one by one.
+// https://github.com/nginxinc/nginx-kubernetes-gateway/issues/551
 type EventLoop struct {
 	handler  EventHandler
 	preparer FirstEventBatchPreparer
@@ -113,7 +114,6 @@ func (el *EventLoop) Start(ctx context.Context) error {
 			// Add the event to the current batch.
 			el.nextBatch = append(el.nextBatch, e)
 
-			// FIXME(pleshakov): Log more details about the event like resource GVK and ns/name.
 			el.logger.Info(
 				"added an event to the next batch",
 				"type", fmt.Sprintf("%T", e),
diff --git a/internal/manager/manager.go b/internal/manager/manager.go
index e2dab9cc83..de9d5a6f82 100644
--- a/internal/manager/manager.go
+++ b/internal/manager/manager.go
@@ -138,11 +138,8 @@ func Start(cfg config.Config) error {
 		GatewayClassName: cfg.GatewayClassName,
 		Client:           mgr.GetClient(),
 		PodIP:            cfg.PodIP,
-		// FIXME(pleshakov) Make sure each component:
-		// (1) Has a dedicated named logger.
-		// (2) Get it from the Manager (the WithName is done here for all components).
-		Logger: cfg.Logger.WithName("statusUpdater"),
-		Clock:  status.NewRealClock(),
+		Logger:           cfg.Logger.WithName("statusUpdater"),
+		Clock:            status.NewRealClock(),
 	})
 
 	eventHandler := events.NewEventHandlerImpl(events.EventHandlerConfig{
diff --git a/internal/nginx/config/servers.go b/internal/nginx/config/servers.go
index 98ecb5907e..03383b75f3 100644
--- a/internal/nginx/config/servers.go
+++ b/internal/nginx/config/servers.go
@@ -124,6 +124,7 @@ func createLocations(pathRules []dataplane.PathRule, listenerPort int) []http.Lo
 			// FIXME(pleshakov): Ensure dataplane.Configuration -related types don't include v1beta1 types, so that
 			// we don't need to make any assumptions like above here. After fixing this, ensure that there is a test
 			// for checking the imported Webhook validation catches the case above.
+			// https://github.com/nginxinc/nginx-kubernetes-gateway/issues/660
 
 			// RequestRedirect and proxying are mutually exclusive.
 			if r.Filters.RequestRedirect != nil {
@@ -146,6 +147,7 @@ func createLocations(pathRules []dataplane.PathRule, listenerPort int) []http.Lo
 		if len(matches) > 0 {
 			// FIXME(sberman): De-dupe matches and associated locations
 			// so we don't need nginx/njs to perform unnecessary matching.
+			// https://github.com/nginxinc/nginx-kubernetes-gateway/issues/662
 			b, err := json.Marshal(matches)
 			if err != nil {
 				// panic is safe here because we should never fail to marshal the match unless we constructed it incorrectly.
@@ -244,7 +246,6 @@ func createHTTPMatch(match v1beta1.HTTPRouteMatch, redirectPath string) httpMatc
 		headers := make([]string, 0, len(match.Headers))
 		headerNames := make(map[string]struct{})
 
-		// FIXME(kate-osborn): For now we only support type "Exact".
 		for _, h := range match.Headers {
 			if *h.Type == v1beta1.HeaderMatchExact {
 				// duplicate header names are not permitted by the spec
@@ -262,7 +263,6 @@ func createHTTPMatch(match v1beta1.HTTPRouteMatch, redirectPath string) httpMatc
 	if match.QueryParams != nil {
 		params := make([]string, 0, len(match.QueryParams))
 
-		// FIXME(kate-osborn): For now we only support type "Exact".
 		for _, p := range match.QueryParams {
 			if *p.Type == v1beta1.QueryParamMatchExact {
 				params = append(params, createQueryParamKeyValString(p))
diff --git a/internal/nginx/config/upstreams_template.go b/internal/nginx/config/upstreams_template.go
index ce58f7fce5..2058e3881a 100644
--- a/internal/nginx/config/upstreams_template.go
+++ b/internal/nginx/config/upstreams_template.go
@@ -2,6 +2,7 @@ package config
 
 // FIXME(kate-osborn): Dynamically calculate upstream zone size based on the number of upstreams.
 // 512k will support up to 648 upstream servers.
+// https://github.com/nginxinc/nginx-kubernetes-gateway/issues/483
 var upstreamsTemplateText = `
 {{ range $u := . }}
 upstream {{ $u.Name }} {
diff --git a/internal/nginx/config/validation/http_njs_match.go b/internal/nginx/config/validation/http_njs_match.go
index 1e5efc2d6b..eb4ec93087 100644
--- a/internal/nginx/config/validation/http_njs_match.go
+++ b/internal/nginx/config/validation/http_njs_match.go
@@ -37,8 +37,8 @@ func (HTTPNJSMatchValidator) ValidatePathInMatch(path string) error {
 		return errors.New(msg)
 	}
 
-	// FIXME(pleshakov): This is temporary until https://github.com/nginxinc/nginx-kubernetes-gateway/issues/428
-	// is fixed.
+	// FIXME(pleshakov): This function will no longer be
+	// needed once https://github.com/nginxinc/nginx-kubernetes-gateway/issues/428 is fixed.
 	// That's because the location path gets into the set directive in the location block.
 	// Example: set $http_matches "[{\"redirectPath\":\"/coffee_route0\" ...
 	// Where /coffee is tha path.
diff --git a/internal/nginx/runtime/manager.go b/internal/nginx/runtime/manager.go
index ba745aead3..ee8892c139 100644
--- a/internal/nginx/runtime/manager.go
+++ b/internal/nginx/runtime/manager.go
@@ -52,6 +52,7 @@ func (m *ManagerImpl) Reload(ctx context.Context) error {
 	// FIXME(pleshakov)
 	// (1) ensure the reload actually happens.
 	// (2) ensure that in case of an error, the error message can be seen by the admins.
+	// https://github.com/nginxinc/nginx-kubernetes-gateway/issues/664
 
 	// for now, to prevent a subsequent reload starting before the in-flight reload finishes, we simply sleep.
 	// Fixing (1) will make the sleep unnecessary.
diff --git a/internal/state/change_processor.go b/internal/state/change_processor.go
index c556af935d..0536e84138 100644
--- a/internal/state/change_processor.go
+++ b/internal/state/change_processor.go
@@ -172,7 +172,6 @@ func NewChangeProcessorImpl(cfg ChangeProcessorConfig) *ChangeProcessorImpl {
 	}
 }
 
-// FIXME(pleshakov)
 // Currently, changes (upserts/delete) trigger rebuilding of the configuration, even if the change doesn't change
 // the configuration or the statuses of the resources. For example, a change in a Gateway resource that doesn't
 // belong to the NGINX Gateway or an HTTPRoute that doesn't belong to any of the Gateways of the NGINX Gateway.
diff --git a/internal/state/dataplane/configuration.go b/internal/state/dataplane/configuration.go
index 1eb4342ab2..6db9253543 100644
--- a/internal/state/dataplane/configuration.go
+++ b/internal/state/dataplane/configuration.go
@@ -23,10 +23,10 @@ const (
 // Configuration is an intermediate representation of dataplane configuration.
 type Configuration struct {
 	// HTTPServers holds all HTTPServers.
-	// FIXME(pleshakov) We assume that all servers are HTTP and listen on port 80.
+	// We assume that all servers are HTTP and listen on port 80.
 	HTTPServers []VirtualServer
 	// SSLServers holds all SSLServers.
-	// FIXME(kate-osborn) We assume that all SSL servers listen on port 443.
+	// We assume that all SSL servers listen on port 443.
 	SSLServers []VirtualServer
 	// Upstreams holds all unique Upstreams.
 	Upstreams []Upstream
@@ -87,8 +87,6 @@ type MatchRule struct {
 	// Filters holds the filters for the MatchRule.
 	Filters Filters
 	// Source is the corresponding HTTPRoute resource.
-	// FIXME(pleshakov): Consider referencing only the parts needed for the config generation rather than
-	// the entire resource.
 	Source *v1beta1.HTTPRoute
 	// BackendGroup is the group of Backends that the rule routes to.
 	BackendGroup BackendGroup
@@ -135,7 +133,6 @@ func (r *MatchRule) GetMatch() v1beta1.HTTPRouteMatch {
 }
 
 // BuildConfiguration builds the Configuration from the Graph.
-// FIXME(pleshakov) For now we only handle paths with prefix matches. Handle exact and regex matches
 func BuildConfiguration(ctx context.Context, g *graph.Graph, resolver resolver.ServiceResolver) Configuration {
 	if g.GatewayClass == nil || !g.GatewayClass.Valid {
 		return Configuration{}
@@ -372,8 +369,6 @@ func (hpr *hostPathRules) buildServers() []VirtualServer {
 		hostname := getListenerHostname(l.Source.Hostname)
 		// Generate a 404 ssl server block for listeners with no routes or listeners with wildcard (match-all) routes.
 		// This server overrides the default ssl server.
-		// FIXME(kate-osborn): when we support regex hostnames (e.g. *.example.com)
-		// we will have to modify this check to catch regex hostnames.
 		if len(l.Routes) == 0 || hostname == wildcardHostname {
 			s := VirtualServer{
 				Hostname: hostname,
@@ -503,7 +498,7 @@ func convertPathType(pathType v1beta1.PathMatchType) PathType {
 
 // listenerHostnameMoreSpecific returns true if host1 is more specific than host2 (using length).
 //
-// FIXME(sberman): Since the only caller of this function specifies listener hostnames that are both
+// Since the only caller of this function specifies listener hostnames that are both
 // bound to the same route hostname, this function assumes that host1 and host2 match, either
 // exactly or as a substring.
 //
diff --git a/internal/state/graph/gateway_listener.go b/internal/state/graph/gateway_listener.go
index 5bf859c59c..a4b3e64be8 100644
--- a/internal/state/graph/gateway_listener.go
+++ b/internal/state/graph/gateway_listener.go
@@ -12,7 +12,7 @@ import (
 )
 
 // Listener represents a Listener of the Gateway resource.
-// FIXME(pleshakov) For now, we only support HTTP and HTTPS listeners.
+// For now, we only support HTTP and HTTPS listeners.
 type Listener struct {
 	// Source holds the source of the Listener from the Gateway resource.
 	Source v1beta1.Listener
diff --git a/internal/state/graph/httproute.go b/internal/state/graph/httproute.go
index 64f13db9aa..850754609a 100644
--- a/internal/state/graph/httproute.go
+++ b/internal/state/graph/httproute.go
@@ -53,9 +53,6 @@ type ParentRefAttachmentStatus struct {
 // Route represents an HTTPRoute.
 type Route struct {
 	// Source is the source resource of the Route.
-	// FIXME(pleshakov)
-	// For now, we assume that the source is only HTTPRoute.
-	// Later we can support more types - TLSRoute, TCPRoute and UDPRoute.
 	Source *v1beta1.HTTPRoute
 	// ParentRefs includes ParentRefs with NKG Gateways only.
 	ParentRefs []ParentRef
@@ -231,7 +228,7 @@ func buildRoute(
 
 		if atLeastOneValid {
 			// FIXME(pleshakov): Partial validity for HTTPRoute rules is not defined in the Gateway API spec yet.
-			// See https://github.com/kubernetes-sigs/gateway-api/issues/1696
+			// See https://github.com/nginxinc/nginx-kubernetes-gateway/issues/485
 			msg = "Some rules are invalid: " + msg
 			r.Conditions = append(r.Conditions, conditions.NewTODO(msg))
 		} else {
@@ -311,9 +308,6 @@ func bindRouteToListeners(r *Route, gw *Gateway) {
 // tryToAttachRouteToListeners tries to attach the route to the listeners that match the parentRef and the hostnames.
 // If it succeeds in attaching at least one listener it will return true and the condition will be empty.
 // If it fails to attach the route, it will return false and the failure condition.
-// FIXME(pleshakov)
-// For now, let's do simple matching.
-// However, we need to also support wildcard matching.
 func tryToAttachRouteToListeners(
 	refStatus *ParentRefAttachmentStatus,
 	sectionName *v1beta1.SectionName,
@@ -324,7 +318,7 @@ func tryToAttachRouteToListeners(
 
 	if !listenerExists {
 		// FIXME(pleshakov): Add a proper condition once it is available in the Gateway API.
-		// https://github.com/nginxinc/nginx-kubernetes-gateway/issues/306
+		// https://github.com/nginxinc/nginx-kubernetes-gateway/issues/665
 		return conditions.NewTODO("listener is not found"), false
 	}
 
diff --git a/internal/state/secrets/secrets.go b/internal/state/secrets/secrets.go
index 4096aec08a..81102ea16e 100644
--- a/internal/state/secrets/secrets.go
+++ b/internal/state/secrets/secrets.go
@@ -92,6 +92,7 @@ type FileManager interface {
 }
 
 // FIXME(kate-osborn): Is it necessary to make this concurrent-safe?
+// https://github.com/nginxinc/nginx-kubernetes-gateway/issues/441
 type SecretDiskMemoryManagerImpl struct {
 	requestedSecrets map[types.NamespacedName]requestedSecret
 	secretStore      SecretStore