-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Support multiple log security configs in Ingresses #1513
Conversation
internal/k8s/controller.go
Outdated
@@ -2050,7 +2050,7 @@ func (lbc *LoadBalancerController) createIngressEx(ing *networking.Ingress, vali | |||
if apLogConfAntn, exists := ingEx.Ingress.Annotations[configs.AppProtectLogConfAnnotation]; exists { | |||
logConf, logDst, err := lbc.getAppProtectLogConfAndDst(ing) | |||
if err != nil { | |||
glog.Warningf("Error Getting App Protect policy %v for Ingress %v/%v: %v", apLogConfAntn, ing.Namespace, ing.Name, err) | |||
glog.Warningf("Error Getting App Protect Log Comnfig %v for Ingress %v/%v: %v", apLogConfAntn, ing.Namespace, ing.Name, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
glog.Warningf("Error Getting App Protect Log Comnfig %v for Ingress %v/%v: %v", apLogConfAntn, ing.Namespace, ing.Name, err) | |
glog.Warningf("Error Getting App Protect Log Config %v for Ingress %v/%v: %v", apLogConfAntn, ing.Namespace, ing.Name, err) |
@@ -131,6 +131,15 @@ func ParseResourceReferenceAnnotation(ns, antn string) string { | |||
return antn | |||
} | |||
|
|||
// ParseResourceReferenceAnnotationList returns a slice of ns/names strings | |||
func ParseResourceReferenceAnnotationList(ns, antns string) []string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optional: Maybe this should be just called annotations
? For a moment I understand antns
to mean annotationNamespace
func ParseResourceReferenceAnnotationList(ns, antns string) []string { | |
func ParseResourceReferenceAnnotationList(ns, annotations string) []string { |
func ParseResourceReferenceAnnotationList(ns, antns string) []string { | |
func ParseResourceReferenceAnnotationList(ns, antnRefs string) []string { |
Feel free to ignore if you think it's clear.
36609d1
to
0bf0a0a
Compare
internal/k8s/controller.go
Outdated
logDsts = strings.Split(ing.Annotations[configs.AppProtectLogConfDstAnnotation], ",") | ||
logConfNsNs := appprotect.ParseResourceReferenceAnnotationList(ing.Namespace, ing.Annotations[configs.AppProtectLogConfAnnotation]) | ||
if len(logDsts) != len(logConfNsNs) { | ||
return nil, []string{""}, fmt.Errorf("Error Validating App Protect Destination and config for Ingress %v: LogConf and LogDestination must be equal length", ing.Name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we have an empty string in this return value ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to @soneillf5 why not return nil?
internal/configs/configurator.go
Outdated
for i, logConf := range ingEx.AppProtectLogConf { | ||
logConfFileName := appProtectLogConfFileNameFromUnstruct(logConf) | ||
logConfContent := generateApResourceFileContent(logConf) | ||
cnf.nginxManager.CreateAppProtectResourceFile(logConfFileName, logConfContent) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just looked into this function, I'm surprised it doesn't return an error. @pleshakov is there any reason the manager doesn't return errors from most of it's functions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@soneillf5 those errors are file IO related and treated as unrecoverable to prevent NGINX from diverging from the desired configuration.
if pol == namespace+"/"+name || (namespace == ing.Namespace && pol == name) { | ||
return true | ||
if resName, exists := ing.Annotations[rc.annotation]; exists { | ||
resNames := strings.Split(resName, ",") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need to split and iterate? Would a string.Contains(resName, namespace+"/"+name)
work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the second case when the namespace is assumed to be the namespace of the Ingress resource makes things difficult cause it could be "name1,name2" or "ns1/name1,ns2/name2" or "name1,ns2/name2"
Could you add additional test cases for the new code in TestAppProtectResourceIsReferencedByIngresses ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rafwegv please see my comments and suggestions.
Additionally, could we update the docs about changed annotations https://docs.nginx.com/nginx-ingress-controller/configuration/ingress-resources/advanced-configuration-with-annotations/#app-protect ?
Also, do we need to update WAF policy as well https://docs.nginx.com/nginx-ingress-controller/configuration/policy-resource/#waf ? Perhaps in a different PR? I suggest sharing the changes to the fields before starting implementing so that we all agree on the new configuration.
internal/k8s/controller.go
Outdated
logDsts = strings.Split(ing.Annotations[configs.AppProtectLogConfDstAnnotation], ",") | ||
logConfNsNs := appprotect.ParseResourceReferenceAnnotationList(ing.Namespace, ing.Annotations[configs.AppProtectLogConfAnnotation]) | ||
if len(logDsts) != len(logConfNsNs) { | ||
return nil, []string{""}, fmt.Errorf("Error Validating App Protect Destination and config for Ingress %v: LogConf and LogDestination must be equal length", ing.Name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to @soneillf5 why not return nil?
if pol == namespace+"/"+name || (namespace == ing.Namespace && pol == name) { | ||
return true | ||
if resName, exists := ing.Annotations[rc.annotation]; exists { | ||
resNames := strings.Split(resName, ",") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the second case when the namespace is assumed to be the namespace of the Ingress resource makes things difficult cause it could be "name1,name2" or "ns1/name1,ns2/name2" or "name1,ns2/name2"
Could you add additional test cases for the new code in TestAppProtectResourceIsReferencedByIngresses ?
# both lists need to be the same length, if one of the referenced configs is invalid/non-existent then no logconfs are applied. | ||
doc["metadata"]["annotations"][ | ||
"appprotect.f5.com/app-protect-security-log" | ||
] = f"{test_namespace}/logconf,{test_namespace}/logconf" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vepatel Perhaps we should also test that implicit namespace works?
] = f"{test_namespace}/logconf,{test_namespace}/logconf" | |
] = f"{test_namespace}/logconf,logconf" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but I've deployed test resources in a separate namespace here so need to reference by the same as nothing related to test goes to default ns.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @rafwegv
the changes look good! just a few small suggestions.
Additionally, could we update the docs about changed annotations https://docs.nginx.com/nginx-ingress-controller/configuration/ingress-resources/advanced-configuration-with-annotations/#app-protect ?
are you planning to update docs in a separate PR?
AppProtectLogconfs []string | ||
} | ||
|
||
type AppProtectLog struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
neat: could you add a doc string for consistency? since the type is exported
internal/k8s/controller.go
Outdated
@@ -2160,27 +2159,38 @@ func (lbc *LoadBalancerController) createIngressEx(ing *networking.Ingress, vali | |||
return ingEx | |||
} | |||
|
|||
func (lbc *LoadBalancerController) getAppProtectLogConfAndDst(ing *networking.Ingress) (logConf *unstructured.Unstructured, logDst string, err error) { | |||
logConfNsN := appprotect.ParseResourceReferenceAnnotation(ing.Namespace, ing.Annotations[configs.AppProtectLogConfAnnotation]) | |||
func (lbc *LoadBalancerController) getAppProtectLogConfAndDst(ing *networking.Ingress) (apLog []configs.AppProtectLog, err error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because we use err
variable throughout the code in the method body and without any intention to return its value, I suggest dropping the names in (apLog []configs.AppProtectLog, err error)
-> ([]configs.AppProtectLog, error)
and inside the body, simply declare var apLog []configs.AppProtectLog
. And a small suggestion, because we have a slice, better to use plural apLogs
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
Proposed changes
Will be introduced in NAP 3.2
Support multiple log destinations and formats. Since we split log config into two annotations, allowing admins to setup multiple log destination is implemented as a coma separated value to the existing log conf annotations, where a config corresponds to the same index of the destination annotation.
for example:
appprotect.f5.com/app-protect-security-log: "default/logconf,default/logconf_splunk"
appprotect.f5.com/app-protect-security-log-destination: "syslog:server=127.0.0.1:514,syslog:server=192.168.1.1:514"
Both lists need to be the same length, if one of the referenced configs is ivalid/nonexistent no logconfs are aplied.
Checklist
Before creating a PR, run through this checklist and mark each as complete.