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

Fail ingress creation if specified staticIP name does not exist #1080

Merged
merged 2 commits into from
May 15, 2020
Merged
Show file tree
Hide file tree
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
6 changes: 5 additions & 1 deletion pkg/loadbalancers/addresses.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,11 @@ func (l *L7) checkStaticIP() (err error) {
}
managedStaticIPName := l.namer.ForwardingRule(namer.HTTPProtocol)
// Don't manage staticIPs if the user has specified an IP.
if address, manageStaticIP := l.getEffectiveIP(); !manageStaticIP {
address, manageStaticIP, err := l.getEffectiveIP()
if err != nil {
return err
}
if !manageStaticIP {
klog.V(3).Infof("Not managing user specified static IP %v", address)
if flags.F.EnableDeleteUnusedFrontends {
// Delete ingress controller managed static ip if exists.
Expand Down
31 changes: 18 additions & 13 deletions pkg/loadbalancers/forwarding_rules.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import (

"github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud"
"github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/meta"
"k8s.io/api/core/v1"
v1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/types"
"k8s.io/ingress-gce/pkg/annotations"
"k8s.io/ingress-gce/pkg/composite"
Expand All @@ -42,7 +42,10 @@ func (l *L7) checkHttpForwardingRule() (err error) {
return fmt.Errorf("cannot create forwarding rule without proxy")
}
name := l.namer.ForwardingRule(namer.HTTPProtocol)
address, _ := l.getEffectiveIP()
address, _, err := l.getEffectiveIP()
if err != nil {
return err
}
fw, err := l.checkForwardingRule(name, l.tp.SelfLink, address, httpDefaultPortRange)
if err != nil {
return err
Expand All @@ -57,7 +60,10 @@ func (l *L7) checkHttpsForwardingRule() (err error) {
return nil
}
name := l.namer.ForwardingRule(namer.HTTPSProtocol)
address, _ := l.getEffectiveIP()
address, _, err := l.getEffectiveIP()
if err != nil {
return err
}
fws, err := l.checkForwardingRule(name, l.tps.SelfLink, address, httpsDefaultPortRange)
if err != nil {
return err
Expand Down Expand Up @@ -150,15 +156,14 @@ func (l *L7) checkForwardingRule(name, proxyLink, ip, portRange string) (fw *com
}

// getEffectiveIP returns a string with the IP to use in the HTTP and HTTPS
// forwarding rules, and a boolean indicating if this is an IP the controller
// should manage or not.
func (l *L7) getEffectiveIP() (string, bool) {
// forwarding rules, a boolean indicating if this is an IP the controller
// should manage or not and an error if the specified IP was not found.
func (l *L7) getEffectiveIP() (string, bool, error) {

// A note on IP management:
// User specifies a different IP on startup:
// - We create a forwarding rule with the given IP.
// - If this ip doesn't exist in GCE, we create another one in the hope
// that they will rectify it later on.
// - If this ip doesn't exist in GCE, an error is thrown which fails ingress creation.
// - In the happy case, no static ip is created or deleted by this controller.
// Controller allocates a staticIP/ephemeralIP, but user changes it:
// - We still delete the old static IP, but only when we tear down the
Expand All @@ -175,16 +180,16 @@ func (l *L7) getEffectiveIP() (string, bool) {
// Existing static IPs allocated to forwarding rules will get orphaned
// till the Ingress is torn down.
if ip, err := l.cloud.GetGlobalAddress(l.runtimeInfo.StaticIPName); err != nil || ip == nil {
klog.Warningf("The given static IP name %v doesn't translate to an existing global static IP, ignoring it and allocating a new IP: %v",
l.runtimeInfo.StaticIPName, err)
return "", false, fmt.Errorf("the given static IP name %v doesn't translate to an existing global static IP.",
l.runtimeInfo.StaticIPName)
} else {
return ip.Address, false
return ip.Address, false, nil
}
}
if l.ip != nil {
return l.ip.Address, true
return l.ip.Address, true, nil
}
return "", true
return "", true, nil
}

// ensureForwardingRule creates a forwarding rule with the given name, if it does not exist. It updates the existing
Expand Down
32 changes: 32 additions & 0 deletions pkg/loadbalancers/loadbalancers_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -987,6 +987,38 @@ func TestCreateBothLoadBalancers(t *testing.T) {
}
}

// Test StaticIP annotation behavior.
// When a non-existent StaticIP value is specified, ingress creation must fail.
func TestStaticIP(t *testing.T) {
j := newTestJig(t)
gceUrlMap := utils.NewGCEURLMap()
gceUrlMap.DefaultBackend = &utils.ServicePort{NodePort: 31234, BackendNamer: j.namer}
gceUrlMap.PutPathRulesForHost("bar.example.com", []utils.PathRule{{Path: "/bar", Backend: utils.ServicePort{NodePort: 30000, BackendNamer: j.namer}}})
ing := newIngress()
ing.Annotations = map[string]string{
"StaticIPNameKey": "teststaticip",
}
lbInfo := &L7RuntimeInfo{
AllowHTTP: true,
TLS: []*TLSCerts{{Key: "key", Cert: "cert"}},
UrlMap: gceUrlMap,
Ingress: ing,
StaticIPName: "teststaticip",
}

if _, err := j.pool.Ensure(lbInfo); err == nil {
t.Fatalf("expected error ensuring ingress with non-existent static ip")
}
// Create static IP
err := j.fakeGCE.ReserveGlobalAddress(&compute.Address{Name: "teststaticip", Address: "1.2.3.4"})
if err != nil {
t.Fatalf("ip address reservation failed - %v", err)
}
if _, err := j.pool.Ensure(lbInfo); err != nil {
t.Fatalf("unexpected error %v", err)
}
}

// Test setting frontendconfig Ssl policy
func TestFrontendConfigSslPolicy(t *testing.T) {
flags.F.EnableFrontendConfig = true
Expand Down