Skip to content
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

Merged
merged 5 commits into from
Jul 9, 2021

Conversation

kd7lxl
Copy link
Contributor

@kd7lxl kd7lxl commented Jun 9, 2021

The docs say:

it is possible to use a volume to mount the files /etc/nginx/geoip/GeoLite2-City.mmdb and /etc/nginx/geoip/GeoLite2-ASN.mmdb, avoiding the overhead of the download.

however, this was not possible because the config only iterated over downloaded geoip db files:

{{ range $index, $file := $all.MaxmindEditionFiles }}

MaxmindEditionFiles is only set in DownloadGeoLite2DB():
// DownloadGeoLite2DB downloads the required databases by the
// GeoIP2 NGINX module using a license key from MaxMind.
func DownloadGeoLite2DB() error {
for _, dbName := range strings.Split(MaxmindEditionIDs, ",") {
err := downloadDatabase(dbName)
if err != nil {
return err
}
MaxmindEditionFiles = append(MaxmindEditionFiles, dbName+dbExtension)
}
return nil
}

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

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

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:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I've read the CONTRIBUTION guide
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@k8s-ci-robot k8s-ci-robot added do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 9, 2021
@k8s-ci-robot
Copy link
Contributor

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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jun 9, 2021
@kd7lxl kd7lxl changed the title discover mounted geoip db files. Fixes #6012 discover mounted geoip db files Jun 9, 2021
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Jun 9, 2021
@kd7lxl kd7lxl changed the title discover mounted geoip db files fix: discover mounted geoip db files Jun 9, 2021
@rikatz
Copy link
Contributor

rikatz commented Jul 1, 2021

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 1, 2021
@kd7lxl kd7lxl marked this pull request as draft July 6, 2021 17:25
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 6, 2021
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 6, 2021
@kd7lxl kd7lxl force-pushed the mounted-geoip branch 3 times, most recently from a2e3fb1 to 78e690f Compare July 6, 2021 18:48
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 6, 2021
@kd7lxl kd7lxl force-pushed the mounted-geoip branch 2 times, most recently from 13069ec to e5a9d82 Compare July 6, 2021 21:39
@kd7lxl kd7lxl marked this pull request as ready for review July 6, 2021 21:42
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 6, 2021
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")
Copy link
Member

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

Copy link
Contributor Author

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.

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.

Copy link
Member

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.

@strongjz
Copy link
Member

strongjz commented Jul 9, 2021

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 9, 2021
@k8s-ci-robot
Copy link
Contributor

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 9, 2021
@k8s-ci-robot k8s-ci-robot merged commit abf22b2 into kubernetes:master Jul 9, 2021
@kd7lxl kd7lxl deleted the mounted-geoip branch July 9, 2021 00:29
kd7lxl added a commit to kd7lxl/ingress-nginx that referenced this pull request Aug 9, 2021
* fix: discover mounted geoip db files

* add test

* fix runtime reload of config.MaxmindEditionFiles

* add e2e test

* log missing geoip2 db
k8s-ci-robot pushed a commit that referenced this pull request Aug 10, 2021
* fix: discover mounted geoip db files

* add test

* fix runtime reload of config.MaxmindEditionFiles

* add e2e test

* log missing geoip2 db
rchshld pushed a commit to joomcode/ingress-nginx that referenced this pull request May 19, 2023
* fix: discover mounted geoip db files

* add test

* fix runtime reload of config.MaxmindEditionFiles

* add e2e test

* log missing geoip2 db
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature: Allow using mounted MaxMind GeoIP databases
4 participants