Set Gateway Programmed condition (#658)

* Set Gateway Programmed condition When the Gateway configuration has been successfully uploaded to nginx, we will set the status Programmed to true. If there's any error with updating nginx, then we set Programmed to false. We'll also set a negative status on the HTTPRoutes in the system to explain that the Gateway is not programmed. Moved the buildStatuses logic into the status package and removed redundant tests.
nginxinc · Jun 1, 2023 · ef84a6d · ef84a6d
1 parent 64dda4c
commit ef84a6d
Show file tree

Hide file tree

Showing 22 changed files with 621 additions and 1,238 deletions.
diff --git a/docs/gateway-api-compatibility.md b/docs/gateway-api-compatibility.md
@@ -39,11 +39,11 @@ of the [static-mode](./cli-help.md#static-mode) command.
 
 Fields:
 * `spec`
-	* `controllerName` - supported.
-	* `parametersRef` - not supported.
-	* `description` - supported.
+    * `controllerName` - supported.
+    * `parametersRef` - not supported.
+    * `description` - supported.
 * `status`
-	* `conditions` - partially supported.
+    * `conditions` - partially supported.
 
 ### Gateway
 
@@ -54,18 +54,18 @@ See [static-mode](./cli-help.md#static-mode) command for more info.
 
 Fields:
 * `spec`
-	* `gatewayClassName` - supported.
-	* `listeners`
-		* `name` - supported.
-		* `hostname` - partially supported. Wildcard hostnames like `*.example.com` are not yet supported.
-		* `port` - partially supported. Allowed values: `80` for HTTP listeners and `443` for HTTPS listeners.
-		* `protocol` - partially supported. Allowed values: `HTTP`, `HTTPS`.
-		* `tls`
-		  * `mode` - partially supported. Allowed value: `Terminate`.
-		  * `certificateRefs` - partially supported. The TLS certificate and key must be stored in a Secret resource of type `kubernetes.io/tls` in the same namespace as the Gateway resource. Only a single reference is supported. You must deploy the Secret before the Gateway resource. Secret rotation (watching for updates) is not supported.
-		  * `options` - not supported.
-		* `allowedRoutes` - not supported. 
-	* `addresses` - not supported.
+    * `gatewayClassName` - supported.
+    * `listeners`
+        * `name` - supported.
+        * `hostname` - partially supported. Wildcard hostnames like `*.example.com` are not yet supported.
+        * `port` - partially supported. Allowed values: `80` for HTTP listeners and `443` for HTTPS listeners.
+        * `protocol` - partially supported. Allowed values: `HTTP`, `HTTPS`.
+        * `tls`
+          * `mode` - partially supported. Allowed value: `Terminate`.
+          * `certificateRefs` - partially supported. The TLS certificate and key must be stored in a Secret resource of type `kubernetes.io/tls` in the same namespace as the Gateway resource. Only a single reference is supported. You must deploy the Secret before the Gateway resource. Secret rotation (watching for updates) is not supported.
+          * `options` - not supported.
+        * `allowedRoutes` - not supported.
+    * `addresses` - not supported.
 * `status`
   * `addresses` - Pod IPAddress supported.
   * `conditions` - Supported (Condition/Status/Reason):
@@ -75,11 +75,14 @@ Fields:
     * `Accepted/False/Invalid`
     * `Accepted/False/UnsupportedValue`: Custom reason for when a value of a field in a Gateway is invalid or not supported.
     * `Accepted/False/GatewayConflict`: Custom reason for when the Gateway is ignored due to a conflicting Gateway. NKG only supports a single Gateway.
+    * `Programmed/True/Programmed`
+    * `Programmed/False/Invalid`
+    * `Programmed/False/GatewayConflict`: Custom reason for when the Gateway is ignored due to a conflicting Gateway. NKG only supports a single Gateway.
   * `listeners`
-	* `name` - supported.
-	* `supportedKinds` - not supported.
-	* `attachedRoutes` - supported.
-	* `conditions` - Supported (Condition/Status/Reason):
+    * `name` - supported.
+    * `supportedKinds` - not supported.
+    * `attachedRoutes` - supported.
+    * `conditions` - Supported (Condition/Status/Reason):
       * `Accepted/True/Accepted`
       * `Accepted/False/UnsupportedProtocol`
       * `Accepted/False/InvalidCertificateRef`
@@ -101,26 +104,27 @@ Fields:
   * `parentRefs` - partially supported. Port not supported.
   * `hostnames` - partially supported. Wildcard binding is not supported: a hostname like `example.com` will not bind to a listener with the hostname `*.example.com`. However, `example.com` will bind to a listener with the empty hostname.
   * `rules`
-	* `matches`
-	  * `path` - partially supported. Only `PathPrefix` and `Exact` types.
-	  * `headers` - partially supported. Only `Exact` type.
-	  * `queryParams` - partially supported. Only `Exact` type. 
-	  * `method` -  supported.
-	* `filters`
-		* `type` - supported.
-		* `requestRedirect` - supported except for the experimental `path` field. If multiple filters with `requestRedirect` are configured, NGINX Kubernetes Gateway will choose the first one and ignore the rest. 
-		* `requestHeaderModifier`, `requestMirror`, `urlRewrite`, `extensionRef` - not supported.
-	* `backendRefs` - partially supported. Backend ref `filters` are not supported.
+    * `matches`
+      * `path` - partially supported. Only `PathPrefix` and `Exact` types.
+      * `headers` - partially supported. Only `Exact` type.
+      * `queryParams` - partially supported. Only `Exact` type.
+      * `method` -  supported.
+    * `filters`
+        * `type` - supported.
+        * `requestRedirect` - supported except for the experimental `path` field. If multiple filters with `requestRedirect` are configured, NGINX Kubernetes Gateway will choose the first one and ignore the rest.
+        * `requestHeaderModifier`, `requestMirror`, `urlRewrite`, `extensionRef` - not supported.
+    * `backendRefs` - partially supported. Backend ref `filters` are not supported.
 * `status`
   * `parents`
-	* `parentRef` - supported.
-	* `controllerName` - supported.
-	* `conditions` - partially supported. Supported (Condition/Status/Reason):
-    	*  `Accepted/True/Accepted`
-    	*  `Accepted/False/NoMatchingListenerHostname`
+    * `parentRef` - supported.
+    * `controllerName` - supported.
+    * `conditions` - partially supported. Supported (Condition/Status/Reason):
+        *  `Accepted/True/Accepted`
+        *  `Accepted/False/NoMatchingListenerHostname`
         *  `Accepted/False/NoMatchingParent`
         *  `Accepted/False/UnsupportedValue`: Custom reason for when the HTTPRoute includes an invalid or unsupported value.
         *  `Accepted/False/InvalidListener`: Custom reason for when the HTTPRoute references an invalid listener.
+        *  `Accepted/False/GatewayNotProgrammed`: Custom reason for when the Gateway is not Programmed. HTTPRoute may be valid and configured, but will maintain this status as long as the Gateway is not Programmed.
         *  `ResolvedRefs/True/ResolvedRefs`
         *  `ResolvedRefs/False/InvalidKind`
         *  `ResolvedRefs/False/RefNotPermitted`

diff --git a/internal/events/handler.go b/internal/events/handler.go
@@ -14,6 +14,7 @@ import (
 	"github.com/nginxinc/nginx-kubernetes-gateway/internal/nginx/runtime"
 	"github.com/nginxinc/nginx-kubernetes-gateway/internal/state"
 	"github.com/nginxinc/nginx-kubernetes-gateway/internal/state/dataplane"
+	"github.com/nginxinc/nginx-kubernetes-gateway/internal/state/resolver"
 	"github.com/nginxinc/nginx-kubernetes-gateway/internal/state/secrets"
 	"github.com/nginxinc/nginx-kubernetes-gateway/internal/status"
 )
@@ -35,6 +36,8 @@ type EventHandlerConfig struct {
 	SecretStore secrets.SecretStore
 	// SecretMemoryManager is the state SecretMemoryManager.
 	SecretMemoryManager secrets.SecretDiskMemoryManager
+	// ServiceResolver resolves Services to Endpoints.
+	ServiceResolver resolver.ServiceResolver
 	// Generator is the nginx config Generator.
 	Generator config.Generator
 	// NginxFileMgr is the file Manager for nginx.
@@ -74,20 +77,22 @@ func (h *EventHandlerImpl) HandleEventBatch(ctx context.Context, batch EventBatc
 		}
 	}
 
-	changed, conf, statuses := h.cfg.Processor.Process(ctx)
+	changed, graph := h.cfg.Processor.Process()
 	if !changed {
 		h.cfg.Logger.Info("Handling events didn't result into NGINX configuration changes")
 		return
 	}
 
-	err := h.updateNginx(ctx, conf)
+	var nginxReloadRes status.NginxReloadResult
+	err := h.updateNginx(ctx, dataplane.BuildConfiguration(ctx, graph, h.cfg.ServiceResolver))
 	if err != nil {
 		h.cfg.Logger.Error(err, "Failed to update NGINX configuration")
+		nginxReloadRes.Error = err
 	} else {
 		h.cfg.Logger.Info("NGINX configuration was successfully updated")
 	}
 
-	h.cfg.StatusUpdater.Update(ctx, statuses)
+	h.cfg.StatusUpdater.Update(ctx, status.BuildStatuses(graph, nginxReloadRes))
 }
 
 func (h *EventHandlerImpl) updateNginx(ctx context.Context, conf dataplane.Configuration) error {

diff --git a/internal/events/handler_test.go b/internal/events/handler_test.go
@@ -19,8 +19,8 @@ import (
 	"github.com/nginxinc/nginx-kubernetes-gateway/internal/nginx/config/configfakes"
 	"github.com/nginxinc/nginx-kubernetes-gateway/internal/nginx/file/filefakes"
 	"github.com/nginxinc/nginx-kubernetes-gateway/internal/nginx/runtime/runtimefakes"
-	"github.com/nginxinc/nginx-kubernetes-gateway/internal/state"
 	"github.com/nginxinc/nginx-kubernetes-gateway/internal/state/dataplane"
+	"github.com/nginxinc/nginx-kubernetes-gateway/internal/state/graph"
 	"github.com/nginxinc/nginx-kubernetes-gateway/internal/state/secrets/secretsfakes"
 	"github.com/nginxinc/nginx-kubernetes-gateway/internal/state/statefakes"
 	"github.com/nginxinc/nginx-kubernetes-gateway/internal/status/statusfakes"
@@ -50,7 +50,7 @@ var _ = Describe("EventHandler", func() {
 		fakeStatusUpdater       *statusfakes.FakeUpdater
 	)
 
-	expectReconfig := func(expectedConf dataplane.Configuration, expectedCfg []byte, expectedStatuses state.Statuses) {
+	expectReconfig := func(expectedConf dataplane.Configuration, expectedCfg []byte) {
 		Expect(fakeProcessor.ProcessCallCount()).Should(Equal(1))
 
 		Expect(fakeGenerator.GenerateCallCount()).Should(Equal(1))
@@ -64,8 +64,6 @@ var _ = Describe("EventHandler", func() {
 		Expect(fakeNginxRuntimeMgr.ReloadCallCount()).Should(Equal(1))
 
 		Expect(fakeStatusUpdater.UpdateCallCount()).Should(Equal(1))
-		_, statuses := fakeStatusUpdater.UpdateArgsForCall(0)
-		Expect(statuses).Should(Equal(expectedStatuses))
 	}
 
 	BeforeEach(func() {
@@ -93,10 +91,8 @@ var _ = Describe("EventHandler", func() {
 		DescribeTable(
 			"A batch with one event",
 			func(e interface{}) {
-				fakeConf := dataplane.Configuration{}
-				fakeStatuses := state.Statuses{}
 				changed := true
-				fakeProcessor.ProcessReturns(changed, fakeConf, fakeStatuses)
+				fakeProcessor.ProcessReturns(changed, &graph.Graph{})
 
 				fakeCfg := []byte("fake")
 				fakeGenerator.GenerateReturns(fakeCfg)
@@ -120,7 +116,7 @@ var _ = Describe("EventHandler", func() {
 				}
 
 				// Check that a reconfig happened
-				expectReconfig(fakeConf, fakeCfg, fakeStatuses)
+				expectReconfig(dataplane.Configuration{}, fakeCfg)
 			},
 			Entry(
 				"HTTPRoute upsert",
@@ -258,10 +254,8 @@ var _ = Describe("EventHandler", func() {
 		batch = append(batch, upserts...)
 		batch = append(batch, deletes...)
 
-		fakeConf := dataplane.Configuration{}
 		changed := true
-		fakeStatuses := state.Statuses{}
-		fakeProcessor.ProcessReturns(changed, fakeConf, fakeStatuses)
+		fakeProcessor.ProcessReturns(changed, &graph.Graph{})
 
 		fakeCfg := []byte("fake")
 		fakeGenerator.GenerateReturns(fakeCfg)
@@ -294,7 +288,7 @@ var _ = Describe("EventHandler", func() {
 		Expect(fakeSecretStore.DeleteArgsForCall(0)).Should(Equal(secretNsName))
 
 		// Check that a reconfig happened
-		expectReconfig(fakeConf, fakeCfg, fakeStatuses)
+		expectReconfig(dataplane.Configuration{}, fakeCfg)
 	})
 
 	Describe("Edge cases", func() {

diff --git a/internal/manager/manager.go b/internal/manager/manager.go
@@ -133,7 +133,6 @@ func Start(cfg config.Config) error {
 		GatewayCtlrName:      cfg.GatewayCtlrName,
 		GatewayClassName:     cfg.GatewayClassName,
 		SecretMemoryManager:  secretMemoryMgr,
-		ServiceResolver:      resolver.NewServiceResolverImpl(mgr.GetClient()),
 		RelationshipCapturer: relationship.NewCapturerImpl(),
 		Logger:               cfg.Logger.WithName("changeProcessor"),
 		Validators: validation.Validators{
@@ -160,6 +159,7 @@ func Start(cfg config.Config) error {
 		Processor:           processor,
 		SecretStore:         secretStore,
 		SecretMemoryManager: secretMemoryMgr,
+		ServiceResolver:     resolver.NewServiceResolverImpl(mgr.GetClient()),
 		Generator:           configGenerator,
 		Logger:              cfg.Logger.WithName("eventHandler"),
 		NginxFileMgr:        nginxFileMgr,

diff --git a/internal/nginx/runtime/manager.go b/internal/nginx/runtime/manager.go
@@ -51,7 +51,6 @@ 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.

diff --git a/internal/state/change_processor.go b/internal/state/change_processor.go
@@ -1,7 +1,6 @@
 package state
 
 import (
-	"context"
 	"fmt"
 	"sync"
 
@@ -18,10 +17,8 @@ import (
 
 	gwapivalidation "sigs.k8s.io/gateway-api/apis/v1beta1/validation"
 
-	"github.com/nginxinc/nginx-kubernetes-gateway/internal/state/dataplane"
 	"github.com/nginxinc/nginx-kubernetes-gateway/internal/state/graph"
 	"github.com/nginxinc/nginx-kubernetes-gateway/internal/state/relationship"
-	"github.com/nginxinc/nginx-kubernetes-gateway/internal/state/resolver"
 	"github.com/nginxinc/nginx-kubernetes-gateway/internal/state/secrets"
 	"github.com/nginxinc/nginx-kubernetes-gateway/internal/state/validation"
 )
@@ -35,7 +32,7 @@ const (
 
 type extractGVKFunc func(obj client.Object) schema.GroupVersionKind
 
-// ChangeProcessor processes the changes to resources producing the internal representation
+// ChangeProcessor processes the changes to resources and produces a graph-like representation
 // of the Gateway configuration. It only supports one GatewayClass resource.
 type ChangeProcessor interface {
 	// CaptureUpsertChange captures an upsert change to a resource.
@@ -46,19 +43,15 @@ type ChangeProcessor interface {
 	// The method panics if the resource is of unsupported type or if the passed Gateway is different from the one
 	// this ChangeProcessor was created for.
 	CaptureDeleteChange(resourceType client.Object, nsname types.NamespacedName)
-	// Process processes any captured changes and produces an internal representation of the Gateway configuration and
-	// the status information about the processed resources.
-	// If no changes were captured, the changed return argument will be false and both the configuration and statuses
-	// will be empty.
-	Process(ctx context.Context) (changed bool, conf dataplane.Configuration, statuses Statuses)
+	// Process produces a graph-like representation of GatewayAPI resources.
+	// If no changes were captured, the changed return argument will be false and graph will be empty.
+	Process() (changed bool, graphCfg *graph.Graph)
 }
 
 // ChangeProcessorConfig holds configuration parameters for ChangeProcessorImpl.
 type ChangeProcessorConfig struct {
 	// SecretMemoryManager is the secret memory manager.
 	SecretMemoryManager secrets.SecretDiskMemoryManager
-	// ServiceResolver resolves Services to Endpoints.
-	ServiceResolver resolver.ServiceResolver
 	// RelationshipCapturer captures relationships between Kubernetes API resources and Gateway API resources.
 	RelationshipCapturer relationship.Capturer
 	// Validators validate resources according to data-plane specific rules.
@@ -197,26 +190,21 @@ func (c *ChangeProcessorImpl) CaptureDeleteChange(resourceType client.Object, ns
 	c.updater.Delete(resourceType, nsname)
 }
 
-func (c *ChangeProcessorImpl) Process(
-	ctx context.Context,
-) (changed bool, conf dataplane.Configuration, statuses Statuses) {
+func (c *ChangeProcessorImpl) Process() (bool, *graph.Graph) {
 	c.lock.Lock()
 	defer c.lock.Unlock()
 
 	if !c.getAndResetClusterStateChanged() {
-		return false, conf, statuses
+		return false, nil
 	}
 
-	g := graph.BuildGraph(
+	graphCfg := graph.BuildGraph(
 		c.clusterState,
 		c.cfg.GatewayCtlrName,
 		c.cfg.GatewayClassName,
 		c.cfg.SecretMemoryManager,
 		c.cfg.Validators,
 	)
 
-	conf = dataplane.BuildConfiguration(ctx, g, c.cfg.ServiceResolver)
-	statuses = buildStatuses(g)
-
-	return true, conf, statuses
+	return true, graphCfg
 }