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

Reload only once during the start #1788

Merged
merged 2 commits into from
Aug 10, 2021
Merged
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
101 changes: 71 additions & 30 deletions internal/configs/configurator.go
Original file line number Diff line number Diff line change
@@ -94,6 +94,10 @@ type metricLabelsIndex struct {
}

// Configurator configures NGINX.
// Until reloads are enabled via EnableReloads(), the Configurator will not reload NGINX and update NGINX Plus
// upstream servers via NGINX Plus API for configuration changes.
// This allows the Ingress Controller to incrementally build the NGINX configuration during the IC start and
// then apply it at the end of the start.
type Configurator struct {
nginxManager nginx.Manager
staticCfgParams *StaticConfigParams
@@ -111,6 +115,7 @@ type Configurator struct {
isPrometheusEnabled bool
latencyCollector latCollector.LatencyCollector
isLatencyMetricsEnabled bool
isReloadsEnabled bool
}

// NewConfigurator creates a new Configurator.
@@ -146,6 +151,7 @@ func NewConfigurator(nginxManager nginx.Manager, staticCfgParams *StaticConfigPa
isPrometheusEnabled: isPrometheusEnabled,
latencyCollector: latencyCollector,
isLatencyMetricsEnabled: isLatencyMetricsEnabled,
isReloadsEnabled: false,
}
return &cnf
}
@@ -248,7 +254,7 @@ func (cnf *Configurator) AddOrUpdateIngress(ingEx *IngressEx) (Warnings, error)
return warnings, fmt.Errorf("Error adding or updating ingress %v/%v: %w", ingEx.Ingress.Namespace, ingEx.Ingress.Name, err)
}

