Skip to content

Commit

Permalink
Support warnings for TLS and JWK secrets in Ing
Browse files Browse the repository at this point in the history
  • Loading branch information
@pleshakov
pleshakov committed Dec 7, 2020
1 parent c037ea7 commit b64293d
Show file tree
Hide file tree
Showing 6 changed files with 280 additions and 113 deletions.
121 changes: 73 additions & 48 deletions internal/configs/configurator.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -230,19 +230,20 @@ func (cnf *Configurator) deleteIngressMetricsLabels(key string) {
}

// AddOrUpdateIngress adds or updates NGINX configuration for the Ingress resource.
func (cnf *Configurator) AddOrUpdateIngress(ingEx *IngressEx) error {
if err := cnf.addOrUpdateIngress(ingEx); err != nil {
return fmt.Errorf("Error adding or updating ingress %v/%v: %v", ingEx.Ingress.Namespace, ingEx.Ingress.Name, err)
func (cnf *Configurator) AddOrUpdateIngress(ingEx *IngressEx) (Warnings, error) {
warnings, err := cnf.addOrUpdateIngress(ingEx)
if err != nil {
return warnings, fmt.Errorf("Error adding or updating ingress %v/%v: %v", ingEx.Ingress.Namespace, ingEx.Ingress.Name, err)
}

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

return nil
return warnings, nil
}

func (cnf *Configurator) addOrUpdateIngress(ingEx *IngressEx) error {
func (cnf *Configurator) addOrUpdateIngress(ingEx *IngressEx) (Warnings, error) {
apResources := cnf.updateApResources(ingEx)

if jwtKey, exists := ingEx.Ingress.Annotations[JWTKeyAnnotation]; exists {
Expand All @@ -253,54 +254,58 @@ func (cnf *Configurator) addOrUpdateIngress(ingEx *IngressEx) error {
}

isMinion := false
nginxCfg := generateNginxCfg(ingEx, apResources, isMinion, cnf.cfgParams, cnf.isPlus, cnf.IsResolverConfigured(),
nginxCfg, warnings := generateNginxCfg(ingEx, apResources, isMinion, cnf.cfgParams, cnf.isPlus, cnf.IsResolverConfigured(),
cnf.staticCfgParams, cnf.isWildcardEnabled)
name := objectMetaToFileName(&ingEx.Ingress.ObjectMeta)
content, err := cnf.templateExecutor.ExecuteIngressConfigTemplate(&nginxCfg)
if err != nil {
return fmt.Errorf("Error generating Ingress Config %v: %v", name, err)
return warnings, fmt.Errorf("Error generating Ingress Config %v: %v", name, err)
}
cnf.nginxManager.CreateConfig(name, content)

cnf.ingresses[name] = ingEx
if (cnf.isPlus && cnf.isPrometheusEnabled) || cnf.isLatencyMetricsEnabled {
cnf.updateIngressMetricsLabels(ingEx, nginxCfg.Upstreams)
}
return nil
return warnings, nil
}

// AddOrUpdateMergeableIngress adds or updates NGINX configuration for the Ingress resources with Mergeable Types.
func (cnf *Configurator) AddOrUpdateMergeableIngress(mergeableIngs *MergeableIngresses) error {
if err := cnf.addOrUpdateMergeableIngress(mergeableIngs); err != nil {
return fmt.Errorf("Error when adding or updating ingress %v/%v: %v", mergeableIngs.Master.Ingress.Namespace, mergeableIngs.Master.Ingress.Name, err)
func (cnf *Configurator) AddOrUpdateMergeableIngress(mergeableIngs *MergeableIngresses) (Warnings, error) {
warnings, err := cnf.addOrUpdateMergeableIngress(mergeableIngs)
if err != nil {
return warnings, fmt.Errorf("Error when adding or updating ingress %v/%v: %v", mergeableIngs.Master.Ingress.Namespace, mergeableIngs.Master.Ingress.Name, err)
}

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

return nil
return warnings, nil
}

func (cnf *Configurator) addOrUpdateMergeableIngress(mergeableIngs *MergeableIngresses) error {
func (cnf *Configurator) addOrUpdateMergeableIngress(mergeableIngs *MergeableIngresses) (Warnings, error) {
masterApResources := cnf.updateApResources(mergeableIngs.Master)

// LocalSecretStore will not set Path if the secret is not on the filesystem.
// However, NGINX configuration for an Ingress resource, to handle the case of a missing secret,
// relies on the path to be always configured.
if jwtKey, exists := mergeableIngs.Master.Ingress.Annotations[JWTKeyAnnotation]; exists {
mergeableIngs.Master.SecretRefs[jwtKey].Path = cnf.nginxManager.GetFilenameForSecret(mergeableIngs.Master.Ingress.Namespace + "-" + jwtKey)
}
for _, minion := range mergeableIngs.Minions {
if jwtKey, exists := minion.Ingress.Annotations[JWTKeyAnnotation]; exists {
// LocalSecretStore will not set Path if the secret is not on the filesystem.
// However, NGINX configuration for an Ingress resource, to handle the case of a missing secret,
// relies on the path to be always configured.
minion.SecretRefs[jwtKey].Path = cnf.nginxManager.GetFilenameForSecret(minion.Ingress.Namespace + "-" + jwtKey)
}
}

nginxCfg := generateNginxCfgForMergeableIngresses(mergeableIngs, masterApResources, cnf.cfgParams, cnf.isPlus,
nginxCfg, warnings := generateNginxCfgForMergeableIngresses(mergeableIngs, masterApResources, cnf.cfgParams, cnf.isPlus,
cnf.IsResolverConfigured(), cnf.staticCfgParams, cnf.isWildcardEnabled)

name := objectMetaToFileName(&mergeableIngs.Master.Ingress.ObjectMeta)
content, err := cnf.templateExecutor.ExecuteIngressConfigTemplate(&nginxCfg)
if err != nil {
return fmt.Errorf("Error generating Ingress Config %v: %v", name, err)
return warnings, fmt.Errorf("Error generating Ingress Config %v: %v", name, err)
}
cnf.nginxManager.CreateConfig(name, content)

Expand All @@ -314,7 +319,7 @@ func (cnf *Configurator) addOrUpdateMergeableIngress(mergeableIngs *MergeableIng
cnf.updateIngressMetricsLabels(mergeableIngs.Master, nginxCfg.Upstreams)
}

return nil
return warnings, nil
}

func (cnf *Configurator) updateVirtualServerMetricsLabels(virtualServerEx *VirtualServerEx, upstreams []version2.Upstream) {
Expand Down Expand Up @@ -565,17 +570,19 @@ func (cnf *Configurator) AddOrUpdateResources(ingExes []*IngressEx, mergeableIng
allWarnings := newWarnings()

for _, ingEx := range ingExes {
err := cnf.addOrUpdateIngress(ingEx)
warnings, err := cnf.addOrUpdateIngress(ingEx)
if err != nil {
return allWarnings, fmt.Errorf("Error adding or updating ingress %v/%v: %v", ingEx.Ingress.Namespace, ingEx.Ingress.Name, err)
}
allWarnings.Add(warnings)
}

for _, m := range mergeableIngresses {
err := cnf.addOrUpdateMergeableIngress(m)
warnings, err := cnf.addOrUpdateMergeableIngress(m)
if err != nil {
return allWarnings, fmt.Errorf("Error adding or updating mergeableIngress %v/%v: %v", m.Master.Ingress.Namespace, m.Master.Ingress.Name, err)
}
allWarnings.Add(warnings)
}

for _, vsEx := range virtualServerExes {
Expand Down Expand Up @@ -704,7 +711,8 @@ func (cnf *Configurator) UpdateEndpoints(ingExes []*IngressEx) error {
reloadPlus := false

for _, ingEx := range ingExes {
err := cnf.addOrUpdateIngress(ingEx)
// It is safe to ignore warnings here as no new warnings should appear when updating Endpoints for Ingresses
_, err := cnf.addOrUpdateIngress(ingEx)
if err != nil {
return fmt.Errorf("Error adding or updating ingress %v/%v: %v", ingEx.Ingress.Namespace, ingEx.Ingress.Name, err)
}
Expand Down Expand Up @@ -735,7 +743,8 @@ func (cnf *Configurator) UpdateEndpointsMergeableIngress(mergeableIngresses []*M
reloadPlus := false

for i := range mergeableIngresses {
err := cnf.addOrUpdateMergeableIngress(mergeableIngresses[i])
// It is safe to ignore warnings here as no new warnings should appear when updating Endpoints for Ingresses
_, err := cnf.addOrUpdateMergeableIngress(mergeableIngresses[i])
if err != nil {
return fmt.Errorf("Error adding or updating mergeableIngress %v/%v: %v", mergeableIngresses[i].Master.Ingress.Namespace, mergeableIngresses[i].Master.Ingress.Name, err)
}
Expand Down Expand Up @@ -953,14 +962,18 @@ func (cnf *Configurator) UpdateConfig(cfgParams *ConfigParams, ingExes []*Ingres
cnf.nginxManager.CreateMainConfig(mainCfgContent)

for _, ingEx := range ingExes {
if err := cnf.addOrUpdateIngress(ingEx); err != nil {
warnings, err := cnf.addOrUpdateIngress(ingEx)
if err != nil {
return allWarnings, err
}
allWarnings.Add(warnings)
}
for _, mergeableIng := range mergeableIngs {
if err := cnf.addOrUpdateMergeableIngress(mergeableIng); err != nil {
warnings, err := cnf.addOrUpdateMergeableIngress(mergeableIng)
if err != nil {
return allWarnings, err
}
allWarnings.Add(warnings)
}
for _, vsEx := range virtualServerExes {
warnings, err := cnf.addOrUpdateVirtualServer(vsEx)
Expand Down Expand Up @@ -1188,80 +1201,92 @@ func generateApResourceFileContent(apResource *unstructured.Unstructured) []byte
}

// AddOrUpdateAppProtectResource updates Ingresses that use App Protect Resources
func (cnf *Configurator) AddOrUpdateAppProtectResource(resource *unstructured.Unstructured, ingExes []*IngressEx, mergeableIngresses []*MergeableIngresses) error {
func (cnf *Configurator) AddOrUpdateAppProtectResource(resource *unstructured.Unstructured, ingExes []*IngressEx, mergeableIngresses []*MergeableIngresses) (Warnings, error) {
allWarnings := newWarnings()

for _, ingEx := range ingExes {
err := cnf.addOrUpdateIngress(ingEx)
warnings, err := cnf.addOrUpdateIngress(ingEx)
if err != nil {
return fmt.Errorf("Error adding or updating ingress %v/%v: %v", ingEx.Ingress.Namespace, ingEx.Ingress.Name, err)
return allWarnings, fmt.Errorf("Error adding or updating ingress %v/%v: %v", ingEx.Ingress.Namespace, ingEx.Ingress.Name, err)
}
allWarnings.Add(warnings)
}

for _, m := range mergeableIngresses {
err := cnf.addOrUpdateMergeableIngress(m)
warnings, err := cnf.addOrUpdateMergeableIngress(m)
if err != nil {
return fmt.Errorf("Error adding or updating mergeableIngress %v/%v: %v", m.Master.Ingress.Namespace, m.Master.Ingress.Name, err)
return allWarnings, fmt.Errorf("Error adding or updating mergeableIngress %v/%v: %v", m.Master.Ingress.Namespace, m.Master.Ingress.Name, err)
}
allWarnings.Add(warnings)
}

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

return nil
return allWarnings, nil
}

// DeleteAppProtectPolicy updates Ingresses that use AP Policy after that policy is deleted
func (cnf *Configurator) DeleteAppProtectPolicy(polNamespaceName string, ingExes []*IngressEx, mergeableIngresses []*MergeableIngresses) error {
func (cnf *Configurator) DeleteAppProtectPolicy(polNamespaceName string, ingExes []*IngressEx, mergeableIngresses []*MergeableIngresses) (Warnings, error) {
fName := strings.Replace(polNamespaceName, "/", "_", 1)
polFileName := appProtectPolicyFolder + fName
cnf.nginxManager.DeleteAppProtectResourceFile(polFileName)

allWarnings := newWarnings()

for _, ingEx := range ingExes {
err := cnf.addOrUpdateIngress(ingEx)
warnings, err := cnf.addOrUpdateIngress(ingEx)
if err != nil {
return fmt.Errorf("Error adding or updating ingress %v/%v: %v", ingEx.Ingress.Namespace, ingEx.Ingress.Name, err)
return allWarnings, fmt.Errorf("Error adding or updating ingress %v/%v: %v", ingEx.Ingress.Namespace, ingEx.Ingress.Name, err)
}
allWarnings.Add(warnings)
}

for _, m := range mergeableIngresses {
err := cnf.addOrUpdateMergeableIngress(m)
warnings, err := cnf.addOrUpdateMergeableIngress(m)
if err != nil {
return fmt.Errorf("Error adding or updating mergeableIngress %v/%v: %v", m.Master.Ingress.Namespace, m.Master.Ingress.Name, err)
return allWarnings, fmt.Errorf("Error adding or updating mergeableIngress %v/%v: %v", m.Master.Ingress.Namespace, m.Master.Ingress.Name, err)
}
allWarnings.Add(warnings)
}

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

return nil
return allWarnings, nil
}

// DeleteAppProtectLogConf updates Ingresses that use AP Log Configuration after that policy is deleted
func (cnf *Configurator) DeleteAppProtectLogConf(logConfNamespaceName string, ingExes []*IngressEx, mergeableIngresses []*MergeableIngresses) error {
func (cnf *Configurator) DeleteAppProtectLogConf(logConfNamespaceName string, ingExes []*IngressEx, mergeableIngresses []*MergeableIngresses) (Warnings, error) {
fName := strings.Replace(logConfNamespaceName, "/", "_", 1)
logConfFileName := appProtectLogConfFolder + fName
cnf.nginxManager.DeleteAppProtectResourceFile(logConfFileName)

allWarnings := newWarnings()

for _, ingEx := range ingExes {
err := cnf.addOrUpdateIngress(ingEx)
warnings, err := cnf.addOrUpdateIngress(ingEx)
if err != nil {
return fmt.Errorf("Error adding or updating ingress %v/%v: %v", ingEx.Ingress.Namespace, ingEx.Ingress.Name, err)
return allWarnings, fmt.Errorf("Error adding or updating ingress %v/%v: %v", ingEx.Ingress.Namespace, ingEx.Ingress.Name, err)
}
allWarnings.Add(warnings)
}

for _, m := range mergeableIngresses {
err := cnf.addOrUpdateMergeableIngress(m)
warnings, err := cnf.addOrUpdateMergeableIngress(m)
if err != nil {
return fmt.Errorf("Error adding or updating mergeableIngress %v/%v: %v", m.Master.Ingress.Namespace, m.Master.Ingress.Name, err)
return allWarnings, fmt.Errorf("Error adding or updating mergeableIngress %v/%v: %v", m.Master.Ingress.Namespace, m.Master.Ingress.Name, err)
}
allWarnings.Add(warnings)
}

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

return nil
return allWarnings, nil
}

// AddInternalRouteConfig adds internal route server to NGINX Configuration and
Expand Down
22 changes: 17 additions & 5 deletions internal/configs/configurator_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,10 +67,13 @@ func TestAddOrUpdateIngress(t *testing.T) {

ingress := createCafeIngressEx()

err = cnf.AddOrUpdateIngress(&ingress)
warnings, err := cnf.AddOrUpdateIngress(&ingress)
if err != nil {
t.Errorf("AddOrUpdateIngress returned: \n%v, but expected: \n%v", err, nil)
}
if len(warnings) != 0 {
t.Errorf("AddOrUpdateIngress returned warnings: %v", warnings)
}

cnfHasIngress := cnf.HasIngress(ingress.Ingress)
if !cnfHasIngress {
Expand All @@ -86,10 +89,13 @@ func TestAddOrUpdateMergeableIngress(t *testing.T) {

mergeableIngess := createMergeableCafeIngress()

err = cnf.AddOrUpdateMergeableIngress(mergeableIngess)
warnings, err := cnf.AddOrUpdateMergeableIngress(mergeableIngess)
if err != nil {
t.Errorf("AddOrUpdateMergeableIngress returned \n%v, expected \n%v", err, nil)
}
if len(warnings) != 0 {
t.Errorf("AddOrUpdateMergeableIngress returned warnings: %v", warnings)
}

cnfHasMergeableIngress := cnf.HasIngress(mergeableIngess.Master.Ingress)
if !cnfHasMergeableIngress {
Expand All @@ -105,9 +111,12 @@ func TestAddOrUpdateIngressFailsWithInvalidIngressTemplate(t *testing.T) {

ingress := createCafeIngressEx()

err = cnf.AddOrUpdateIngress(&ingress)
warnings, err := cnf.AddOrUpdateIngress(&ingress)
if err == nil {
t.Errorf("AddOrUpdateIngressFailsWithInvalidTemplate returned \n%v, but expected \n%v", nil, "template execution error")
t.Errorf("AddOrUpdateIngress returned \n%v, but expected \n%v", nil, "template execution error")
}
if len(warnings) != 0 {
t.Errorf("AddOrUpdateIngress returned warnings: %v", warnings)
}
}

Expand All @@ -119,10 +128,13 @@ func TestAddOrUpdateMergeableIngressFailsWithInvalidIngressTemplate(t *testing.T

mergeableIngess := createMergeableCafeIngress()

err = cnf.AddOrUpdateMergeableIngress(mergeableIngess)
warnings, err := cnf.AddOrUpdateMergeableIngress(mergeableIngess)
if err == nil {
t.Errorf("AddOrUpdateMergeableIngress returned \n%v, but expected \n%v", nil, "template execution error")
}
if len(warnings) != 0 {
t.Errorf("AddOrUpdateMergeableIngress returned warnings: %v", warnings)
}
}

func TestUpdateEndpoints(t *testing.T) {
Expand Down
Loading

0 comments on commit b64293d

Please sign in to comment.