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

Usage of GeoIP2 variables CRASH the ingress in case maxmind-license is out of download quota (or download fails) #11457

Closed
ThorstenEngel opened this issue Jun 12, 2024 · 9 comments
Labels
needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-priority needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one.

Comments

@ThorstenEngel
Copy link

Hi, I use v1.10.1 of the ingress and have GeoIP2 enabled. If I encounter too many reloads (in my case I was upgrading my EKS-Cluster and the ingress), the database download fails. In consequence, usage of GeoIP2 variables (I use them for populating upstream-headers) will lead to an error.

Excerpt from the log:

I0612 17:36:03.967097       7 flags.go:387] "downloading maxmind GeoIP2 databases" 
...

E0612 17:36:04.901483       7 maxmind.go:74] GeoLite2-City.mmdb not found 
W0612 17:36:04.901505       7 store.go:1214] The GeoIP2 feature is enabled but the databases are missing. Disabling 
...
E0612 17:36:06.374855       7 controller.go:205] Unexpected failure reloading the backend: 
 
------------------------------------------------------------------------------- 
Error: exit status 1 
2024/06/12 17:36:06 [emerg] 25#25: unknown "geoip2_city" variable 
nginx: [emerg] unknown "geoip2_city" variable 
 
2T17:36:06.375058138Z nginx: configuration file /tmp/nginx/nginx-cfg1060362931 test failed 
...

When the GeoIP2 database download fails, it causes the ingress to disable the feature, leading to errors due to the missing variables. A viable fix for this case (enabled GeoIP2 feature, missing databases lead to disabled GeoIP2 feature) could be default values like empty strings.

Here is my ConfigMap:

kind: ConfigMap
apiVersion: v1
metadata:
  name: upstream-custom-headers
  namespace: ingress-nginx
  labels:
    app.kubernetes.io/name: ingress-nginx
    app.kubernetes.io/part-of: ingress-nginx
data:
  X-Correlation-ID: $request_id
  X-GeoIP-Country-Code: $geoip2_city_country_code
  X-GeoIP-Country-Name: $geoip2_city_country_name
  X-GeoIP-Region: $geoip2_region_name
  X-GeoIP-City: $geoip2_city
  X-GeoIP-Postal-Code: $geoip2_postal_code
  X-GeoIP-Latitude: $geoip2_latitude
  X-GeoIP-Longitude: $geoip2_longitude

It would be great, if you guys could address this. I currently must disable the GeoIP2 feature, as using it might crash my ingresses.

Warm regards,

thorsten

@ThorstenEngel ThorstenEngel added the kind/bug Categorizes issue or PR as related to a bug. label Jun 12, 2024
@k8s-ci-robot k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Jun 12, 2024
@k8s-ci-robot
Copy link
Contributor

This issue is currently awaiting triage.

If Ingress contributors determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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-sigs/prow repository.

@longwuyuan
Copy link
Contributor

/remove-kind bug

I was recently commenting on a PR related to maxmind and I happened to put the db's tar.gz, in the webroot of a nginx:alpine image based pod, that was adjacent in the same namespace as the controller. Then I could use a existing flag for the controller executable like "--maxmind-mirror=http://adjacentpod" that way I download only once from maxmind and serve it perpetually from that adjacent pod.

#11435 (comment)

Wondering this flag fits as a solution for the problem you mentioned
image

@k8s-ci-robot k8s-ci-robot added needs-kind Indicates a PR lacks a `kind/foo` label and requires one. and removed kind/bug Categorizes issue or PR as related to a bug. labels Jun 12, 2024
@longwuyuan
Copy link
Contributor

Also wondering about the fit of these in your problem https://github.com/ffha/geolite-mirror & https://github.com/clashdev/geolite.clash.dev

@ThorstenEngel
Copy link
Author

I think, the ingress-nginx really belongs to the most critical building blocks in a k8s-deployment. So I think, it should be as resilient as possible. A hard dependency of an external source introduces a weakness - which in my eyes should be avoided if possible.

Yes, these solutions are a workaround, but add additional complexity. Thanks for the tip!

@longwuyuan
Copy link
Contributor

Thanks @ThorstenEngel for your update. I agree the controller should be resilient. The default values like you suggested help with the Geoip2 use case.

I am not a developer. My opinion is different though. Download fails based change in the project code, requires maintenance, that is impossible with available resources. It also sets a undesired precedence for other features of the controller, to expect similar granular out-of-scope code.

But please wait for others to comment.

Copy link

This is stale, but we won't close it automatically, just bare in mind the maintainers may be busy with other tasks and will reach your issue ASAP. If you have any question or request to prioritize this, please reach #ingress-nginx-dev on Kubernetes Slack.

@github-actions github-actions bot added the lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. label Jul 14, 2024
@matteovivona
Copy link

@ThorstenEngel I don't know if it helps, but I chose to download the database from a secondary source suggested from @longwuyuan, and mount the mmdb with an initContainer

By installing ingress-nginx from Helm I created this initContainer

  extraInitContainers:
    - name: download-geolite2-country
      image: busybox:1.36
      command: ['sh', '-c', 'wget https://git.io/GeoLite2-Country.mmdb --no-check-certificate && mv GeoLite2-Country.mmdb /home/GeoLite2-Country.mmdb']
      volumeMounts:
        - name: geolite2-country
          mountPath: /home

then a volume and volumemounts

  extraVolumes:
    - name: geolite2-country
      emptyDir: {}
  extraVolumeMounts:
    - name: geolite2-country
      mountPath: /etc/ingress-controller/geoip/GeoLite2-Country.mmdb
      subPath: GeoLite2-Country.mmdb

on extraArgs I pass only this option maxmind-edition-ids: GeoLite2-Country without the MaxMind licenses

@github-actions github-actions bot removed the lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. label Jul 23, 2024
@ThorstenEngel
Copy link
Author

Yes, this would help. I personally switched to AWS Cloudfront headers (they do GeoIP Coding as well, and we use it anyqay). So I have 1 component less to take care of.

@longwuyuan
Copy link
Contributor

@ThorstenEngel can we close this issue ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-priority needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one.
Projects
Development

No branches or pull requests

4 participants