Skip to content

Commit

Permalink
Cleanup:
Browse files Browse the repository at this point in the history 
- get rid of one letter variables named 'L/l/I/|/!/1'
- removed unused parameters from assert func in l4 tests
- split l4//l7 neg endpoint calculators into separate files
- refactor l4 neg calculator

no business logic or test logic has been changed in this commit
  • Loading branch information
@cezarygerard
cezarygerard committed Aug 25, 2022
1 parent eb6d2ba commit 826b038
Show file tree
Hide file tree
Showing 18 changed files with 740 additions and 713 deletions.
8 changes: 4 additions & 4 deletions pkg/backends/ig_linker.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78,10 +78,10 @@ func NewInstanceGroupLinker(instancePool instances.NodePool, backendPool Pool) L
}

// Link implements Link.
func (l *instanceGroupLinker) Link(sp utils.ServicePort, groups []GroupKey) error {
func (igl *instanceGroupLinker) Link(sp utils.ServicePort, groups []GroupKey) error {
var igLinks []string
for _, group := range groups {
ig, err := l.instancePool.Get(sp.IGName(), group.Zone)
ig, err := igl.instancePool.Get(sp.IGName(), group.Zone)
if err != nil {
return fmt.Errorf("error retrieving IG for linking with backend %+v: %w", sp, err)
}
Expand All @@ -91,7 +91,7 @@ func (l *instanceGroupLinker) Link(sp utils.ServicePort, groups []GroupKey) erro
// ig_linker only supports L7 HTTP(s) External Load Balancer
// Hardcoded here since IGs are not supported for non GA-Global right now
// TODO(shance): find a way to remove hardcoded values
be, err := l.backendPool.Get(sp.BackendName(), meta.VersionGA, meta.Global)
be, err := igl.backendPool.Get(sp.BackendName(), meta.VersionGA, meta.Global)
if err != nil {
return err
}
Expand Down Expand Up @@ -126,7 +126,7 @@ func (l *instanceGroupLinker) Link(sp utils.ServicePort, groups []GroupKey) erro
newBackends := backendsForIGs(addIGs, bm, &sp)
be.Backends = append(originalIGBackends, newBackends...)

if err := l.backendPool.Update(be); err != nil {
if err := igl.backendPool.Update(be); err != nil {
if utils.IsHTTPErrorCode(err, http.StatusBadRequest) {
klog.V(2).Infof("Updating backend service backends with balancing mode %v failed, will try another mode. err:%v", bm, err)
errs = append(errs, err.Error())
Expand Down
12 changes: 6 additions & 6 deletions pkg/backends/neg_linker.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ func NewNEGLinker(
}

// Link implements Link.
func (l *negLinker) Link(sp utils.ServicePort, groups []GroupKey) error {
func (nl *negLinker) Link(sp utils.ServicePort, groups []GroupKey) error {
version := befeatures.VersionFromServicePort(&sp)
var negSelfLinks []string
var err error
Expand All @@ -70,10 +70,10 @@ func (l *negLinker) Link(sp utils.ServicePort, groups []GroupKey) error {

negUrl := ""
svcNegKey := fmt.Sprintf("%s/%s", sp.ID.Service.Namespace, negName)
negUrl, ok := getNegUrlFromSvcneg(svcNegKey, group.Zone, l.svcNegLister)
negUrl, ok := getNegUrlFromSvcneg(svcNegKey, group.Zone, nl.svcNegLister)
if !ok {
klog.V(4).Infof("Falling back to use NEG API to retreive NEG url for NEG %q", negName)
neg, err := l.negGetter.GetNetworkEndpointGroup(negName, group.Zone, version)
neg, err := nl.negGetter.GetNetworkEndpointGroup(negName, group.Zone, version)
if err != nil {
return err
}
Expand All @@ -85,11 +85,11 @@ func (l *negLinker) Link(sp utils.ServicePort, groups []GroupKey) error {
beName := sp.BackendName()
scope := befeatures.ScopeFromServicePort(&sp)

key, err := composite.CreateKey(l.cloud, beName, scope)
key, err := composite.CreateKey(nl.cloud, beName, scope)
if err != nil {
return err
}
backendService, err := composite.GetBackendService(l.cloud, key, version)
backendService, err := composite.GetBackendService(nl.cloud, key, version)
if err != nil {
return err
}
Expand All @@ -111,7 +111,7 @@ func (l *negLinker) Link(sp utils.ServicePort, groups []GroupKey) error {
klog.V(2).Infof("Backends changed for service port %s, removing: %s, adding: %s, changed: %s", sp.ID, diff.toRemove(), diff.toAdd(), diff.changed)

backendService.Backends = mergedBackend
return composite.UpdateBackendService(l.cloud, key, backendService)
return composite.UpdateBackendService(nl.cloud, key, backendService)
}

type backendDiff struct {
Expand Down
32 changes: 16 additions & 16 deletions pkg/loadbalancers/addresses.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,59 +30,59 @@ import (
)

// 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 == "" {
func (l7 *L7) checkStaticIP() (err error) {
if l7.fw == nil || l7.fw.IPAddress == "" {
return fmt.Errorf("will not create static IP without a forwarding rule")
}
managedStaticIPName := l.namer.ForwardingRule(namer.HTTPProtocol)
managedStaticIPName := l7.namer.ForwardingRule(namer.HTTPProtocol)
// Don't manage staticIPs if the user has specified an IP.
address, manageStaticIP, err := l.getEffectiveIP()
address, manageStaticIP, err := l7.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.
if ip, ok := l.ingress.Annotations[annotations.StaticIPKey]; ok && ip == managedStaticIPName {
return l.deleteStaticIP()
if ip, ok := l7.ingress.Annotations[annotations.StaticIPKey]; ok && ip == managedStaticIPName {
return l7.deleteStaticIP()
}
}
return nil
}

key, err := l.CreateKey(managedStaticIPName)
key, err := l7.CreateKey(managedStaticIPName)
if err != nil {
return err
}

ip, _ := composite.GetAddress(l.cloud, key, meta.VersionGA)
ip, _ := composite.GetAddress(l7.cloud, key, meta.VersionGA)
if ip == nil {
klog.V(3).Infof("Creating static ip %v", managedStaticIPName)
address := l.newStaticAddress(managedStaticIPName)
address := l7.newStaticAddress(managedStaticIPName)

err = composite.CreateAddress(l.cloud, key, address)
err = composite.CreateAddress(l7.cloud, key, address)
if err != nil {
if utils.IsHTTPErrorCode(err, http.StatusConflict) ||
utils.IsHTTPErrorCode(err, http.StatusBadRequest) {
klog.V(3).Infof("IP %v(%v) is already reserved, assuming it is OK to use.",
l.fw.IPAddress, managedStaticIPName)
l7.fw.IPAddress, managedStaticIPName)
return nil
}
return err
}
ip, err = composite.GetAddress(l.cloud, key, meta.VersionGA)
ip, err = composite.GetAddress(l7.cloud, key, meta.VersionGA)
if err != nil {
return err
}
}
l.ip = ip
l7.ip = ip
return nil
}

func (l *L7) newStaticAddress(name string) *composite.Address {
isInternal := utils.IsGCEL7ILBIngress(&l.ingress)
address := &composite.Address{Name: name, Address: l.fw.IPAddress, Version: meta.VersionGA}
func (l7 *L7) newStaticAddress(name string) *composite.Address {
isInternal := utils.IsGCEL7ILBIngress(&l7.ingress)
address := &composite.Address{Name: name, Address: l7.fw.IPAddress, Version: meta.VersionGA}
if isInternal {
// Used for L7 ILB
address.AddressType = "INTERNAL"
Expand Down
74 changes: 37 additions & 37 deletions pkg/loadbalancers/certificates.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,38 +28,38 @@ import (

const SslCertificateMissing = "SslCertificateMissing"

func (l *L7) checkSSLCert() error {
isL7ILB := utils.IsGCEL7ILBIngress(l.runtimeInfo.Ingress)
tr := translator.NewTranslator(isL7ILB, l.namer)
env := &translator.Env{Region: l.cloud.Region(), Project: l.cloud.ProjectID()}
translatorCerts := tr.ToCompositeSSLCertificates(env, l.runtimeInfo.TLSName, l.runtimeInfo.TLS, l.Versions().SslCertificate)
func (l7 *L7) checkSSLCert() error {
isL7ILB := utils.IsGCEL7ILBIngress(l7.runtimeInfo.Ingress)
tr := translator.NewTranslator(isL7ILB, l7.namer)
env := &translator.Env{Region: l7.cloud.Region(), Project: l7.cloud.ProjectID()}
translatorCerts := tr.ToCompositeSSLCertificates(env, l7.runtimeInfo.TLSName, l7.runtimeInfo.TLS, l7.Versions().SslCertificate)

// Use both pre-shared and secret-based certs if available,
// combining encountered errors.
errs := []error{}

// Get updated value of certificate for comparison
existingSecretsSslCerts, err := l.getIngressManagedSslCerts()
existingSecretsSslCerts, err := l7.getIngressManagedSslCerts()
if err != nil {
errs = append(errs, err)
// Do not continue if getIngressManagedSslCerts() failed.
return utils.JoinErrs(errs)
}

l.oldSSLCerts = existingSecretsSslCerts
sslCerts, err := l.createSslCertificates(existingSecretsSslCerts, translatorCerts)
l7.oldSSLCerts = existingSecretsSslCerts
sslCerts, err := l7.createSslCertificates(existingSecretsSslCerts, translatorCerts)
if err != nil {
errs = append(errs, err)
}
l.sslCerts = sslCerts
l7.sslCerts = sslCerts
if len(errs) > 0 {
return utils.JoinErrs(errs)
}
return nil
}

// createSslCertificates creates SslCertificates based on kubernetes secrets in Ingress configuration.
func (l *L7) createSslCertificates(existingCerts, translatorCerts []*composite.SslCertificate) ([]*composite.SslCertificate, error) {
func (l7 *L7) createSslCertificates(existingCerts, translatorCerts []*composite.SslCertificate) ([]*composite.SslCertificate, error) {
var result []*composite.SslCertificate

existingCertsMap := getMapfromCertList(existingCerts)
Expand Down Expand Up @@ -95,23 +95,23 @@ func (l *L7) createSslCertificates(existingCerts, translatorCerts []*composite.S
}
// Controller needs to create the certificate, no need to check if it exists and delete. If it did exist, it
// would have been listed in the populateSSLCert function and matched in the check above.
klog.V(2).Infof("Creating new sslCertificate %q for LB %q", translatorCert.Name, l)
translatorCert.Version = l.Versions().SslCertificate
key, err := l.CreateKey(translatorCert.Name)
klog.V(2).Infof("Creating new sslCertificate %q for LB %q", translatorCert.Name, l7)
translatorCert.Version = l7.Versions().SslCertificate
key, err := l7.CreateKey(translatorCert.Name)
if err != nil {
klog.Errorf("l.CreateKey(%s) = %v", translatorCert.Name, err)
klog.Errorf("l7.CreateKey(%s) = %v", translatorCert.Name, err)
return nil, err
}
err = composite.CreateSslCertificate(l.cloud, key, translatorCert)
err = composite.CreateSslCertificate(l7.cloud, key, translatorCert)
if err != nil {
klog.Errorf("Failed to create new sslCertificate %q for %q - %v", translatorCert.Name, l, err)
klog.Errorf("Failed to create new sslCertificate %q for %q - %v", translatorCert.Name, l7, err)
failedCerts = append(failedCerts, translatorCert.Name+" Error:"+err.Error())
continue
}
visitedCertMap[translatorCert.Name] = fmt.Sprintf("secret cert:%q", translatorCert.Certificate)

// Get SSLCert
cert, err := composite.GetSslCertificate(l.cloud, key, translatorCert.Version)
cert, err := composite.GetSslCertificate(l7.cloud, key, translatorCert.Version)
if err != nil {
klog.Errorf("GetSslCertificate(_, %v, %v) = %v", key, translatorCert.Version, err)
return nil, err
Expand Down Expand Up @@ -140,33 +140,33 @@ func getMapfromCertList(certs []*composite.SslCertificate) map[string]*composite
// getIngressManagedSslCerts fetches SslCertificate resources created and managed by this load balancer
// instance. These SslCertificate resources were created based on kubernetes secrets in Ingress
// configuration.
func (l *L7) getIngressManagedSslCerts() ([]*composite.SslCertificate, error) {
func (l7 *L7) getIngressManagedSslCerts() ([]*composite.SslCertificate, error) {
var result []*composite.SslCertificate

// Currently we list all certs available in gcloud and filter the ones managed by this loadbalancer instance. This is
// to make sure we garbage collect any old certs that this instance might have lost track of due to crashes.
// Can be a performance issue if there are too many global certs, default quota is only 10.
// Use an empty name parameter since we only care about the scope
// TODO: (shance) refactor this so we don't need an empty arg
key, err := l.CreateKey("")
key, err := l7.CreateKey("")
if err != nil {
return nil, err
}
version := l.Versions().SslCertificate
certs, err := composite.ListSslCertificates(l.cloud, key, version)
version := l7.Versions().SslCertificate
certs, err := composite.ListSslCertificates(l7.cloud, key, version)
if err != nil {
return nil, err
}
for _, c := range certs {
if l.namer.IsCertNameForLB(c.Name) {
klog.V(4).Infof("Populating ssl cert %s for l7 %s", c.Name, l)
if l7.namer.IsCertNameForLB(c.Name) {
klog.V(4).Infof("Populating ssl cert %s for l7 %s", c.Name, l7)
result = append(result, c)
}
}
if len(result) == 0 {
// Check for legacy cert since that follows a different naming convention
klog.V(4).Infof("Looking for legacy ssl certs")
expectedCertLinks, err := l.getSslCertLinkInUse()
expectedCertLinks, err := l7.getSslCertLinkInUse()
if err != nil {
// Return nil if target proxy doesn't exist.
return nil, utils.IgnoreHTTPNotFound(err)
Expand All @@ -179,30 +179,30 @@ func (l *L7) getIngressManagedSslCerts() ([]*composite.SslCertificate, error) {
continue
}

if !l.namer.IsLegacySSLCert(name) {
if !l7.namer.IsLegacySSLCert(name) {
continue
}
key, err := l.CreateKey(name)
key, err := l7.CreateKey(name)
if err != nil {
return nil, err
}
cert, _ := composite.GetSslCertificate(l.cloud, key, version)
cert, _ := composite.GetSslCertificate(l7.cloud, key, version)
if cert != nil {
klog.V(4).Infof("Populating legacy ssl cert %s for l7 %s", cert.Name, l)
klog.V(4).Infof("Populating legacy ssl cert %s for l7 %s", cert.Name, l7)
result = append(result, cert)
}
}
}
return result, nil
}

func (l *L7) deleteOldSSLCerts() {
if len(l.oldSSLCerts) == 0 {
func (l7 *L7) deleteOldSSLCerts() {
if len(l7.oldSSLCerts) == 0 {
return
}
certsMap := getMapfromCertList(l.sslCerts)
for _, cert := range l.oldSSLCerts {
if !l.namer.IsCertNameForLB(cert.Name) && !l.namer.IsLegacySSLCert(cert.Name) {
certsMap := getMapfromCertList(l7.sslCerts)
for _, cert := range l7.oldSSLCerts {
if !l7.namer.IsCertNameForLB(cert.Name) && !l7.namer.IsLegacySSLCert(cert.Name) {
// retain cert if it is managed by GCE(non-ingress)
continue
}
Expand All @@ -211,17 +211,17 @@ func (l *L7) deleteOldSSLCerts() {
continue
}
klog.V(3).Infof("Cleaning up old SSL Certificate %s", cert.Name)
key, _ := l.CreateKey(cert.Name)
if certErr := utils.IgnoreHTTPNotFound(composite.DeleteSslCertificate(l.cloud, key, l.Versions().SslCertificate)); certErr != nil {
key, _ := l7.CreateKey(cert.Name)
if certErr := utils.IgnoreHTTPNotFound(composite.DeleteSslCertificate(l7.cloud, key, l7.Versions().SslCertificate)); certErr != nil {
klog.Errorf("Old cert %s delete failed - %v", cert.Name, certErr)
}
}
}

// Returns true if the input array of certs is identical to the certs in the L7 config.
// Returns false if there is any mismatch
func (l *L7) compareCerts(certLinks []string) bool {
certsMap := getMapfromCertList(l.sslCerts)
func (l7 *L7) compareCerts(certLinks []string) bool {
certsMap := getMapfromCertList(l7.sslCerts)
if len(certLinks) != len(certsMap) {
klog.V(4).Infof("Loadbalancer has %d certs, target proxy has %d certs", len(certsMap), len(certLinks))
return false
Expand Down
Loading

0 comments on commit 826b038

Please sign in to comment.