Skip to content

Commit

Permalink
Refacter code to use new ObjectType (#2121)
Browse files Browse the repository at this point in the history
* Refacter code to use new ObjectType

Problem: We currently use the variable objType
to refer to a client.Object's type (skeleton of
an object). We also use the variable obj to refer
to a full client.Object (object with fields filled
out). However, in many parts of the codebase these
two variables are used closely together and are
both of type client.Object which can be a little
confusing.

Solution: I created a new type called ObjectType
and refactored the codebase to utilize it.
  • Loading branch information
sarthyparty authored Jun 12, 2024
1 parent b429f74 commit 7654cb6
Show file tree
Hide file tree
Showing 17 changed files with 74 additions and 44 deletions.
5 changes: 3 additions & 2 deletions internal/framework/controller/reconciler.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (
"sigs.k8s.io/controller-runtime/pkg/reconcile"

"github.com/nginxinc/nginx-gateway-fabric/internal/framework/events"
ngftypes "github.com/nginxinc/nginx-gateway-fabric/internal/framework/types"
)

// NamespacedNameFilterFunc is a function that returns true if the resource should be processed by the reconciler.
Expand All @@ -24,7 +25,7 @@ type ReconcilerConfig struct {
// Getter gets a resource from the k8s API.
Getter Getter
// ObjectType is the type of the resource that the reconciler will reconcile.
ObjectType client.Object
ObjectType ngftypes.ObjectType
// EventCh is the channel where the reconciler will send events.
EventCh chan<- interface{}
// NamespacedNameFilter filters resources the controller will process. Can be nil.
Expand Down Expand Up @@ -52,7 +53,7 @@ func NewReconciler(cfg ReconcilerConfig) *Reconciler {
}
}

