-
Notifications
You must be signed in to change notification settings - Fork 8.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: discover mounted geoip db files #7228
Conversation
Hi @kd7lxl. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/ok-to-test |
a2e3fb1
to
78e690f
Compare
13069ec
to
e5a9d82
Compare
if nginx.MaxmindLicenseKey != "" || nginx.MaxmindMirror != "" { | ||
klog.InfoS("downloading maxmind GeoIP2 databases") | ||
if err = nginx.DownloadGeoLite2DB(nginx.MaxmindRetriesCount, nginx.MaxmindRetriesTimeout); err != nil { | ||
klog.ErrorS(err, "unexpected error downloading GeoIP2 database") |
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.
should we return a nil config here? , also shouldn't we log the errr inline #311
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.
To preserve existing behavior, errors are ignored and handled later by disabling geoip2 support. Returning a nil config would be a breaking change to any users expecting the existing behavior. My intent with this PR is to fix the feature without any breaking changes.
ingress-nginx/internal/ingress/controller/store/store.go
Lines 885 to 887 in 78afe7e
if s.backendConfig.UseGeoIP2 && !nginx.GeoLite2DBExists() { | |
klog.Warning("The GeoIP2 feature is enabled but the databases are missing. Disabling") | |
s.backendConfig.UseGeoIP2 = false |
I'm not sure why this behavior was chosen initially -- personally I would prefer it to return a nil config so it's easier for an operator to identify problems with the geoip2 download.
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.
Agreed, I reviewed master and realized it was the same.
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kd7lxl, strongjz The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
* fix: discover mounted geoip db files * add test * fix runtime reload of config.MaxmindEditionFiles * add e2e test * log missing geoip2 db
* fix: discover mounted geoip db files * add test * fix runtime reload of config.MaxmindEditionFiles * add e2e test * log missing geoip2 db
* fix: discover mounted geoip db files * add test * fix runtime reload of config.MaxmindEditionFiles * add e2e test * log missing geoip2 db
The docs say:
however, this was not possible because the config only iterated over downloaded geoip db files:
ingress-nginx/rootfs/etc/nginx/template/nginx.tmpl
Line 177 in 93070fa
MaxmindEditionFiles
is only set inDownloadGeoLite2DB()
:ingress-nginx/internal/nginx/maxmind.go
Lines 61 to 72 in 93070fa
What this PR does / why we need it:
This PR fixes #6012 by initializing
nginx.MaxmindEditionFiles
with the files in/etc/nginx/geoip
.Types of changes
Which issue/s this PR fixes
fixes #6012
How Has This Been Tested?
New e2e test case included in PR asserts that an existing GeoIP db file is loaded and results in the
geoip2
section being included in the config.Checklist: