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

Error out on invalid ingress frontend configuration #950

Merged
merged 1 commit into from
Nov 20, 2019
Merged

Error out on invalid ingress frontend configuration #950

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
16 changes: 6 additions & 10 deletions pkg/loadbalancers/l7.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,8 @@ const (
// DefaultPath is the path used if none is specified. It is a valid path
// recognized by GCE.
DefaultPath = "/*"

invalidConfigErrorMessage = "invalid ingress frontend configuration, please check your usage of the 'kubernetes.io/ingress.allow-http' annotation."
)

// L7RuntimeInfo is info passed to this module from the controller runtime.
Expand Down Expand Up @@ -152,10 +154,11 @@ func (l *L7) UrlMap() *composite.UrlMap {
}

func (l *L7) edgeHop() error {
// Keeps track if we will "try" to setup frontend resources based on user configuration.
// If user configuration dictates we do not, then we emit an event.
willConfigureFrontend := false
sslConfigured := l.runtimeInfo.TLS != nil || l.runtimeInfo.TLSName != ""
// Return an error if user configuration species that both HTTP & HTTPS are not to be configured.
if !l.runtimeInfo.AllowHTTP && !sslConfigured {
return fmt.Errorf(invalidConfigErrorMessage)
}

// Check for invalid L7-ILB HTTPS config before attempting sync
if flags.F.EnableL7Ilb && utils.IsGCEL7ILBIngress(l.runtimeInfo.Ingress) && sslConfigured && l.runtimeInfo.AllowHTTP {
Expand All @@ -167,7 +170,6 @@ func (l *L7) edgeHop() error {
return err
}
if l.runtimeInfo.AllowHTTP {
willConfigureFrontend = true
if err := l.edgeHopHttp(); err != nil {
return err
}
Expand All @@ -185,7 +187,6 @@ func (l *L7) edgeHop() error {
}
}
if sslConfigured {
willConfigureFrontend = true
klog.V(3).Infof("validating https for %v", l)
if err := l.edgeHopHttps(); err != nil {
return err
Expand All @@ -196,11 +197,6 @@ func (l *L7) edgeHop() error {
}
klog.V(2).Infof("Successfully deleted unused HTTPS frontend resources for load-balancer %s", l)
}

if !willConfigureFrontend {
l.recorder.Eventf(l.runtimeInfo.Ingress, corev1.EventTypeNormal, "WillNotConfigureFrontend", "Will not configure frontend based on Ingress specification. Please check your usage of the 'kubernetes.io/ingress.allow-http' annotation.")
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like the error message here is much better than what you have now. Can you try to incorporate some of this in to the new message?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

}

return nil
}

Expand Down
10 changes: 9 additions & 1 deletion pkg/loadbalancers/loadbalancers_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1543,7 +1543,15 @@ func TestResourceDeletionWithProtocol(t *testing.T) {
delete(expectCerts, certName1)
}

if lb, err = j.pool.Ensure(lbInfo); err != nil {
lb, err = j.pool.Ensure(lbInfo)
if tc.disableHTTP && tc.disableHTTPS {
// we expect an invalid ingress configuration error here.
if err == nil || !strings.Contains(err.Error(), invalidConfigErrorMessage) {
t.Fatalf("pool.Ensure(%+v) = %v, want %v", lbInfo, err, fmt.Errorf(invalidConfigErrorMessage))
}
return
}
if err != nil {
t.Fatalf("pool.Ensure(%+v) = %v, want nil", lbInfo, err)
}
// Update ingress annotations
Expand Down