[Framework] implement ingress operator by weilerN · Pull Request #75 · v3io/scaler

weilerN · 2025-07-03T15:57:22Z

Description

Full description in the LLD that linked to this Jira - https://iguazio.atlassian.net/browse/NUC-498
This PR introduces a new IngressWatcher component that uses Kubernetes informers to watch for Ingress resource changes and updates an internal IngressHostCache accordingly. The watcher handles add, update, and delete events, extracts relevant data (host, path, target list), and reflects those changes in the in-memory cache.

Affected Areas

pkg/kube/ingress.go – new file with the IngressWatcher implementation
Uses IngressHostCache from pkg/ingresscache
Integrates with k8s.io/client-go for informers

Testing

Unit tests coverage for all event handler paths (add, update, delete) correctly modify the ingress cache.
Unit tests validates cache sync behavior and logging of edge cases (e.g., missing host/path).
Integrations tests are out of scope for this PR, will be introduced as part of the code changes in Nuclio

Changes Made

Added IngressWatcher struct to manage and respond to Kubernetes Ingress resource changes.
Implemented handlers for Add, Update, and Delete events.
Extracted ingress data: host, path, targets via helper methods.
Registered shared informer factory with label selector support.

Additional notes

Assumes each Ingress contains exactly one rule and one path.
Includes detailed error logging for resilience and observability.
Additional PR for integrating the ingress cache and ingress watcher is needed

rokatyy

Some questions and suggestion to begin with. Also, please make sure that all errors start with capitalised letters

rokatyy · 2025-07-04T08:58:39Z

pkg/kube/ingress.go

