Skip to content

Commit

Permalink
Clean old code and move helper functions (#8946)
Browse files Browse the repository at this point in the history
  • Loading branch information
@rikatz
rikatz authored Aug 21, 2022
1 parent a98c637 commit 4508493
Show file tree
Hide file tree
Showing 7 changed files with 374 additions and 292 deletions.
94 changes: 5 additions & 89 deletions internal/ingress/controller/controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ import (
"k8s.io/ingress-nginx/internal/k8s"
"k8s.io/ingress-nginx/internal/nginx"
"k8s.io/ingress-nginx/pkg/apis/ingress"
utilingress "k8s.io/ingress-nginx/pkg/util/ingress"
"k8s.io/klog/v2"
)

Expand Down Expand Up @@ -163,7 +164,7 @@ func (n *NGINXController) syncIngress(interface{}) error {

n.metricCollector.SetHosts(hosts)

if !n.IsDynamicConfigurationEnough(pcfg) {
if !utilingress.IsDynamicConfigurationEnough(pcfg, n.runningConfig) {
klog.InfoS("Configuration changes detected, backend reload required")

hash, _ := hashstructure.Hash(pcfg, &hashstructure.HashOptions{
Expand Down Expand Up @@ -223,9 +224,9 @@ func (n *NGINXController) syncIngress(interface{}) error {
return err
}

ri := getRemovedIngresses(n.runningConfig, pcfg)
re := getRemovedHosts(n.runningConfig, pcfg)
rc := getRemovedCertificateSerialNumbers(n.runningConfig, pcfg)
ri := utilingress.GetRemovedIngresses(n.runningConfig, pcfg)
re := utilingress.GetRemovedHosts(n.runningConfig, pcfg)
rc := utilingress.GetRemovedCertificateSerialNumbers(n.runningConfig, pcfg)
n.metricCollector.RemoveMetrics(ri, re, rc)

n.runningConfig = pcfg
Expand Down Expand Up @@ -1623,91 +1624,6 @@ func extractTLSSecretName(host string, ing *ingress.Ingress,
return ""
}

// getRemovedHosts returns a list of the hostnames
// that are not associated anymore to the NGINX configuration.
func getRemovedHosts(rucfg, newcfg *ingress.Configuration) []string {
old := sets.NewString()
new := sets.NewString()

for _, s := range rucfg.Servers {
if !old.Has(s.Hostname) {
old.Insert(s.Hostname)
}
}

for _, s := range newcfg.Servers {
if !new.Has(s.Hostname) {
new.Insert(s.Hostname)
}
}

return old.Difference(new).List()
}

func getRemovedCertificateSerialNumbers(rucfg, newcfg *ingress.Configuration) []string {
oldCertificates := sets.NewString()
newCertificates := sets.NewString()

for _, server := range rucfg.Servers {
if server.SSLCert == nil {
continue
}
identifier := server.SSLCert.Identifier()
if identifier != "" {
if !oldCertificates.Has(identifier) {
oldCertificates.Insert(identifier)
}
}
}

for _, server := range newcfg.Servers {
if server.SSLCert == nil {
continue
}
identifier := server.SSLCert.Identifier()
if identifier != "" {
if !newCertificates.Has(identifier) {
newCertificates.Insert(identifier)
}
}
}

return oldCertificates.Difference(newCertificates).List()
}

func getRemovedIngresses(rucfg, newcfg *ingress.Configuration) []string {
oldIngresses := sets.NewString()
newIngresses := sets.NewString()

for _, server := range rucfg.Servers {
for _, location := range server.Locations {
if location.Ingress == nil {
continue
}

ingKey := k8s.MetaNamespaceKey(location.Ingress)
if !oldIngresses.Has(ingKey) {
oldIngresses.Insert(ingKey)
}
}
}

for _, server := range newcfg.Servers {
for _, location := range server.Locations {
if location.Ingress == nil {
continue
}

ingKey := k8s.MetaNamespaceKey(location.Ingress)
if !newIngresses.Has(ingKey) {
newIngresses.Insert(ingKey)
}
}
}

return oldIngresses.Difference(newIngresses).List()
}

// checks conditions for whether or not an upstream should be created for a custom default backend
func shouldCreateUpstreamForLocationDefaultBackend(upstream *ingress.Backend, location *ingress.Location) bool {
return (upstream.Name == location.Backend) &&
Expand Down
88 changes: 4 additions & 84 deletions internal/ingress/controller/nginx.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@ import (
"github.com/eapache/channels"
apiv1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/util/intstr"
"k8s.io/apimachinery/pkg/util/sets"
"k8s.io/client-go/kubernetes/scheme"
v1core "k8s.io/client-go/kubernetes/typed/core/v1"
"k8s.io/client-go/tools/record"
Expand All @@ -61,6 +60,8 @@ import (
"k8s.io/ingress-nginx/pkg/apis/ingress"

"k8s.io/ingress-nginx/pkg/util/file"
utilingress "k8s.io/ingress-nginx/pkg/util/ingress"

klog "k8s.io/klog/v2"
)

Expand Down Expand Up @@ -601,10 +602,9 @@ func (n NGINXController) generateTemplate(cfg ngx_config.Configuration, ingressC
IsIPV6Enabled: n.isIPV6Enabled && !cfg.DisableIpv6,
NginxStatusIpv4Whitelist: cfg.NginxStatusIpv4Whitelist,
NginxStatusIpv6Whitelist: cfg.NginxStatusIpv6Whitelist,
RedirectServers: buildRedirects(ingressCfg.Servers),
RedirectServers: utilingress.BuildRedirects(ingressCfg.Servers),
IsSSLPassthroughEnabled: n.cfg.EnableSSLPassthrough,
ListenPorts: n.cfg.ListenPorts,
PublishService: n.GetPublishService(),
EnableMetrics: n.cfg.EnableMetrics,
MaxmindEditionFiles: n.cfg.MaxmindEditionFiles,
HealthzURI: nginx.HealthPath,
Expand Down Expand Up @@ -832,24 +832,6 @@ func clearL4serviceEndpoints(config *ingress.Configuration) {
config.UDPEndpoints = clearedUDPL4Services
}

// IsDynamicConfigurationEnough returns whether a Configuration can be
// dynamically applied, without reloading the backend.
func (n *NGINXController) IsDynamicConfigurationEnough(pcfg *ingress.Configuration) bool {
copyOfRunningConfig := *n.runningConfig
copyOfPcfg := *pcfg

copyOfRunningConfig.Backends = []*ingress.Backend{}
copyOfPcfg.Backends = []*ingress.Backend{}

clearL4serviceEndpoints(&copyOfRunningConfig)
clearL4serviceEndpoints(&copyOfPcfg)

clearCertificates(&copyOfRunningConfig)
clearCertificates(&copyOfPcfg)

return copyOfRunningConfig.Equal(&copyOfPcfg)
}

// configureDynamically encodes new Backends in JSON format and POSTs the
// payload to an internal HTTP endpoint handled by Lua.
func (n *NGINXController) configureDynamically(pcfg *ingress.Configuration) error {
Expand Down Expand Up @@ -1019,7 +1001,7 @@ func configureCertificates(rawServers []*ingress.Server) error {
}
}

redirects := buildRedirects(rawServers)
redirects := utilingress.BuildRedirects(rawServers)
for _, redirect := range redirects {
configure(redirect.From, redirect.SSLCert)
}
Expand Down Expand Up @@ -1139,65 +1121,3 @@ func cleanTempNginxCfg() error {

return nil
}

type redirect struct {
From string
To string
SSLCert *ingress.SSLCert
}

func buildRedirects(servers []*ingress.Server) []*redirect {
names := sets.String{}
redirectServers := make([]*redirect, 0)

for _, srv := range servers {
if !srv.RedirectFromToWWW {
continue
}

to := srv.Hostname

var from string
if strings.HasPrefix(to, "www.") {
from = strings.TrimPrefix(to, "www.")
} else {
from = fmt.Sprintf("www.%v", to)
}

if names.Has(to) {
continue
}

klog.V(3).InfoS("Creating redirect", "from", from, "to", to)
found := false
for _, esrv := range servers {
if esrv.Hostname == from {
found = true
break
}
}

if found {
klog.Warningf("Already exists an Ingress with %q hostname. Skipping creation of redirection from %q to %q.", from, from, to)
continue
}

r := &redirect{
From: from,
To: to,
}

if srv.SSLCert != nil {
if ssl.IsValidHostname(from, srv.SSLCert.CN) {
r.SSLCert = srv.SSLCert
} else {
klog.Warningf("the server %v has SSL configured but the SSL certificate does not contains a CN for %v. Redirects will not work for HTTPS to HTTPS", from, to)
}
}

redirectServers = append(redirectServers, r)
names.Insert(to)
}

return redirectServers
}
112 changes: 0 additions & 112 deletions internal/ingress/controller/nginx_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,118 +36,6 @@ import (
"k8s.io/ingress-nginx/pkg/apis/ingress"
)

func TestIsDynamicConfigurationEnough(t *testing.T) {
backends := []*ingress.Backend{{
Name: "fakenamespace-myapp-80",
Endpoints: []ingress.Endpoint{
{
Address: "10.0.0.1",
Port: "8080",
},
{
Address: "10.0.0.2",
Port: "8080",
},
},
}}

servers := []*ingress.Server{{
Hostname: "myapp.fake",
Locations: []*ingress.Location{
{
Path: "/",
Backend: "fakenamespace-myapp-80",
},
},
SSLCert: &ingress.SSLCert{
PemCertKey: "fake-certificate",
},
}}

commonConfig := &ingress.Configuration{
Backends: backends,
Servers: servers,
}

n := &NGINXController{
runningConfig: &ingress.Configuration{
Backends: backends,
Servers: servers,
},
cfg: &Configuration{},
}

newConfig := commonConfig
if !n.IsDynamicConfigurationEnough(newConfig) {
t.Errorf("When new config is same as the running config it should be deemed as dynamically configurable")
}

newConfig = &ingress.Configuration{
Backends: []*ingress.Backend{{Name: "another-backend-8081"}},
Servers: []*ingress.Server{{Hostname: "myapp1.fake"}},
}
if n.IsDynamicConfigurationEnough(newConfig) {
t.Errorf("Expected to not be dynamically configurable when there's more than just backends change")
}

newConfig = &ingress.Configuration{
Backends: []*ingress.Backend{{Name: "a-backend-8080"}},
Servers: servers,
}

if !n.IsDynamicConfigurationEnough(newConfig) {
t.Errorf("Expected to be dynamically configurable when only backends change")
}

newServers := []*ingress.Server{{
Hostname: "myapp1.fake",
Locations: []*ingress.Location{
{
Path: "/",
Backend: "fakenamespace-myapp-80",
},
},
SSLCert: &ingress.SSLCert{
PemCertKey: "fake-certificate",
},
}}

newConfig = &ingress.Configuration{
Backends: backends,
Servers: newServers,
}
if n.IsDynamicConfigurationEnough(newConfig) {
t.Errorf("Expected to not be dynamically configurable when dynamic certificates is enabled and a non-certificate field in servers is updated")
}

newServers[0].Hostname = "myapp.fake"
newServers[0].SSLCert.PemCertKey = "new-fake-certificate"

newConfig = &ingress.Configuration{
Backends: backends,
Servers: newServers,
}
if !n.IsDynamicConfigurationEnough(newConfig) {
t.Errorf("Expected to be dynamically configurable when only SSLCert changes")
}

newConfig = &ingress.Configuration{
Backends: []*ingress.Backend{{Name: "a-backend-8080"}},
Servers: newServers,
}
if !n.IsDynamicConfigurationEnough(newConfig) {
t.Errorf("Expected to be dynamically configurable when backend and SSLCert changes")
}

if !n.runningConfig.Equal(commonConfig) {
t.Errorf("Expected running config to not change")
}

if !newConfig.Equal(&ingress.Configuration{Backends: []*ingress.Backend{{Name: "a-backend-8080"}}, Servers: newServers}) {
t.Errorf("Expected new config to not change")
}
}

func TestConfigureDynamically(t *testing.T) {
listener, err := tryListen("tcp", fmt.Sprintf(":%v", nginx.StatusPort))
if err != nil {
Expand Down
3 changes: 0 additions & 3 deletions internal/k8s/main.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -121,9 +121,6 @@ func MetaNamespaceKey(obj interface{}) string {
return key
}

// IsIngressV1Ready indicates if the running Kubernetes version is at least v1.19.0
var IsIngressV1Ready bool

// IngressNGINXController defines the valid value of IngressClass
// Controller field for ingress-nginx
const IngressNGINXController = "k8s.io/ingress-nginx"
Expand Down
Loading

0 comments on commit 4508493

Please sign in to comment.