func (r *Reconciler) newObject(objectType client.Object) client.Object {
func (r *Reconciler) newObject(objectType ngftypes.ObjectType) ngftypes.ObjectType {
if r.cfg.OnlyMetadata {
partialObj := &metav1.PartialObjectMetadata{}
partialObj.SetGroupVersionKind(objectType.GetObjectKind().GroupVersionKind())
Expand Down
5 changes: 3 additions & 2 deletions internal/framework/controller/register.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
"sigs.k8s.io/controller-runtime/pkg/predicate"

"github.com/nginxinc/nginx-gateway-fabric/internal/framework/controller/index"
ngftypes "github.com/nginxinc/nginx-gateway-fabric/internal/framework/types"
)

const (
Expand Down Expand Up @@ -82,7 +83,7 @@ func defaultConfig() config {
// The registered controller will send events to the provided channel.
func Register(
ctx context.Context,
objectType client.Object,
objectType ngftypes.ObjectType,
mgr manager.Manager,
eventCh chan<- interface{},
options ...Option,
Expand Down Expand Up @@ -137,7 +138,7 @@ func Register(
func addIndex(
ctx context.Context,
indexer client.FieldIndexer,
objectType client.Object,
objectType ngftypes.ObjectType,
field string,
indexerFunc client.IndexerFunc,
) error {
Expand Down
4 changes: 2 additions & 2 deletions internal/framework/controller/register_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ import (
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/apimachinery/pkg/types"
utilruntime "k8s.io/apimachinery/pkg/util/runtime"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/client/fake"
"sigs.k8s.io/controller-runtime/pkg/log/zap"
v1 "sigs.k8s.io/gateway-api/apis/v1"
Expand All @@ -24,6 +23,7 @@ import (
"github.com/nginxinc/nginx-gateway-fabric/internal/framework/controller/index"
"github.com/nginxinc/nginx-gateway-fabric/internal/framework/controller/predicate"
"github.com/nginxinc/nginx-gateway-fabric/internal/framework/kinds"
ngftypes "github.com/nginxinc/nginx-gateway-fabric/internal/framework/types"
)

func TestRegister(t *testing.T) {
Expand Down Expand Up @@ -62,7 +62,7 @@ func TestRegister(t *testing.T) {

tests := []struct {
fakes fakes
objectType client.Object
objectType ngftypes.ObjectType
expectedErr error
msg string
expectedMgrAddCallCount int
Expand Down
4 changes: 3 additions & 1 deletion internal/framework/events/event.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ package events
import (
"k8s.io/apimachinery/pkg/types"
"sigs.k8s.io/controller-runtime/pkg/client"

ngftypes "github.com/nginxinc/nginx-gateway-fabric/internal/framework/types"
)

// EventBatch is a batch of events to be handled at once.
Expand All @@ -17,7 +19,7 @@ type UpsertEvent struct {
// DeleteEvent representing deleting a resource.
type DeleteEvent struct {
// Type is the resource type. For example, if the event is for *v1.HTTPRoute, pass &v1.HTTPRoute{} as Type.
Type client.Object
Type ngftypes.ObjectType
// NamespacedName is the namespace & name of the deleted resource.
NamespacedName types.NamespacedName
}
5 changes: 3 additions & 2 deletions internal/framework/status/updater.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,12 @@ import (
"sigs.k8s.io/controller-runtime/pkg/client"

"github.com/nginxinc/nginx-gateway-fabric/internal/framework/controller"
ngftypes "github.com/nginxinc/nginx-gateway-fabric/internal/framework/types"
)

// UpdateRequest is a request to update the status of a resource.
type UpdateRequest struct {
ResourceType client.Object
ResourceType ngftypes.ObjectType
Setter Setter
NsName types.NamespacedName
}
Expand Down Expand Up @@ -83,7 +84,7 @@ func (u *Updater) Update(ctx context.Context, reqs ...UpdateRequest) {
func (u *Updater) writeStatuses(
ctx context.Context,
nsname types.NamespacedName,
resourceType client.Object,
resourceType ngftypes.ObjectType,
statusSetter Setter,
) {
obj := resourceType.DeepCopyObject().(client.Object)
Expand Down
4 changes: 4 additions & 0 deletions internal/framework/types/doc.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
/*
Package types contains types that are shared by the provisioner and static modes.
*/
package types
7 changes: 7 additions & 0 deletions internal/framework/types/types.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
package types

import "sigs.k8s.io/controller-runtime/pkg/client"

// ObjectType is used when we only care about the type of client.Object.
// The fields of the client.Object may be empty.
type ObjectType client.Object
3 changes: 2 additions & 1 deletion internal/mode/provisioner/manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"github.com/nginxinc/nginx-gateway-fabric/internal/framework/events"
"github.com/nginxinc/nginx-gateway-fabric/internal/framework/gatewayclass"
"github.com/nginxinc/nginx-gateway-fabric/internal/framework/status"
ngftypes "github.com/nginxinc/nginx-gateway-fabric/internal/framework/types"
)

// Config is configuration for the provisioner mode.
Expand Down Expand Up @@ -63,7 +64,7 @@ func StartManager(cfg Config) error {
// Note: for any new object type or a change to the existing one,
// make sure to also update firstBatchPreparer creation below
controllerRegCfgs := []struct {
objectType client.Object
objectType ngftypes.ObjectType
options []controller.Option
}{
{
Expand Down
3 changes: 2 additions & 1 deletion internal/mode/static/manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ import (
"github.com/nginxinc/nginx-gateway-fabric/internal/framework/kinds"
"github.com/nginxinc/nginx-gateway-fabric/internal/framework/runnables"
"github.com/nginxinc/nginx-gateway-fabric/internal/framework/status"
ngftypes "github.com/nginxinc/nginx-gateway-fabric/internal/framework/types"
"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/config"
"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/metrics/collectors"
ngxcfg "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config"
Expand Down Expand Up @@ -356,7 +357,7 @@ func registerControllers(
controlConfigNSName types.NamespacedName,
) error {
type ctlrCfg struct {
objectType client.Object
objectType ngftypes.ObjectType
options []controller.Option
}

Expand Down
9 changes: 5 additions & 4 deletions internal/mode/static/state/change_processor.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
ngfAPI "github.com/nginxinc/nginx-gateway-fabric/apis/v1alpha1"
"github.com/nginxinc/nginx-gateway-fabric/internal/framework/gatewayclass"
"github.com/nginxinc/nginx-gateway-fabric/internal/framework/kinds"
ngftypes "github.com/nginxinc/nginx-gateway-fabric/internal/framework/types"
"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/policies"
"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/graph"
"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/validation"
Expand Down Expand Up @@ -50,7 +51,7 @@ type ChangeProcessor interface {
// CaptureDeleteChange captures a delete change to a resource.
// 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)
CaptureDeleteChange(resourceType ngftypes.ObjectType, nsname types.NamespacedName)
// Process produces a graph-like representation of GatewayAPI resources.
// If no changes were captured, the changed return argument will be NoChange and graph will be empty.
Process() (changeType ChangeType, graphCfg *graph.Graph)
Expand Down Expand Up @@ -114,11 +115,11 @@ func NewChangeProcessorImpl(cfg ChangeProcessorConfig) *ChangeProcessorImpl {
clusterState: clusterStore,
}

isReferenced := func(obj client.Object, nsname types.NamespacedName) bool {
isReferenced := func(obj ngftypes.ObjectType, nsname types.NamespacedName) bool {
return processor.latestGraph != nil && processor.latestGraph.IsReferenced(obj, nsname)
}

isNGFPolicyRelevant := func(obj client.Object, nsname types.NamespacedName) bool {
isNGFPolicyRelevant := func(obj ngftypes.ObjectType, nsname types.NamespacedName) bool {
pol, ok := obj.(policies.Policy)
if !ok {
return false
Expand Down Expand Up @@ -241,7 +242,7 @@ func (c *ChangeProcessorImpl) CaptureUpsertChange(obj client.Object) {
c.updater.Upsert(obj)
}

func (c *ChangeProcessorImpl) CaptureDeleteChange(resourceType client.Object, nsname types.NamespacedName) {
func (c *ChangeProcessorImpl) CaptureDeleteChange(resourceType ngftypes.ObjectType, nsname types.NamespacedName) {
c.lock.Lock()
defer c.lock.Unlock()

Expand Down
3 changes: 2 additions & 1 deletion internal/mode/static/state/change_processor_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (
"github.com/nginxinc/nginx-gateway-fabric/internal/framework/gatewayclass"
"github.com/nginxinc/nginx-gateway-fabric/internal/framework/helpers"
"github.com/nginxinc/nginx-gateway-fabric/internal/framework/kinds"
ngftypes "github.com/nginxinc/nginx-gateway-fabric/internal/framework/types"
"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state"
staticConds "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/conditions"
"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/graph"
Expand Down Expand Up @@ -2328,7 +2329,7 @@ var _ = Describe("ChangeProcessor", func() {

DescribeTable(
"CaptureDeleteChange must panic",
func(resourceType client.Object, nsname types.NamespacedName) {
func(resourceType ngftypes.ObjectType, nsname types.NamespacedName) {
process := func() {
processor.CaptureDeleteChange(resourceType, nsname)
}
Expand Down
12 changes: 8 additions & 4 deletions internal/mode/static/state/changed_predicate.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,20 +3,22 @@ package state
import (
"k8s.io/apimachinery/pkg/types"
"sigs.k8s.io/controller-runtime/pkg/client"

ngftypes "github.com/nginxinc/nginx-gateway-fabric/internal/framework/types"
)

// stateChangedPredicate determines whether upsert and delete events constitute a change in state.
type stateChangedPredicate interface {
// upsert returns true if the newObject changes state.
upsert(oldObject, newObject client.Object) bool
// delete returns true if the deletion of the object changes state.
delete(object client.Object, nsname types.NamespacedName) bool
delete(object ngftypes.ObjectType, nsname types.NamespacedName) bool
}

// funcPredicate applies the stateChanged function on upsert and delete. On upsert, the newObject is passed.
// Implements stateChangedPredicate.
type funcPredicate struct {
stateChanged func(object client.Object, nsname types.NamespacedName) bool
stateChanged func(object ngftypes.ObjectType, nsname types.NamespacedName) bool
}

func (f funcPredicate) upsert(_, newObject client.Object) bool {
Expand All @@ -27,7 +29,7 @@ func (f funcPredicate) upsert(_, newObject client.Object) bool {
return f.stateChanged(newObject, client.ObjectKeyFromObject(newObject))
}

func (f funcPredicate) delete(object client.Object, nsname types.NamespacedName) bool {
func (f funcPredicate) delete(object ngftypes.ObjectType, nsname types.NamespacedName) bool {
return f.stateChanged(object, nsname)
}

Expand All @@ -53,4 +55,6 @@ func (a annotationChangedPredicate) upsert(oldObject, newObject client.Object) b
return oldAnnotation != newAnnotation
}

func (a annotationChangedPredicate) delete(_ client.Object, _ types.NamespacedName) bool { return true }
func (a annotationChangedPredicate) delete(_ ngftypes.ObjectType, _ types.NamespacedName) bool {
return true
}
6 changes: 4 additions & 2 deletions internal/mode/static/state/changed_predicate_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,12 @@ import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/types"
"sigs.k8s.io/controller-runtime/pkg/client"

ngftypes "github.com/nginxinc/nginx-gateway-fabric/internal/framework/types"
)

func TestFuncPredicate(t *testing.T) {
alwaysTrueFunc := func(_ client.Object, _ types.NamespacedName) bool { return true }
alwaysTrueFunc := func(_ ngftypes.ObjectType, _ types.NamespacedName) bool { return true }
emptyObject := &v1.Pod{}

p := funcPredicate{stateChanged: alwaysTrueFunc}
Expand All @@ -23,7 +25,7 @@ func TestFuncPredicate(t *testing.T) {
}

func TestFuncPredicate_Panic(t *testing.T) {
alwaysTrueFunc := func(_ client.Object, _ types.NamespacedName) bool { return true }
alwaysTrueFunc := func(_ ngftypes.ObjectType, _ types.NamespacedName) bool { return true }

p := funcPredicate{stateChanged: alwaysTrueFunc}

Expand Down
3 changes: 2 additions & 1 deletion internal/mode/static/state/graph/graph.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import (
ngfAPI "github.com/nginxinc/nginx-gateway-fabric/apis/v1alpha1"
"github.com/nginxinc/nginx-gateway-fabric/internal/framework/controller/index"
"github.com/nginxinc/nginx-gateway-fabric/internal/framework/kinds"
ngftypes "github.com/nginxinc/nginx-gateway-fabric/internal/framework/types"
"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/policies"
"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/validation"
)
Expand Down Expand Up @@ -79,7 +80,7 @@ type Graph struct {
type ProtectedPorts map[int32]string

// IsReferenced returns true if the Graph references the resource.
func (g *Graph) IsReferenced(resourceType client.Object, nsname types.NamespacedName) bool {
func (g *Graph) IsReferenced(resourceType ngftypes.ObjectType, nsname types.NamespacedName) bool {
switch obj := resourceType.(type) {
case *v1.Secret:
_, exists := g.ReferencedSecrets[nsname]
Expand Down
19 changes: 10 additions & 9 deletions internal/mode/static/state/statefakes/fake_change_processor.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading

0 comments on commit 7654cb6

Please sign in to comment.