if err := cnf.nginxManager.Reload(nginx.ReloadForOtherUpdate); err != nil {
if err := cnf.reload(nginx.ReloadForOtherUpdate); err != nil {
return warnings, fmt.Errorf("Error reloading NGINX for %v/%v: %w", ingEx.Ingress.Namespace, ingEx.Ingress.Name, err)
}

@@ -289,7 +295,7 @@ func (cnf *Configurator) AddOrUpdateMergeableIngress(mergeableIngs *MergeableIng
return warnings, fmt.Errorf("Error when adding or updating ingress %v/%v: %w", mergeableIngs.Master.Ingress.Namespace, mergeableIngs.Master.Ingress.Name, err)
}

if err := cnf.nginxManager.Reload(nginx.ReloadForOtherUpdate); err != nil {
if err := cnf.reload(nginx.ReloadForOtherUpdate); err != nil {
return warnings, fmt.Errorf("Error reloading NGINX for %v/%v: %w", mergeableIngs.Master.Ingress.Namespace, mergeableIngs.Master.Ingress.Name, err)
}

@@ -420,7 +426,7 @@ func (cnf *Configurator) AddOrUpdateVirtualServer(virtualServerEx *VirtualServer
return warnings, fmt.Errorf("Error adding or updating VirtualServer %v/%v: %w", virtualServerEx.VirtualServer.Namespace, virtualServerEx.VirtualServer.Name, err)
}

if err := cnf.nginxManager.Reload(nginx.ReloadForOtherUpdate); err != nil {
if err := cnf.reload(nginx.ReloadForOtherUpdate); err != nil {
return warnings, fmt.Errorf("Error reloading NGINX for VirtualServer %v/%v: %w", virtualServerEx.VirtualServer.Namespace, virtualServerEx.VirtualServer.Name, err)
}

@@ -465,7 +471,7 @@ func (cnf *Configurator) AddOrUpdateVirtualServers(virtualServerExes []*VirtualS
allWarnings.Add(warnings)
}

if err := cnf.nginxManager.Reload(nginx.ReloadForOtherUpdate); err != nil {
if err := cnf.reload(nginx.ReloadForOtherUpdate); err != nil {
return allWarnings, fmt.Errorf("Error when reloading NGINX when updating Policy: %w", err)
}

@@ -547,7 +553,7 @@ func (cnf *Configurator) AddOrUpdateTransportServer(transportServerEx *Transport
return fmt.Errorf("Error adding or updating TransportServer %v/%v: %w", transportServerEx.TransportServer.Namespace, transportServerEx.TransportServer.Name, err)
}

if err := cnf.nginxManager.Reload(nginx.ReloadForOtherUpdate); err != nil {
if err := cnf.reload(nginx.ReloadForOtherUpdate); err != nil {
return fmt.Errorf("Error reloading NGINX for TransportServer %v/%v: %w", transportServerEx.TransportServer.Namespace, transportServerEx.TransportServer.Name, err)
}

@@ -665,7 +671,7 @@ func (cnf *Configurator) AddOrUpdateResources(resources ExtendedResources) (Warn
}
}

if err := cnf.nginxManager.Reload(nginx.ReloadForOtherUpdate); err != nil {
if err := cnf.reload(nginx.ReloadForOtherUpdate); err != nil {
return allWarnings, fmt.Errorf("Error when reloading NGINX when updating resources: %w", err)
}

@@ -686,7 +692,7 @@ func (cnf *Configurator) AddOrUpdateSpecialTLSSecrets(secret *api_v1.Secret, sec
cnf.nginxManager.CreateSecret(secretName, data, nginx.TLSSecretFileMode)
}

if err := cnf.nginxManager.Reload(nginx.ReloadForOtherUpdate); err != nil {
if err := cnf.reload(nginx.ReloadForOtherUpdate); err != nil {
return fmt.Errorf("Error when reloading NGINX when updating the special Secrets: %w", err)
}

@@ -725,7 +731,7 @@ func (cnf *Configurator) DeleteIngress(key string) error {
cnf.deleteIngressMetricsLabels(key)
}

if err := cnf.nginxManager.Reload(nginx.ReloadForOtherUpdate); err != nil {
if err := cnf.reload(nginx.ReloadForOtherUpdate); err != nil {
return fmt.Errorf("Error when removing ingress %v: %w", key, err)
}

@@ -742,7 +748,7 @@ func (cnf *Configurator) DeleteVirtualServer(key string) error {
cnf.deleteVirtualServerMetricsLabels(key)
}

if err := cnf.nginxManager.Reload(nginx.ReloadForOtherUpdate); err != nil {
if err := cnf.reload(nginx.ReloadForOtherUpdate); err != nil {
return fmt.Errorf("Error when removing VirtualServer %v: %w", key, err)
}

@@ -760,7 +766,7 @@ func (cnf *Configurator) DeleteTransportServer(key string) error {
return fmt.Errorf("Error when removing TransportServer %v: %w", key, err)
}

err = cnf.nginxManager.Reload(nginx.ReloadForOtherUpdate)
err = cnf.reload(nginx.ReloadForOtherUpdate)
if err != nil {
return fmt.Errorf("Error when removing TransportServer %v: %w", key, err)
}
@@ -807,7 +813,7 @@ func (cnf *Configurator) UpdateEndpoints(ingExes []*IngressEx) error {
return nil
}

if err := cnf.nginxManager.Reload(nginx.ReloadForEndpointsUpdate); err != nil {
if err := cnf.reload(nginx.ReloadForEndpointsUpdate); err != nil {
return fmt.Errorf("Error reloading NGINX when updating endpoints: %w", err)
}

@@ -841,7 +847,7 @@ func (cnf *Configurator) UpdateEndpointsMergeableIngress(mergeableIngresses []*M
return nil
}

if err := cnf.nginxManager.Reload(nginx.ReloadForEndpointsUpdate); err != nil {
if err := cnf.reload(nginx.ReloadForEndpointsUpdate); err != nil {
return fmt.Errorf("Error reloading NGINX when updating endpoints for %v: %w", mergeableIngresses, err)
}

@@ -873,7 +879,7 @@ func (cnf *Configurator) UpdateEndpointsForVirtualServers(virtualServerExes []*V
return nil
}

if err := cnf.nginxManager.Reload(nginx.ReloadForEndpointsUpdate); err != nil {
if err := cnf.reload(nginx.ReloadForEndpointsUpdate); err != nil {
return fmt.Errorf("Error reloading NGINX when updating endpoints: %w", err)
}

@@ -887,7 +893,7 @@ func (cnf *Configurator) updatePlusEndpointsForVirtualServer(virtualServerEx *Vi

endpoints := createEndpointsFromUpstream(upstream)

err := cnf.nginxManager.UpdateServersInPlus(upstream.Name, endpoints, serverCfg)
err := cnf.updateServersInPlus(upstream.Name, endpoints, serverCfg)
if err != nil {
return fmt.Errorf("Couldn't update the endpoints for %v: %w", upstream.Name, err)
}
@@ -920,7 +926,7 @@ func (cnf *Configurator) UpdateEndpointsForTransportServers(transportServerExes
return nil
}

if err := cnf.nginxManager.Reload(nginx.ReloadForEndpointsUpdate); err != nil {
if err := cnf.reload(nginx.ReloadForEndpointsUpdate); err != nil {
return fmt.Errorf("Error reloading NGINX when updating endpoints: %w", err)
}

@@ -937,7 +943,7 @@ func (cnf *Configurator) updatePlusEndpointsForTransportServer(transportServerEx
endpointsKey := GenerateEndpointsKey(transportServerEx.TransportServer.Namespace, u.Service, nil, uint16(u.Port))
endpoints := transportServerEx.Endpoints[endpointsKey]

err := cnf.nginxManager.UpdateStreamServersInPlus(name, endpoints)
err := cnf.updateStreamServersInPlus(name, endpoints)
if err != nil {
return fmt.Errorf("Couldn't update the endpoints for %v: %w", u.Name, err)
}
@@ -963,7 +969,7 @@ func (cnf *Configurator) updatePlusEndpoints(ingEx *IngressEx) error {
glog.V(3).Infof("Service %s is Type ExternalName, skipping NGINX Plus endpoints update via API", ingEx.Ingress.Spec.Backend.ServiceName)
} else {
name := getNameForUpstream(ingEx.Ingress, emptyHost, ingEx.Ingress.Spec.Backend)
err := cnf.nginxManager.UpdateServersInPlus(name, endps, cfg)
err := cnf.updateServersInPlus(name, endps, cfg)
if err != nil {
return fmt.Errorf("Couldn't update the endpoints for %v: %w", name, err)
}
@@ -985,7 +991,7 @@ func (cnf *Configurator) updatePlusEndpoints(ingEx *IngressEx) error {
}

name := getNameForUpstream(ingEx.Ingress, rule.Host, &path.Backend)
err := cnf.nginxManager.UpdateServersInPlus(name, endps, cfg)
err := cnf.updateServersInPlus(name, endps, cfg)
if err != nil {
return fmt.Errorf("Couldn't update the endpoints for %v: %w", name, err)
}
@@ -996,8 +1002,38 @@ func (cnf *Configurator) updatePlusEndpoints(ingEx *IngressEx) error {
return nil
}

// EnableReloads enables NGINX reloads meaning that configuration changes will be followed by a reload.
func (cnf *Configurator) EnableReloads() {
cnf.isReloadsEnabled = true
}

func (cnf *Configurator) reload(isEndpointsUpdate bool) error {
if !cnf.isReloadsEnabled {
return nil
}

return cnf.nginxManager.Reload(isEndpointsUpdate)
}

func (cnf *Configurator) updateServersInPlus(upstream string, servers []string, config nginx.ServerConfig) error {
if !cnf.isReloadsEnabled {
return nil
}

return cnf.nginxManager.UpdateServersInPlus(upstream, servers, config)
}

func (cnf *Configurator) updateStreamServersInPlus(upstream string, servers []string) error {
if !cnf.isReloadsEnabled {
return nil
}

return cnf.nginxManager.UpdateStreamServersInPlus(upstream, servers)
}

// UpdateConfig updates NGINX configuration parameters.
func (cnf *Configurator) UpdateConfig(cfgParams *ConfigParams, ingExes []*IngressEx, mergeableIngs []*MergeableIngresses, virtualServerExes []*VirtualServerEx) (Warnings, error) {
//gocyclo:ignore
func (cnf *Configurator) UpdateConfig(cfgParams *ConfigParams, resources ExtendedResources) (Warnings, error) {
tomasohaodha marked this conversation as resolved.
Show resolved Hide resolved
cnf.cfgParams = cfgParams
allWarnings := newWarnings()

@@ -1037,36 +1073,41 @@ func (cnf *Configurator) UpdateConfig(cfgParams *ConfigParams, ingExes []*Ingres
}
cnf.nginxManager.CreateMainConfig(mainCfgContent)

for _, ingEx := range ingExes {
for _, ingEx := range resources.IngressExes {
warnings, err := cnf.addOrUpdateIngress(ingEx)
if err != nil {
return allWarnings, err
}
allWarnings.Add(warnings)
}
for _, mergeableIng := range mergeableIngs {
for _, mergeableIng := range resources.MergeableIngresses {
warnings, err := cnf.addOrUpdateMergeableIngress(mergeableIng)
if err != nil {
return allWarnings, err
}
allWarnings.Add(warnings)
}
for _, vsEx := range virtualServerExes {
for _, vsEx := range resources.VirtualServerExes {
warnings, err := cnf.addOrUpdateVirtualServer(vsEx)
if err != nil {
return allWarnings, err
}
allWarnings.Add(warnings)
}

// we don't need to regenerate config for TransportServers, because:
// (1) Changes to the ConfigMap don't affect TransportServer configs directly
// (2) addOrUpdateTransportServer doesn't return any warnings that we need to propagate to the caller.
// if (1) and (2) is no longer the case, we need to generate the config for TransportServers

if mainCfg.OpenTracingLoadModule {
if err := cnf.addOrUpdateOpenTracingTracerConfig(mainCfg.OpenTracingTracerConfig); err != nil {
return allWarnings, fmt.Errorf("Error when updating OpenTracing tracer config: %w", err)
}
}

cnf.nginxManager.SetOpenTracing(mainCfg.OpenTracingLoadModule)
if err := cnf.nginxManager.Reload(nginx.ReloadForOtherUpdate); err != nil {
if err := cnf.reload(nginx.ReloadForOtherUpdate); err != nil {
return allWarnings, fmt.Errorf("Error when updating config from ConfigMap: %w", err)
}

@@ -1089,7 +1130,7 @@ func (cnf *Configurator) UpdateTransportServers(updatedTSExes []*TransportServer
}
}

if err := cnf.nginxManager.Reload(nginx.ReloadForOtherUpdate); err != nil {
if err := cnf.reload(nginx.ReloadForOtherUpdate); err != nil {
return fmt.Errorf("Error when updating TransportServers: %w", err)
}

@@ -1195,7 +1236,7 @@ func (cnf *Configurator) AddOrUpdateSpiffeCerts(svidResponse *workload.X509SVIDs
cnf.nginxManager.CreateSecret(spiffeCertFileName, createSpiffeCert(svid.Certificates), spiffeCertsFileMode)
cnf.nginxManager.CreateSecret(spiffeBundleFileName, createSpiffeCert(svid.TrustBundle), spiffeCertsFileMode)

err = cnf.nginxManager.Reload(nginx.ReloadForOtherUpdate)
err = cnf.reload(nginx.ReloadForOtherUpdate)
if err != nil {
return fmt.Errorf("error when reloading NGINX when updating the SPIFFE Certs: %w", err)
}
@@ -1310,7 +1351,7 @@ func (cnf *Configurator) AddOrUpdateAppProtectResource(resource *unstructured.Un
allWarnings.Add(warnings)
}

if err := cnf.nginxManager.Reload(nginx.ReloadForOtherUpdate); err != nil {
if err := cnf.reload(nginx.ReloadForOtherUpdate); err != nil {
return allWarnings, fmt.Errorf("Error when reloading NGINX when updating %v: %w", resource.GetKind(), err)
}

@@ -1351,7 +1392,7 @@ func (cnf *Configurator) DeleteAppProtectPolicy(polNamespaceName string, ingExes
allWarnings.Add(warnings)
}

if err := cnf.nginxManager.Reload(nginx.ReloadForOtherUpdate); err != nil {
if err := cnf.reload(nginx.ReloadForOtherUpdate); err != nil {
return allWarnings, fmt.Errorf("Error when reloading NGINX when removing App Protect Policy: %w", err)
}

@@ -1391,7 +1432,7 @@ func (cnf *Configurator) DeleteAppProtectLogConf(logConfNamespaceName string, in
allWarnings.Add(warnings)
}

if err := cnf.nginxManager.Reload(nginx.ReloadForOtherUpdate); err != nil {
if err := cnf.reload(nginx.ReloadForOtherUpdate); err != nil {
return allWarnings, fmt.Errorf("Error when reloading NGINX when removing App Protect Log Configuration: %w", err)
}

@@ -1440,7 +1481,7 @@ func (cnf *Configurator) RefreshAppProtectUserSigs(
fmt.Fprintf(&builder, "app_protect_user_defined_signatures %s;\n", fName)
}
cnf.nginxManager.CreateAppProtectResourceFile(appProtectUserSigIndex, []byte(builder.String()))
return allWarnings, cnf.nginxManager.Reload(nginx.ReloadForOtherUpdate)
return allWarnings, cnf.reload(nginx.ReloadForOtherUpdate)
}

// AddInternalRouteConfig adds internal route server to NGINX Configuration and reloads NGINX
@@ -1453,7 +1494,7 @@ func (cnf *Configurator) AddInternalRouteConfig() error {
return fmt.Errorf("Error when writing main Config: %w", err)
}
cnf.nginxManager.CreateMainConfig(mainCfgContent)
if err := cnf.nginxManager.Reload(nginx.ReloadForOtherUpdate); err != nil {
if err := cnf.reload(nginx.ReloadForOtherUpdate); err != nil {
return fmt.Errorf("Error when reloading nginx: %w", err)
}
return nil
18 changes: 16 additions & 2 deletions internal/configs/configurator_test.go
Original file line number Diff line number Diff line change
@@ -41,7 +41,14 @@ func createTestConfigurator() (*Configurator, error) {

manager := nginx.NewFakeManager("/etc/nginx")

return NewConfigurator(manager, createTestStaticConfigParams(), NewDefaultConfigParams(), templateExecutor, templateExecutorV2, false, false, nil, false, nil, false), nil
cnf, err := NewConfigurator(manager, createTestStaticConfigParams(), NewDefaultConfigParams(), templateExecutor, templateExecutorV2, false, false, nil, false, nil, false), nil
if err != nil {
return nil, err
}

cnf.isReloadsEnabled = true

return cnf, nil
}

func createTestConfiguratorInvalidIngressTemplate() (*Configurator, error) {
@@ -57,7 +64,14 @@ func createTestConfiguratorInvalidIngressTemplate() (*Configurator, error) {

manager := nginx.NewFakeManager("/etc/nginx")

return NewConfigurator(manager, createTestStaticConfigParams(), NewDefaultConfigParams(), templateExecutor, &version2.TemplateExecutor{}, false, false, nil, false, nil, false), nil
cnf, err := NewConfigurator(manager, createTestStaticConfigParams(), NewDefaultConfigParams(), templateExecutor, &version2.TemplateExecutor{}, false, false, nil, false, nil, false), nil
if err != nil {
return nil, err
}

cnf.isReloadsEnabled = true

return cnf, nil
}

func TestAddOrUpdateIngress(t *testing.T) {
Loading