+func (iw *IngressWatcher) IngressHandlerDeleteFunc(obj interface{}) {
+	host, path, targets, err := iw.extractIngressValuesFromIngressResource(obj)
+	if err != nil {
+		iw.logger.WarnWith("Delete ingress handler failure- failed to extract values from object", "error", err)
+		return
+	}
+
+	if err = iw.ingressCache.Delete(host, path, targets); err != nil {
+		iw.logger.WarnWith("Delete ingress handler failure- failed delete from cache", "error", err, "object", obj)
+		return
+	}


please rename all this methods to not have ingress in them, it is already in the name of IngressWatcher, so it can be simply Add, Update, Delete

Agreed, it can not have Func, e.g. AddHandler etc

Fixed here - CR comments

rokatyy · 2025-07-04T09:03:11Z

pkg/kube/ingress.go

+}
+
+// extractHostPathTarget extracts the host, path, and targets from the ingress resource
+func (iw *IngressWatcher) extractHostPathTarget(ingress *networkingv1.Ingress) (string, string, []string, error) {


How about a structure to return all these objects?

Suggested change

func (iw *IngressWatcher) extractHostPathTarget(ingress *networkingv1.Ingress) (string, string, []string, error) {

type IngressValues struct {

Host string

Path string

Targets []string

}

it will improve readability and get rid of many "" and etc

Fixed here - CR comments

rokatyy · 2025-07-04T09:05:49Z

pkg/kube/ingress.go

+		return "", errors.New("no rules found in ingress")
+	}
+
+	// Ingress must contain exactly one rule


It is not a rule in k8s, so if it contains more, we should still ignore that and get the first one, but add log about that

Fixed here - CR comments

rokatyy · 2025-07-04T09:05:58Z

pkg/kube/ingress.go

+	// Ingress must contain exactly one rule
+	httpPath := rule.HTTP.Paths[0]


Fixed here - CR comments

rokatyy · 2025-07-04T09:07:07Z

pkg/kube/ingress.go

+	}
+
+	if err = iw.ingressCache.Set(newHost, newPath, newTargets); err != nil {
+		iw.logger.WarnWith("Update ingress handler failure- failed to add the new value", "error", err, "object", newObj)


Suggested change

iw.logger.WarnWith("Update ingress handler failure- failed to add the new value", "error", err, "object", newObj)

iw.logger.WarnWith("Update ingress handler failure - failed to add the new value",

"error", err, "object", newObj)

Fixed here - CR comments

rokatyy · 2025-07-04T09:07:16Z

pkg/kube/ingress.go

+	// if the host or path has changed, we need to delete the old entry
+	if oldHost != newHost || oldPath != newPath {
+		if err = iw.ingressCache.Delete(oldHost, oldPath, oldTargets); err != nil {
+			iw.logger.WarnWith("Update ingress handler failure - failed to delete old ingress", "error", err)


Suggested change

iw.logger.WarnWith("Update ingress handler failure - failed to delete old ingress", "error", err)

iw.logger.WarnWith("Update ingress handler failure - failed to delete old ingress",

"error", err)

and same in all the places above

Fixed here - CR comments

rokatyy · 2025-07-04T09:07:39Z

pkg/kube/ingress.go

+	}
+
+	if err = iw.ingressCache.Set(host, path, targets); err != nil {
+		iw.logger.WarnWith("Add ingress handler failure- failed to add the new value", "error", err, "object", obj)


Suggested change

iw.logger.WarnWith("Add ingress handler failure- failed to add the new value", "error", err, "object", obj)

iw.logger.WarnWith("Add ingress handler failure - failed to add the new value",

"error", err, "object", obj)

Fixed here - CR comments

rokatyy · 2025-07-04T09:10:28Z

pkg/kube/ingress.go

+func (iw *IngressWatcher) getPathFromIngress(ingress *networkingv1.Ingress) (string, error) {
+	if ingress == nil {
+		return "", errors.New("ingress is nil")
+	}
+
+	if len(ingress.Spec.Rules) == 0 {
+		return "", errors.New("no rules found in ingress")
+	}
+


duplication of the code in getHostFromIngress, can be moved to smth like

func (iw *IngressWatcher) getFirstRule(ingress *networkingv1.Ingress) (*networkingv1.IngressRule, error) { if ingress == nil { return nil, errors.New("ingress is nil") } if len(ingress.Spec.Rules) == 0 { return nil, errors.New("no rules found in ingress") } return &ingress.Spec.Rules[0], nil }

Fixed here - CR comments

rokatyy · 2025-07-04T09:16:15Z

pkg/kube/ingress.go

+	targets, err := iw.resolveTargetsCallback(ingress)
+	if err != nil {
+		return "", "", nil, errors.Wrap(err, "Failed to extract targets from ingress labels")
+	}
+


what if resolveTargetsCallback does some additional filtering and returns empty targets if ingress doesn't meet the conditions, would it make sense to put it above getHostFromIngress?

I agree that since the callback function is external to the scaler, it's appropriate to check for nil before using the returned targets.

The order of the function calls isn't critical, but I originally structured them to follow the logical route hierarchy: host -> path -> targets.
If I understood you correctly, you'd like the target resolution to come first since it’s the most complex part, and placing it earlier supports a fail-fast approach. That makes sense, and I’ll update the order accordingly.

Fixed here - CR comments

rokatyy · 2025-07-04T09:18:03Z

pkg/kube/ingress.go

+	"k8s.io/client-go/tools/cache"
+)
+
+type ResolveTargetsFromIngressCallback func(ingress *networkingv1.Ingress) ([]string, error)


Please add a docstring for this type, as its function is expected to be implemented in a custom way. We should provide some guidance on what the implementation should include.

Fixed here - CR comments

TomerShor · 2025-07-06T06:45:58Z

pkg/kube/ingress.go

+	ctx context.Context,
+	dlxLogger logger.Logger,
+	kubeClient kubernetes.Interface,
+	ingressCache *ingresscache.IngressCache,


In the struct above the property is defined as the IngressHostCache interface, but here you receive a pointer to the actual implementation IngressCache.
This should also be the interface.

Fixed here - CR comments

TomerShor · 2025-07-06T06:46:50Z

pkg/kube/ingress.go

+
+	ingressWatcher := &IngressWatcher{
+		ctx:                    ctx,
+		logger:                 dlxLogger,


Suggested change

logger: dlxLogger,

logger: dlxLogger.GetChild("watcher"),

Fixed here - CR comments

TomerShor · 2025-07-06T06:47:42Z

pkg/kube/ingress.go

+) (*IngressWatcher, error) {
+	factory := informers.NewSharedInformerFactoryWithOptions(
+		kubeClient,
+		30*time.Second,


This resync interval should be configurable

Fixed here - CR comments

TomerShor · 2025-07-06T06:48:50Z

pkg/kube/ingress.go

+}
+
+func (iw *IngressWatcher) Start() error {
+	iw.logger.Debug("Starting IngressWatcher")


Suggested change

iw.logger.Debug("Starting IngressWatcher")

iw.logger.Info("Starting ingress watcher")

Fixed here - CR comments

TomerShor · 2025-07-06T06:49:03Z

pkg/kube/ingress.go

+	iw.factory.Start(iw.ctx.Done())
+
+	if !cache.WaitForCacheSync(iw.ctx.Done(), iw.informer.HasSynced) {
+		return errors.New("failed to sync ingress cache")


Suggested change

return errors.New("failed to sync ingress cache")

return errors.New("Failed to sync ingress cache")

Fixed here - CR comments

TomerShor · 2025-07-06T07:02:52Z

pkg/kube/ingress_test.go

+// TestIngressWatcherTestSuite runs the test suite
+func TestIngressWatcherTestSuite(t *testing.T) {
+	suite.Run(t, new(IngressWatcherTestSuite))
+}


Move to the bottom of the file

Fixed here - CR comments

TomerShor · 2025-07-06T07:03:49Z

pkg/kube/ingress_test.go

+		name              string
+		testArgs          ingressArgs
+		expectedResult    []string
+		initialStateCache *ingressArgs


Suggested change

initialStateCache *ingressArgs

initialCachedData *ingressArgs

Fixed here - CR comments

TomerShor · 2025-07-06T07:05:30Z

pkg/kube/ingress_test.go

+	type ingressArgs struct {
+		host    string
+		path    string
+		targets []string
+	}


Following @rokatyy 's suggestion, this can be a struct in ingress.go.
But generally - if you see that you defined the same struct multiple times - define it once outside of the tests.

Fixed here - CR comments

TomerShor · 2025-07-06T07:05:44Z

pkg/kube/ingress_test.go

+	for _, testCase := range []struct {
+		name              string
+		expectedResults   []expectedResults
+		initialStateCache *ingressArgs


Suggested change

initialStateCache *ingressArgs

initialCacheData *ingressArgs

Fixed here - CR comments

TomerShor · 2025-07-06T07:06:18Z

pkg/kube/ingress_test.go

+	type expectedResult struct {
+		host           string
+		path           string
+		targets        []string
+		expectError    bool
+		expectErrorMsg string
+	}


Same for this - move it to the top of the file.

Fixed here - CR comments

rokatyy

LGTM 👍 , just a minor thing regarding the comment

rokatyy · 2025-07-07T09:25:29Z

pkg/kube/ingress.go

+//
+// Implementation guidelines:
+// - Return a non-nil slice when targets are successfully resolved
+// - Return a non-nil error if resolution fails or no targets are found


why error when no targets found? maybe user code contain some custom filtering. I think that's ok not to throw error in this case, just ignore if empty

Fixed here - CR comments

pkg/kube/ingress.go

TomerShor

Last minor comments from me

pkg/kube/ingress.go

weilerN added 7 commits July 3, 2025 10:37

Introduce the ingress operator

aa20e5c

Add EventHandler

bbdd049

extract values from an ingress resource

2c19d37

add ingressCache as part of init

a9027ed

add unit tests

131f1b0

fix linter

15c78b1

go.mod additions

ad1e484

weilerN force-pushed the nuc-498-implement-ingress-operator branch from 629f57b to ad1e484 Compare July 3, 2025 16:18

add license

426af9e

weilerN requested review from TomerShor and rokatyy July 3, 2025 16:26

weilerN marked this pull request as ready for review July 3, 2025 16:27

rokatyy requested changes Jul 4, 2025

View reviewed changes

TomerShor requested changes Jul 6, 2025

View reviewed changes

CR comments

fc972c8

weilerN requested review from TomerShor and rokatyy July 6, 2025 16:04

rokatyy approved these changes Jul 7, 2025

View reviewed changes

TomerShor requested changes Jul 7, 2025

View reviewed changes

CR comments - change warning logs

0522781

weilerN force-pushed the nuc-498-implement-ingress-operator branch from d00d4c6 to 0522781 Compare July 7, 2025 14:11

CR comments

90bdbd4

weilerN requested a review from TomerShor July 7, 2025 14:21

logs improvements

429e078

TomerShor requested changes Jul 8, 2025

View reviewed changes

CR comments

2f8b01a

weilerN requested a review from TomerShor July 8, 2025 08:35

TomerShor approved these changes Jul 8, 2025

View reviewed changes

TomerShor merged commit 23d5465 into v3io:development Jul 8, 2025
3 checks passed

-func (iw *IngressWatcher) extractHostPathTarget(ingress *networkingv1.Ingress) (string, string, []string, error) {
+type IngressValues struct {
+    Host    string
+    Path    string
+    Targets []string
+}

		// Ingress must contain exactly one rule
		httpPath := rule.HTTP.Paths[0]

	iw.logger.WarnWith("Update ingress handler failure- failed to add the new value", "error", err, "object", newObj)
	iw.logger.WarnWith("Update ingress handler failure - failed to add the new value",
	"error", err, "object", newObj)

	iw.logger.WarnWith("Update ingress handler failure - failed to delete old ingress", "error", err)
	iw.logger.WarnWith("Update ingress handler failure - failed to delete old ingress",
	"error", err)

	iw.logger.WarnWith("Add ingress handler failure- failed to add the new value", "error", err, "object", obj)
	iw.logger.WarnWith("Add ingress handler failure - failed to add the new value",
	"error", err, "object", obj)

	iw.logger.Debug("Starting IngressWatcher")
	iw.logger.Info("Starting ingress watcher")

	return errors.New("failed to sync ingress cache")
	return errors.New("Failed to sync ingress cache")

	initialStateCache *ingressArgs
	initialCachedData *ingressArgs

Conversation

weilerN commented Jul 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Affected Areas

Testing

Changes Made

Additional notes

Uh oh!

rokatyy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weilerN Jul 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weilerN commented Jul 3, 2025 •

edited

Loading

weilerN Jul 6, 2025 •

edited

Loading