Added maxmindMirror value to helm chart

[lgyurci]

What this PR does / why we need it:

The option to set a mirror for the maxmind geoip databases is already in the code, the option is only missing from the helm chart. This PR makes a maxmind mirror configurable from helm values.

Types of changes

• [?] Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• CVE Report (Scanner found CVE and adding report)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation only

How Has This Been Tested?

Ran helm chart locally, it installed, and the controller tried to use the mirror I provided in the values properly

Checklist:

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

[k8s-ci-robot]

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.

[k8s-ci-robot]

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

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: lgyurci
Once this PR has been reviewed and has the lgtm label, please assign ubergesundheit for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• charts/ingress-nginx/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[netlify]

✅ Deploy Preview for kubernetes-ingress-nginx canceled.

Name	Link
🔨 Latest commit	79e00ef
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-ingress-nginx/deploys/666327e55f50be0008dbbe9a

[longwuyuan]

@lgyurci what is the link to the code for "option to set a mirror for the maxmind geoip databases"

[lgyurci]

ingress-nginx/internal/nginx/maxmind.go

Lines 135 to 138 in ac9e40a

	if mirror != "" {
	return fmt.Sprintf("%s/%s.tar.gz", mirror, dbName)
	}
	return fmt.Sprintf(maxmindURL, licenseKey, dbName)

@longwuyuan Are you thinking about this?

[longwuyuan]

@lgyurci, not sure if you are asking or telling. I am just trying to verify your issue description

The option to set a mirror for the maxmind geoip databases is already in the code, 

[lgyurci]

@longwuyuan

ingress-nginx/pkg/flags/flags.go

Line 233 in ac9e40a

flags.StringVar(&nginx.MaxmindMirror, "maxmind-mirror", "", `Maxmind mirror url (example: http://geoip.local/databases.`)

and

ingress-nginx/internal/nginx/maxmind.go

Lines 135 to 138 in ac9e40a

	if mirror != "" {
	return fmt.Sprintf("%s/%s.tar.gz", mirror, dbName)
	}
	return fmt.Sprintf(maxmindURL, licenseKey, dbName)

This parameter (--maxmind-mirror) is also documented:

ingress-nginx/docs/user-guide/cli-arguments.md

Line 47 in ac9e40a

