Merge pull request #112 from LiorLieberman/per-provider-ingress-fetching-isolation

Change ingress fetching to be isolated per provider

Author: k8s-ci-robot

pkg/i2gw/ingress2gateway.go

@@ -45,30 +45,19 @@ func ToGatewayAPIResources(ctx context.Context, namespace string, inputFile stri
 	}
 	cl = client.NewNamespacedClient(cl, namespace)
 
-	var ingresses networkingv1.IngressList
-
 	providerByName, err := constructProviders(&ProviderConf{
-		Client: cl,
+		Client:    cl,
+		Namespace: namespace,
 	}, providers)
 	if err != nil {
 		return nil, err
 	}
 
-	resources := InputResources{}
-
 	if inputFile != "" {
-		if err = ConstructIngressesFromFile(&ingresses, inputFile, namespace); err != nil {
-			return nil, fmt.Errorf("failed to read ingresses from file: %w", err)
-		}
-		resources.Ingresses = ingresses.Items
 		if err = readProviderResourcesFromFile(ctx, providerByName, inputFile); err != nil {
 			return nil, err
 		}
 	} else {
-		if err = ConstructIngressesFromCluster(ctx, cl, &ingresses); err != nil {
-			return nil, fmt.Errorf("failed to read ingresses from cluster: %w", err)
-		}
-		resources.Ingresses = ingresses.Items
 		if err = readProviderResourcesFromCluster(ctx, providerByName); err != nil {
 			return nil, err
 		}
@@ -79,7 +68,8 @@ func ToGatewayAPIResources(ctx context.Context, namespace string, inputFile stri
 		errs             field.ErrorList
 	)
 	for _, provider := range providerByName {
-		providerGatewayResources, conversionErrs := provider.ToGatewayAPI(resources)
+		// TODO(#113) Remove input resources from ToGatewayAPI function
+		providerGatewayResources, conversionErrs := provider.ToGatewayAPI(InputResources{})
 		errs = append(errs, conversionErrs...)
 		gatewayResources = append(gatewayResources, providerGatewayResources)
 	}
@@ -136,7 +126,8 @@ func constructProviders(conf *ProviderConf, providers []string) (map[ProviderNam
 // ExtractObjectsFromReader extracts all objects from a reader,
 // which is created from YAML or JSON input files.
 // It retrieves all objects, including nested ones if they are contained within a list.
-func ExtractObjectsFromReader(reader io.Reader) ([]*unstructured.Unstructured, error) {
+// The function takes a namespace parameter to optionally return only namespaced resources.
+func ExtractObjectsFromReader(reader io.Reader, namespace string) ([]*unstructured.Unstructured, error) {
 	d := kubeyaml.NewYAMLOrJSONDecoder(reader, 4096)
 	var objs []*unstructured.Unstructured
 	for {
@@ -150,6 +141,9 @@ func ExtractObjectsFromReader(reader io.Reader) ([]*unstructured.Unstructured, e
 		if u == nil {
 			continue
 		}
+		if namespace != "" && u.GetNamespace() != namespace {
+			continue
+		}
 		objs = append(objs, u)
 	}
 
@@ -187,15 +181,12 @@ func ConstructIngressesFromFile(l *networkingv1.IngressList, inputFile string, n
 	}
 
 	reader := bytes.NewReader(stream)
-	objs, err := ExtractObjectsFromReader(reader)
+	objs, err := ExtractObjectsFromReader(reader, namespace)
 	if err != nil {
 		return err
 	}
 
 	for _, f := range objs {
-		if namespace != "" && f.GetNamespace() != namespace {
-			continue
-		}
 		if !f.GroupVersionKind().Empty() && f.GroupVersionKind().Kind == "Ingress" {
 			var i networkingv1.Ingress
 			err = runtime.DefaultUnstructuredConverter.

pkg/i2gw/ingress2gateway_test.go

@@ -17,19 +17,21 @@ limitations under the License.
 package i2gw
 
 import (
-	"context"
+	"bytes"
 	"fmt"
+	"os"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
 	networkingv1 "k8s.io/api/networking/v1"
 	apiequality "k8s.io/apimachinery/pkg/api/equality"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 )
 
-func Test_constructIngressesFromFile(t *testing.T) {
+func Test_ExtractObjectsFromReader(t *testing.T) {
 	ingress1 := ingress(443, "ingress1", "namespace1")
 	ingress2 := ingress(80, "ingress2", "namespace2")
 	ingressNoNamespace := ingress(80, "ingress-no-namespace", "")
@@ -60,10 +62,17 @@ func Test_constructIngressesFromFile(t *testing.T) {
 
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
-			gotIngressList := &networkingv1.IngressList{}
-			err := ConstructIngressesFromFile(gotIngressList, tc.filePath, tc.namespace)
+			stream, err := os.ReadFile(tc.filePath)
 			if err != nil {
-				t.Errorf("Failed to open test file: %v", err)
+				t.Errorf("failed to read file %s: %v", tc.filePath, err)
+			}
+			unstructuredObjects, err := ExtractObjectsFromReader(bytes.NewReader(stream), tc.namespace)
+			if err != nil {
+				t.Errorf("failed to extract objects: %s", err)
+			}
+			gotIngressList, err := ingressListFromUnstructured(unstructuredObjects)
+			if err != nil {
+				t.Errorf("got unexpected error: %v", err)
 			}
 			compareIngressLists(t, gotIngressList, tc.wantIngressList)
 		})
@@ -111,6 +120,21 @@ func ingress(port int32, name, namespace string) networkingv1.Ingress {
 	return ing
 }
 
+func ingressListFromUnstructured(unstructuredObjects []*unstructured.Unstructured) (*networkingv1.IngressList, error) {
+	ingressList := &networkingv1.IngressList{}
+	for _, f := range unstructuredObjects {
+		if !f.GroupVersionKind().Empty() && f.GroupVersionKind().Kind == "Ingress" {
+			var i networkingv1.Ingress
+			err := runtime.DefaultUnstructuredConverter.
+				FromUnstructured(f.UnstructuredContent(), &i)
+			if err != nil {
+				return nil, err
+			}
+			ingressList.Items = append(ingressList.Items, i)
+		}
+	}
+	return ingressList, nil
+}
 func compareIngressLists(t *testing.T, gotIngressList *networkingv1.IngressList, wantIngressList []networkingv1.Ingress) {
 	for i, got := range gotIngressList.Items {
 		want := wantIngressList[i]
@@ -120,37 +144,6 @@ func compareIngressLists(t *testing.T, gotIngressList *networkingv1.IngressList,
 	}
 }
 
-func Test_constructIngressesFromCluster(t *testing.T) {
-	ingress1 := ingress(443, "ingress1", "namespace1")
-	ingress2 := ingress(80, "ingress2", "namespace2")
-	testCases := []struct {
-		name          string
-		runtimeObjs   []runtime.Object
-		wantIngresses []networkingv1.Ingress
-	}{{
-		name:          "Test cluster client with 2 resources",
-		runtimeObjs:   []runtime.Object{&ingress1, &ingress2},
-		wantIngresses: []networkingv1.Ingress{ingress1, ingress2},
-	}, {
-		name:          "Test cluster client without resources",
-		runtimeObjs:   []runtime.Object{},
-		wantIngresses: []networkingv1.Ingress{},
-	},
-	}
-
-	for _, tc := range testCases {
-		t.Run(tc.name, func(t *testing.T) {
-			gotIngresses := &networkingv1.IngressList{}
-			cl := fake.NewClientBuilder().WithRuntimeObjects(tc.runtimeObjs...).Build()
-			err := ConstructIngressesFromCluster(context.Background(), cl, gotIngresses)
-			if err != nil {
-				t.Errorf("test failed unexpectedly: %v", err)
-			}
-			compareIngressLists(t, gotIngresses, tc.wantIngresses)
-		})
-	}
-}
-
 func Test_constructProviders(t *testing.T) {
 	supportProviders := []string{"ingress-nginx"}
 	for _, provider := range supportProviders {

pkg/i2gw/provider.go

@@ -42,7 +42,8 @@ type ProviderConstructor func(conf *ProviderConf) Provider
 // ProviderConf contains all the configuration required for every concrete
 // Provider implementation.
 type ProviderConf struct {
-	Client client.Client
+	Client    client.Client
+	Namespace string
 }
 
 // The Provider interface specifies the required functionality which needs to be

pkg/i2gw/providers/ingressnginx/converter.go

@@ -19,40 +19,42 @@ package ingressnginx
 import (
 	"github.com/kubernetes-sigs/ingress2gateway/pkg/i2gw"
 	"github.com/kubernetes-sigs/ingress2gateway/pkg/i2gw/providers/common"
+	networkingv1 "k8s.io/api/networking/v1"
 	"k8s.io/apimachinery/pkg/util/validation/field"
 )
 
 // converter implements the ToGatewayAPI function of i2gw.ResourceConverter interface.
 type converter struct {
-	conf *i2gw.ProviderConf
-
 	featureParsers []i2gw.FeatureParser
 }
 
 // newConverter returns an ingress-nginx converter instance.
-func newConverter(conf *i2gw.ProviderConf) *converter {
+func newConverter() *converter {
 	return &converter{
-		conf: conf,
 		featureParsers: []i2gw.FeatureParser{
 			canaryFeature,
 		},
 	}
 }
 
-// ToGatewayAPI converts the received i2gw.InputResources to i2gw.GatewayResources
-// including the ingress-nginx specific features.
-func (c *converter) ToGatewayAPI(resources i2gw.InputResources) (i2gw.GatewayResources, field.ErrorList) {
+func (c *converter) convert(storage *storage) (i2gw.GatewayResources, field.ErrorList) {
+
+	// TODO(liorliberman) temporary until we decide to change ToGateway and featureParsers to get a map of [types.NamespacedName]*networkingv1.Ingress instead of a list
+	ingressList := []networkingv1.Ingress{}
+	for _, ing := range storage.Ingresses {
+		ingressList = append(ingressList, *ing)
+	}
 
 	// Convert plain ingress resources to gateway resources, ignoring all
 	// provider-specific features.
-	gatewayResources, errs := common.ToGateway(resources.Ingresses, i2gw.ProviderImplementationSpecificOptions{})
+	gatewayResources, errs := common.ToGateway(ingressList, i2gw.ProviderImplementationSpecificOptions{})
 	if len(errs) > 0 {
 		return i2gw.GatewayResources{}, errs
 	}
 
 	for _, parseFeatureFunc := range c.featureParsers {
 		// Apply the feature parsing function to the gateway resources, one by one.
-		parseErrs := parseFeatureFunc(resources, &gatewayResources)
+		parseErrs := parseFeatureFunc(i2gw.InputResources{Ingresses: ingressList}, &gatewayResources)
 		// Append the parsing errors to the error list.
 		errs = append(errs, parseErrs...)
 	}

pkg/i2gw/providers/ingressnginx/converter_test.go

@@ -42,14 +42,14 @@ func Test_ToGateway(t *testing.T) {
 
 	testCases := []struct {
 		name                     string
-		ingresses                []networkingv1.Ingress
+		ingresses                map[types.NamespacedName]*networkingv1.Ingress
 		expectedGatewayResources i2gw.GatewayResources
 		expectedErrors           field.ErrorList
 	}{
 		{
 			name: "canary deployment",
-			ingresses: []networkingv1.Ingress{
-				{
+			ingresses: map[types.NamespacedName]*networkingv1.Ingress{
+				{Namespace: "default", Name: "production"}: {
 					ObjectMeta: metav1.ObjectMeta{Name: "production", Namespace: "default"},
 					Spec: networkingv1.IngressSpec{
 						IngressClassName: ptrTo("ingress-nginx"),
@@ -73,7 +73,7 @@ func Test_ToGateway(t *testing.T) {
 						}},
 					},
 				},
-				{
+				{Namespace: "default", Name: "canary"}: {
 					ObjectMeta: metav1.ObjectMeta{
 						Name:      "canary",
 						Namespace: "default",
@@ -168,8 +168,8 @@ func Test_ToGateway(t *testing.T) {
 		},
 		{
 			name: "ImplementationSpecific HTTPRouteMatching",
-			ingresses: []networkingv1.Ingress{
-				{
+			ingresses: map[types.NamespacedName]*networkingv1.Ingress{
+				{Namespace: "default", Name: "implementation-specific-regex"}: {
 					ObjectMeta: metav1.ObjectMeta{
 						Name:      "implementation-specific-regex",
 						Namespace: "default",
@@ -215,11 +215,21 @@ func Test_ToGateway(t *testing.T) {
 
 			provider := NewProvider(&i2gw.ProviderConf{})
 
-			resources := i2gw.InputResources{
-				Ingresses: tc.ingresses,
-			}
+			nginxProvider := provider.(*Provider)
+			nginxProvider.storage.Ingresses = tc.ingresses
+
+			// TODO(#113) we pass an empty i2gw.InputResources temporarily until we change ToGatewayAPI function on the interface
+			gatewayResources, errs := provider.ToGatewayAPI(i2gw.InputResources{})
 
-			gatewayResources, errs := provider.ToGatewayAPI(resources)
+			if len(errs) != len(tc.expectedErrors) {
+				t.Errorf("Expected %d errors, got %d: %+v", len(tc.expectedErrors), len(errs), errs)
+			} else {
+				for i, e := range errs {
+					if errors.Is(e, tc.expectedErrors[i]) {
+						t.Errorf("Unexpected error message at %d index. Got %s, want: %s", i, e, tc.expectedErrors[i])
+					}
+				}
+			}
 
 			if len(gatewayResources.HTTPRoutes) != len(tc.expectedGatewayResources.HTTPRoutes) {
 				t.Errorf("Expected %d HTTPRoutes, got %d: %+v",
@@ -248,16 +258,6 @@ func Test_ToGateway(t *testing.T) {
 					}
 				}
 			}
-
-			if len(errs) != len(tc.expectedErrors) {
-				t.Errorf("Expected %d errors, got %d: %+v", len(tc.expectedErrors), len(errs), errs)
-			} else {
-				for i, e := range errs {
-					if errors.Is(e, tc.expectedErrors[i]) {
-						t.Errorf("Unexpected error message at %d index. Got %s, want: %s", i, e, tc.expectedErrors[i])
-					}
-				}
-			}
 		})
 	}
 }

pkg/i2gw/providers/ingressnginx/ingressnginx.go

@@ -17,29 +17,59 @@ limitations under the License.
 package ingressnginx
 
 import (
+	"context"
+	"fmt"
+
 	"github.com/kubernetes-sigs/ingress2gateway/pkg/i2gw"
+	"k8s.io/apimachinery/pkg/util/validation/field"
 )
 
 // The Name of the provider.
 const Name = "ingress-nginx"
+const NginxIngressClass = "nginx"
 
 func init() {
 	i2gw.ProviderConstructorByName[Name] = NewProvider
 }
 
 // Provider implements the i2gw.Provider interface.
 type Provider struct {
-	conf *i2gw.ProviderConf
-
-	*resourceReader
-	*converter
+	storage        *storage
+	resourceReader *resourceReader
+	converter      *converter
 }
 
 // NewProvider constructs and returns the ingress-nginx implementation of i2gw.Provider.
 func NewProvider(conf *i2gw.ProviderConf) i2gw.Provider {
 	return &Provider{
-		conf:           conf,
+		storage:        newResourcesStorage(),
 		resourceReader: newResourceReader(conf),
-		converter:      newConverter(conf),
+		converter:      newConverter(),
+	}
+}
+
+// ToGatewayAPI converts the received i2gw.InputResources to i2gw.GatewayResources
+// including the ingress-nginx specific features.
+func (p *Provider) ToGatewayAPI(_ i2gw.InputResources) (i2gw.GatewayResources, field.ErrorList) {
+	return p.converter.convert(p.storage)
+}
+
+func (p *Provider) ReadResourcesFromCluster(ctx context.Context) error {
+	storage, err := p.resourceReader.readResourcesFromCluster(ctx)
+	if err != nil {
+		return fmt.Errorf("failed to read resources from cluster: %w", err)
+	}
+
+	p.storage = storage
+	return nil
+}
+
+func (p *Provider) ReadResourcesFromFile(ctx context.Context, filename string) error {
+	storage, err := p.resourceReader.readResourcesFromFile(ctx, filename)
+	if err != nil {
+		return fmt.Errorf("failed to read resources from file: %w", err)
 	}
+
+	p.storage = storage
+	return nil
 }