From fe6d647c1df954e4b9a1e42473138879b341fc3d Mon Sep 17 00:00:00 2001 From: tomer pasman Date: Wed, 31 Aug 2022 10:17:44 +0300 Subject: [PATCH 01/12] Create warning for not default splunk value in apdoslogconf --- .../app_protect_dos_configuration.go | 19 ++++++++++++++++--- internal/k8s/controller.go | 13 +++++++++++++ pkg/apis/dos/validation/dos.go | 18 +++++++++++++++--- 3 files changed, 44 insertions(+), 6 deletions(-) diff --git a/internal/k8s/appprotectdos/app_protect_dos_configuration.go b/internal/k8s/appprotectdos/app_protect_dos_configuration.go index f6aecfe1df..70c1e73a9b 100644 --- a/internal/k8s/appprotectdos/app_protect_dos_configuration.go +++ b/internal/k8s/appprotectdos/app_protect_dos_configuration.go @@ -114,11 +114,14 @@ func (ci *Configuration) AddOrUpdatePolicy(policyObj *unstructured.Unstructured) resNsName := appprotectcommon.GetNsName(policyObj) policy, err := createAppProtectDosPolicyEx(policyObj) ci.dosPolicies[resNsName] = policy + op := AddOrUpdate if err != nil { - changes = append(changes, Change{Op: Delete, Resource: policy}) + op = Delete problems = append(problems, Problem{Object: policyObj, Reason: "Rejected", Message: err.Error()}) } + changes = append(changes, Change{Op: op, Resource: policy}) + protectedResources := ci.GetDosProtectedThatReferencedDosPolicy(resNsName) for _, p := range protectedResources { proChanges, proProblems := ci.AddOrUpdateDosProtectedResource(p) @@ -134,11 +137,14 @@ func (ci *Configuration) AddOrUpdateLogConf(logConfObj *unstructured.Unstructure resNsName := appprotectcommon.GetNsName(logConfObj) logConf, err := createAppProtectDosLogConfEx(logConfObj) ci.dosLogConfs[resNsName] = logConf + op := AddOrUpdate if err != nil { - changes = append(changes, Change{Op: Delete, Resource: logConf}) + op = Delete problems = append(problems, Problem{Object: logConfObj, Reason: "Rejected", Message: err.Error()}) } + changes = append(changes, Change{Op: op, Resource: logConf}) + protectedResources := ci.GetDosProtectedThatReferencedDosLogConf(resNsName) for _, p := range protectedResources { proChanges, proProblems := ci.AddOrUpdateDosProtectedResource(p) @@ -357,7 +363,7 @@ func createAppProtectDosPolicyEx(policyObj *unstructured.Unstructured) (*DosPoli } func createAppProtectDosLogConfEx(dosLogConfObj *unstructured.Unstructured) (*DosLogConfEx, error) { - err := validation.ValidateAppProtectDosLogConf(dosLogConfObj) + warning, err := validation.ValidateAppProtectDosLogConf(dosLogConfObj) if err != nil { return &DosLogConfEx{ Obj: dosLogConfObj, @@ -365,6 +371,13 @@ func createAppProtectDosLogConfEx(dosLogConfObj *unstructured.Unstructured) (*Do ErrorMsg: fmt.Sprintf("failed to store ApDosLogconf: %v", err), }, err } + if warning != "" { + return &DosLogConfEx{ + Obj: dosLogConfObj, + IsValid: true, + ErrorMsg: warning, + }, nil + } return &DosLogConfEx{ Obj: dosLogConfObj, IsValid: true, diff --git a/internal/k8s/controller.go b/internal/k8s/controller.go index 5b39932f91..f477d130eb 100644 --- a/internal/k8s/controller.go +++ b/internal/k8s/controller.go @@ -1475,6 +1475,19 @@ func (lbc *LoadBalancerController) processAppProtectDosChanges(changes []appprot lbc.updateResourcesStatusAndEvents(resources, warnings, err) msg := fmt.Sprintf("Configuration for %s/%s was added or updated", impl.Obj.Namespace, impl.Obj.Name) lbc.recorder.Event(impl.Obj, api_v1.EventTypeNormal, "AddedOrUpdated", msg) + case *appprotectdos.DosPolicyEx: + msg := fmt.Sprintf("Configuration was added or updated") + lbc.recorder.Event(impl.Obj, api_v1.EventTypeNormal, "AddedOrUpdated", msg) + case *appprotectdos.DosLogConfEx: + eventType := api_v1.EventTypeNormal + eventTitle := "AddedOrUpdated" + msg := fmt.Sprintf("Configuration was added or updated") + if impl.ErrorMsg != "" { + msg += fmt.Sprintf(" ; with warning(s): %s", impl.ErrorMsg) + eventTitle = "AddedOrUpdatedWithWarning" + eventType = api_v1.EventTypeWarning + } + lbc.recorder.Event(impl.Obj, eventType, eventTitle, msg) } } else if c.Op == appprotectdos.Delete { switch impl := c.Resource.(type) { diff --git a/pkg/apis/dos/validation/dos.go b/pkg/apis/dos/validation/dos.go index 55cb97555e..55f2305354 100644 --- a/pkg/apis/dos/validation/dos.go +++ b/pkg/apis/dos/validation/dos.go @@ -90,15 +90,27 @@ func validateResourceReference(ref string) error { return nil } +// validateDefaultContent log configuration support only splunk format +func validateDefaultContent(obj *unstructured.Unstructured) string { + content, _, _ := unstructured.NestedMap(obj.Object, "spec", "content") + if content["format"] != "splunk" { + unstructured.SetNestedField(obj.Object, "splunk", "spec", "content", "format") + return "Support only splunk format" + } + + return "" +} + // ValidateAppProtectDosLogConf validates LogConfiguration resource -func ValidateAppProtectDosLogConf(logConf *unstructured.Unstructured) error { +func ValidateAppProtectDosLogConf(logConf *unstructured.Unstructured) (string, error) { lcName := logConf.GetName() err := validation2.ValidateRequiredFields(logConf, appProtectDosLogConfRequiredFields) if err != nil { - return fmt.Errorf("error validating App Protect Dos Log Configuration %v: %w", lcName, err) + return "", fmt.Errorf("error validating App Protect Dos Log Configuration %v: %w", lcName, err) } + warning := validateDefaultContent(logConf) - return nil + return warning, nil } var ( From 4fec2c87141b18801ecaf51388d5f35803bb9939 Mon Sep 17 00:00:00 2001 From: tomer pasman Date: Wed, 31 Aug 2022 10:21:31 +0300 Subject: [PATCH 02/12] Refactor func name to validateAppProtectDosLogConfDefaultContent --- pkg/apis/dos/validation/dos.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/apis/dos/validation/dos.go b/pkg/apis/dos/validation/dos.go index 55f2305354..113c7dee4f 100644 --- a/pkg/apis/dos/validation/dos.go +++ b/pkg/apis/dos/validation/dos.go @@ -91,7 +91,7 @@ func validateResourceReference(ref string) error { } // validateDefaultContent log configuration support only splunk format -func validateDefaultContent(obj *unstructured.Unstructured) string { +func validateAppProtectDosLogConfDefaultContent(obj *unstructured.Unstructured) string { content, _, _ := unstructured.NestedMap(obj.Object, "spec", "content") if content["format"] != "splunk" { unstructured.SetNestedField(obj.Object, "splunk", "spec", "content", "format") @@ -108,7 +108,7 @@ func ValidateAppProtectDosLogConf(logConf *unstructured.Unstructured) (string, e if err != nil { return "", fmt.Errorf("error validating App Protect Dos Log Configuration %v: %w", lcName, err) } - warning := validateDefaultContent(logConf) + warning := validateAppProtectDosLogConfDefaultContent(logConf) return warning, nil } From 4c67f6046946c8fb3568b94f2b783ff051d2d193 Mon Sep 17 00:00:00 2001 From: tomer pasman Date: Wed, 31 Aug 2022 11:28:59 +0300 Subject: [PATCH 03/12] Refactor tests --- .../app_protect_dos_configuration_test.go | 23 ++++++++++++-- pkg/apis/dos/validation/dos_test.go | 30 +++++++++++++++++-- 2 files changed, 49 insertions(+), 4 deletions(-) diff --git a/internal/k8s/appprotectdos/app_protect_dos_configuration_test.go b/internal/k8s/appprotectdos/app_protect_dos_configuration_test.go index 23eb9df4ea..d740bcf653 100644 --- a/internal/k8s/appprotectdos/app_protect_dos_configuration_test.go +++ b/internal/k8s/appprotectdos/app_protect_dos_configuration_test.go @@ -70,7 +70,8 @@ func TestCreateAppProtectDosLogConfEx(t *testing.T) { logConf: &unstructured.Unstructured{ Object: map[string]interface{}{ "spec": map[string]interface{}{ - "content": map[string]interface{}{}, + "content": map[string]interface{}{ + "format": "splunk"}, "filter": map[string]interface{}{}, }, }, @@ -211,6 +212,11 @@ func TestAddOrUpdateDosPolicy(t *testing.T) { }, }, } + + basicPolicyResource := &DosPolicyEx{ + Obj: basicTestPolicy, + IsValid: true, + } invalidTestPolicy := &unstructured.Unstructured{ Object: map[string]interface{}{ "metadata": map[string]interface{}{ @@ -245,6 +251,10 @@ func TestAddOrUpdateDosPolicy(t *testing.T) { { policy: basicTestPolicy, expectedChanges: []Change{ + { + Resource: basicPolicyResource, + Op: AddOrUpdate, + }, { Resource: &DosProtectedResourceEx{ Obj: basicResource, @@ -298,11 +308,16 @@ func TestAddOrUpdateDosLogConf(t *testing.T) { "name": "testlogconf", }, "spec": map[string]interface{}{ - "content": map[string]interface{}{}, + "content": map[string]interface{}{ + "format": "splunk"}, "filter": map[string]interface{}{}, }, }, } + validLogConfResource := &DosLogConfEx{ + Obj: validLogConf, + IsValid: true, + } invalidLogConf := &unstructured.Unstructured{ Object: map[string]interface{}{ "metadata": map[string]interface{}{ @@ -345,6 +360,10 @@ func TestAddOrUpdateDosLogConf(t *testing.T) { { logconf: validLogConf, expectedChanges: []Change{ + { + Resource: validLogConfResource, + Op: AddOrUpdate, + }, { Resource: &DosProtectedResourceEx{ Obj: basicResource, diff --git a/pkg/apis/dos/validation/dos_test.go b/pkg/apis/dos/validation/dos_test.go index 11081f7633..ae08ff34d2 100644 --- a/pkg/apis/dos/validation/dos_test.go +++ b/pkg/apis/dos/validation/dos_test.go @@ -242,18 +242,21 @@ func TestValidateAppProtectDosLogConf(t *testing.T) { tests := []struct { logConf *unstructured.Unstructured expectErr bool + expectWarn bool msg string }{ { logConf: &unstructured.Unstructured{ Object: map[string]interface{}{ "spec": map[string]interface{}{ - "content": map[string]interface{}{}, + "content": map[string]interface{}{ + "format": "splunk"}, "filter": map[string]interface{}{}, }, }, }, expectErr: false, + expectWarn: false, msg: "valid log conf", }, { @@ -265,6 +268,7 @@ func TestValidateAppProtectDosLogConf(t *testing.T) { }, }, expectErr: true, + expectWarn: false, msg: "invalid log conf with no content field", }, { @@ -276,6 +280,7 @@ func TestValidateAppProtectDosLogConf(t *testing.T) { }, }, expectErr: true, + expectWarn: false, msg: "invalid log conf with no filter field", }, { @@ -288,18 +293,39 @@ func TestValidateAppProtectDosLogConf(t *testing.T) { }, }, expectErr: true, + expectWarn: false, msg: "invalid log conf with no spec field", }, + { + logConf: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "spec": map[string]interface{}{ + "content": map[string]interface{}{ + "format": "user-defined"}, + "filter": map[string]interface{}{}, + }, + }, + }, + expectErr: false, + expectWarn: true, + msg: "Support only splunk format", + }, } for _, test := range tests { - err := ValidateAppProtectDosLogConf(test.logConf) + warn, err := ValidateAppProtectDosLogConf(test.logConf) if test.expectErr && err == nil { t.Errorf("validateAppProtectDosLogConf() returned no error for the case of %s", test.msg) } if !test.expectErr && err != nil { t.Errorf("validateAppProtectDosLogConf() returned unexpected error %v for the case of %s", err, test.msg) } + if test.expectWarn && warn == "" { + t.Errorf("validateAppProtectDosLogConf() returned no warning for the case of %s", test.msg) + } + if !test.expectWarn && warn != "" { + t.Errorf("validateAppProtectDosLogConf() returned unexpected warning: %s, for the case of %s", warn, test.msg) + } } } From 4fdbfb9146fd5e5ec6360905bdb1ea9c971d43bf Mon Sep 17 00:00:00 2001 From: tomer pasman Date: Wed, 31 Aug 2022 12:33:59 +0300 Subject: [PATCH 04/12] Fix Lint errors --- .../app_protect_dos_configuration.go | 16 ++++++------ internal/k8s/controller.go | 26 +++++++++---------- pkg/apis/dos/validation/dos.go | 14 ++++++---- 3 files changed, 30 insertions(+), 26 deletions(-) diff --git a/internal/k8s/appprotectdos/app_protect_dos_configuration.go b/internal/k8s/appprotectdos/app_protect_dos_configuration.go index 70c1e73a9b..f01a671a97 100644 --- a/internal/k8s/appprotectdos/app_protect_dos_configuration.go +++ b/internal/k8s/appprotectdos/app_protect_dos_configuration.go @@ -120,7 +120,7 @@ func (ci *Configuration) AddOrUpdatePolicy(policyObj *unstructured.Unstructured) problems = append(problems, Problem{Object: policyObj, Reason: "Rejected", Message: err.Error()}) } - changes = append(changes, Change{Op: op, Resource: policy}) + changes = append(changes, Change{Op: op, Resource: policy}) protectedResources := ci.GetDosProtectedThatReferencedDosPolicy(resNsName) for _, p := range protectedResources { @@ -139,11 +139,11 @@ func (ci *Configuration) AddOrUpdateLogConf(logConfObj *unstructured.Unstructure ci.dosLogConfs[resNsName] = logConf op := AddOrUpdate if err != nil { - op = Delete + op = Delete problems = append(problems, Problem{Object: logConfObj, Reason: "Rejected", Message: err.Error()}) } - changes = append(changes, Change{Op: op, Resource: logConf}) + changes = append(changes, Change{Op: op, Resource: logConf}) protectedResources := ci.GetDosProtectedThatReferencedDosLogConf(resNsName) for _, p := range protectedResources { @@ -372,11 +372,11 @@ func createAppProtectDosLogConfEx(dosLogConfObj *unstructured.Unstructured) (*Do }, err } if warning != "" { - return &DosLogConfEx{ - Obj: dosLogConfObj, - IsValid: true, - ErrorMsg: warning, - }, nil + return &DosLogConfEx{ + Obj: dosLogConfObj, + IsValid: true, + ErrorMsg: warning, + }, nil } return &DosLogConfEx{ Obj: dosLogConfObj, diff --git a/internal/k8s/controller.go b/internal/k8s/controller.go index f477d130eb..473a0077e0 100644 --- a/internal/k8s/controller.go +++ b/internal/k8s/controller.go @@ -1475,19 +1475,19 @@ func (lbc *LoadBalancerController) processAppProtectDosChanges(changes []appprot lbc.updateResourcesStatusAndEvents(resources, warnings, err) msg := fmt.Sprintf("Configuration for %s/%s was added or updated", impl.Obj.Namespace, impl.Obj.Name) lbc.recorder.Event(impl.Obj, api_v1.EventTypeNormal, "AddedOrUpdated", msg) - case *appprotectdos.DosPolicyEx: - msg := fmt.Sprintf("Configuration was added or updated") - lbc.recorder.Event(impl.Obj, api_v1.EventTypeNormal, "AddedOrUpdated", msg) - case *appprotectdos.DosLogConfEx: - eventType := api_v1.EventTypeNormal - eventTitle := "AddedOrUpdated" - msg := fmt.Sprintf("Configuration was added or updated") - if impl.ErrorMsg != "" { - msg += fmt.Sprintf(" ; with warning(s): %s", impl.ErrorMsg) - eventTitle = "AddedOrUpdatedWithWarning" - eventType = api_v1.EventTypeWarning - } - lbc.recorder.Event(impl.Obj, eventType, eventTitle, msg) + case *appprotectdos.DosPolicyEx: + msg := "Configuration was added or updated" + lbc.recorder.Event(impl.Obj, api_v1.EventTypeNormal, "AddedOrUpdated", msg) + case *appprotectdos.DosLogConfEx: + eventType := api_v1.EventTypeNormal + eventTitle := "AddedOrUpdated" + msg := "Configuration was added or updated" + if impl.ErrorMsg != "" { + msg += fmt.Sprintf(" ; with warning(s): %s", impl.ErrorMsg) + eventTitle = "AddedOrUpdatedWithWarning" + eventType = api_v1.EventTypeWarning + } + lbc.recorder.Event(impl.Obj, eventType, eventTitle, msg) } } else if c.Op == appprotectdos.Delete { switch impl := c.Resource.(type) { diff --git a/pkg/apis/dos/validation/dos.go b/pkg/apis/dos/validation/dos.go index 113c7dee4f..e89b781e1a 100644 --- a/pkg/apis/dos/validation/dos.go +++ b/pkg/apis/dos/validation/dos.go @@ -92,11 +92,15 @@ func validateResourceReference(ref string) error { // validateDefaultContent log configuration support only splunk format func validateAppProtectDosLogConfDefaultContent(obj *unstructured.Unstructured) string { - content, _, _ := unstructured.NestedMap(obj.Object, "spec", "content") - if content["format"] != "splunk" { - unstructured.SetNestedField(obj.Object, "splunk", "spec", "content", "format") - return "Support only splunk format" - } + content, _, _ := unstructured.NestedMap(obj.Object, "spec", "content") + if content["format"] != "splunk" { + msg := "Support only splunk format" + err := unstructured.SetNestedField(obj.Object, "splunk", "spec", "content", "format") + if err != nil { + msg += ", Change format to splunk failed" + } + return msg + } return "" } From d944e4b3d622acc8be08e1e484d03af42da15acf Mon Sep 17 00:00:00 2001 From: tomer pasman Date: Thu, 8 Sep 2022 09:53:48 +0300 Subject: [PATCH 05/12] Fix Lint errors --- .../app_protect_dos_configuration_test.go | 38 +++++------ pkg/apis/dos/validation/dos_test.go | 66 +++++++++---------- 2 files changed, 52 insertions(+), 52 deletions(-) diff --git a/internal/k8s/appprotectdos/app_protect_dos_configuration_test.go b/internal/k8s/appprotectdos/app_protect_dos_configuration_test.go index d740bcf653..13bee9c42d 100644 --- a/internal/k8s/appprotectdos/app_protect_dos_configuration_test.go +++ b/internal/k8s/appprotectdos/app_protect_dos_configuration_test.go @@ -71,8 +71,8 @@ func TestCreateAppProtectDosLogConfEx(t *testing.T) { Object: map[string]interface{}{ "spec": map[string]interface{}{ "content": map[string]interface{}{ - "format": "splunk"}, - "filter": map[string]interface{}{}, + "format": "splunk"}, + "filter": map[string]interface{}{}, }, }, }, @@ -214,9 +214,9 @@ func TestAddOrUpdateDosPolicy(t *testing.T) { } basicPolicyResource := &DosPolicyEx{ - Obj: basicTestPolicy, - IsValid: true, - } + Obj: basicTestPolicy, + IsValid: true, + } invalidTestPolicy := &unstructured.Unstructured{ Object: map[string]interface{}{ "metadata": map[string]interface{}{ @@ -251,10 +251,10 @@ func TestAddOrUpdateDosPolicy(t *testing.T) { { policy: basicTestPolicy, expectedChanges: []Change{ - { - Resource: basicPolicyResource, - Op: AddOrUpdate, - }, + { + Resource: basicPolicyResource, + Op: AddOrUpdate, + }, { Resource: &DosProtectedResourceEx{ Obj: basicResource, @@ -309,15 +309,15 @@ func TestAddOrUpdateDosLogConf(t *testing.T) { }, "spec": map[string]interface{}{ "content": map[string]interface{}{ - "format": "splunk"}, - "filter": map[string]interface{}{}, + "format": "splunk"}, + "filter": map[string]interface{}{}, }, }, } - validLogConfResource := &DosLogConfEx{ - Obj: validLogConf, - IsValid: true, - } + validLogConfResource := &DosLogConfEx{ + Obj: validLogConf, + IsValid: true, + } invalidLogConf := &unstructured.Unstructured{ Object: map[string]interface{}{ "metadata": map[string]interface{}{ @@ -360,10 +360,10 @@ func TestAddOrUpdateDosLogConf(t *testing.T) { { logconf: validLogConf, expectedChanges: []Change{ - { - Resource: validLogConfResource, - Op: AddOrUpdate, - }, + { + Resource: validLogConfResource, + Op: AddOrUpdate, + }, { Resource: &DosProtectedResourceEx{ Obj: basicResource, diff --git a/pkg/apis/dos/validation/dos_test.go b/pkg/apis/dos/validation/dos_test.go index ae08ff34d2..84afe843c9 100644 --- a/pkg/apis/dos/validation/dos_test.go +++ b/pkg/apis/dos/validation/dos_test.go @@ -240,24 +240,24 @@ func TestValidateAppProtectDosAccessLogDest(t *testing.T) { func TestValidateAppProtectDosLogConf(t *testing.T) { t.Parallel() tests := []struct { - logConf *unstructured.Unstructured - expectErr bool + logConf *unstructured.Unstructured + expectErr bool expectWarn bool - msg string + msg string }{ { logConf: &unstructured.Unstructured{ Object: map[string]interface{}{ "spec": map[string]interface{}{ "content": map[string]interface{}{ - "format": "splunk"}, - "filter": map[string]interface{}{}, + "format": "splunk"}, + "filter": map[string]interface{}{}, }, }, }, - expectErr: false, + expectErr: false, expectWarn: false, - msg: "valid log conf", + msg: "valid log conf", }, { logConf: &unstructured.Unstructured{ @@ -267,9 +267,9 @@ func TestValidateAppProtectDosLogConf(t *testing.T) { }, }, }, - expectErr: true, + expectErr: true, expectWarn: false, - msg: "invalid log conf with no content field", + msg: "invalid log conf with no content field", }, { logConf: &unstructured.Unstructured{ @@ -279,9 +279,9 @@ func TestValidateAppProtectDosLogConf(t *testing.T) { }, }, }, - expectErr: true, + expectErr: true, expectWarn: false, - msg: "invalid log conf with no filter field", + msg: "invalid log conf with no filter field", }, { logConf: &unstructured.Unstructured{ @@ -292,24 +292,24 @@ func TestValidateAppProtectDosLogConf(t *testing.T) { }, }, }, - expectErr: true, + expectErr: true, expectWarn: false, - msg: "invalid log conf with no spec field", + msg: "invalid log conf with no spec field", + }, + { + logConf: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "spec": map[string]interface{}{ + "content": map[string]interface{}{ + "format": "user-defined"}, + "filter": map[string]interface{}{}, + }, + }, + }, + expectErr: false, + expectWarn: true, + msg: "Support only splunk format", }, - { - logConf: &unstructured.Unstructured{ - Object: map[string]interface{}{ - "spec": map[string]interface{}{ - "content": map[string]interface{}{ - "format": "user-defined"}, - "filter": map[string]interface{}{}, - }, - }, - }, - expectErr: false, - expectWarn: true, - msg: "Support only splunk format", - }, } for _, test := range tests { @@ -320,12 +320,12 @@ func TestValidateAppProtectDosLogConf(t *testing.T) { if !test.expectErr && err != nil { t.Errorf("validateAppProtectDosLogConf() returned unexpected error %v for the case of %s", err, test.msg) } - if test.expectWarn && warn == "" { - t.Errorf("validateAppProtectDosLogConf() returned no warning for the case of %s", test.msg) - } - if !test.expectWarn && warn != "" { - t.Errorf("validateAppProtectDosLogConf() returned unexpected warning: %s, for the case of %s", warn, test.msg) - } + if test.expectWarn && warn == "" { + t.Errorf("validateAppProtectDosLogConf() returned no warning for the case of %s", test.msg) + } + if !test.expectWarn && warn != "" { + t.Errorf("validateAppProtectDosLogConf() returned unexpected warning: %s, for the case of %s", warn, test.msg) + } } } From a96d34f2b104a82340c762c26a912a45cfeb1059 Mon Sep 17 00:00:00 2001 From: tomer pasman Date: Thu, 8 Sep 2022 10:14:19 +0300 Subject: [PATCH 06/12] Fix Lint errors --- .../k8s/appprotectdos/app_protect_dos_configuration_test.go | 6 ++++-- pkg/apis/dos/validation/dos_test.go | 6 ++++-- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/internal/k8s/appprotectdos/app_protect_dos_configuration_test.go b/internal/k8s/appprotectdos/app_protect_dos_configuration_test.go index 13bee9c42d..a51aade603 100644 --- a/internal/k8s/appprotectdos/app_protect_dos_configuration_test.go +++ b/internal/k8s/appprotectdos/app_protect_dos_configuration_test.go @@ -71,7 +71,8 @@ func TestCreateAppProtectDosLogConfEx(t *testing.T) { Object: map[string]interface{}{ "spec": map[string]interface{}{ "content": map[string]interface{}{ - "format": "splunk"}, + "format": "splunk", + }, "filter": map[string]interface{}{}, }, }, @@ -309,7 +310,8 @@ func TestAddOrUpdateDosLogConf(t *testing.T) { }, "spec": map[string]interface{}{ "content": map[string]interface{}{ - "format": "splunk"}, + "format": "splunk", + }, "filter": map[string]interface{}{}, }, }, diff --git a/pkg/apis/dos/validation/dos_test.go b/pkg/apis/dos/validation/dos_test.go index 84afe843c9..5309e3c73f 100644 --- a/pkg/apis/dos/validation/dos_test.go +++ b/pkg/apis/dos/validation/dos_test.go @@ -250,7 +250,8 @@ func TestValidateAppProtectDosLogConf(t *testing.T) { Object: map[string]interface{}{ "spec": map[string]interface{}{ "content": map[string]interface{}{ - "format": "splunk"}, + "format": "splunk", + }, "filter": map[string]interface{}{}, }, }, @@ -301,7 +302,8 @@ func TestValidateAppProtectDosLogConf(t *testing.T) { Object: map[string]interface{}{ "spec": map[string]interface{}{ "content": map[string]interface{}{ - "format": "user-defined"}, + "format": "user-defined", + }, "filter": map[string]interface{}{}, }, }, From 62b446b1d96853e0a4aaa1e3233d4efb2eb41157 Mon Sep 17 00:00:00 2001 From: tomer pasman Date: Thu, 8 Sep 2022 13:25:47 +0300 Subject: [PATCH 07/12] Make warning for apdoslogconf --- .../appprotectdos.f5.com_apdoslogconfs.yaml | 2 - docs/content/app-protect-dos/configuration.md | 12 ++---- .../app-protect-dos/apdos-logconf.yaml | 3 -- .../app-protect-dos/apdos-logconf.yaml | 3 -- .../app_protect_dos_configuration_test.go | 41 ++++++++++++++----- pkg/apis/dos/validation/dos.go | 18 ++++---- pkg/apis/dos/validation/dos_test.go | 33 +++++++-------- tests/data/dos/dos-logconf.yaml | 3 -- .../data/virtual-server-dos/dos-logconf.yaml | 3 -- 9 files changed, 56 insertions(+), 62 deletions(-) diff --git a/deployments/common/crds/appprotectdos.f5.com_apdoslogconfs.yaml b/deployments/common/crds/appprotectdos.f5.com_apdoslogconfs.yaml index f6e0f7110e..e23e87184b 100644 --- a/deployments/common/crds/appprotectdos.f5.com_apdoslogconfs.yaml +++ b/deployments/common/crds/appprotectdos.f5.com_apdoslogconfs.yaml @@ -38,13 +38,11 @@ spec: - splunk - arcsight - user-defined - default: splunk type: string format_string: type: string max_message_size: pattern: ^([1-9]|[1-5][0-9]|6[0-4])k$ - default: 5k type: string type: object filter: diff --git a/docs/content/app-protect-dos/configuration.md b/docs/content/app-protect-dos/configuration.md index b7125a7e26..c6ca9850a2 100644 --- a/docs/content/app-protect-dos/configuration.md +++ b/docs/content/app-protect-dos/configuration.md @@ -112,12 +112,9 @@ For example, say you want to log state changing requests for your Ingress resour ```json { "filter": { - "request_type": "all" - }, - "content": { - "format": "default", - "max_request_size": "any", - "max_message_size": "64k" + "traffic-mitigation-stats": "all", + "bad-actors": "top 10", + "attack-signatures": "top 10" } } ``` @@ -130,9 +127,6 @@ kind: APDosLogConf metadata: name: doslogconf spec: - content: - format: splunk - max_message_size: 64k filter: traffic-mitigation-stats: all bad-actors: top 10 diff --git a/examples/custom-resources/app-protect-dos/apdos-logconf.yaml b/examples/custom-resources/app-protect-dos/apdos-logconf.yaml index 885a524647..688f3ea786 100644 --- a/examples/custom-resources/app-protect-dos/apdos-logconf.yaml +++ b/examples/custom-resources/app-protect-dos/apdos-logconf.yaml @@ -3,9 +3,6 @@ kind: APDosLogConf metadata: name: doslogconf spec: - content: - format: splunk - max_message_size: 64k filter: traffic-mitigation-stats: all bad-actors: top 10 diff --git a/examples/ingress-resources/app-protect-dos/apdos-logconf.yaml b/examples/ingress-resources/app-protect-dos/apdos-logconf.yaml index 885a524647..688f3ea786 100644 --- a/examples/ingress-resources/app-protect-dos/apdos-logconf.yaml +++ b/examples/ingress-resources/app-protect-dos/apdos-logconf.yaml @@ -3,9 +3,6 @@ kind: APDosLogConf metadata: name: doslogconf spec: - content: - format: splunk - max_message_size: 64k filter: traffic-mitigation-stats: all bad-actors: top 10 diff --git a/internal/k8s/appprotectdos/app_protect_dos_configuration_test.go b/internal/k8s/appprotectdos/app_protect_dos_configuration_test.go index a51aade603..e8215c32d3 100644 --- a/internal/k8s/appprotectdos/app_protect_dos_configuration_test.go +++ b/internal/k8s/appprotectdos/app_protect_dos_configuration_test.go @@ -70,9 +70,6 @@ func TestCreateAppProtectDosLogConfEx(t *testing.T) { logConf: &unstructured.Unstructured{ Object: map[string]interface{}{ "spec": map[string]interface{}{ - "content": map[string]interface{}{ - "format": "splunk", - }, "filter": map[string]interface{}{}, }, }, @@ -88,10 +85,39 @@ func TestCreateAppProtectDosLogConfEx(t *testing.T) { logConf: &unstructured.Unstructured{ Object: map[string]interface{}{ "spec": map[string]interface{}{ - "content": map[string]interface{}{}, + "content": map[string]interface{}{ + "format": "splunk", + }, + "filter": map[string]interface{}{}, }, }, }, + expectedLogConfEx: &DosLogConfEx{ + IsValid: true, + ErrorMsg: "Content field doesn't supported, use splunk format.", + }, + wantErr: false, + msg: "Valid DosLogConf with warning", + }, + { + logConf: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "spec": map[string]interface{}{}, + }, + }, + expectedLogConfEx: &DosLogConfEx{ + IsValid: true, + ErrorMsg: "Content field doesn't supported, use splunk format.", + }, + wantErr: false, + msg: "Valid DosLogConf with warning", + }, + { + logConf: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "spec": map[string]interface{}{}, + }, + }, expectedLogConfEx: &DosLogConfEx{ IsValid: false, ErrorMsg: "failed to store ApDosLogconf: error validating App Protect Dos Log Configuration : required field map[] not found", @@ -309,9 +335,6 @@ func TestAddOrUpdateDosLogConf(t *testing.T) { "name": "testlogconf", }, "spec": map[string]interface{}{ - "content": map[string]interface{}{ - "format": "splunk", - }, "filter": map[string]interface{}{}, }, }, @@ -326,9 +349,7 @@ func TestAddOrUpdateDosLogConf(t *testing.T) { "namespace": "testing", "name": "invalid-logconf", }, - "spec": map[string]interface{}{ - "content": map[string]interface{}{}, - }, + "spec": map[string]interface{}{}, }, } basicResource := &v1beta1.DosProtectedResource{ diff --git a/pkg/apis/dos/validation/dos.go b/pkg/apis/dos/validation/dos.go index e89b781e1a..b95b97db2f 100644 --- a/pkg/apis/dos/validation/dos.go +++ b/pkg/apis/dos/validation/dos.go @@ -19,7 +19,6 @@ var appProtectDosPolicyRequiredFields = [][]string{ } var appProtectDosLogConfRequiredFields = [][]string{ - {"spec", "content"}, {"spec", "filter"}, } @@ -90,15 +89,12 @@ func validateResourceReference(ref string) error { return nil } -// validateDefaultContent log configuration support only splunk format -func validateAppProtectDosLogConfDefaultContent(obj *unstructured.Unstructured) string { - content, _, _ := unstructured.NestedMap(obj.Object, "spec", "content") - if content["format"] != "splunk" { - msg := "Support only splunk format" - err := unstructured.SetNestedField(obj.Object, "splunk", "spec", "content", "format") - if err != nil { - msg += ", Change format to splunk failed" - } +// checkAppProtectDosLogConfContentField check conetent field doesnt appear in dos log +func checkAppProtectDosLogConfContentField(obj *unstructured.Unstructured) string { + _, found, err := unstructured.NestedMap(obj.Object, "spec", "content") + if err == nil && found { + unstructured.RemoveNestedField(obj.Object, "spec", "content") + msg := "Content field doesn't supported, use splunk format." return msg } @@ -112,7 +108,7 @@ func ValidateAppProtectDosLogConf(logConf *unstructured.Unstructured) (string, e if err != nil { return "", fmt.Errorf("error validating App Protect Dos Log Configuration %v: %w", lcName, err) } - warning := validateAppProtectDosLogConfDefaultContent(logConf) + warning := checkAppProtectDosLogConfContentField(logConf) return warning, nil } diff --git a/pkg/apis/dos/validation/dos_test.go b/pkg/apis/dos/validation/dos_test.go index 5309e3c73f..fdb92c2661 100644 --- a/pkg/apis/dos/validation/dos_test.go +++ b/pkg/apis/dos/validation/dos_test.go @@ -249,9 +249,6 @@ func TestValidateAppProtectDosLogConf(t *testing.T) { logConf: &unstructured.Unstructured{ Object: map[string]interface{}{ "spec": map[string]interface{}{ - "content": map[string]interface{}{ - "format": "splunk", - }, "filter": map[string]interface{}{}, }, }, @@ -263,54 +260,54 @@ func TestValidateAppProtectDosLogConf(t *testing.T) { { logConf: &unstructured.Unstructured{ Object: map[string]interface{}{ - "spec": map[string]interface{}{ - "filter": map[string]interface{}{}, - }, + "spec": map[string]interface{}{}, }, }, expectErr: true, expectWarn: false, - msg: "invalid log conf with no content field", + msg: "invalid log conf with no filter field", }, { logConf: &unstructured.Unstructured{ Object: map[string]interface{}{ - "spec": map[string]interface{}{ - "content": map[string]interface{}{}, + "something": map[string]interface{}{ + "filter": map[string]interface{}{}, }, }, }, expectErr: true, expectWarn: false, - msg: "invalid log conf with no filter field", + msg: "invalid log conf with no spec field", }, { logConf: &unstructured.Unstructured{ Object: map[string]interface{}{ - "something": map[string]interface{}{ - "content": map[string]interface{}{}, - "filter": map[string]interface{}{}, + "spec": map[string]interface{}{ + "content": map[string]interface{}{ + "format": "user-defined", + }, + "filter": map[string]interface{}{}, }, }, }, - expectErr: true, - expectWarn: false, - msg: "invalid log conf with no spec field", + expectErr: false, + expectWarn: true, + msg: "Support only splunk format", }, { logConf: &unstructured.Unstructured{ Object: map[string]interface{}{ "spec": map[string]interface{}{ + "filter": map[string]interface{}{}, "content": map[string]interface{}{ "format": "user-defined", }, - "filter": map[string]interface{}{}, }, }, }, expectErr: false, expectWarn: true, - msg: "Support only splunk format", + msg: "valid log conf with warning filter field", }, } diff --git a/tests/data/dos/dos-logconf.yaml b/tests/data/dos/dos-logconf.yaml index 885a524647..688f3ea786 100644 --- a/tests/data/dos/dos-logconf.yaml +++ b/tests/data/dos/dos-logconf.yaml @@ -3,9 +3,6 @@ kind: APDosLogConf metadata: name: doslogconf spec: - content: - format: splunk - max_message_size: 64k filter: traffic-mitigation-stats: all bad-actors: top 10 diff --git a/tests/data/virtual-server-dos/dos-logconf.yaml b/tests/data/virtual-server-dos/dos-logconf.yaml index 885a524647..688f3ea786 100644 --- a/tests/data/virtual-server-dos/dos-logconf.yaml +++ b/tests/data/virtual-server-dos/dos-logconf.yaml @@ -3,9 +3,6 @@ kind: APDosLogConf metadata: name: doslogconf spec: - content: - format: splunk - max_message_size: 64k filter: traffic-mitigation-stats: all bad-actors: top 10 From daf1cc9190f1423bc3d189eae33a23b462c1da40 Mon Sep 17 00:00:00 2001 From: tomer pasman Date: Thu, 8 Sep 2022 13:29:08 +0300 Subject: [PATCH 08/12] Update helm crds --- .../helm-chart/crds/appprotectdos.f5.com_apdoslogconfs.yaml | 2 -- 1 file changed, 2 deletions(-) diff --git a/deployments/helm-chart/crds/appprotectdos.f5.com_apdoslogconfs.yaml b/deployments/helm-chart/crds/appprotectdos.f5.com_apdoslogconfs.yaml index f6e0f7110e..e23e87184b 100644 --- a/deployments/helm-chart/crds/appprotectdos.f5.com_apdoslogconfs.yaml +++ b/deployments/helm-chart/crds/appprotectdos.f5.com_apdoslogconfs.yaml @@ -38,13 +38,11 @@ spec: - splunk - arcsight - user-defined - default: splunk type: string format_string: type: string max_message_size: pattern: ^([1-9]|[1-5][0-9]|6[0-4])k$ - default: 5k type: string type: object filter: From 6d4a88b42d611b6a148d99ac8f471b671993b109 Mon Sep 17 00:00:00 2001 From: tomer pasman Date: Thu, 8 Sep 2022 14:08:58 +0300 Subject: [PATCH 09/12] Fix python testss --- .../app_protect_dos_configuration_test.go | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/internal/k8s/appprotectdos/app_protect_dos_configuration_test.go b/internal/k8s/appprotectdos/app_protect_dos_configuration_test.go index e8215c32d3..3138408957 100644 --- a/internal/k8s/appprotectdos/app_protect_dos_configuration_test.go +++ b/internal/k8s/appprotectdos/app_protect_dos_configuration_test.go @@ -99,19 +99,6 @@ func TestCreateAppProtectDosLogConfEx(t *testing.T) { wantErr: false, msg: "Valid DosLogConf with warning", }, - { - logConf: &unstructured.Unstructured{ - Object: map[string]interface{}{ - "spec": map[string]interface{}{}, - }, - }, - expectedLogConfEx: &DosLogConfEx{ - IsValid: true, - ErrorMsg: "Content field doesn't supported, use splunk format.", - }, - wantErr: false, - msg: "Valid DosLogConf with warning", - }, { logConf: &unstructured.Unstructured{ Object: map[string]interface{}{ From ebe00907612fe312cfa99713135b3eb5b47f4eff Mon Sep 17 00:00:00 2001 From: tomer pasman Date: Thu, 29 Sep 2022 09:38:36 +0300 Subject: [PATCH 10/12] Change warning msg --- .../k8s/appprotectdos/app_protect_dos_configuration_test.go | 2 +- pkg/apis/dos/validation/dos.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/k8s/appprotectdos/app_protect_dos_configuration_test.go b/internal/k8s/appprotectdos/app_protect_dos_configuration_test.go index 3138408957..caa85749ff 100644 --- a/internal/k8s/appprotectdos/app_protect_dos_configuration_test.go +++ b/internal/k8s/appprotectdos/app_protect_dos_configuration_test.go @@ -94,7 +94,7 @@ func TestCreateAppProtectDosLogConfEx(t *testing.T) { }, expectedLogConfEx: &DosLogConfEx{ IsValid: true, - ErrorMsg: "Content field doesn't supported, use splunk format.", + ErrorMsg: "the content field is not supported, defaulting to splunk format.", }, wantErr: false, msg: "Valid DosLogConf with warning", diff --git a/pkg/apis/dos/validation/dos.go b/pkg/apis/dos/validation/dos.go index b95b97db2f..e0b3b5fdaf 100644 --- a/pkg/apis/dos/validation/dos.go +++ b/pkg/apis/dos/validation/dos.go @@ -94,7 +94,7 @@ func checkAppProtectDosLogConfContentField(obj *unstructured.Unstructured) strin _, found, err := unstructured.NestedMap(obj.Object, "spec", "content") if err == nil && found { unstructured.RemoveNestedField(obj.Object, "spec", "content") - msg := "Content field doesn't supported, use splunk format." + msg := "the content field is not supported, defaulting to splunk format." return msg } From 027a75abf0869f387c289f1a8741bc3c07c38e30 Mon Sep 17 00:00:00 2001 From: Luca Comellini Date: Thu, 29 Sep 2022 08:52:26 -0700 Subject: [PATCH 11/12] Update pkg/apis/dos/validation/dos.go Co-authored-by: Jcahilltorre <78599298+Jcahilltorre@users.noreply.github.com> --- pkg/apis/dos/validation/dos.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/apis/dos/validation/dos.go b/pkg/apis/dos/validation/dos.go index e0b3b5fdaf..2cb51a244e 100644 --- a/pkg/apis/dos/validation/dos.go +++ b/pkg/apis/dos/validation/dos.go @@ -94,7 +94,7 @@ func checkAppProtectDosLogConfContentField(obj *unstructured.Unstructured) strin _, found, err := unstructured.NestedMap(obj.Object, "spec", "content") if err == nil && found { unstructured.RemoveNestedField(obj.Object, "spec", "content") - msg := "the content field is not supported, defaulting to splunk format." + msg := "the Content field is not supported, defaulting to splunk format." return msg } From 1b20368c94458e21ad8f08633e953c0db2682bec Mon Sep 17 00:00:00 2001 From: Luca Comellini Date: Thu, 29 Sep 2022 08:53:18 -0700 Subject: [PATCH 12/12] Update internal/k8s/appprotectdos/app_protect_dos_configuration_test.go --- .../k8s/appprotectdos/app_protect_dos_configuration_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/k8s/appprotectdos/app_protect_dos_configuration_test.go b/internal/k8s/appprotectdos/app_protect_dos_configuration_test.go index caa85749ff..8b66934763 100644 --- a/internal/k8s/appprotectdos/app_protect_dos_configuration_test.go +++ b/internal/k8s/appprotectdos/app_protect_dos_configuration_test.go @@ -94,7 +94,7 @@ func TestCreateAppProtectDosLogConfEx(t *testing.T) { }, expectedLogConfEx: &DosLogConfEx{ IsValid: true, - ErrorMsg: "the content field is not supported, defaulting to splunk format.", + ErrorMsg: "the Content field is not supported, defaulting to splunk format.", }, wantErr: false, msg: "Valid DosLogConf with warning",