| `--maxmind-mirror` | Maxmind mirror url (example: http://geoip.local/databases. |

[longwuyuan]

If you are trying to set a optional cli-argument flag to the controller executable, then the current method everybody uses is the key extraArgs , in the values file

ingress-nginx/charts/ingress-nginx/values.yaml

Line 201 in ac9e40a

extraArgs: {}

So that will look like helm install .......... --set controller.extraArgs.maxmind-mirror=$mirrorURL

What is the need to create a unnecessary duplicate and redundant process to add optional cli-flag to the executable, via helm chart values file ?

[longwuyuan]

@lgyurci I am not able to test your change. Can you see below info and advice what I am doing wrong

[~/Documents/ingressnginxwork/11435/ingress-nginx] main
% git remote -v
origin  git@github.com:lgyurci/ingress-nginx.git (fetch)
origin  git@github.com:lgyurci/ingress-nginx.git (push)
[~/Documents/ingressnginxwork/11435/ingress-nginx] main
% 

% helm -n ingress-nginx install ingress-nginx ./ingress-nginx/charts/ingress-nginx -f ./values.yaml --create-namespace

% cat values.yaml 
controller:
  config:
    use-geoip2: true
  maxmindLicenseKey: $mylicence
  maxmindMirror: "https://download.maxmind.com/geoip/databases/GeoLite2-City/download?suffix=tar.gz"
  service:
    externalTrafficPolicy: Local
[~/Documents/ingressnginxwork/11435] 


% helm ls -A
NAME            NAMESPACE       REVISION        UPDATED                                 STATUS          CHART                   APP VERSION
ingress-nginx   ingress-nginx   1               2024-06-08 21:27:33.814858241 +0530 IST deployed        ingress-nginx-4.10.1    1.10.1  
%

% helm -n ingress-nginx get values ingress-nginx 
USER-SUPPLIED VALUES:
controller:
  config:
    use-geoip2: true
  maxmindLicenseKey: $mylicense
  maxmindMirror: https://download.maxmind.com/geoip/databases/GeoLite2-ASN/download?suffix=tar.gz
  service:
    externalTrafficPolicy: Local
[~/Documents/ingressnginxwork/11435/ingress-nginx] main



% k -n ingress-nginx describe cm ingress-nginx-controller 
Name:         ingress-nginx-controller
Namespace:    ingress-nginx
Labels:       app.kubernetes.io/component=controller
              app.kubernetes.io/instance=ingress-nginx
              app.kubernetes.io/managed-by=Helm
              app.kubernetes.io/name=ingress-nginx
              app.kubernetes.io/part-of=ingress-nginx
              app.kubernetes.io/version=1.10.1
              helm.sh/chart=ingress-nginx-4.10.1
Annotations:  meta.helm.sh/release-name: ingress-nginx
              meta.helm.sh/release-namespace: ingress-nginx

Data
====
allow-snippet-annotations:
----
false
use-geoip2:
----
true

BinaryData
====

Events:
  Type    Reason  Age   From                      Message
  ----    ------  ----  ----                      -------
  Normal  CREATE  12m   nginx-ingress-controller  ConfigMap ingress-nginx/ingress-nginx-controller
[~/Documents/ingressnginxwork/11435/ingress-nginx] main

% k -n ingress-nginx exec ingress-nginx-controller-5d7b7589d9-nn4k5 -- cat /etc/nginx/nginx.conf > ngin
x.conf
[~/Documents/ingressnginxwork/11435] 
% ls -l
total 20
drwxr-xr-x. 1 me me   918 Jun  8 11:14 ingress-nginx
-rw-r--r--. 1 me me 13757 Jun  8 21:44 nginx.conf
-rw-r--r--. 1 me me   249 Jun  8 21:26 values.yaml
[~/Documents/ingressnginxwork/11435] 
% grep -i geoip nginx.conf 
[~/Documents/ingressnginxwork/11435] 

I can not find any geoip config in nginx.conf. I got the mirrorURL from the maxmind website as a permalink
nginx.conf.txt

Now I am going to try without the mirrorURL and update

[longwuyuan]

ok, without the controller.maxmindMirror key, i see geoip2 config. But if install the chart from a clone of your fork and use the controller.maxmindMirror key, then I don't see any geoip config.

I got a permalink URL from the maxmind website to use as the mirror URL

[lgyurci]

Alright, so my guess is, that you put the mirror url in the wrong format (you actually used the full path to the GeoLite2-ASN database, with parameters - and thats the controllers job). Secondly, with a maxmind mirror, you shouldn't use a license key, because (normally) an internal mirror shouldn't require one. So you actually can't test this functionality with MaxMind itself as its own mirror, you have to set one up locally. But all of this might be irrevelant, because:

I actually overlooked the extraArgs section, and I wasn't aware that you can pass arbitrary arguments to the controller from the helm chart. Thank you for shedding light on that. I do not wish to argue about the necessity of this pull request, feel free to close this if you think putting this option to a more easily reachable area is not that important. And thanks again for looking into this!

[longwuyuan]

@lgyurci thanks for the update as it helps a lot in testing.

I just now tried https://github.com/ffha/geolite-mirror & https://github.com/clashdev/geolite.clash.dev (found them on google search) . Still the logs say download is attempted but db not found. See below

-------------------------------------------------------------------------------
NGINX Ingress controller                           
  Release:       v1.10.1                           
  Build:         4fb5aac1dd3669daa3a14d9de3e3cdb371b4c518
  Repository:    https://github.com/kubernetes/ingress-nginx
  nginx version: nginx/1.25.3

-------------------------------------------------------------------------------

I0608 18:16:20.101703      13 flags.go:387] "downloading maxmind GeoIP2 databases"
W0608 18:16:21.055052      13 client_config.go:618] Neither --kubeconfig nor --master was specified.  Using the inClusterConfig.  This might not work.
I0608 18:16:21.055217      13 main.go:205] "Creating API client" host="https://10.96.0.1:443"
I0608 18:16:21.059971      13 main.go:248] "Running in Kubernetes cluster" major="1" minor="30" git="v1.30.0" state="clean" commit="7c48c2bd72b9bf5c44d21d7338cc7bea77d0ad2a" platform="linux/amd64"
I0608 18:16:21.377326      13 main.go:101] "SSL fake certificate created" file="/etc/ingress-controller/ssl/default-fake-certificate.pem"
I0608 18:16:21.391920      13 ssl.go:535] "loading tls certificate" path="/usr/local/certificates/cert" key="/usr/local/certificates/key"
E0608 18:16:21.395614      13 maxmind.go:74] GeoLite2-City.mmdb not found
W0608 18:16:21.395631      13 store.go:1214] The GeoIP2 feature is enabled but the databases are missing. Disabling
I0608 18:16:21.399554      13 nginx.go:264] "Starting NGINX Ingress controller"
I0608 18:16:21.404645      13 event.go:364] Event(v1.ObjectReference{Kind:"ConfigMap", Namespace:"ingress-nginx", Name:"ingress-nginx-controller", UID:"db469f89-6f94-40f5-8176-388d094faee0", APIVersion:"v1",
 ResourceVersion:"3806", FieldPath:""}): type: 'Normal' reason: 'CREATE' ConfigMap ingress-nginx/ingress-nginx-controller
