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.

Sign up for GitHub

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

Jump to bottom

Support warnings for secrets #1265

Merged
merged 3 commits into from
Dec 7, 2020
Merged

Support warnings for secrets #1265

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c037ea7
Remove unused field
pleshakov Dec 3, 2020
b64293d
Support warnings for TLS and JWK secrets in Ing
pleshakov Dec 3, 2020
2da1c32
Support warnings for TLS secrets in VS
pleshakov Dec 4, 2020
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
121 changes: 73 additions & 48 deletions internal/configs/configurator.go
View file
Open in desktop
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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be good to include resNs/resName here?

Suggested change
return allWarnings, 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/%v: %v", resource.GetKind(), resource.GetNamespace(), resource.GetName(), err)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same enhancement can be applied to the other app protect related methods. better to do it in a separate PR

}

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
Open in desktop
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