diff --git a/internal/configs/configurator_test.go b/internal/configs/configurator_test.go index 869023cc5e..e7839173a3 100644 --- a/internal/configs/configurator_test.go +++ b/internal/configs/configurator_test.go @@ -8,6 +8,7 @@ import ( "github.com/prometheus/client_golang/prometheus" networking "k8s.io/api/networking/v1beta1" meta_v1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "github.com/nginxinc/kubernetes-ingress/internal/configs/version1" "github.com/nginxinc/kubernetes-ingress/internal/configs/version2" @@ -1068,3 +1069,199 @@ func TestUpdateTransportServerMetricsLabels(t *testing.T) { t.Errorf("deleteTransportServerMetricsLabels() updated labels to \n%+v but expected \n%+v", cnf.labelUpdater, expectedLabelUpdater) } } + +func TestUpdateApResources(t *testing.T) { + appProtectPolicy := &unstructured.Unstructured{ + Object: map[string]interface{}{ + "metadata": map[string]interface{}{ + "namespace": "test-ns", + "name": "test-name", + }, + }, + } + appProtectLogConf := &unstructured.Unstructured{ + Object: map[string]interface{}{ + "metadata": map[string]interface{}{ + "namespace": "test-ns", + "name": "test-name", + }, + }, + } + appProtectLogDst := "test-dst" + + tests := []struct { + ingEx *IngressEx + expected map[string]string + msg string + }{ + { + ingEx: &IngressEx{ + Ingress: &networking.Ingress{ + ObjectMeta: meta_v1.ObjectMeta{}, + }, + }, + expected: map[string]string{}, + msg: "no app protect resources", + }, + { + ingEx: &IngressEx{ + Ingress: &networking.Ingress{ + ObjectMeta: meta_v1.ObjectMeta{}, + }, + AppProtectPolicy: appProtectPolicy, + }, + expected: map[string]string{ + "policy": "/etc/nginx/waf/nac-policies/test-ns_test-name", + }, + msg: "app protect policy", + }, + { + ingEx: &IngressEx{ + Ingress: &networking.Ingress{ + ObjectMeta: meta_v1.ObjectMeta{}, + }, + AppProtectLogConf: appProtectLogConf, + AppProtectLogDst: appProtectLogDst, + }, + expected: map[string]string{ + "logconf": "/etc/nginx/waf/nac-logconfs/test-ns_test-name test-dst", + }, + msg: "app protect log conf", + }, + { + ingEx: &IngressEx{ + Ingress: &networking.Ingress{ + ObjectMeta: meta_v1.ObjectMeta{}, + }, + AppProtectPolicy: appProtectPolicy, + AppProtectLogConf: appProtectLogConf, + AppProtectLogDst: appProtectLogDst, + }, + expected: map[string]string{ + "policy": "/etc/nginx/waf/nac-policies/test-ns_test-name", + "logconf": "/etc/nginx/waf/nac-logconfs/test-ns_test-name test-dst", + }, + msg: "app protect policy and log conf", + }, + } + + conf, err := createTestConfigurator() + if err != nil { + t.Errorf("Failed to create a test configurator: %v", err) + } + + for _, test := range tests { + result := conf.updateApResources(test.ingEx) + if !reflect.DeepEqual(result, test.expected) { + t.Errorf("updateApResources() returned \n%v but exexpected\n%v for the case of %s", result, test.expected, test.msg) + } + } +} + +func TestUpdateApResourcesForVs(t *testing.T) { + apPolRefs := map[string]*unstructured.Unstructured{ + "test-ns-1/test-name-1": { + Object: map[string]interface{}{ + "metadata": map[string]interface{}{ + "namespace": "test-ns-1", + "name": "test-name-1", + }, + }, + }, + "test-ns-2/test-name-2": { + Object: map[string]interface{}{ + "metadata": map[string]interface{}{ + "namespace": "test-ns-2", + "name": "test-name-2", + }, + }, + }, + } + logConfRefs := map[string]*unstructured.Unstructured{ + "test-ns-1/test-name-1": { + Object: map[string]interface{}{ + "metadata": map[string]interface{}{ + "namespace": "test-ns-1", + "name": "test-name-1", + }, + }, + }, + "test-ns-2/test-name-2": { + Object: map[string]interface{}{ + "metadata": map[string]interface{}{ + "namespace": "test-ns-2", + "name": "test-name-2", + }, + }, + }, + } + + tests := []struct { + vsEx *VirtualServerEx + expected map[string]string + msg string + }{ + { + vsEx: &VirtualServerEx{ + VirtualServer: &conf_v1.VirtualServer{ + ObjectMeta: meta_v1.ObjectMeta{}, + }, + }, + expected: map[string]string{}, + msg: "no app protect resources", + }, + { + vsEx: &VirtualServerEx{ + VirtualServer: &conf_v1.VirtualServer{ + ObjectMeta: meta_v1.ObjectMeta{}, + }, + ApPolRefs: apPolRefs, + }, + expected: map[string]string{ + "test-ns-1/test-name-1": "/etc/nginx/waf/nac-policies/test-ns-1_test-name-1", + "test-ns-2/test-name-2": "/etc/nginx/waf/nac-policies/test-ns-2_test-name-2", + }, + msg: "app protect policies", + }, + { + vsEx: &VirtualServerEx{ + VirtualServer: &conf_v1.VirtualServer{ + ObjectMeta: meta_v1.ObjectMeta{}, + }, + LogConfRefs: logConfRefs, + }, + expected: map[string]string{ + "test-ns-1/test-name-1": "/etc/nginx/waf/nac-logconfs/test-ns-1_test-name-1", + "test-ns-2/test-name-2": "/etc/nginx/waf/nac-logconfs/test-ns-2_test-name-2", + }, + msg: "app protect log confs", + }, + { + vsEx: &VirtualServerEx{ + VirtualServer: &conf_v1.VirtualServer{ + ObjectMeta: meta_v1.ObjectMeta{}, + }, + ApPolRefs: apPolRefs, + LogConfRefs: logConfRefs, + }, + expected: map[string]string{ + // this is a bug - the result needs to include both policies and log confs + "test-ns-1/test-name-1": "/etc/nginx/waf/nac-logconfs/test-ns-1_test-name-1", + "test-ns-2/test-name-2": "/etc/nginx/waf/nac-logconfs/test-ns-2_test-name-2", + }, + msg: "app protect policies and log confs", + }, + } + + conf, err := createTestConfigurator() + if err != nil { + t.Errorf("Failed to create a test configurator: %v", err) + } + + for _, test := range tests { + result := conf.updateApResourcesForVs(test.vsEx) + if !reflect.DeepEqual(result, test.expected) { + t.Errorf("updateApResourcesForVs() returned \n%v but exexpected\n%v for the case of %s", result, test.expected, test.msg) + } + } +} diff --git a/internal/configs/ingress_test.go b/internal/configs/ingress_test.go index d12071f375..52640b66a1 100644 --- a/internal/configs/ingress_test.go +++ b/internal/configs/ingress_test.go @@ -11,6 +11,7 @@ import ( v1 "k8s.io/api/core/v1" networking "k8s.io/api/networking/v1beta1" meta_v1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/util/intstr" "github.com/nginxinc/kubernetes-ingress/internal/configs/version1" @@ -20,10 +21,11 @@ func TestGenerateNginxCfg(t *testing.T) { cafeIngressEx := createCafeIngressEx() configParams := NewDefaultConfigParams() - expected := createExpectedConfigForCafeIngressEx() + isPlus := false + expected := createExpectedConfigForCafeIngressEx(isPlus) apRes := make(map[string]string) - result, warnings := generateNginxCfg(&cafeIngressEx, apRes, false, configParams, false, false, &StaticConfigParams{}, false) + result, warnings := generateNginxCfg(&cafeIngressEx, apRes, false, configParams, isPlus, false, &StaticConfigParams{}, false) if diff := cmp.Diff(expected, result); diff != "" { t.Errorf("generateNginxCfg() returned unexpected result (-want +got):\n%s", diff) @@ -48,7 +50,9 @@ func TestGenerateNginxCfgForJWT(t *testing.T) { configParams := NewDefaultConfigParams() - expected := createExpectedConfigForCafeIngressEx() + isPlus := true + + expected := createExpectedConfigForCafeIngressEx(isPlus) expected.Servers[0].JWTAuth = &version1.JWTAuth{ Key: "/etc/nginx/secrets/default-cafe-jwk", Realm: "Cafe App", @@ -63,7 +67,7 @@ func TestGenerateNginxCfgForJWT(t *testing.T) { } apRes := make(map[string]string) - result, warnings := generateNginxCfg(&cafeIngressEx, apRes, false, configParams, true, false, &StaticConfigParams{}, false) + result, warnings := generateNginxCfg(&cafeIngressEx, apRes, false, configParams, isPlus, false, &StaticConfigParams{}, false) if !reflect.DeepEqual(result.Servers[0].JWTAuth, expected.Servers[0].JWTAuth) { t.Errorf("generateNginxCfg returned \n%v, but expected \n%v", result.Servers[0].JWTAuth, expected.Servers[0].JWTAuth) @@ -173,7 +177,7 @@ func TestGenerateIngressPath(t *testing.T) { } } -func createExpectedConfigForCafeIngressEx() version1.IngressNginxConfig { +func createExpectedConfigForCafeIngressEx(isPlus bool) version1.IngressNginxConfig { coffeeUpstream := version1.Upstream{ Name: "default-cafe-ingress-cafe.example.com-coffee-svc-80", LBMethod: "random two least_conn", @@ -188,6 +192,15 @@ func createExpectedConfigForCafeIngressEx() version1.IngressNginxConfig { }, }, } + if isPlus { + coffeeUpstream.UpstreamLabels = version1.UpstreamLabels{ + Service: "coffee-svc", + ResourceType: "ingress", + ResourceName: "cafe-ingress", + ResourceNamespace: "default", + } + } + teaUpstream := version1.Upstream{ Name: "default-cafe-ingress-cafe.example.com-tea-svc-80", LBMethod: "random two least_conn", @@ -202,6 +215,15 @@ func createExpectedConfigForCafeIngressEx() version1.IngressNginxConfig { }, }, } + if isPlus { + teaUpstream.UpstreamLabels = version1.UpstreamLabels{ + Service: "tea-svc", + ResourceType: "ingress", + ResourceName: "cafe-ingress", + ResourceNamespace: "default", + } + } + expected := version1.IngressNginxConfig{ Upstreams: []version1.Upstream{ coffeeUpstream, @@ -324,12 +346,14 @@ func createCafeIngressEx() IngressEx { func TestGenerateNginxCfgForMergeableIngresses(t *testing.T) { mergeableIngresses := createMergeableCafeIngress() - expected := createExpectedConfigForMergeableCafeIngress() + + isPlus := false + expected := createExpectedConfigForMergeableCafeIngress(isPlus) configParams := NewDefaultConfigParams() masterApRes := make(map[string]string) - result, warnings := generateNginxCfgForMergeableIngresses(mergeableIngresses, masterApRes, configParams, false, false, &StaticConfigParams{}, false) + result, warnings := generateNginxCfgForMergeableIngresses(mergeableIngresses, masterApRes, configParams, isPlus, false, &StaticConfigParams{}, false) if diff := cmp.Diff(expected, result); diff != "" { t.Errorf("generateNginxCfgForMergeableIngresses() returned unexpected result (-want +got):\n%s", diff) @@ -388,7 +412,9 @@ func TestGenerateNginxCfgForMergeableIngressesForJWT(t *testing.T) { Path: "/etc/nginx/secrets/default-coffee-jwk", } - expected := createExpectedConfigForMergeableCafeIngress() + isPlus := true + + expected := createExpectedConfigForMergeableCafeIngress(isPlus) expected.Servers[0].JWTAuth = &version1.JWTAuth{ Key: "/etc/nginx/secrets/default-cafe-jwk", Realm: "Cafe", @@ -415,7 +441,6 @@ func TestGenerateNginxCfgForMergeableIngressesForJWT(t *testing.T) { minionJwtKeyFileNames := make(map[string]string) minionJwtKeyFileNames[objectMetaToFileName(&mergeableIngresses.Minions[0].Ingress.ObjectMeta)] = "/etc/nginx/secrets/default-coffee-jwk" configParams := NewDefaultConfigParams() - isPlus := true masterApRes := make(map[string]string) result, warnings := generateNginxCfgForMergeableIngresses(mergeableIngresses, masterApRes, configParams, isPlus, false, &StaticConfigParams{}, false) @@ -578,7 +603,7 @@ func createMergeableCafeIngress() *MergeableIngresses { return mergeableIngresses } -func createExpectedConfigForMergeableCafeIngress() version1.IngressNginxConfig { +func createExpectedConfigForMergeableCafeIngress(isPlus bool) version1.IngressNginxConfig { coffeeUpstream := version1.Upstream{ Name: "default-cafe-ingress-coffee-minion-cafe.example.com-coffee-svc-80", LBMethod: "random two least_conn", @@ -593,6 +618,15 @@ func createExpectedConfigForMergeableCafeIngress() version1.IngressNginxConfig { }, }, } + if isPlus { + coffeeUpstream.UpstreamLabels = version1.UpstreamLabels{ + Service: "coffee-svc", + ResourceType: "ingress", + ResourceName: "cafe-ingress-coffee-minion", + ResourceNamespace: "default", + } + } + teaUpstream := version1.Upstream{ Name: "default-cafe-ingress-tea-minion-cafe.example.com-tea-svc-80", LBMethod: "random two least_conn", @@ -607,6 +641,15 @@ func createExpectedConfigForMergeableCafeIngress() version1.IngressNginxConfig { }, }, } + if isPlus { + teaUpstream.UpstreamLabels = version1.UpstreamLabels{ + Service: "tea-svc", + ResourceType: "ingress", + ResourceName: "cafe-ingress-tea-minion", + ResourceNamespace: "default", + } + } + expected := version1.IngressNginxConfig{ Upstreams: []version1.Upstream{ coffeeUpstream, @@ -786,14 +829,16 @@ func TestGenerateNginxCfgForSpiffe(t *testing.T) { cafeIngressEx := createCafeIngressEx() configParams := NewDefaultConfigParams() - expected := createExpectedConfigForCafeIngressEx() + isPlus := false + + expected := createExpectedConfigForCafeIngressEx(isPlus) expected.SpiffeClientCerts = true for i := range expected.Servers[0].Locations { expected.Servers[0].Locations[i].SSL = true } apResources := make(map[string]string) - result, warnings := generateNginxCfg(&cafeIngressEx, apResources, false, configParams, false, false, + result, warnings := generateNginxCfg(&cafeIngressEx, apResources, false, configParams, isPlus, false, &StaticConfigParams{NginxServiceMesh: true}, false) if diff := cmp.Diff(expected, result); diff != "" { @@ -810,12 +855,14 @@ func TestGenerateNginxCfgForInternalRoute(t *testing.T) { cafeIngressEx.Ingress.Annotations[internalRouteAnnotation] = "true" configParams := NewDefaultConfigParams() - expected := createExpectedConfigForCafeIngressEx() + isPlus := false + + expected := createExpectedConfigForCafeIngressEx(isPlus) expected.Servers[0].SpiffeCerts = true expected.Ingress.Annotations[internalRouteAnnotation] = "true" apResources := make(map[string]string) - result, warnings := generateNginxCfg(&cafeIngressEx, apResources, false, configParams, false, false, + result, warnings := generateNginxCfg(&cafeIngressEx, apResources, false, configParams, isPlus, false, &StaticConfigParams{NginxServiceMesh: true, EnableInternalRoutes: true}, false) if diff := cmp.Diff(expected, result); diff != "" { @@ -1251,3 +1298,99 @@ func TestGenerateJWTConfig(t *testing.T) { } } } + +func TestGenerateNginxCfgForAppProtect(t *testing.T) { + cafeIngressEx := createCafeIngressEx() + cafeIngressEx.Ingress.Annotations["appprotect.f5.com/app-protect-enable"] = "True" + cafeIngressEx.Ingress.Annotations["appprotect.f5.com/app-protect-security-log-enable"] = "True" + cafeIngressEx.AppProtectPolicy = &unstructured.Unstructured{ + Object: map[string]interface{}{ + "metadata": map[string]interface{}{ + "namespace": "default", + "name": "dataguard-alarm", + }, + }, + } + cafeIngressEx.AppProtectLogConf = &unstructured.Unstructured{ + Object: map[string]interface{}{ + "metadata": map[string]interface{}{ + "namespace": "default", + "name": "logconf", + }, + }, + } + + configParams := NewDefaultConfigParams() + apRes := map[string]string{ + appProtectPolicyKey: "/etc/nginx/waf/nac-policies/default_dataguard-alarm", + appProtectLogConfKey: "/etc/nginx/waf/nac-logconfs/default_logconf syslog:server=127.0.0.1:514", + } + staticCfgParams := &StaticConfigParams{ + MainAppProtectLoadModule: true, + } + + isPlus := true + + expected := createExpectedConfigForCafeIngressEx(isPlus) + expected.Servers[0].AppProtectEnable = "on" + expected.Servers[0].AppProtectPolicy = "/etc/nginx/waf/nac-policies/default_dataguard-alarm" + expected.Servers[0].AppProtectLogConf = "/etc/nginx/waf/nac-logconfs/default_logconf syslog:server=127.0.0.1:514" + expected.Servers[0].AppProtectLogEnable = "on" + expected.Ingress.Annotations = cafeIngressEx.Ingress.Annotations + + result, warnings := generateNginxCfg(&cafeIngressEx, apRes, false, configParams, isPlus, false, staticCfgParams, false) + if diff := cmp.Diff(expected, result); diff != "" { + t.Errorf("generateNginxCfg() returned unexpected result (-want +got):\n%s", diff) + } + if len(warnings) != 0 { + t.Errorf("generateNginxCfg() returned warnings: %v", warnings) + } +} + +func TestGenerateNginxCfgForMergeableIngressesForAppProtect(t *testing.T) { + mergeableIngresses := createMergeableCafeIngress() + mergeableIngresses.Master.Ingress.Annotations["appprotect.f5.com/app-protect-enable"] = "True" + mergeableIngresses.Master.Ingress.Annotations["appprotect.f5.com/app-protect-security-log-enable"] = "True" + mergeableIngresses.Master.AppProtectPolicy = &unstructured.Unstructured{ + Object: map[string]interface{}{ + "metadata": map[string]interface{}{ + "namespace": "default", + "name": "dataguard-alarm", + }, + }, + } + mergeableIngresses.Master.AppProtectLogConf = &unstructured.Unstructured{ + Object: map[string]interface{}{ + "metadata": map[string]interface{}{ + "namespace": "default", + "name": "logconf", + }, + }, + } + + configParams := NewDefaultConfigParams() + apRes := map[string]string{ + appProtectPolicyKey: "/etc/nginx/waf/nac-policies/default_dataguard-alarm", + appProtectLogConfKey: "/etc/nginx/waf/nac-logconfs/default_logconf syslog:server=127.0.0.1:514", + } + staticCfgParams := &StaticConfigParams{ + MainAppProtectLoadModule: true, + } + + isPlus := true + + expected := createExpectedConfigForMergeableCafeIngress(isPlus) + expected.Servers[0].AppProtectEnable = "on" + expected.Servers[0].AppProtectPolicy = "/etc/nginx/waf/nac-policies/default_dataguard-alarm" + expected.Servers[0].AppProtectLogConf = "/etc/nginx/waf/nac-logconfs/default_logconf syslog:server=127.0.0.1:514" + expected.Servers[0].AppProtectLogEnable = "on" + expected.Ingress.Annotations = mergeableIngresses.Master.Ingress.Annotations + + result, warnings := generateNginxCfgForMergeableIngresses(mergeableIngresses, apRes, configParams, isPlus, false, staticCfgParams, false) + if diff := cmp.Diff(expected, result); diff != "" { + t.Errorf("generateNginxCfgForMergeableIngresses() returned unexpected result (-want +got):\n%s", diff) + } + if len(warnings) != 0 { + t.Errorf("generateNginxCfgForMergeableIngresses() returned warnings: %v", warnings) + } +} diff --git a/internal/k8s/appprotect/app_protect_configuration.go b/internal/k8s/appprotect/app_protect_configuration.go index e93508d727..3efcf6a395 100644 --- a/internal/k8s/appprotect/app_protect_configuration.go +++ b/internal/k8s/appprotect/app_protect_configuration.go @@ -201,7 +201,7 @@ func (s appProtectUserSigSlice) Swap(i, j int) { } func createAppProtectPolicyEx(policyObj *unstructured.Unstructured) (*PolicyEx, error) { - err := ValidateAppProtectPolicy(policyObj) + err := validateAppProtectPolicy(policyObj) if err != nil { errMsg := fmt.Sprintf("Error validating policy %s: %v", policyObj.GetName(), err) return &PolicyEx{Obj: policyObj, IsValid: false, ErrorMsg: failedValidationErrorMsg}, fmt.Errorf(errMsg) @@ -255,7 +255,7 @@ func buildRevTimes(requirement map[string]interface{}) (RevTimes, error) { } func createAppProtectLogConfEx(logConfObj *unstructured.Unstructured) (*LogConfEx, error) { - err := ValidateAppProtectLogConf(logConfObj) + err := validateAppProtectLogConf(logConfObj) if err != nil { return &LogConfEx{ Obj: logConfObj, diff --git a/internal/k8s/appprotect/app_protect_configuration_test.go b/internal/k8s/appprotect/app_protect_configuration_test.go index 3c7082e961..8a3f5909b6 100644 --- a/internal/k8s/appprotect/app_protect_configuration_test.go +++ b/internal/k8s/appprotect/app_protect_configuration_test.go @@ -749,11 +749,21 @@ func TestAddOrUpdateUserSig(t *testing.T) { }, }, } + testUserSig1Invalid := &unstructured.Unstructured{ + Object: map[string]interface{}{ + "metadata": map[string]interface{}{ + "namespace": "testing", + "name": "test1", + "uid": "1", + "creationTimestamp": "2020-01-23T18:32:02Z", + }, + }, + } testUserSig3 := &unstructured.Unstructured{ Object: map[string]interface{}{ "metadata": map[string]interface{}{ "namespace": "testing", - "name": "test2", + "name": "test3", "uid": "5", "creationTimestamp": "2020-01-23T18:32:02Z", }, @@ -766,6 +776,7 @@ func TestAddOrUpdateUserSig(t *testing.T) { }, }, } + appProtectConfiguration := newConfigurationImpl() appProtectConfiguration.UserSigs["testing/test1"] = &UserSigEx{ Obj: testUserSig1, @@ -834,6 +845,22 @@ func TestAddOrUpdateUserSig(t *testing.T) { }, msg: "Duplicate tags", }, + { + usersig: testUserSig1Invalid, + expectedUserSigChange: UserSigChange{ + UserSigs: []*unstructured.Unstructured{ + testUserSigDupTag, + }, + }, + expectedProblems: []Problem{ + { + Object: testUserSig1Invalid, + Message: "Validation Failed", + Reason: "Rejected", + }, + }, + msg: "UserSig becomes valid after previous tag holder became invalid", + }, { usersig: testUserSig3, expectedUserSigChange: UserSigChange{ @@ -843,11 +870,11 @@ func TestAddOrUpdateUserSig(t *testing.T) { }, }, UserSigs: []*unstructured.Unstructured{ - testUserSig1, + testUserSigDupTag, testUserSig3, }, }, - msg: "Policy gets set to valid", + msg: "Policy becomes valid after a UserSig with the right tag was added", }, } diff --git a/internal/k8s/appprotect/app_protect_resources.go b/internal/k8s/appprotect/app_protect_resources.go index 93a45a7380..d885604cd8 100644 --- a/internal/k8s/appprotect/app_protect_resources.go +++ b/internal/k8s/appprotect/app_protect_resources.go @@ -23,9 +23,9 @@ var appProtectUserSigRequiredSlices = [][]string{ {"spec", "signatures"}, } -func validateRequiredFields(policy *unstructured.Unstructured, fieldsList [][]string) error { +func validateRequiredFields(obj *unstructured.Unstructured, fieldsList [][]string) error { for _, fields := range fieldsList { - field, found, err := unstructured.NestedMap(policy.Object, fields...) + field, found, err := unstructured.NestedMap(obj.Object, fields...) if err != nil { return fmt.Errorf("Error checking for required field %v: %v", field, err) } @@ -36,9 +36,9 @@ func validateRequiredFields(policy *unstructured.Unstructured, fieldsList [][]st return nil } -func validateRequiredSlices(policy *unstructured.Unstructured, fieldsList [][]string) error { +func validateRequiredSlices(obj *unstructured.Unstructured, fieldsList [][]string) error { for _, fields := range fieldsList { - field, found, err := unstructured.NestedSlice(policy.Object, fields...) + field, found, err := unstructured.NestedSlice(obj.Object, fields...) if err != nil { return fmt.Errorf("Error checking for required field %v: %v", field, err) } @@ -49,21 +49,8 @@ func validateRequiredSlices(policy *unstructured.Unstructured, fieldsList [][]st return nil } -func validateRequiredStrings(policy *unstructured.Unstructured, fieldsList [][]string) error { - for _, fields := range fieldsList { - field, found, err := unstructured.NestedString(policy.Object, fields...) - if err != nil { - return fmt.Errorf("Error checking for required field %v: %v", field, err) - } - if !found { - return fmt.Errorf("Required field %v not found", field) - } - } - return nil -} - -// ValidateAppProtectPolicy validates Policy resource -func ValidateAppProtectPolicy(policy *unstructured.Unstructured) error { +// validateAppProtectPolicy validates Policy resource +func validateAppProtectPolicy(policy *unstructured.Unstructured) error { polName := policy.GetName() err := validateRequiredFields(policy, appProtectPolicyRequiredFields) @@ -74,8 +61,8 @@ func ValidateAppProtectPolicy(policy *unstructured.Unstructured) error { return nil } -// ValidateAppProtectLogConf validates LogConfiguration resource -func ValidateAppProtectLogConf(logConf *unstructured.Unstructured) error { +// validateAppProtectLogConf validates LogConfiguration resource +func validateAppProtectLogConf(logConf *unstructured.Unstructured) error { lcName := logConf.GetName() err := validateRequiredFields(logConf, appProtectLogConfRequiredFields) if err != nil { diff --git a/internal/k8s/appprotect/app_protect_resources_test.go b/internal/k8s/appprotect/app_protect_resources_test.go index 0f058121bf..465760400d 100644 --- a/internal/k8s/appprotect/app_protect_resources_test.go +++ b/internal/k8s/appprotect/app_protect_resources_test.go @@ -3,8 +3,315 @@ package appprotect import ( "strings" "testing" + + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" ) +func TestValidateRequiredFields(t *testing.T) { + tests := []struct { + obj *unstructured.Unstructured + fieldsList [][]string + expectErr bool + msg string + }{ + { + obj: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "a": map[string]interface{}{}, + "b": map[string]interface{}{}, + }, + }, + fieldsList: [][]string{{"a"}, {"b"}}, + expectErr: false, + msg: "valid object with 2 fields", + }, + { + obj: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "a": map[string]interface{}{}, + }, + }, + fieldsList: [][]string{{"a"}, {"b"}}, + expectErr: true, + msg: "invalid object with a missing field", + }, + { + obj: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "a": map[string]interface{}{}, + "x": map[string]interface{}{}, + }, + }, + fieldsList: [][]string{{"a"}, {"b"}}, + expectErr: true, + msg: "invalid object with a wrong field", + }, + { + obj: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "a": map[string]interface{}{ + "b": map[string]interface{}{}, + }, + }, + }, + fieldsList: [][]string{{"a", "b"}}, + expectErr: false, + msg: "valid object with nested field", + }, + { + obj: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "a": map[string]interface{}{ + "x": map[string]interface{}{}, + }, + }, + }, + fieldsList: [][]string{{"a", "b"}}, + expectErr: true, + msg: "invalid object with a wrong nested field", + }, + { + obj: &unstructured.Unstructured{ + Object: map[string]interface{}{}, + }, + fieldsList: nil, + expectErr: false, + msg: "valid object with no validation", + }, + { + obj: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "a": "wrong-type", // must be map[string]interface{} + }, + }, + fieldsList: [][]string{{"a"}}, + expectErr: true, + msg: "invalid object with a field of wrong type", + }, + } + + for _, test := range tests { + err := validateRequiredFields(test.obj, test.fieldsList) + if test.expectErr && err == nil { + t.Errorf("validateRequiredFields() returned no error for the case of %s", test.msg) + } + if !test.expectErr && err != nil { + t.Errorf("validateRequiredFields() returned unexpected error %v for the case of %s", err, test.msg) + } + } +} + +func TestValidateRequiredSlices(t *testing.T) { + tests := []struct { + obj *unstructured.Unstructured + fieldsList [][]string + expectErr bool + msg string + }{ + { + obj: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "a": []interface{}{}, + "b": []interface{}{}, + }, + }, + fieldsList: [][]string{{"a"}, {"b"}}, + expectErr: false, + msg: "valid object with 2 fields", + }, + { + obj: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "a": []interface{}{}, + }, + }, + fieldsList: [][]string{{"a"}, {"b"}}, + expectErr: true, + msg: "invalid object with a field", + }, + { + obj: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "a": []interface{}{}, + "x": []interface{}{}, + }, + }, + fieldsList: [][]string{{"a"}, {"b"}}, + expectErr: true, + msg: "invalid object with a wrong field", + }, + { + obj: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "a": map[string]interface{}{ + "b": []interface{}{}, + }, + }, + }, + fieldsList: [][]string{{"a", "b"}}, + expectErr: false, + msg: "valid object with nested field", + }, + { + obj: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "a": map[string]interface{}{ + "x": []interface{}{}, + }, + }, + }, + fieldsList: [][]string{{"a", "b"}}, + expectErr: true, + msg: "invalid object with a wrong nested field", + }, + { + obj: &unstructured.Unstructured{ + Object: map[string]interface{}{}, + }, + fieldsList: nil, + expectErr: false, + msg: "valid object with no validation", + }, + { + obj: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "a": "wrong-type", // must be [string]interface{} + }, + }, + fieldsList: [][]string{{"a"}}, + expectErr: true, + msg: "invalid object with a field of wrong type", + }, + } + + for _, test := range tests { + err := validateRequiredSlices(test.obj, test.fieldsList) + if test.expectErr && err == nil { + t.Errorf("validateRequiredSlices() returned no error for the case of %s", test.msg) + } + if !test.expectErr && err != nil { + t.Errorf("validateRequiredSlices() returned unexpected error %v for the case of %s", err, test.msg) + } + } +} + +func TestValidateAppProtectPolicy(t *testing.T) { + tests := []struct { + policy *unstructured.Unstructured + expectErr bool + msg string + }{ + { + policy: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "spec": map[string]interface{}{ + "policy": map[string]interface{}{}, + }, + }, + }, + expectErr: false, + msg: "valid policy", + }, + { + policy: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "spec": map[string]interface{}{ + "something": map[string]interface{}{}, + }, + }, + }, + expectErr: true, + msg: "invalid policy with no policy field", + }, + { + policy: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "something": map[string]interface{}{ + "policy": map[string]interface{}{}, + }, + }, + }, + expectErr: true, + msg: "invalid policy with no spec field", + }, + } + + for _, test := range tests { + err := validateAppProtectPolicy(test.policy) + if test.expectErr && err == nil { + t.Errorf("validateAppProtectPolicy() returned no error for the case of %s", test.msg) + } + if !test.expectErr && err != nil { + t.Errorf("validateAppProtectPolicy() returned unexpected error %v for the case of %s", err, test.msg) + } + } +} + +func TestValidateAppProtectLogConf(t *testing.T) { + tests := []struct { + logConf *unstructured.Unstructured + expectErr bool + msg string + }{ + { + logConf: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "spec": map[string]interface{}{ + "content": map[string]interface{}{}, + "filter": map[string]interface{}{}, + }, + }, + }, + expectErr: false, + msg: "valid log conf", + }, + { + logConf: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "spec": map[string]interface{}{ + "filter": map[string]interface{}{}, + }, + }, + }, + expectErr: true, + msg: "invalid log conf with no content field", + }, + { + logConf: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "spec": map[string]interface{}{ + "content": map[string]interface{}{}, + }, + }, + }, + expectErr: true, + msg: "invalid log conf with no filter field", + }, + { + logConf: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "something": map[string]interface{}{ + "content": map[string]interface{}{}, + "filter": map[string]interface{}{}, + }, + }, + }, + expectErr: true, + msg: "invalid log conf with no spec field", + }, + } + + for _, test := range tests { + err := validateAppProtectLogConf(test.logConf) + if test.expectErr && err == nil { + t.Errorf("validateAppProtectLogConf() returned no error for the case of %s", test.msg) + } + if !test.expectErr && err != nil { + t.Errorf("validateAppProtectLogConf() returned unexpected error %v for the case of %s", err, test.msg) + } + } +} + func TestValidateAppProtectLogDestinationAnnotation(t *testing.T) { // Positive test cases var posDstAntns = []string{"stderr", "syslog:server=localhost:9000", "syslog:server=10.1.1.2:9000", "/var/log/ap.log"} @@ -33,3 +340,97 @@ func TestValidateAppProtectLogDestinationAnnotation(t *testing.T) { } } } + +func TestValidateAppProtectUserSig(t *testing.T) { + tests := []struct { + userSig *unstructured.Unstructured + expectErr bool + msg string + }{ + { + userSig: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "spec": map[string]interface{}{ + "signatures": []interface{}{}, + }, + }, + }, + expectErr: false, + msg: "valid user sig", + }, + { + userSig: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "spec": map[string]interface{}{ + "something": []interface{}{}, + }, + }, + }, + expectErr: true, + msg: "invalid user sig with no signatures", + }, + { + userSig: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "something": map[string]interface{}{ + "signatures": []interface{}{}, + }, + }, + }, + expectErr: true, + msg: "invalid user sign with no spec field", + }, + } + + for _, test := range tests { + err := validateAppProtectUserSig(test.userSig) + if test.expectErr && err == nil { + t.Errorf("validateAppProtectUserSig() returned no error for the case of %s", test.msg) + } + if !test.expectErr && err != nil { + t.Errorf("validateAppProtectUserSig() returned unexpected error %v for the case of %s", err, test.msg) + } + } +} + +func TestParseResourceReferenceAnnotation(t *testing.T) { + tests := []struct { + ns, antn, expected string + }{ + { + ns: "default", + antn: "resource", + expected: "default/resource", + }, + { + ns: "default", + antn: "ns-1/resource", + expected: "ns-1/resource", + }, + } + + for _, test := range tests { + result := ParseResourceReferenceAnnotation(test.ns, test.antn) + if result != test.expected { + t.Errorf("ParseResourceReferenceAnnotation(%q,%q) returned %q but expected %q", test.ns, test.antn, result, test.expected) + } + } +} + +func TestGenNsName(t *testing.T) { + obj := &unstructured.Unstructured{ + Object: map[string]interface{}{ + "metadata": map[string]interface{}{ + "namespace": "default", + "name": "resource", + }, + }, + } + + expected := "default/resource" + + result := GetNsName(obj) + if result != expected { + t.Errorf("GetNsName() returned %q but expected %q", result, expected) + } +} diff --git a/internal/k8s/handlers.go b/internal/k8s/handlers.go index 8e9508972c..7784478a79 100644 --- a/internal/k8s/handlers.go +++ b/internal/k8s/handlers.go @@ -483,12 +483,12 @@ func createIngressLinkHandlers(lbc *LoadBalancerController) cache.ResourceEventH UpdateFunc: func(old, cur interface{}) { oldLink := old.(*unstructured.Unstructured) curLink := cur.(*unstructured.Unstructured) - updated, err := compareSpecs(oldLink, curLink) + different, err := areResourcesDifferent(oldLink, curLink) if err != nil { glog.V(3).Infof("Error when comparing IngressLinks: %v", err) lbc.AddSyncQueue(curLink) } - if updated { + if different { glog.V(3).Infof("IngressLink %v changed, syncing", oldLink.GetName()) lbc.AddSyncQueue(curLink) } @@ -506,12 +506,12 @@ func createAppProtectPolicyHandlers(lbc *LoadBalancerController) cache.ResourceE UpdateFunc: func(oldObj, obj interface{}) { oldPol := oldObj.(*unstructured.Unstructured) newPol := obj.(*unstructured.Unstructured) - updated, err := compareSpecs(oldPol, newPol) + different, err := areResourcesDifferent(oldPol, newPol) if err != nil { glog.V(3).Infof("Error when comparing policy %v", err) lbc.AddSyncQueue(newPol) } - if updated { + if different { glog.V(3).Infof("ApPolicy %v changed, syncing", oldPol.GetName()) lbc.AddSyncQueue(newPol) } @@ -523,7 +523,8 @@ func createAppProtectPolicyHandlers(lbc *LoadBalancerController) cache.ResourceE return handlers } -func compareSpecs(oldresource, resource *unstructured.Unstructured) (bool, error) { +// areResourcesDifferent returns true if the resources are different based on their spec. +func areResourcesDifferent(oldresource, resource *unstructured.Unstructured) (bool, error) { oldSpec, found, err := unstructured.NestedMap(oldresource.Object, "spec") if !found { glog.V(3).Infof("Warning, oldspec has unexpected format") @@ -555,12 +556,12 @@ func createAppProtectLogConfHandlers(lbc *LoadBalancerController) cache.Resource UpdateFunc: func(oldObj, obj interface{}) { oldConf := oldObj.(*unstructured.Unstructured) newConf := obj.(*unstructured.Unstructured) - updated, err := compareSpecs(oldConf, newConf) + different, err := areResourcesDifferent(oldConf, newConf) if err != nil { glog.V(3).Infof("Error when comparing LogConfs %v", err) lbc.AddSyncQueue(newConf) } - if updated { + if different { glog.V(3).Infof("ApLogConf %v changed, syncing", oldConf.GetName()) lbc.AddSyncQueue(newConf) } @@ -582,12 +583,12 @@ func createAppProtectUserSigHandlers(lbc *LoadBalancerController) cache.Resource UpdateFunc: func(oldObj, obj interface{}) { oldSig := oldObj.(*unstructured.Unstructured) newSig := obj.(*unstructured.Unstructured) - updated, err := compareSpecs(oldSig, newSig) + different, err := areResourcesDifferent(oldSig, newSig) if err != nil { glog.V(3).Infof("Error when comparing UserSigs %v", err) lbc.AddSyncQueue(newSig) } - if updated { + if different { glog.V(3).Infof("ApUserSig %v changed, syncing", oldSig.GetName()) lbc.AddSyncQueue(newSig) } diff --git a/internal/k8s/handlers_test.go b/internal/k8s/handlers_test.go index 8cf777183c..d0ff18b7ad 100644 --- a/internal/k8s/handlers_test.go +++ b/internal/k8s/handlers_test.go @@ -4,6 +4,7 @@ import ( "testing" v1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/util/intstr" ) @@ -154,3 +155,121 @@ func TestHasServicePortChanges(t *testing.T) { } } } + +func TestAreResourcesDifferent(t *testing.T) { + tests := []struct { + oldR, newR *unstructured.Unstructured + expected, expectErr bool + msg string + }{ + { + oldR: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "spec": true, // wrong type + }, + }, + newR: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "spec": map[string]interface{}{}, + }, + }, + expected: false, + expectErr: true, + msg: "invalid old resource", + }, + { + oldR: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "spec": map[string]interface{}{}, + }, + }, + newR: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "spec": true, // wrong type + }, + }, + expected: false, + expectErr: true, + msg: "invalid new resource", + }, + { + oldR: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "spec": map[string]interface{}{}, + }, + }, + newR: &unstructured.Unstructured{ + Object: map[string]interface{}{}, + }, + expected: false, + expectErr: true, + msg: "new resource with missing spec", + }, + { + oldR: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "spec": map[string]interface{}{ + "field": "a", + }, + }, + }, + newR: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "spec": map[string]interface{}{ + "field": "a", + }, + }, + }, + expected: false, + expectErr: false, + msg: "equal resources", + }, + { + oldR: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "spec": map[string]interface{}{ + "field": "a", + }, + }, + }, + newR: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "spec": map[string]interface{}{ + "field": "b", + }, + }, + }, + expected: true, + expectErr: false, + msg: "not equal resources", + }, + { + oldR: &unstructured.Unstructured{ + Object: map[string]interface{}{}, + }, + newR: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "spec": map[string]interface{}{ + "field": "b", + }, + }, + }, + expected: true, + expectErr: false, + msg: "not equal resources with with first resource missing spec", + }, + } + + for _, test := range tests { + result, err := areResourcesDifferent(test.oldR, test.newR) + if result != test.expected { + t.Errorf("areResourcesDifferent() returned %v but expected %v for the case of %s", result, test.expected, test.msg) + } + if test.expectErr && err == nil { + t.Errorf("areResourcesDifferent() returned no error for the case of %s", test.msg) + } + if !test.expectErr && err != nil { + t.Errorf("areResourcesDifferent() returned unexpected error %v for the case of %s", err, test.msg) + } + } +}