E0608 18:16:21.406117      13 maxmind.go:74] GeoLite2-City.mmdb not found
W0608 18:16:21.406131      13 store.go:1214] The GeoIP2 feature is enabled but the databases are missing. Disabling
I0608 18:16:22.601032      13 nginx.go:307] "Starting NGINX process"

The long story is that there was another issue on geoip2 and the root-cause was user configuring deprecated variable. So feedbacks like this are the best source of improvements.

Now, even as I was typing this update, I tried out this values file (without Licence)

[~/Documents/ingressnginxwork/11435] 
% cat values.yaml 
controller:
  config:
    use-geoip2: true
  maxmindMirror: "https://geolite.clash.dev"
  service:
    externalTrafficPolicy: Local
[~/Documents/ingressnginxwork/11435] 

And I still get that same error of db not found ;

I0608 18:24:02.152319      13 flags.go:387] "downloading maxmind GeoIP2 databases"
W0608 18:24:02.983731      13 client_config.go:618] Neither --kubeconfig nor --master was specified.  Using the inClusterConfig.  This might not work.
I0608 18:24:02.983999      13 main.go:205] "Creating API client" host="https://10.96.0.1:443"
I0608 18:24:02.989298      13 main.go:248] "Running in Kubernetes cluster" major="1" minor="30" git="v1.30.0" state="clean" commit="7c48c2bd72b9bf5c44d21d7338cc7bea77d0ad2a" platform="linux/amd64"
I0608 18:24:03.275413      13 main.go:101] "SSL fake certificate created" file="/etc/ingress-controller/ssl/default-fake-certificate.pem"
I0608 18:24:03.288059      13 ssl.go:535] "loading tls certificate" path="/usr/local/certificates/cert" key="/usr/local/certificates/key"
E0608 18:24:03.292635      13 maxmind.go:74] GeoLite2-City.mmdb not found
W0608 18:24:03.292655      13 store.go:1214] The GeoIP2 feature is enabled but the databases are missing. Disabling

So the idea is to fill the PR with related info as iterations are hard to come by.

If you are interested, a docs PR also would be most welcome. I practically was able to test only after your kind co-operation over multiple messages.

As for the new key in values file, this testing shows, I can use the extraArgs key if its about using a option flag to the controller executable. But there is no data in tests or docs on why there is a error for db not found, even though the flag is provided to the controller executable as an argument and the download was attempted.

I am not a developer so maybe you can choose to use the extraArgs key and close the PR yourself. Or --force --amend your git commits to improve docs instead. Or post test-cases and your own test-data that shows improvement of the chart, with the new key in values file, over the extraArgs key.

There is lack of contributors so any improvements contributed are super appreciated.

Thanks again .

[longwuyuan]

The challenge is that even though the last test is a accurate spec to the related func()

and the db is recent

% tar -ztvf GeoLite2-City.tar.gz 
drwxrwxr-x 0/0               0 2024-06-07 21:19 GeoLite2-City_20240607/
-rw-r--r-- 0/0             398 2024-06-07 21:19 GeoLite2-City_20240607/LICENSE.txt
-rw-r--r-- 0/0              55 2024-06-07 21:19 GeoLite2-City_20240607/COPYRIGHT.txt
-rw-r--r-- 0/0        50097859 2024-06-07 21:19 GeoLite2-City_20240607/GeoLite2-City.mmdb
-rw-r--r-- 0/0             116 2024-06-07 21:19 GeoLite2-City_20240607/README.txt

there is no reason stated for not finding the DB.

Maybe verbose logging is needed but I doubt it will explain the reason for not finding db, unless controller its run live in debug mode

[lgyurci]

