Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: fix checkObjectNamespaceLabels func param type #2477

Merged
merged 1 commit into from
Jan 23, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions internal/provider/kubernetes/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -580,11 +580,11 @@
if len(r.namespaceLabels) != 0 {
var rgs []gwapiv1b1.ReferenceGrant
for _, refGrant := range refGrants {
ns := refGrant.GetNamespace()
ok, err := r.checkObjectNamespaceLabels(ns)
refGrant := refGrant
ok, err := r.checkObjectNamespaceLabels(&refGrant)

Check warning on line 584 in internal/provider/kubernetes/controller.go

View check run for this annotation

Codecov / codecov/patch

internal/provider/kubernetes/controller.go#L583-L584

Added lines #L583 - L584 were not covered by tests
if err != nil {
// TODO: should return? or just proceed?
return nil, fmt.Errorf("failed to check namespace labels for ReferenceGrant %s in namespace %s: %w", refGrant.GetName(), ns, err)
return nil, fmt.Errorf("failed to check namespace labels for ReferenceGrant %s in namespace %s: %w", refGrant.GetName(), refGrant.GetNamespace(), err)

Check warning on line 587 in internal/provider/kubernetes/controller.go

View check run for this annotation

Codecov / codecov/patch

internal/provider/kubernetes/controller.go#L587

Added line #L587 was not covered by tests
}
if !ok {
// TODO: should log?
Expand Down Expand Up @@ -624,11 +624,11 @@
if len(r.namespaceLabels) != 0 {
var gtws []gwapiv1.Gateway
for _, gtw := range gateways {
ns := gtw.GetNamespace()
ok, err := r.checkObjectNamespaceLabels(ns)
gtw := gtw
ok, err := r.checkObjectNamespaceLabels(&gtw)
if err != nil {
// TODO: should return? or just proceed?
return fmt.Errorf("failed to check namespace labels for gateway %s in namespace %s: %w", gtw.GetName(), ns, err)
return fmt.Errorf("failed to check namespace labels for gateway %s in namespace %s: %w", gtw.GetName(), gtw.GetNamespace(), err)

Check warning on line 631 in internal/provider/kubernetes/controller.go

View check run for this annotation

Codecov / codecov/patch

internal/provider/kubernetes/controller.go#L631

Added line #L631 was not covered by tests
}

if ok {
Expand Down
6 changes: 3 additions & 3 deletions internal/provider/kubernetes/filters.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,11 @@
if len(r.namespaceLabels) != 0 {
var extRs []unstructured.Unstructured
for _, extR := range uExtResources {
ns := extR.GetNamespace()
ok, err := r.checkObjectNamespaceLabels(ns)
extR := extR
ok, err := r.checkObjectNamespaceLabels(&extR)

Check warning on line 30 in internal/provider/kubernetes/filters.go

View check run for this annotation

Codecov / codecov/patch

internal/provider/kubernetes/filters.go#L29-L30

Added lines #L29 - L30 were not covered by tests
if err != nil {
// TODO: should return? or just proceed?
return nil, fmt.Errorf("failed to check namespace labels for ExtensionRefFilter %s in namespace %s: %w", extR.GetName(), ns, err)
return nil, fmt.Errorf("failed to check namespace labels for ExtensionRefFilter %s in namespace %s: %w", extR.GetName(), extR.GetNamespace(), err)

Check warning on line 33 in internal/provider/kubernetes/filters.go

View check run for this annotation

Codecov / codecov/patch

internal/provider/kubernetes/filters.go#L33

Added line #L33 was not covered by tests
}
if ok {
extRs = append(extRs, extR)
Expand Down
15 changes: 10 additions & 5 deletions internal/provider/kubernetes/predicates.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
corev1 "k8s.io/api/core/v1"
discoveryv1 "k8s.io/api/discovery/v1"
kerrors "k8s.io/apimachinery/pkg/api/errors"
matav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/fields"
"k8s.io/apimachinery/pkg/types"
"sigs.k8s.io/controller-runtime/pkg/client"
Expand Down Expand Up @@ -45,7 +46,7 @@ func (r *gatewayAPIReconciler) hasMatchingController(obj client.Object) bool {
// hasMatchingNamespaceLabels returns true if the namespace of provided object has
// the provided labels or false otherwise.
func (r *gatewayAPIReconciler) hasMatchingNamespaceLabels(obj client.Object) bool {
ok, err := r.checkObjectNamespaceLabels(obj.GetNamespace())
ok, err := r.checkObjectNamespaceLabels(obj)
if err != nil {
r.log.Error(
err, "failed to get Namespace",
Expand All @@ -61,10 +62,14 @@ type NamespaceGetter interface {
}

// checkObjectNamespaceLabels checks if labels of namespace of the object is a subset of namespaceLabels
// TODO: check if param can be an interface, so the caller doesn't need to get the namespace before calling
// this function.
func (r *gatewayAPIReconciler) checkObjectNamespaceLabels(nsString string) (bool, error) {
// TODO: add validation here because some objects don't have namespace
func (r *gatewayAPIReconciler) checkObjectNamespaceLabels(obj matav1.Object) (bool, error) {

var nsString string
// TODO: it requires extra condition validate cluster resources or resources without namespace?
if nsString = obj.GetNamespace(); len(nsString) == 0 {
return false, nil
}

ns := &corev1.Namespace{}
if err := r.client.Get(
context.Background(),
Expand Down
100 changes: 100 additions & 0 deletions internal/provider/kubernetes/predicates_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -495,3 +495,103 @@ func TestValidateDeploymentForReconcile(t *testing.T) {
})
}
}

func TestCheckObjectNamespaceLabels(t *testing.T) {

testCases := []struct {
name string
object client.Object
reconcileLabels []string
ns *corev1.Namespace
expect bool
}{
{
name: "matching labels of namespace of the object is a subset of namespaceLabels",
object: test.GetHTTPRoute(
types.NamespacedName{
Name: "foo-route",
Namespace: "foo",
},
"eg",
types.NamespacedName{
Name: "foo-svc",
Namespace: "foo",
},
8080,
),
ns: &corev1.Namespace{
ObjectMeta: v1.ObjectMeta{
Name: "foo",
Labels: map[string]string{
"label-1": "",
},
},
},
reconcileLabels: []string{"label-1"},
expect: true,
},
{
name: "non-matching labels of namespace of the object is a subset of namespaceLabels",
object: test.GetHTTPRoute(
types.NamespacedName{
Name: "bar-route",
Namespace: "bar",
},
"eg",
types.NamespacedName{
Name: "bar-svc",
Namespace: "bar",
},
8080,
),
ns: &corev1.Namespace{
ObjectMeta: v1.ObjectMeta{
Name: "bar",
Labels: map[string]string{
"label-2": "",
},
},
},
reconcileLabels: []string{"label-1"},
expect: false,
},
{
name: "non-matching labels of namespace of the cluster-level object is a subset of namespaceLabels",
object: &corev1.Namespace{
ObjectMeta: v1.ObjectMeta{
Name: "foo-1",
Labels: map[string]string{
"label-1": "",
},
},
},
ns: &corev1.Namespace{
ObjectMeta: v1.ObjectMeta{
Name: "bar-1",
Labels: map[string]string{
"label-1": "",
},
},
},
reconcileLabels: []string{"label-1"},
expect: false,
},
}

// Create the reconciler.
logger := logging.DefaultLogger(v1alpha1.LogLevelInfo)

r := gatewayAPIReconciler{
classController: v1alpha1.GatewayControllerName,
log: logger,
}

for _, tc := range testCases {
tc := tc
r.client = fakeclient.NewClientBuilder().WithObjects(tc.ns).Build()
r.namespaceLabels = tc.reconcileLabels
ok, err := r.checkObjectNamespaceLabels(tc.object)
require.NoError(t, err)
require.Equal(t, tc.expect, ok)
}
}
30 changes: 15 additions & 15 deletions internal/provider/kubernetes/routes.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,11 +37,11 @@
if len(r.namespaceLabels) != 0 {
var rts []gwapiv1a2.TLSRoute
for _, rt := range tlsRoutes {
ns := rt.GetNamespace()
ok, err := r.checkObjectNamespaceLabels(ns)
rt := rt
ok, err := r.checkObjectNamespaceLabels(&rt)
if err != nil {
// TODO: should return? or just proceed?
return fmt.Errorf("failed to check namespace labels for TLSRoute %s in namespace %s: %w", rt.GetName(), ns, err)
return fmt.Errorf("failed to check namespace labels for TLSRoute %s in namespace %s: %w", rt.GetName(), rt.GetNamespace(), err)

Check warning on line 44 in internal/provider/kubernetes/routes.go

View check run for this annotation

Codecov / codecov/patch

internal/provider/kubernetes/routes.go#L44

Added line #L44 was not covered by tests
}

if ok {
Expand Down Expand Up @@ -118,11 +118,11 @@
if len(r.namespaceLabels) != 0 {
var grs []gwapiv1a2.GRPCRoute
for _, gr := range grpcRoutes {
ns := gr.GetNamespace()
ok, err := r.checkObjectNamespaceLabels(ns)
gr := gr
ok, err := r.checkObjectNamespaceLabels(&gr)
if err != nil {
// TODO: should return? or just proceed?
return fmt.Errorf("failed to check namespace labels for GRPCRoute %s in namespace %s: %w", gr.GetName(), ns, err)
return fmt.Errorf("failed to check namespace labels for GRPCRoute %s in namespace %s: %w", gr.GetName(), gr.GetNamespace(), err)

Check warning on line 125 in internal/provider/kubernetes/routes.go

View check run for this annotation

Codecov / codecov/patch

internal/provider/kubernetes/routes.go#L125

Added line #L125 was not covered by tests
}
if ok {
grs = append(grs, gr)
Expand Down Expand Up @@ -247,11 +247,11 @@
if len(r.namespaceLabels) != 0 {
var hrs []gwapiv1.HTTPRoute
for _, hr := range httpRoutes {
ns := hr.GetNamespace()
ok, err := r.checkObjectNamespaceLabels(ns)
hr := hr
ok, err := r.checkObjectNamespaceLabels(&hr)
if err != nil {
// TODO: should return? or just proceed?
return fmt.Errorf("failed to check namespace labels for HTTPRoute %s in namespace %s: %w", hr.GetName(), ns, err)
return fmt.Errorf("failed to check namespace labels for HTTPRoute %s in namespace %s: %w", hr.GetName(), hr.GetNamespace(), err)

Check warning on line 254 in internal/provider/kubernetes/routes.go

View check run for this annotation

Codecov / codecov/patch

internal/provider/kubernetes/routes.go#L254

Added line #L254 was not covered by tests
}

if ok {
Expand Down Expand Up @@ -420,11 +420,11 @@
if len(r.namespaceLabels) != 0 {
var trs []gwapiv1a2.TCPRoute
for _, tr := range tcpRoutes {
ns := tr.GetNamespace()
ok, err := r.checkObjectNamespaceLabels(ns)
tr := tr
ok, err := r.checkObjectNamespaceLabels(&tr)
if err != nil {
// TODO: should return? or just proceed?
return fmt.Errorf("failed to check namespace labels for TCPRoute %s in namespace %s: %w", tr.GetName(), ns, err)
return fmt.Errorf("failed to check namespace labels for TCPRoute %s in namespace %s: %w", tr.GetName(), tr.GetNamespace(), err)

Check warning on line 427 in internal/provider/kubernetes/routes.go

View check run for this annotation

Codecov / codecov/patch

internal/provider/kubernetes/routes.go#L427

Added line #L427 was not covered by tests
}

if ok {
Expand Down Expand Up @@ -501,11 +501,11 @@
if len(r.namespaceLabels) != 0 {
var urs []gwapiv1a2.UDPRoute
for _, ur := range udpRoutes {
ns := ur.GetNamespace()
ok, err := r.checkObjectNamespaceLabels(ns)
ur := ur
ok, err := r.checkObjectNamespaceLabels(&ur)
if err != nil {
// TODO: should return? or just proceed?
return fmt.Errorf("failed to check namespace labels for UDPRoute %s in namespace %s: %w", ur.GetName(), ns, err)
return fmt.Errorf("failed to check namespace labels for UDPRoute %s in namespace %s: %w", ur.GetName(), ur.GetNamespace(), err)

Check warning on line 508 in internal/provider/kubernetes/routes.go

View check run for this annotation

Codecov / codecov/patch

internal/provider/kubernetes/routes.go#L508

Added line #L508 was not covered by tests
}

if ok {
Expand Down