From d5344ecc0ab1a2b8e9eec237132db0aec1ec4933 Mon Sep 17 00:00:00 2001 From: Spencer Hance Date: Fri, 15 May 2020 15:00:24 -0700 Subject: [PATCH] Implements regional static IP support for L7-ILB ingress This is done via a regional-static-ip annotation on the ingress --- pkg/annotations/ingress.go | 46 ++++++++++-- pkg/annotations/ingress_test.go | 42 +++++++++-- pkg/composite/meta/meta.go | 2 +- pkg/controller/controller.go | 7 +- pkg/fuzz/features/static_ip.go | 2 +- pkg/fuzz/helpers.go | 2 +- pkg/loadbalancers/addresses.go | 24 +++++-- pkg/loadbalancers/forwarding_rules.go | 12 +++- pkg/loadbalancers/forwarding_rules_test.go | 84 ++++++++++++++++++++++ pkg/loadbalancers/l7.go | 11 +-- pkg/loadbalancers/loadbalancers_test.go | 51 +++++++++++-- 11 files changed, 249 insertions(+), 34 deletions(-) create mode 100644 pkg/loadbalancers/forwarding_rules_test.go diff --git a/pkg/annotations/ingress.go b/pkg/annotations/ingress.go index 44720539c9..a21c0c5444 100644 --- a/pkg/annotations/ingress.go +++ b/pkg/annotations/ingress.go @@ -17,9 +17,11 @@ limitations under the License. package annotations import ( + "errors" "strconv" "k8s.io/api/networking/v1beta1" + "k8s.io/ingress-gce/pkg/flags" ) const ( @@ -34,12 +36,19 @@ const ( // rules for port 443 based on the TLS section. AllowHTTPKey = "kubernetes.io/ingress.allow-http" - // StaticIPNameKey tells the Ingress controller to use a specific GCE + // GlobalStaticIPNameKey tells the Ingress controller to use a specific GCE // static ip for its forwarding rules. If specified, the Ingress controller // assigns the static ip by this name to the forwarding rules of the given // Ingress. The controller *does not* manage this ip, it is the users // responsibility to create/delete it. - StaticIPNameKey = "kubernetes.io/ingress.global-static-ip-name" + GlobalStaticIPNameKey = "kubernetes.io/ingress.global-static-ip-name" + + // RegionalStaticIPNameKey tells the Ingress controller to use a specific GCE + // internal static ip for its forwarding rules. If specified, the Ingress controller + // assigns the static ip by this name to the forwarding rules of the given + // Ingress. The controller *does not* manage this ip, it is the users + // responsibility to create/delete it. + RegionalStaticIPNameKey = "kubernetes.io/ingress.regional-static-ip-name" // PreSharedCertKey represents the specific pre-shared SSL // certificate for the Ingress controller to use. The controller *does not* @@ -50,7 +59,7 @@ const ( // IngressClassKey picks a specific "class" for the Ingress. The controller // only processes Ingresses with this annotation either unset, or set - // to either gceIngessClass or the empty string. + // to either gceIngressClass or the empty string. IngressClassKey = "kubernetes.io/ingress.class" GceIngressClass = "gce" GceMultiIngressClass = "gce-multi-cluster" @@ -131,8 +140,35 @@ func (ing *Ingress) UseNamedTLS() string { return val } -func (ing *Ingress) StaticIPName() string { - val, ok := ing.v[StaticIPNameKey] +func (ing *Ingress) StaticIPName() (string, error) { + if !flags.F.EnableL7Ilb { + return ing.GlobalStaticIPName(), nil + } + + globalIp := ing.GlobalStaticIPName() + regionalIp := ing.RegionalStaticIPName() + + if globalIp != "" && regionalIp != "" { + return "", errors.New("Error: both global-static-ip and regional-static-ip cannot be specified") + } + + if regionalIp != "" { + return regionalIp, nil + } + + return globalIp, nil +} + +func (ing *Ingress) GlobalStaticIPName() string { + val, ok := ing.v[GlobalStaticIPNameKey] + if !ok { + return "" + } + return val +} + +func (ing *Ingress) RegionalStaticIPName() string { + val, ok := ing.v[RegionalStaticIPNameKey] if !ok { return "" } diff --git a/pkg/annotations/ingress_test.go b/pkg/annotations/ingress_test.go index 55fd061a1e..1d8a3e53bb 100644 --- a/pkg/annotations/ingress_test.go +++ b/pkg/annotations/ingress_test.go @@ -21,28 +21,49 @@ import ( "k8s.io/api/networking/v1beta1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/ingress-gce/pkg/flags" ) func TestIngress(t *testing.T) { for _, tc := range []struct { + desc string ing *v1beta1.Ingress allowHTTP bool useNamedTLS string staticIPName string ingressClass string + wantErr bool }{ { + desc: "Empty ingress", ing: &v1beta1.Ingress{}, allowHTTP: true, // defaults to true. }, { + desc: "Global and Regional StaticIP Specified", ing: &v1beta1.Ingress{ ObjectMeta: metav1.ObjectMeta{ Annotations: map[string]string{ - AllowHTTPKey: "false", - IngressClassKey: "gce", - PreSharedCertKey: "shared-cert-key", - StaticIPNameKey: "1.2.3.4", + GlobalStaticIPNameKey: "1.2.3.4", + RegionalStaticIPNameKey: "10.0.0.0", + IngressClassKey: GceL7ILBIngressClass, + }, + }, + }, + ingressClass: GceL7ILBIngressClass, + staticIPName: "", + allowHTTP: true, + wantErr: true, + }, + { + desc: "Test most annotations", + ing: &v1beta1.Ingress{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + AllowHTTPKey: "false", + IngressClassKey: "gce", + PreSharedCertKey: "shared-cert-key", + GlobalStaticIPNameKey: "1.2.3.4", }, }, }, @@ -53,14 +74,23 @@ func TestIngress(t *testing.T) { }, } { ing := FromIngress(tc.ing) + + if tc.ingressClass == GceL7ILBIngressClass { + flags.F.EnableL7Ilb = true + } + if x := ing.AllowHTTP(); x != tc.allowHTTP { t.Errorf("ingress %+v; AllowHTTP() = %v, want %v", tc.ing, x, tc.allowHTTP) } if x := ing.UseNamedTLS(); x != tc.useNamedTLS { t.Errorf("ingress %+v; UseNamedTLS() = %v, want %v", tc.ing, x, tc.useNamedTLS) } - if x := ing.StaticIPName(); x != tc.staticIPName { - t.Errorf("ingress %+v; StaticIPName() = %v, want %v", tc.ing, x, tc.staticIPName) + staticIp, err := ing.StaticIPName() + if (err != nil) != tc.wantErr { + t.Errorf("ingress: %+v, err = %v, wantErr = %v", tc.ing, err, tc.wantErr) + } + if staticIp != tc.staticIPName { + t.Errorf("ingress %+v; GlobalStaticIPName() = %v, want %v", tc.ing, staticIp, tc.staticIPName) } if x := ing.IngressClass(); x != tc.ingressClass { t.Errorf("ingress %+v; IngressClass() = %v, want %v", tc.ing, x, tc.ingressClass) diff --git a/pkg/composite/meta/meta.go b/pkg/composite/meta/meta.go index fbca8093b5..39d64a8353 100644 --- a/pkg/composite/meta/meta.go +++ b/pkg/composite/meta/meta.go @@ -36,7 +36,7 @@ const ( // The other types that are discovered as dependencies will simply be wrapped with a composite struct // The format of the map is ServiceName -> k8s-cloud-provider wrapper name var MainServices = map[string]string{ - "Address": "Addresses", + "Address": "Addresses", "BackendService": "BackendServices", "ForwardingRule": "ForwardingRules", "HealthCheck": "HealthChecks", diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go index 4fb268b8de..b68d987b07 100644 --- a/pkg/controller/controller.go +++ b/pkg/controller/controller.go @@ -673,12 +673,17 @@ func (lbc *LoadBalancerController) toRuntimeInfo(ing *v1beta1.Ingress, urlMap *u feConfig = feConfig.DeepCopy() } + staticIPName, err := annotations.StaticIPName() + if err != nil { + return nil, err + } + return &loadbalancers.L7RuntimeInfo{ TLS: tls, TLSName: annotations.UseNamedTLS(), Ingress: ing, AllowHTTP: annotations.AllowHTTP(), - StaticIPName: annotations.StaticIPName(), + StaticIPName: staticIPName, UrlMap: urlMap, FrontendConfig: feConfig, }, nil diff --git a/pkg/fuzz/features/static_ip.go b/pkg/fuzz/features/static_ip.go index 0f3dab6ab0..86a32a33c1 100644 --- a/pkg/fuzz/features/static_ip.go +++ b/pkg/fuzz/features/static_ip.go @@ -69,7 +69,7 @@ func (v *staticIPValidator) ConfigureAttributes(env fuzz.ValidatorEnv, ing *v1be // CheckResponse implements fuzz.FeatureValidator func (v *staticIPValidator) CheckResponse(host, path string, resp *http.Response, body []byte) (fuzz.CheckResponseAction, error) { - addrName := annotations.FromIngress(v.ing).StaticIPName() + addrName := annotations.FromIngress(v.ing).GlobalStaticIPName() if addrName == "" { return fuzz.CheckResponseContinue, nil } diff --git a/pkg/fuzz/helpers.go b/pkg/fuzz/helpers.go index 0ce4b66ba3..968baf560f 100644 --- a/pkg/fuzz/helpers.go +++ b/pkg/fuzz/helpers.go @@ -262,7 +262,7 @@ func (i *IngressBuilder) AddStaticIP(name string) *IngressBuilder { if i.ing.Annotations == nil { i.ing.Annotations = make(map[string]string) } - i.ing.Annotations[annotations.StaticIPNameKey] = name + i.ing.Annotations[annotations.GlobalStaticIPNameKey] = name return i } diff --git a/pkg/loadbalancers/addresses.go b/pkg/loadbalancers/addresses.go index ea59755ce9..d76b8b9d9b 100644 --- a/pkg/loadbalancers/addresses.go +++ b/pkg/loadbalancers/addresses.go @@ -20,16 +20,19 @@ import ( "fmt" "net/http" - "google.golang.org/api/compute/v1" + "github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/meta" "k8s.io/ingress-gce/pkg/annotations" + "k8s.io/ingress-gce/pkg/composite" "k8s.io/ingress-gce/pkg/flags" "k8s.io/ingress-gce/pkg/utils" "k8s.io/ingress-gce/pkg/utils/namer" "k8s.io/klog" ) -// checkStaticIP reserves a static IP allocated to the Forwarding Rule. +// checkStaticIP reserves a regional or global static IP allocated to the Forwarding Rule. func (l *L7) checkStaticIP() (err error) { + isInternal := flags.F.EnableL7Ilb && utils.IsGCEL7ILBIngress(&l.ingress) + if l.fw == nil || l.fw.IPAddress == "" { return fmt.Errorf("will not create static IP without a forwarding rule") } @@ -50,10 +53,21 @@ func (l *L7) checkStaticIP() (err error) { return nil } - ip, _ := l.cloud.GetGlobalAddress(managedStaticIPName) + key, err := l.CreateKey(managedStaticIPName) + if err != nil { + return err + } + + ip, _ := composite.GetAddress(l.cloud, key, meta.VersionGA) if ip == nil { klog.V(3).Infof("Creating static ip %v", managedStaticIPName) - err = l.cloud.ReserveGlobalAddress(&compute.Address{Name: managedStaticIPName, Address: l.fw.IPAddress}) + address := &composite.Address{Name: managedStaticIPName, Address: l.fw.IPAddress, Version: meta.VersionGA} + if isInternal { + // Used for L7 ILB + address.AddressType = "INTERNAL" + } + + err = composite.CreateAddress(l.cloud, key, address) if err != nil { if utils.IsHTTPErrorCode(err, http.StatusConflict) || utils.IsHTTPErrorCode(err, http.StatusBadRequest) { @@ -63,7 +77,7 @@ func (l *L7) checkStaticIP() (err error) { } return err } - ip, err = l.cloud.GetGlobalAddress(managedStaticIPName) + ip, err = composite.GetAddress(l.cloud, key, meta.VersionGA) if err != nil { return err } diff --git a/pkg/loadbalancers/forwarding_rules.go b/pkg/loadbalancers/forwarding_rules.go index 94f19474b9..00f0ab386d 100644 --- a/pkg/loadbalancers/forwarding_rules.go +++ b/pkg/loadbalancers/forwarding_rules.go @@ -163,11 +163,17 @@ func (l *L7) getEffectiveIP() (string, bool, error) { // TODO: Handle the last case better. if l.runtimeInfo.StaticIPName != "" { + key, err := l.CreateKey(l.runtimeInfo.StaticIPName) + if err != nil { + return "", false, err + } + // 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 { - return "", false, fmt.Errorf("the given static IP name %v doesn't translate to an existing global static IP.", - l.runtimeInfo.StaticIPName) + // TODO(shance): Replace version + if ip, err := composite.GetAddress(l.cloud, key, meta.VersionGA); err != nil || ip == nil { + klog.Warningf("The given static IP name %v doesn't translate to an existing static IP, ignoring it and allocating a new IP: %v", + l.runtimeInfo.StaticIPName, err) } else { return ip.Address, false, nil } diff --git a/pkg/loadbalancers/forwarding_rules_test.go b/pkg/loadbalancers/forwarding_rules_test.go new file mode 100644 index 0000000000..a325e2da1e --- /dev/null +++ b/pkg/loadbalancers/forwarding_rules_test.go @@ -0,0 +1,84 @@ +package loadbalancers + +import ( + "testing" + + "github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/meta" + "k8s.io/ingress-gce/pkg/composite" + "k8s.io/legacy-cloud-providers/gce" +) + +func TestGetEffectiveIP(t *testing.T) { + testCases := []struct { + desc string + address *composite.Address + scope meta.KeyType + wantIp string + wantManaged bool + wantErr bool + }{ + { + desc: "L7 ILB with Address created", + address: &composite.Address{Name: "test-ilb", Address: "10.2.3.4"}, + scope: meta.Regional, + wantIp: "10.2.3.4", + wantManaged: false, + wantErr: false, + }, + { + desc: "L7 ILB without address created", + scope: meta.Regional, + wantManaged: true, + wantErr: false, + }, + { + desc: "XLB with Address created", + address: &composite.Address{Name: "test-ilb", Address: "35.2.3.4"}, + scope: meta.Global, + wantIp: "35.2.3.4", + wantManaged: false, + wantErr: false, + }, + { + desc: "XLB without Address created", + scope: meta.Global, + wantManaged: true, + wantErr: false, + }, + } + for _, tc := range testCases { + t.Run(tc.desc, func(t *testing.T) { + fakeGCE := gce.NewFakeGCECloud(gce.DefaultTestClusterValues()) + l7 := L7{ + cloud: fakeGCE, + scope: tc.scope, + runtimeInfo: &L7RuntimeInfo{StaticIPName: ""}, + } + + // Create Address if specified + if tc.address != nil { + key, err := l7.CreateKey(tc.address.Name) + if err != nil { + t.Fatal(err) + } + err = composite.CreateAddress(fakeGCE, key, tc.address) + if err != nil { + t.Fatal(err) + } + l7.runtimeInfo.StaticIPName = tc.address.Name + } + + ip, managed, err := l7.getEffectiveIP() + if (err != nil) != tc.wantErr { + t.Errorf("getEffectiveIP() error = %v, wantErr %v", err, tc.wantErr) + return + } + if tc.address != nil && ip != tc.wantIp { + t.Errorf("getEffectiveIP() ip = %v, want %v", ip, tc.wantIp) + } + if managed != tc.wantManaged { + t.Errorf("getEffectiveIP() managed = %v, want %v", managed, tc.wantManaged) + } + }) + } +} diff --git a/pkg/loadbalancers/l7.go b/pkg/loadbalancers/l7.go index 74ebdf90b2..abbce2d950 100644 --- a/pkg/loadbalancers/l7.go +++ b/pkg/loadbalancers/l7.go @@ -19,11 +19,11 @@ package loadbalancers import ( "encoding/json" "fmt" - "k8s.io/ingress-gce/pkg/flags" "strings" + "k8s.io/ingress-gce/pkg/flags" + "github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/meta" - "google.golang.org/api/compute/v1" corev1 "k8s.io/api/core/v1" "k8s.io/api/networking/v1beta1" "k8s.io/apimachinery/pkg/types" @@ -56,8 +56,9 @@ type L7RuntimeInfo struct { // AllowHTTP will not setup :80, if TLS is nil and AllowHTTP is set, // no loadbalancer is created. AllowHTTP bool - // The name of a Global Static IP. If specified, the IP associated with + // The name of a Global/Regional Static IP. If specified, the IP associated with // this name is used in the Forwarding Rules for this loadbalancer. + // If this is an l7-ILB ingress, the static IP is assumed to be internal StaticIPName string // UrlMap is our internal representation of a url map. UrlMap *utils.GCEURLMap @@ -96,8 +97,8 @@ type L7 struct { fw *composite.ForwardingRule // fws is the GlobalForwardingRule that points to the TargetHTTPSProxy. fws *composite.ForwardingRule - // ip is the static-ip associated with both GlobalForwardingRules. - ip *compute.Address + // ip is the static-ip associated with both ForwardingRules. + ip *composite.Address // sslCerts is the list of ssl certs associated with the targetHTTPSProxy. sslCerts []*composite.SslCertificate // oldSSLCerts is the list of certs that used to be hooked up to the diff --git a/pkg/loadbalancers/loadbalancers_test.go b/pkg/loadbalancers/loadbalancers_test.go index 7898b66796..172a8db59d 100644 --- a/pkg/loadbalancers/loadbalancers_test.go +++ b/pkg/loadbalancers/loadbalancers_test.go @@ -177,7 +177,7 @@ func TestCreateHTTPLoadBalancer(t *testing.T) { if err != nil || l7 == nil { t.Fatalf("Expected l7 not created, err: %v", err) } - verifyHTTPForwardingRuleAndProxyLinks(t, j, l7) + verifyHTTPForwardingRuleAndProxyLinks(t, j, l7, "") } func TestCreateHTTPILBLoadBalancer(t *testing.T) { @@ -198,7 +198,41 @@ func TestCreateHTTPILBLoadBalancer(t *testing.T) { if err != nil || l7 == nil { t.Fatalf("Expected l7 not created, err: %v", err) } - verifyHTTPForwardingRuleAndProxyLinks(t, j, l7) + verifyHTTPForwardingRuleAndProxyLinks(t, j, l7, "") +} + +func TestCreateHTTPILBLoadBalancerStaticIp(t *testing.T) { + // This should NOT create the forwarding rule and target proxy + // associated with the HTTPS branch of this loadbalancer. + j := newTestJig(t) + + ipName := "test-ilb-static-ip" + ip := "10.1.2.3" + key, err := composite.CreateKey(j.fakeGCE, ipName, features.L7ILBScope()) + if err != nil { + t.Fatal(err) + } + err = composite.CreateAddress(j.fakeGCE, key, &composite.Address{Name: ipName, Version: meta.VersionGA, Address: ip}) + if err != nil { + t.Fatal(err) + } + + 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}}}) + lbInfo := &L7RuntimeInfo{ + AllowHTTP: true, + UrlMap: gceUrlMap, + Ingress: newILBIngress(), + StaticIPName: ipName, + } + + l7, err := j.pool.Ensure(lbInfo) + if err != nil || l7 == nil { + t.Fatalf("Expected l7 not created, err: %v", err) + } + + verifyHTTPForwardingRuleAndProxyLinks(t, j, l7, ip) } func TestCreateHTTPSILBLoadBalancer(t *testing.T) { @@ -299,7 +333,7 @@ func verifyHTTPSForwardingRuleAndProxyLinks(t *testing.T, j *testJig, l7 *L7) { } } -func verifyHTTPForwardingRuleAndProxyLinks(t *testing.T, j *testJig, l7 *L7) { +func verifyHTTPForwardingRuleAndProxyLinks(t *testing.T, j *testJig, l7 *L7, ip string) { t.Helper() versions := l7.Versions() @@ -329,6 +363,11 @@ func verifyHTTPForwardingRuleAndProxyLinks(t *testing.T, j *testJig, l7 *L7) { if fws.Description == "" { t.Errorf("fws.Description not set; expected it to be") } + if ip != "" { + if fws.IPAddress != ip { + t.Fatalf("fws.IPAddress = %q, want %q", fws.IPAddress, ip) + } + } } // Tests that a certificate is created from the provided Key/Cert combo @@ -966,7 +1005,7 @@ func TestCreateBothLoadBalancers(t *testing.T) { } verifyHTTPSForwardingRuleAndProxyLinks(t, j, l7) - verifyHTTPForwardingRuleAndProxyLinks(t, j, l7) + verifyHTTPForwardingRuleAndProxyLinks(t, j, l7, "") // We know the forwarding rules exist, retrieve their addresses. key, err := composite.CreateKey(j.fakeGCE, "", defaultScope) @@ -1276,7 +1315,7 @@ func TestClusterNameChange(t *testing.T) { t.Fatalf("Expected l7 not created") } verifyHTTPSForwardingRuleAndProxyLinks(t, j, l7) - verifyHTTPForwardingRuleAndProxyLinks(t, j, l7) + verifyHTTPForwardingRuleAndProxyLinks(t, j, l7, "") newName := "newName" j.namer.SetUID(newName) @@ -1287,7 +1326,7 @@ func TestClusterNameChange(t *testing.T) { t.Fatalf("Expected L7 name to change.") } verifyHTTPSForwardingRuleAndProxyLinks(t, j, l7) - verifyHTTPForwardingRuleAndProxyLinks(t, j, l7) + verifyHTTPForwardingRuleAndProxyLinks(t, j, l7, "") } func TestInvalidClusterNameChange(t *testing.T) {