Huhh this is strange. I'll have a deeper look at this tomorrow, or monday, and keep you updated! (If you don't figure it out until then)

I have a strong feeling that there is a bug here (in the code probably), so an issue would be nice about this. I'll run some tests locally to see what's wrong.

[lgyurci]

Ok so I couldn't wait until tomorrow, and I tested with the mirrors you provided, and also added some debug prints to the controller code to see actually what's happening:

I0608 22:05:59.348986       7 flags.go:399] "downloading maxmind GeoIP2 databases"
I0608 22:05:59.349170       7 maxmind.go:138] "d-1 " https://geolite.090124.xyz/GeoLite2-City.tar.gz="(MISSING)"
I0608 22:05:59.349196       7 maxmind.go:147] "d0 "
I0608 22:05:59.349226       7 maxmind.go:153] "d1 "
I0608 22:05:59.458084       7 maxmind.go:160] "d2 "
I0608 22:05:59.458168       7 maxmind.go:165] "d31 " 403 Forbidden="(MISSING)"
W0608 22:05:59.458479       7 client_config.go:659] Neither --kubeconfig nor --master was specified.  Using the inClusterConfig.  This might not work.
I0608 22:05:59.458830       7 main.go:205] "Creating API client" host="https://10.185.64.1:443"
I0608 22:05:59.467623       7 main.go:248] "Running in Kubernetes cluster" major="1" minor="26" git="v1.26.15+rke2r1" state="clean" commit="1649f592f1909b97aa3c2a0a8f968a3fd05a7b8b" platform="linux/amd64"

So actually the controller gets a 403 from geolite.090124.xyz (and the same goes for geolite.clash.dev, but that even has a cloudflare captcha) while trying to download the databases, there is probably some user-agent, or other L7 filtering on the site. This 403 is not logged normally for some reason. Please ignore the ="(MISSING)" messages at the end of the lines, I'm not a Go developer.

So my guess would be, that your controller simply just can't reach these sites. I could set up a temporary public mirror for you to try this, if you'd like to.

[longwuyuan]

Thanks again for the update. It helped solve the problem.

I basically minimal-ized the heck out of that mirror URL and the controller successfully configured geoip2.

• I created a deployment from image nginx:alpine using k -n ingress-nginx create deploy maxmindmirror --image nginx:pine --port 80 and got a pod running with nginx webserver in it
  • I created a service of --type ClusterIP for the maxmindmirror deployment with k -n ingress-nginx expose deploy maxmindmirror --port 80

% k -n ingress-nginx get po maxmindmirror-5b69c476cf-29r55 
NAME                             READY   STATUS    RESTARTS   AGE
maxmindmirror-5b69c476cf-29r55   1/1     Running   0          28m
[~/Documents/ingressnginxwork/11435] 
% k -n ingress-nginx get svc maxmindmirror                
NAME            TYPE        CLUSTER-IP     EXTERNAL-IP   PORT(S)   AGE
maxmindmirror   ClusterIP   10.96.96.242   <none>        80/TCP    28m

• So now I can curl a webserver URL from inside the controller pod at URL http://maxmindmirror

/etc/nginx $ hostname 
ingress-nginx-controller-5b88864c5c-tx46x
/etc/nginx $ date
Sun Jun  9 00:11:21 UTC 2024
/etc/nginx $ ping -c 1 maxmindmirror
PING maxmindmirror (10.96.96.242): 56 data bytes
ping: permission denied (are you root?)
/etc/nginx $ curl -I maxmindmirror
HTTP/1.1 200 OK
Server: nginx/1.27.0
Date: Sun, 09 Jun 2024 00:11:49 GMT
Content-Type: text/html
Content-Length: 615
Last-Modified: Wed, 29 May 2024 17:56:56 GMT
Connection: keep-alive
ETag: "66576c68-267"
Accept-Ranges: bytes

/etc/nginx $ 

• Then I downloaded and put the tar.gz files in the webroot of this tiny nginx webserver pod

/ # hostname 
maxmindmirror-5b69c476cf-29r55
/ # date 
Sun Jun  9 00:15:55 UTC 2024
/ # netstat -lntp
Active Internet connections (only servers)
Proto Recv-Q Send-Q Local Address           Foreign Address         State       PID/Program name    
tcp        0      0 0.0.0.0:80              0.0.0.0:*               LISTEN      1/nginx: master pro
netstat: /proc/net/tcp6: No such file or directory
/ # ls -l /usr/share/nginx/html/
total 28344
-rw-r--r--    1 root     root           497 May 29 17:56 50x.html
-rw-r--r--    1 root     root       4727004 Jun  9 00:01 GeoLite2-ASN.tar.gz
-rw-r--r--    1 root     root      24283829 Jun  8 23:54 GeoLite2-City.tar.gz
-rw-r--r--    1 root     root           615 May 29 17:56 index.html
drwxr-xr-x    1 root     root             0 Jun  9 00:01 tmp
/ # 

