From 27139ddebb3196748079e4a5194ce83719628d68 Mon Sep 17 00:00:00 2001 From: Spencer Hance Date: Thu, 14 May 2020 16:49:06 -0700 Subject: [PATCH 1/2] Add GCE Addresses to Composites --- pkg/composite/gen.go | 382 +++++++++++++++++++++++++++++++++++++ pkg/composite/gen_test.go | 77 ++++++++ pkg/composite/meta/meta.go | 3 + 3 files changed, 462 insertions(+) diff --git a/pkg/composite/gen.go b/pkg/composite/gen.go index 0b4e733f18..76583db249 100644 --- a/pkg/composite/gen.go +++ b/pkg/composite/gen.go @@ -33,6 +33,110 @@ import ( "k8s.io/legacy-cloud-providers/gce" ) +// Address is a composite type wrapping the Alpha, Beta, and GA methods for its GCE equivalent +type Address struct { + // Version keeps track of the intended compute version for this Address. + // Note that the compute API's do not contain this field. It is for our + // own bookkeeping purposes. + Version meta.Version `json:"-"` + // Scope keeps track of the intended type of the service (e.g. Global) + // This is also an internal field purely for bookkeeping purposes + Scope meta.KeyType `json:"-"` + + // The static IP address represented by this resource. + Address string `json:"address,omitempty"` + // The type of address to reserve, either INTERNAL or EXTERNAL. If + // unspecified, defaults to EXTERNAL. + AddressType string `json:"addressType,omitempty"` + // [Output Only] Creation timestamp in RFC3339 text format. + CreationTimestamp string `json:"creationTimestamp,omitempty"` + // An optional description of this resource. Provide this field when you + // create the resource. + Description string `json:"description,omitempty"` + // [Output Only] The unique identifier for the resource. This identifier + // is defined by the server. + Id uint64 `json:"id,omitempty,string"` + // The IP version that will be used by this address. Valid options are + // IPV4 or IPV6. This can only be specified for a global address. + IpVersion string `json:"ipVersion,omitempty"` + // [Output Only] Type of the resource. Always compute#address for + // addresses. + Kind string `json:"kind,omitempty"` + // A fingerprint for the labels being applied to this Address, which is + // essentially a hash of the labels set used for optimistic locking. The + // fingerprint is initially generated by Compute Engine and changes + // after every request to modify or update labels. You must always + // provide an up-to-date fingerprint hash in order to update or change + // labels, otherwise the request will fail with error 412 + // conditionNotMet. + // + // To see the latest fingerprint, make a get() request to retrieve an + // Address. + LabelFingerprint string `json:"labelFingerprint,omitempty"` + // Labels to apply to this Address resource. These can be later modified + // by the setLabels method. Each label key/value must comply with + // RFC1035. Label values may be empty. + Labels map[string]string `json:"labels,omitempty"` + // Name of the resource. Provided by the client when the resource is + // created. The name must be 1-63 characters long, and comply with + // RFC1035. Specifically, the name must be 1-63 characters long and + // match the regular expression `[a-z]([-a-z0-9]*[a-z0-9])?`. The first + // character must be a lowercase letter, and all following characters + // (except for the last character) must be a dash, lowercase letter, or + // digit. The last character must be a lowercase letter or digit. + Name string `json:"name,omitempty"` + // The URL of the network in which to reserve the address. This field + // can only be used with INTERNAL type with the VPC_PEERING purpose. + Network string `json:"network,omitempty"` + // This signifies the networking tier used for configuring this address + // and can only take the following values: PREMIUM or STANDARD. Global + // forwarding rules can only be Premium Tier. Regional forwarding rules + // can be either Premium or Standard Tier. Standard Tier addresses + // applied to regional forwarding rules can be used with any external + // load balancer. Regional forwarding rules in Premium Tier can only be + // used with a network load balancer. + // + // If this field is not specified, it is assumed to be PREMIUM. + NetworkTier string `json:"networkTier,omitempty"` + // The prefix length if the resource reprensents an IP range. + PrefixLength int64 `json:"prefixLength,omitempty"` + // The purpose of this resource, which can be one of the following + // values: + // - `GCE_ENDPOINT` for addresses that are used by VM instances, alias + // IP ranges, internal load balancers, and similar resources. + // - `DNS_RESOLVER` for a DNS resolver address in a subnetwork + // - `VPC_PEERING` for addresses that are reserved for VPC peer + // networks. + // - `NAT_AUTO` for addresses that are external IP addresses + // automatically reserved for Cloud NAT. + Purpose string `json:"purpose,omitempty"` + // [Output Only] The URL of the region where the regional address + // resides. This field is not applicable to global addresses. You must + // specify this field as part of the HTTP request URL. + Region string `json:"region,omitempty"` + // [Output Only] Server-defined URL for the resource. + SelfLink string `json:"selfLink,omitempty"` + // [Output Only] Server-defined URL for this resource with the resource + // id. + SelfLinkWithId string `json:"selfLinkWithId,omitempty"` + // [Output Only] The status of the address, which can be one of + // RESERVING, RESERVED, or IN_USE. An address that is RESERVING is + // currently in the process of being reserved. A RESERVED address is + // currently reserved and available to use. An IN_USE address is + // currently being used by another resource and is not available. + Status string `json:"status,omitempty"` + // The URL of the subnetwork in which to reserve the address. If an IP + // address is specified, it must be within the subnetwork's IP range. + // This field can only be used with INTERNAL type with a GCE_ENDPOINT or + // DNS_RESOLVER purpose. + Subnetwork string `json:"subnetwork,omitempty"` + // [Output Only] The URLs of the resources that are using this address. + Users []string `json:"users,omitempty"` + googleapi.ServerResponse `json:"-"` + ForceSendFields []string `json:"-"` + NullFields []string `json:"-"` +} + // AuthenticationPolicy is a composite type wrapping the Alpha, Beta, and GA methods for its GCE equivalent type AuthenticationPolicy struct { // List of authentication methods that can be used for origin @@ -2881,6 +2985,284 @@ type WeightedBackendService struct { NullFields []string `json:"-"` } +func CreateAddress(gceCloud *gce.Cloud, key *meta.Key, address *Address) error { + ctx, cancel := cloudprovider.ContextWithCallTimeout() + defer cancel() + mc := compositemetrics.NewMetricContext("Address", "create", key.Region, key.Zone, string(address.Version)) + + switch address.Version { + case meta.VersionAlpha: + alpha, err := address.ToAlpha() + if err != nil { + return err + } + switch key.Type() { + case meta.Regional: + klog.V(3).Infof("Creating alpha region Address %v", alpha.Name) + alpha.Region = key.Region + return mc.Observe(gceCloud.Compute().AlphaAddresses().Insert(ctx, key, alpha)) + default: + klog.V(3).Infof("Creating alpha Address %v", alpha.Name) + return mc.Observe(gceCloud.Compute().AlphaGlobalAddresses().Insert(ctx, key, alpha)) + } + case meta.VersionBeta: + beta, err := address.ToBeta() + if err != nil { + return err + } + switch key.Type() { + case meta.Regional: + klog.V(3).Infof("Creating beta region Address %v", beta.Name) + beta.Region = key.Region + return mc.Observe(gceCloud.Compute().BetaAddresses().Insert(ctx, key, beta)) + default: + klog.V(3).Infof("Creating beta Address %v", beta.Name) + return mc.Observe(gceCloud.Compute().BetaGlobalAddresses().Insert(ctx, key, beta)) + } + default: + ga, err := address.ToGA() + if err != nil { + return err + } + switch key.Type() { + case meta.Regional: + klog.V(3).Infof("Creating ga region Address %v", ga.Name) + ga.Region = key.Region + return mc.Observe(gceCloud.Compute().Addresses().Insert(ctx, key, ga)) + default: + klog.V(3).Infof("Creating ga Address %v", ga.Name) + return mc.Observe(gceCloud.Compute().GlobalAddresses().Insert(ctx, key, ga)) + } + } +} + +func DeleteAddress(gceCloud *gce.Cloud, key *meta.Key, version meta.Version) error { + ctx, cancel := cloudprovider.ContextWithCallTimeout() + defer cancel() + mc := compositemetrics.NewMetricContext("Address", "delete", key.Region, key.Zone, string(version)) + + switch version { + case meta.VersionAlpha: + switch key.Type() { + case meta.Regional: + klog.V(3).Infof("Deleting alpha region Address %v", key.Name) + return mc.Observe(gceCloud.Compute().AlphaAddresses().Delete(ctx, key)) + default: + klog.V(3).Infof("Deleting alpha Address %v", key.Name) + return mc.Observe(gceCloud.Compute().AlphaGlobalAddresses().Delete(ctx, key)) + } + case meta.VersionBeta: + switch key.Type() { + case meta.Regional: + klog.V(3).Infof("Deleting beta region Address %v", key.Name) + return mc.Observe(gceCloud.Compute().BetaAddresses().Delete(ctx, key)) + default: + klog.V(3).Infof("Deleting beta Address %v", key.Name) + return mc.Observe(gceCloud.Compute().BetaGlobalAddresses().Delete(ctx, key)) + } + default: + switch key.Type() { + case meta.Regional: + klog.V(3).Infof("Deleting ga region Address %v", key.Name) + return mc.Observe(gceCloud.Compute().Addresses().Delete(ctx, key)) + default: + klog.V(3).Infof("Deleting ga Address %v", key.Name) + return mc.Observe(gceCloud.Compute().GlobalAddresses().Delete(ctx, key)) + } + } +} + +func GetAddress(gceCloud *gce.Cloud, key *meta.Key, version meta.Version) (*Address, error) { + ctx, cancel := cloudprovider.ContextWithCallTimeout() + defer cancel() + mc := compositemetrics.NewMetricContext("Address", "get", key.Region, key.Zone, string(version)) + + var gceObj interface{} + var err error + switch version { + case meta.VersionAlpha: + switch key.Type() { + case meta.Regional: + klog.V(3).Infof("Getting alpha region Address %v", key.Name) + gceObj, err = gceCloud.Compute().AlphaAddresses().Get(ctx, key) + default: + klog.V(3).Infof("Getting alpha Address %v", key.Name) + gceObj, err = gceCloud.Compute().AlphaGlobalAddresses().Get(ctx, key) + } + case meta.VersionBeta: + switch key.Type() { + case meta.Regional: + klog.V(3).Infof("Getting beta region Address %v", key.Name) + gceObj, err = gceCloud.Compute().BetaAddresses().Get(ctx, key) + default: + klog.V(3).Infof("Getting beta Address %v", key.Name) + gceObj, err = gceCloud.Compute().BetaGlobalAddresses().Get(ctx, key) + } + default: + switch key.Type() { + case meta.Regional: + klog.V(3).Infof("Getting ga region Address %v", key.Name) + gceObj, err = gceCloud.Compute().Addresses().Get(ctx, key) + default: + klog.V(3).Infof("Getting ga Address %v", key.Name) + gceObj, err = gceCloud.Compute().GlobalAddresses().Get(ctx, key) + } + } + if err != nil { + return nil, mc.Observe(err) + } + compositeType, err := toAddress(gceObj) + if err != nil { + return nil, err + } + if key.Type() == meta.Regional { + compositeType.Scope = meta.Regional + } + compositeType.Version = version + return compositeType, nil +} + +func ListAddresses(gceCloud *gce.Cloud, key *meta.Key, version meta.Version) ([]*Address, error) { + ctx, cancel := cloudprovider.ContextWithCallTimeout() + defer cancel() + mc := compositemetrics.NewMetricContext("Address", "list", key.Region, key.Zone, string(version)) + + var gceObjs interface{} + var err error + switch version { + case meta.VersionAlpha: + switch key.Type() { + case meta.Regional: + klog.V(3).Infof("Listing alpha region Address") + gceObjs, err = gceCloud.Compute().AlphaAddresses().List(ctx, key.Region, filter.None) + default: + klog.V(3).Infof("Listing alpha Address") + gceObjs, err = gceCloud.Compute().AlphaGlobalAddresses().List(ctx, filter.None) + } + case meta.VersionBeta: + switch key.Type() { + case meta.Regional: + klog.V(3).Infof("Listing beta region Address") + gceObjs, err = gceCloud.Compute().BetaAddresses().List(ctx, key.Region, filter.None) + default: + klog.V(3).Infof("Listing beta Address") + gceObjs, err = gceCloud.Compute().BetaGlobalAddresses().List(ctx, filter.None) + } + default: + switch key.Type() { + case meta.Regional: + klog.V(3).Infof("Listing ga region Address") + gceObjs, err = gceCloud.Compute().Addresses().List(ctx, key.Region, filter.None) + default: + klog.V(3).Infof("Listing ga Address") + gceObjs, err = gceCloud.Compute().GlobalAddresses().List(ctx, filter.None) + } + } + if err != nil { + return nil, mc.Observe(err) + } + + compositeObjs, err := toAddressList(gceObjs) + if err != nil { + return nil, err + } + for _, obj := range compositeObjs { + obj.Version = version + } + return compositeObjs, nil +} + +// toAddressList converts a list of compute alpha, beta or GA +// Address into a list of our composite type. +func toAddressList(objs interface{}) ([]*Address, error) { + result := []*Address{} + + err := copyViaJSON(&result, objs) + if err != nil { + return nil, fmt.Errorf("could not copy object %v to %T via JSON: %v", objs, result, err) + } + return result, nil +} + +// toAddress is for package internal use only (not type-safe). +func toAddress(obj interface{}) (*Address, error) { + x := &Address{} + err := copyViaJSON(x, obj) + if err != nil { + return nil, fmt.Errorf("could not copy object %+v to %T via JSON: %v", obj, x, err) + } + return x, nil +} + +// Users external to the package need to pass in the correct type to create a +// composite. + +// AlphaToAddress convert to a composite type. +func AlphaToAddress(obj *computealpha.Address) (*Address, error) { + x := &Address{} + err := copyViaJSON(x, obj) + if err != nil { + return nil, fmt.Errorf("could not copy object %+v to %T via JSON: %v", obj, x, err) + } + return x, nil +} + +// BetaToAddress convert to a composite type. +func BetaToAddress(obj *computebeta.Address) (*Address, error) { + x := &Address{} + err := copyViaJSON(x, obj) + if err != nil { + return nil, fmt.Errorf("could not copy object %+v to %T via JSON: %v", obj, x, err) + } + return x, nil +} + +// GAToAddress convert to a composite type. +func GAToAddress(obj *compute.Address) (*Address, error) { + x := &Address{} + err := copyViaJSON(x, obj) + if err != nil { + return nil, fmt.Errorf("could not copy object %+v to %T via JSON: %v", obj, x, err) + } + return x, nil +} + +// ToAlpha converts our composite type into an alpha type. +// This alpha type can be used in GCE API calls. +func (address *Address) ToAlpha() (*computealpha.Address, error) { + alpha := &computealpha.Address{} + err := copyViaJSON(alpha, address) + if err != nil { + return nil, fmt.Errorf("error converting %T to compute alpha type via JSON: %v", address, err) + } + + return alpha, nil +} + +// ToBeta converts our composite type into an beta type. +// This beta type can be used in GCE API calls. +func (address *Address) ToBeta() (*computebeta.Address, error) { + beta := &computebeta.Address{} + err := copyViaJSON(beta, address) + if err != nil { + return nil, fmt.Errorf("error converting %T to compute beta type via JSON: %v", address, err) + } + + return beta, nil +} + +// ToGA converts our composite type into an ga type. +// This ga type can be used in GCE API calls. +func (address *Address) ToGA() (*compute.Address, error) { + ga := &compute.Address{} + err := copyViaJSON(ga, address) + if err != nil { + return nil, fmt.Errorf("error converting %T to compute ga type via JSON: %v", address, err) + } + + return ga, nil +} + func CreateBackendService(gceCloud *gce.Cloud, key *meta.Key, backendService *BackendService) error { ctx, cancel := cloudprovider.ContextWithCallTimeout() defer cancel() diff --git a/pkg/composite/gen_test.go b/pkg/composite/gen_test.go index 1fef09441e..b16cce6bb1 100644 --- a/pkg/composite/gen_test.go +++ b/pkg/composite/gen_test.go @@ -29,6 +29,83 @@ import ( compute "google.golang.org/api/compute/v1" ) +func TestAddress(t *testing.T) { + // Use reflection to verify that our composite type contains all the + // same fields as the alpha type. + compositeType := reflect.TypeOf(Address{}) + alphaType := reflect.TypeOf(computealpha.Address{}) + betaType := reflect.TypeOf(computebeta.Address{}) + gaType := reflect.TypeOf(compute.Address{}) + + // For the composite type, remove the Version field from consideration + compositeTypeNumFields := compositeType.NumField() - 2 + if compositeTypeNumFields != alphaType.NumField() { + t.Fatalf("%v should contain %v fields. Got %v", alphaType.Name(), alphaType.NumField(), compositeTypeNumFields) + } + + // Compare all the fields by doing a lookup since we can't guarantee that they'll be in the same order + // Make sure that composite type is strictly alpha fields + internal bookkeeping + for i := 2; i < compositeType.NumField(); i++ { + lookupField, found := alphaType.FieldByName(compositeType.Field(i).Name) + if !found { + t.Fatal(fmt.Errorf("Field %v not present in alpha type %v", compositeType.Field(i), alphaType)) + } + if err := compareFields(compositeType.Field(i), lookupField); err != nil { + t.Fatal(err) + } + } + + // Verify that all beta fields are in composite type + if err := typeEquality(betaType, compositeType, false); err != nil { + t.Fatal(err) + } + + // Verify that all GA fields are in composite type + if err := typeEquality(gaType, compositeType, false); err != nil { + t.Fatal(err) + } +} + +// TODO: these tests don't do anything as they are currently structured. +// func TestToAddress(t *testing.T) + +func TestAddressToAlpha(t *testing.T) { + composite := Address{} + expected := &computealpha.Address{} + result, err := composite.ToAlpha() + if err != nil { + t.Fatalf("Address.ToAlpha() error: %v", err) + } + + if !reflect.DeepEqual(result, expected) { + t.Fatalf("Address.ToAlpha() = \ninput = %s\n%s\nwant = \n%s", pretty.Sprint(composite), pretty.Sprint(result), pretty.Sprint(expected)) + } +} +func TestAddressToBeta(t *testing.T) { + composite := Address{} + expected := &computebeta.Address{} + result, err := composite.ToBeta() + if err != nil { + t.Fatalf("Address.ToBeta() error: %v", err) + } + + if !reflect.DeepEqual(result, expected) { + t.Fatalf("Address.ToBeta() = \ninput = %s\n%s\nwant = \n%s", pretty.Sprint(composite), pretty.Sprint(result), pretty.Sprint(expected)) + } +} +func TestAddressToGA(t *testing.T) { + composite := Address{} + expected := &compute.Address{} + result, err := composite.ToGA() + if err != nil { + t.Fatalf("Address.ToGA() error: %v", err) + } + + if !reflect.DeepEqual(result, expected) { + t.Fatalf("Address.ToGA() = \ninput = %s\n%s\nwant = \n%s", pretty.Sprint(composite), pretty.Sprint(result), pretty.Sprint(expected)) + } +} + func TestAuthenticationPolicy(t *testing.T) { compositeType := reflect.TypeOf(AuthenticationPolicy{}) alphaType := reflect.TypeOf(computealpha.AuthenticationPolicy{}) diff --git a/pkg/composite/meta/meta.go b/pkg/composite/meta/meta.go index 112b3ff1a0..fbca8093b5 100644 --- a/pkg/composite/meta/meta.go +++ b/pkg/composite/meta/meta.go @@ -36,6 +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", "BackendService": "BackendServices", "ForwardingRule": "ForwardingRules", "HealthCheck": "HealthChecks", @@ -55,6 +56,7 @@ var MainServices = map[string]string{ // TODO: (shance) Replace this with data gathered from meta.AllServices // Services in NoUpdate will not have an Update() method generated for them var NoUpdate = sets.NewString( + "Address", "ForwardingRule", "HealthStatusForNetworkEndpoint", "TargetHttpProxy", @@ -88,6 +90,7 @@ var Versions = map[string]string{ // DefaultRegionalServices contains services which are regional by default. // Their global type is explicitly labeled (e.g. GlobalForwardingRule) var DefaultRegionalServices = sets.NewString( + "Address", "ForwardingRule", ) From d6080dd6a61a4304baa473c06ef124aede51735f Mon Sep 17 00:00:00 2001 From: Spencer Hance Date: Fri, 15 May 2020 15:00:24 -0700 Subject: [PATCH 2/2] 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 | 29 ++++++-- pkg/loadbalancers/addresses_test.go | 72 +++++++++++++++++++ pkg/loadbalancers/forwarding_rules.go | 10 ++- pkg/loadbalancers/forwarding_rules_test.go | 84 ++++++++++++++++++++++ pkg/loadbalancers/l7.go | 11 +-- pkg/loadbalancers/loadbalancers_test.go | 51 +++++++++++-- 12 files changed, 325 insertions(+), 33 deletions(-) create mode 100644 pkg/loadbalancers/addresses_test.go 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..5a60b33980 100644 --- a/pkg/loadbalancers/addresses.go +++ b/pkg/loadbalancers/addresses.go @@ -20,15 +20,16 @@ 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) { if l.fw == nil || l.fw.IPAddress == "" { return fmt.Errorf("will not create static IP without a forwarding rule") @@ -50,10 +51,17 @@ 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 := l.newStaticAddress(managedStaticIPName) + + err = composite.CreateAddress(l.cloud, key, address) if err != nil { if utils.IsHTTPErrorCode(err, http.StatusConflict) || utils.IsHTTPErrorCode(err, http.StatusBadRequest) { @@ -63,7 +71,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 } @@ -71,3 +79,14 @@ func (l *L7) checkStaticIP() (err error) { l.ip = ip return nil } + +func (l *L7) newStaticAddress(name string) *composite.Address { + isInternal := flags.F.EnableL7Ilb && utils.IsGCEL7ILBIngress(&l.ingress) + address := &composite.Address{Name: name, Address: l.fw.IPAddress, Version: meta.VersionGA} + if isInternal { + // Used for L7 ILB + address.AddressType = "INTERNAL" + } + + return address +} diff --git a/pkg/loadbalancers/addresses_test.go b/pkg/loadbalancers/addresses_test.go new file mode 100644 index 0000000000..ccec77eac6 --- /dev/null +++ b/pkg/loadbalancers/addresses_test.go @@ -0,0 +1,72 @@ +/* +Copyright 2020 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package loadbalancers + +import ( + "testing" + + "github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/meta" + "github.com/google/go-cmp/cmp" + "k8s.io/api/networking/v1beta1" + "k8s.io/ingress-gce/pkg/annotations" + "k8s.io/ingress-gce/pkg/composite" + "k8s.io/ingress-gce/pkg/flags" +) + +func TestNewStaticAddress(t *testing.T) { + testCases := []struct { + desc string + ip string + name string + isInternal bool + expected *composite.Address + }{ + { + desc: "external static address", + ip: "1.2.3.4", + name: "external-addr", + isInternal: false, + expected: &composite.Address{Name: "external-addr", Version: meta.VersionGA, Address: "1.2.3.4"}, + }, + { + desc: "internal static address", + ip: "10.2.3.4", + name: "internal-addr", + isInternal: true, + expected: &composite.Address{Name: "internal-addr", Version: meta.VersionGA, Address: "10.2.3.4", AddressType: "INTERNAL"}, + }, + } + + for _, tc := range testCases { + t.Run(tc.desc, func(t *testing.T) { + l7 := &L7{ + ingress: v1beta1.Ingress{Spec: v1beta1.IngressSpec{}}, + fw: &composite.ForwardingRule{IPAddress: tc.ip}, + } + + if tc.isInternal { + flags.F.EnableL7Ilb = true + l7.ingress.Annotations = map[string]string{annotations.IngressClassKey: "gce-internal"} + } + + result := l7.newStaticAddress(tc.name) + if diff := cmp.Diff(tc.expected, result); diff != "" { + t.Errorf("Got diff for Address (-want +got):\n%s", diff) + } + }) + } +} diff --git a/pkg/loadbalancers/forwarding_rules.go b/pkg/loadbalancers/forwarding_rules.go index 69aa9a4844..e00f0ff6e8 100644 --- a/pkg/loadbalancers/forwarding_rules.go +++ b/pkg/loadbalancers/forwarding_rules.go @@ -163,10 +163,16 @@ 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.", + // TODO(shance): Replace version + if ip, err := composite.GetAddress(l.cloud, key, meta.VersionGA); err != nil || ip == nil { + return "", false, fmt.Errorf("the given static IP name %v doesn't translate to an existing static IP.", l.runtimeInfo.StaticIPName) } 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) {