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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 7 additions & 5 deletions cmd/nginx/flags.go
Original file line number Diff line number Diff line change
Expand Up @@ -308,15 +308,17 @@ https://blog.maxmind.com/2019/12/18/significant-changes-to-accessing-and-using-g
}

var err error
if (nginx.MaxmindLicenseKey != "" || nginx.MaxmindMirror != "") && nginx.MaxmindEditionIDs != "" {
if nginx.MaxmindEditionIDs != "" {
if err = nginx.ValidateGeoLite2DBEditions(); err != nil {
return false, nil, err
}
klog.InfoS("downloading maxmind GeoIP2 databases")
if err = nginx.DownloadGeoLite2DB(nginx.MaxmindRetriesCount, nginx.MaxmindRetriesTimeout); err != nil {
klog.ErrorS(err, "unexpected error downloading GeoIP2 database")
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.

}
}
config.MaxmindEditionFiles = nginx.MaxmindEditionFiles
config.MaxmindEditionFiles = &nginx.MaxmindEditionFiles
}

return false, config, err
Expand Down
2 changes: 1 addition & 1 deletion internal/ingress/controller/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -925,7 +925,7 @@ type TemplateConfig struct {
ListenPorts *ListenPorts
PublishService *apiv1.Service
EnableMetrics bool
MaxmindEditionFiles []string
MaxmindEditionFiles *[]string
MonitorMaxBatchSize int

PID string
Expand Down
2 changes: 1 addition & 1 deletion internal/ingress/controller/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ type Configuration struct {
ValidationWebhookKeyPath string

GlobalExternalAuth *ngx_config.GlobalExternalAuth
MaxmindEditionFiles []string
MaxmindEditionFiles *[]string

MonitorMaxBatchSize int

Expand Down
14 changes: 11 additions & 3 deletions internal/nginx/maxmind.go
Original file line number Diff line number Diff line change
Expand Up @@ -64,12 +64,19 @@ const (

// GeoLite2DBExists checks if the required databases for
// the GeoIP2 NGINX module are present in the filesystem
// and indexes the discovered databases for iteration in
// the config.
func GeoLite2DBExists() bool {
files := []string{}
for _, dbName := range strings.Split(MaxmindEditionIDs, ",") {
if !fileExists(path.Join(geoIPPath, dbName+dbExtension)) {
filename := dbName + dbExtension
if !fileExists(path.Join(geoIPPath, filename)) {
klog.Error(filename, " not found")
return false
}
files = append(files, filename)
}
MaxmindEditionFiles = files
kd7lxl marked this conversation as resolved.
Show resolved Hide resolved

return true
}
Expand Down Expand Up @@ -101,7 +108,6 @@ func DownloadGeoLite2DB(attempts int, period time.Duration) error {
if dlError != nil {
break
}
MaxmindEditionFiles = append(MaxmindEditionFiles, dbName+dbExtension)
}

lastErr = dlError
Expand Down Expand Up @@ -217,11 +223,13 @@ func ValidateGeoLite2DBEditions() error {
return nil
}

func fileExists(filePath string) bool {
func _fileExists(filePath string) bool {
info, err := os.Stat(filePath)
if os.IsNotExist(err) {
return false
}

return !info.IsDir()
}

var fileExists = _fileExists
75 changes: 75 additions & 0 deletions internal/nginx/maxmind_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
/*
Copyright 2020 The Kubernetes Authors.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package nginx

import (
"reflect"
"testing"
)

func resetForTesting() {
fileExists = _fileExists
MaxmindLicenseKey = ""
MaxmindEditionIDs = ""
MaxmindEditionFiles = []string{}
MaxmindMirror = ""
}

func TestGeoLite2DBExists(t *testing.T) {
tests := []struct {
name string
setup func()
want bool
wantFiles []string
}{
{
name: "empty",
wantFiles: []string{},
},
{
name: "existing files",
setup: func() {
MaxmindEditionIDs = "GeoLite2-City,GeoLite2-ASN"
fileExists = func(string) bool {
return true
}
},
want: true,
wantFiles: []string{"GeoLite2-City.mmdb", "GeoLite2-ASN.mmdb"},
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
resetForTesting()
// mimics assignment in flags.go
config := &MaxmindEditionFiles

if tt.setup != nil {
tt.setup()
}
if got := GeoLite2DBExists(); got != tt.want {
t.Errorf("GeoLite2DBExists() = %v, want %v", got, tt.want)
}
if !reflect.DeepEqual(MaxmindEditionFiles, tt.wantFiles) {
t.Errorf("nginx.MaxmindEditionFiles = %v, want %v", MaxmindEditionFiles, tt.wantFiles)
}
if !reflect.DeepEqual(*config, tt.wantFiles) {
t.Errorf("config.MaxmindEditionFiles = %v, want %v", *config, tt.wantFiles)
}
})
}
}
32 changes: 32 additions & 0 deletions test/e2e/settings/geoip2.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,15 +17,23 @@ limitations under the License.
package settings

import (
"context"
"fmt"
"path/filepath"
"strings"

"net/http"

"github.com/onsi/ginkgo"
"github.com/stretchr/testify/assert"

appsv1 "k8s.io/api/apps/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/ingress-nginx/test/e2e/framework"
)

const testdataURL = "https://github.com/maxmind/MaxMind-DB/blob/5a0be1c0320490b8e4379dbd5295a18a648ff156/test-data/GeoLite2-Country-Test.mmdb?raw=true"

var _ = framework.DescribeSetting("Geoip2", func() {
f := framework.NewDefaultFramework("geoip2")

Expand All @@ -35,6 +43,30 @@ var _ = framework.DescribeSetting("Geoip2", func() {
f.NewEchoDeployment()
})

ginkgo.It("should include geoip2 line in config when enabled and db file exists", func() {
edition := "GeoLite2-Country"

err := f.UpdateIngressControllerDeployment(func(deployment *appsv1.Deployment) error {
args := deployment.Spec.Template.Spec.Containers[0].Args
args = append(args, "--maxmind-edition-ids="+edition)
deployment.Spec.Template.Spec.Containers[0].Args = args
_, err := f.KubeClientSet.AppsV1().Deployments(f.Namespace).Update(context.TODO(), deployment, metav1.UpdateOptions{})
return err
})
assert.Nil(ginkgo.GinkgoT(), err, "updating ingress controller deployment flags")

filename := fmt.Sprintf("/etc/nginx/geoip/%s.mmdb", edition)
exec, err := f.ExecIngressPod(fmt.Sprintf(`sh -c "mkdir -p '%s' && wget -O '%s' '%s' 2>&1"`, filepath.Dir(filename), filename, testdataURL))
framework.Logf(exec)
assert.Nil(ginkgo.GinkgoT(), err, fmt.Sprintln("error downloading test geoip2 db", filename))

f.UpdateNginxConfigMapData("use-geoip2", "true")
f.WaitForNginxConfiguration(
func(cfg string) bool {
return strings.Contains(cfg, fmt.Sprintf("geoip2 %s", filename))
})
})

ginkgo.It("should only allow requests from specific countries", func() {
ginkgo.Skip("GeoIP test are temporarily disabled")

Expand Down