• Now I set the maxmind-mirror URL in the values file to http://maxmindmirror and geoip2 module was configured

So it was just the L7 & CDN stuff failing the tests so far.

I now think there is no need to add a new key to the values file, as this PR proposes because a values file like this worked for me just now

% cat values.yaml                                                               
controller:
  config:
    use-geoip2: true
  extraArgs:
    maxmind-mirror: "http://maxmindmirror"
  service:
    externalTrafficPolicy: Local

/close

[k8s-ci-robot]

@longwuyuan: Closed this PR.

In response to this:

Thanks again for the update. It helped solve the problem.

I basically minimal-ized the heck out of that mirror URL and the controller successfully configured geoip2.

• I created a deployment from image nginx:alpine using k -n ingress-nginx create deploy maxmindmirror --image nginx:pine --port 80 and got a pod running with nginx webserver in it
• I created a service of --type ClusterIP for the maxmindmirror deployment with k -n ingress-nginx expose deploy maxmindmirror --port 80

% k -n ingress-nginx get po maxmindmirror-5b69c476cf-29r55 
NAME                             READY   STATUS    RESTARTS   AGE
maxmindmirror-5b69c476cf-29r55   1/1     Running   0          28m
[~/Documents/ingressnginxwork/11435] 
% k -n ingress-nginx get svc maxmindmirror                
NAME            TYPE        CLUSTER-IP     EXTERNAL-IP   PORT(S)   AGE
maxmindmirror   ClusterIP   10.96.96.242   <none>        80/TCP    28m

• So now I can curl a webserver URL from inside the controller pod at URL http://maxmindmirror

/etc/nginx $ hostname 
ingress-nginx-controller-5b88864c5c-tx46x
/etc/nginx $ date
Sun Jun  9 00:11:21 UTC 2024
/etc/nginx $ ping -c 1 maxmindmirror
PING maxmindmirror (10.96.96.242): 56 data bytes
ping: permission denied (are you root?)
/etc/nginx $ curl -I maxmindmirror
HTTP/1.1 200 OK
Server: nginx/1.27.0
Date: Sun, 09 Jun 2024 00:11:49 GMT
Content-Type: text/html
Content-Length: 615
Last-Modified: Wed, 29 May 2024 17:56:56 GMT
Connection: keep-alive
ETag: "66576c68-267"
Accept-Ranges: bytes

/etc/nginx $ 

• Then I downloaded and put the tar.gz files in the webroot of this tiny nginx webserver pod

/ # hostname 
maxmindmirror-5b69c476cf-29r55
/ # date 
Sun Jun  9 00:15:55 UTC 2024
/ # netstat -lntp
Active Internet connections (only servers)
Proto Recv-Q Send-Q Local Address           Foreign Address         State       PID/Program name    
tcp        0      0 0.0.0.0:80              0.0.0.0:*               LISTEN      1/nginx: master pro
netstat: /proc/net/tcp6: No such file or directory
/ # ls -l /usr/share/nginx/html/
total 28344
-rw-r--r--    1 root     root           497 May 29 17:56 50x.html
-rw-r--r--    1 root     root       4727004 Jun  9 00:01 GeoLite2-ASN.tar.gz
-rw-r--r--    1 root     root      24283829 Jun  8 23:54 GeoLite2-City.tar.gz
-rw-r--r--    1 root     root           615 May 29 17:56 index.html
drwxr-xr-x    1 root     root             0 Jun  9 00:01 tmp
/ # 

• Now I set the maxmind-mirror URL in the values file to http://maxmindmirror and geoip2 module was configured

So it was just the L7 & CDN stuff failing the tests so far.

I now think there is no need to add a new key to the values file, as this PR proposes because a values file like this worked for me just now

% cat values.yaml                                                               
controller:
 config:
   use-geoip2: true
 extraArgs:
   maxmind-mirror: "http://maxmindmirror"
 service:
   externalTrafficPolicy: Local

/close

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.