Skip to content

Commit

Permalink
Apdoslogconf will warning case format is not splunk (#2991)
Browse files Browse the repository at this point in the history
  • Loading branch information
@pasmant
pasmant committed Sep 29, 2022
1 parent 2bd2646 commit 28b4a63
Show file tree
Hide file tree
Showing 12 changed files with 129 additions and 59 deletions.
2 changes: 0 additions & 2 deletions deployments/common/crds/appprotectdos.f5.com_apdoslogconfs.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
2 changes: 0 additions & 2 deletions deployments/helm-chart/crds/appprotectdos.f5.com_apdoslogconfs.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
12 changes: 3 additions & 9 deletions docs/content/app-protect-dos/configuration.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
}
}
```
Expand All @@ -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
Expand Down
3 changes: 0 additions & 3 deletions examples/custom-resources/app-protect-dos/apdos-logconf.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
3 changes: 0 additions & 3 deletions examples/ingress-resources/app-protect-dos/apdos-logconf.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
19 changes: 16 additions & 3 deletions internal/k8s/appprotectdos/app_protect_dos_configuration.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -114,11 +114,14 @@ func (ci *Configuration) AddOrUpdatePolicy(policyObj *unstructured.Unstructured)
resNsName := appprotectcommon.GetNsName(policyObj)
policy, err := createAppProtectDosPolicyEx(policyObj)

This comment has been minimized.

Sign in to view

Copy link
@jjngx

jjngx Sep 30, 2022

Contributor

what happens when the policy is nil here?
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)
Expand All @@ -134,11 +137,14 @@ func (ci *Configuration) AddOrUpdateLogConf(logConfObj *unstructured.Unstructure
resNsName := appprotectcommon.GetNsName(logConfObj)
logConf, err := createAppProtectDosLogConfEx(logConfObj)

This comment has been minimized.

Sign in to view

Copy link
@jjngx

jjngx Sep 30, 2022

Contributor

what happens when logConf is nil here and the func returns err?
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)
Expand Down Expand Up @@ -357,14 +363,21 @@ 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,
IsValid: false,
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,
Expand Down
45 changes: 37 additions & 8 deletions internal/k8s/appprotectdos/app_protect_dos_configuration_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,8 +70,7 @@ func TestCreateAppProtectDosLogConfEx(t *testing.T) {
logConf: &unstructured.Unstructured{
Object: map[string]interface{}{
"spec": map[string]interface{}{
"content": map[string]interface{}{},
"filter": map[string]interface{}{},
"filter": map[string]interface{}{},
},
},
},
Expand All @@ -86,10 +85,26 @@ 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: "the Content field is not supported, defaulting to 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",
Expand Down Expand Up @@ -211,6 +226,11 @@ func TestAddOrUpdateDosPolicy(t *testing.T) {
},
},
}

basicPolicyResource := &DosPolicyEx{
Obj: basicTestPolicy,
IsValid: true,
}
invalidTestPolicy := &unstructured.Unstructured{
Object: map[string]interface{}{
"metadata": map[string]interface{}{
Expand Down Expand Up @@ -245,6 +265,10 @@ func TestAddOrUpdateDosPolicy(t *testing.T) {
{
policy: basicTestPolicy,
expectedChanges: []Change{
{
Resource: basicPolicyResource,
Op: AddOrUpdate,
},
{
Resource: &DosProtectedResourceEx{
Obj: basicResource,
Expand Down Expand Up @@ -298,20 +322,21 @@ func TestAddOrUpdateDosLogConf(t *testing.T) {
"name": "testlogconf",
},
"spec": map[string]interface{}{
"content": map[string]interface{}{},
"filter": map[string]interface{}{},
"filter": map[string]interface{}{},
},
},
}
validLogConfResource := &DosLogConfEx{
Obj: validLogConf,
IsValid: true,
}
invalidLogConf := &unstructured.Unstructured{
Object: map[string]interface{}{
"metadata": map[string]interface{}{
"namespace": "testing",
"name": "invalid-logconf",
},
"spec": map[string]interface{}{
"content": map[string]interface{}{},
},
"spec": map[string]interface{}{},
},
}
basicResource := &v1beta1.DosProtectedResource{
Expand Down Expand Up @@ -345,6 +370,10 @@ func TestAddOrUpdateDosLogConf(t *testing.T) {
{
logconf: validLogConf,
expectedChanges: []Change{
{
Resource: validLogConfResource,
Op: AddOrUpdate,
},
{
Resource: &DosProtectedResourceEx{
Obj: basicResource,
Expand Down
13 changes: 13 additions & 0 deletions internal/k8s/controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1506,6 +1506,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 := "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) {
Expand Down
20 changes: 16 additions & 4 deletions pkg/apis/dos/validation/dos.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ var appProtectDosPolicyRequiredFields = [][]string{
}

var appProtectDosLogConfRequiredFields = [][]string{
{"spec", "content"},
{"spec", "filter"},
}

Expand Down Expand Up @@ -90,15 +89,28 @@ func validateResourceReference(ref string) error {
return nil
}

// checkAppProtectDosLogConfContentField check conetent field doesnt appear in dos log
func checkAppProtectDosLogConfContentField(obj *unstructured.Unstructured) string {
_, found, err := unstructured.NestedMap(obj.Object, "spec", "content")

This comment has been minimized.

Sign in to view

Copy link
@jjngx

jjngx Sep 30, 2022 •
edited

Contributor

what happens when err is not nil here? it may be worth to consider handling error and continue with a happy flow that is not indented.
if err == nil && found {
unstructured.RemoveNestedField(obj.Object, "spec", "content")
msg := "the Content field is not supported, defaulting to splunk format."
return msg
}

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 := checkAppProtectDosLogConfContentField(logConf)

return nil
return warning, nil
}

var (
Expand Down
63 changes: 44 additions & 19 deletions pkg/apis/dos/validation/dos_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -240,66 +240,91 @@ func TestValidateAppProtectDosAccessLogDest(t *testing.T) {
func TestValidateAppProtectDosLogConf(t *testing.T) {
t.Parallel()
tests := []struct {
logConf *unstructured.Unstructured
expectErr bool
msg string
logConf *unstructured.Unstructured
expectErr bool
expectWarn bool
msg string
}{
{
logConf: &unstructured.Unstructured{
Object: map[string]interface{}{
"spec": map[string]interface{}{
"content": map[string]interface{}{},
"filter": map[string]interface{}{},
"filter": map[string]interface{}{},
},
},
},
expectErr: false,
msg: "valid log conf",
expectErr: false,
expectWarn: false,
msg: "valid log conf",
},
{
logConf: &unstructured.Unstructured{
Object: map[string]interface{}{
"spec": map[string]interface{}{
"spec": map[string]interface{}{},
},
},
expectErr: true,
expectWarn: false,
msg: "invalid log conf with no filter field",
},
{
logConf: &unstructured.Unstructured{
Object: map[string]interface{}{
"something": map[string]interface{}{
"filter": map[string]interface{}{},
},
},
},
expectErr: true,
msg: "invalid log conf with no content field",
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{}{},
"content": map[string]interface{}{
"format": "user-defined",
},
"filter": map[string]interface{}{},
},
},
},
expectErr: true,
msg: "invalid log conf with no filter field",
expectErr: false,
expectWarn: true,
msg: "Support only splunk format",
},
{
logConf: &unstructured.Unstructured{
Object: map[string]interface{}{
"something": map[string]interface{}{
"content": map[string]interface{}{},
"filter": map[string]interface{}{},
"spec": map[string]interface{}{
"filter": map[string]interface{}{},
"content": map[string]interface{}{
"format": "user-defined",
},
},
},
},
expectErr: true,
msg: "invalid log conf with no spec field",
expectErr: false,
expectWarn: true,
msg: "valid log conf with warning filter field",
},
}

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)
}
}
}

Expand Down

0 comments on commit 28b4a63

Please sign in to comment.