From 9b381641b331143142b361b805f7aa70430c7527 Mon Sep 17 00:00:00 2001 From: Nick Sardo Date: Tue, 18 Apr 2017 10:53:18 -0700 Subject: [PATCH] Fix review comments --- controllers/gce/backends/backends.go | 10 +++++----- controllers/gce/controller/utils.go | 11 +++++----- controllers/gce/healthchecks/healthchecks.go | 20 +++++++++---------- .../gce/healthchecks/healthchecks_test.go | 10 +++++----- 4 files changed, 25 insertions(+), 26 deletions(-) diff --git a/controllers/gce/backends/backends.go b/controllers/gce/backends/backends.go index 977c5a8df..8cdb65cf4 100644 --- a/controllers/gce/backends/backends.go +++ b/controllers/gce/backends/backends.go @@ -165,7 +165,7 @@ func (b *Backends) ensureHealthCheck(sp ServicePort) (string, error) { return "", err } if probe != nil { - glog.V(1).Infof("Applying httpGet settings of readinessProbe to health check on port %+v", sp) + glog.V(2).Infof("Applying httpGet settings of readinessProbe to health check on port %+v", sp) applyProbeSettingsToHC(probe, hc) } } @@ -245,7 +245,7 @@ func (b *Backends) Add(p ServicePort) error { pName := b.namer.BeName(p.Port) be, _ = b.Get(p.Port) if be == nil { - glog.V(1).Infof("Creating backend for %d instance groups, port %v named port %v", len(igs), p.Port, namedPort) + glog.V(2).Infof("Creating backend for %d instance groups, port %v named port %v", len(igs), p.Port, namedPort) be, err = b.create(igs, namedPort, hcLink, p.Protocol, pName) if err != nil { return err @@ -258,7 +258,7 @@ func (b *Backends) Add(p ServicePort) error { } if be.Protocol != string(p.Protocol) || existingHCLink != hcLink { - glog.Infof("Updating backend protocol %v (%v) for change in protocol (%v) or health check", pName, be.Protocol, string(p.Protocol)) + glog.V(2).Infof("Updating backend protocol %v (%v) for change in protocol (%v) or health check", pName, be.Protocol, string(p.Protocol)) be.Protocol = string(p.Protocol) be.HealthChecks = []string{hcLink} if err = b.cloud.UpdateBackendService(be); err != nil { @@ -286,7 +286,7 @@ func (b *Backends) Add(p ServicePort) error { // Delete deletes the Backend for the given port. func (b *Backends) Delete(port int64) (err error) { name := b.namer.BeName(port) - glog.Infof("Deleting backend service %v", name) + glog.V(2).Infof("Deleting backend service %v", name) defer func() { if utils.IsHTTPErrorCode(err, http.StatusNotFound) { err = nil @@ -377,7 +377,7 @@ func (b *Backends) GC(svcNodePorts []ServicePort) error { } pool := b.snapshotter.Snapshot() for port := range pool { - p, err := strconv.ParseInt(port, 10, 64) + p, err := strconv.ParseUint(port, 10, 16) if err != nil { return err } diff --git a/controllers/gce/controller/utils.go b/controllers/gce/controller/utils.go index f86c2c214..697ce58e5 100644 --- a/controllers/gce/controller/utils.go +++ b/controllers/gce/controller/utils.go @@ -141,7 +141,7 @@ func (svc svcAnnotations) ApplicationProtocols() (map[string]utils.AppProtocol, switch proto { case utils.ProtocolHTTP, utils.ProtocolHTTPS: default: - return nil, fmt.Errorf("unexpected port application protocol: %v", proto) + return nil, fmt.Errorf("invalid port application protocol: %v", proto) } } @@ -415,7 +415,6 @@ func (t *GCETranslator) toGCEBackend(be *extensions.IngressBackend, ns string) ( // getServiceNodePort looks in the svc store for a matching service:port, // and returns the nodeport. func (t *GCETranslator) getServiceNodePort(be extensions.IngressBackend, namespace string) (backends.ServicePort, error) { - invalidPort := backends.ServicePort{} obj, exists, err := t.svcLister.Indexer.Get( &api_v1.Service{ ObjectMeta: meta_v1.ObjectMeta{ @@ -424,15 +423,15 @@ func (t *GCETranslator) getServiceNodePort(be extensions.IngressBackend, namespa }, }) if !exists { - return invalidPort, errorNodePortNotFound{be, fmt.Errorf("service %v/%v not found in store", namespace, be.ServiceName)} + return backends.ServicePort{}, errorNodePortNotFound{be, fmt.Errorf("service %v/%v not found in store", namespace, be.ServiceName)} } if err != nil { - return invalidPort, errorNodePortNotFound{be, err} + return backends.ServicePort{}, errorNodePortNotFound{be, err} } svc := obj.(*api_v1.Service) appProtocols, err := svcAnnotations(svc.GetAnnotations()).ApplicationProtocols() if err != nil { - return invalidPort, errorSvcAppProtosParsing{svc, err} + return backends.ServicePort{}, errorSvcAppProtosParsing{svc, err} } var port *api_v1.ServicePort @@ -454,7 +453,7 @@ PortLoop: } if port == nil { - return invalidPort, errorNodePortNotFound{be, fmt.Errorf("could not find matching nodeport from service")} + return backends.ServicePort{}, errorNodePortNotFound{be, fmt.Errorf("could not find matching nodeport from service")} } proto := utils.ProtocolHTTP diff --git a/controllers/gce/healthchecks/healthchecks.go b/controllers/gce/healthchecks/healthchecks.go index 70dd4a07e..4d996dae8 100644 --- a/controllers/gce/healthchecks/healthchecks.go +++ b/controllers/gce/healthchecks/healthchecks.go @@ -76,8 +76,8 @@ func (h *HealthChecks) Sync(hc *HealthCheck) (string, error) { return "", err } - glog.Infof("Creating health check for port %v with protocol %v", hc.Port, hc.Type) - if err = h.cloud.CreateHealthCheck(hc.Out()); err != nil { + glog.V(2).Infof("Creating health check for port %v with protocol %v", hc.Port, hc.Type) + if err = h.cloud.CreateHealthCheck(hc.ToComputeHealthCheck()); err != nil { return "", err } @@ -85,8 +85,8 @@ func (h *HealthChecks) Sync(hc *HealthCheck) (string, error) { } if existingHC.Protocol() != hc.Protocol() { - glog.Infof("Updating health check %v because it has protocol %v but need %v", existingHC.Name, existingHC.Type, hc.Type) - err = h.cloud.UpdateHealthCheck(hc.Out()) + glog.V(2).Infof("Updating health check %v because it has protocol %v but need %v", existingHC.Name, existingHC.Type, hc.Type) + err = h.cloud.UpdateHealthCheck(hc.ToComputeHealthCheck()) return existingHC.SelfLink, err } @@ -94,9 +94,9 @@ func (h *HealthChecks) Sync(hc *HealthCheck) (string, error) { // TODO: reconcile health checks, and compare headers interval etc. // Currently Ingress doesn't expose all the health check params // natively, so some users prefer to hand modify the check. - glog.Infof("Unexpected request path on health check %v, has %v want %v, NOT reconciling", hc.Name, existingHC.RequestPath, hc.RequestPath) + glog.V(2).Infof("Unexpected request path on health check %v, has %v want %v, NOT reconciling", hc.Name, existingHC.RequestPath, hc.RequestPath) } else { - glog.Infof("Health check %v already exists and has the expected path %v", hc.Name, hc.RequestPath) + glog.V(2).Infof("Health check %v already exists and has the expected path %v", hc.Name, hc.RequestPath) } return existingHC.SelfLink, nil @@ -113,7 +113,7 @@ func (h *HealthChecks) getHealthCheckLink(port int64) (string, error) { // Delete deletes the health check by port. func (h *HealthChecks) Delete(port int64) error { name := h.namer.BeName(port) - glog.Infof("Deleting health check %v", name) + glog.V(2).Infof("Deleting health check %v", name) return h.cloud.DeleteHealthCheck(name) } @@ -127,7 +127,7 @@ func (h *HealthChecks) Get(port int64) (*HealthCheck, error) { // DeleteLegacy deletes legacy HTTP health checks func (h *HealthChecks) DeleteLegacy(port int64) error { name := h.namer.BeName(port) - glog.Infof("Deleting legacy HTTP health check %v", name) + glog.V(2).Infof("Deleting legacy HTTP health check %v", name) return h.cloud.DeleteHttpHealthCheck(name) } @@ -197,8 +197,8 @@ func (hc *HealthCheck) Protocol() utils.AppProtocol { return utils.AppProtocol(hc.Type) } -// Out returns a valid compute.HealthCheck object -func (hc *HealthCheck) Out() *compute.HealthCheck { +// ToComputeHealthCheck returns a valid compute.HealthCheck object +func (hc *HealthCheck) ToComputeHealthCheck() *compute.HealthCheck { // Zeroing out child settings as a precaution. GoogleAPI throws an error // if the wrong child struct is set. hc.HealthCheck.HttpsHealthCheck = nil diff --git a/controllers/gce/healthchecks/healthchecks_test.go b/controllers/gce/healthchecks/healthchecks_test.go index 9350c8d54..0f6abc358 100644 --- a/controllers/gce/healthchecks/healthchecks_test.go +++ b/controllers/gce/healthchecks/healthchecks_test.go @@ -63,7 +63,7 @@ func TestHealthCheckAddExisting(t *testing.T) { httpHC := DefaultHealthCheck(3000, utils.ProtocolHTTP) httpHC.Name = namer.BeName(3000) httpHC.RequestPath = "/my-probes-health" - hcp.CreateHealthCheck(httpHC.Out()) + hcp.CreateHealthCheck(httpHC.ToComputeHealthCheck()) // Should not fail adding the same type of health check hc := healthChecks.New(3000, utils.ProtocolHTTP) @@ -82,7 +82,7 @@ func TestHealthCheckAddExisting(t *testing.T) { httpsHC := DefaultHealthCheck(4000, utils.ProtocolHTTPS) httpsHC.Name = namer.BeName(4000) httpsHC.RequestPath = "/my-probes-health" - hcp.CreateHealthCheck(httpsHC.Out()) + hcp.CreateHealthCheck(httpsHC.ToComputeHealthCheck()) hc = healthChecks.New(4000, utils.ProtocolHTTPS) _, err = healthChecks.Sync(hc) @@ -104,11 +104,11 @@ func TestHealthCheckDelete(t *testing.T) { // Create HTTP HC for 1234 hc := DefaultHealthCheck(1234, utils.ProtocolHTTP) hc.Name = namer.BeName(1234) - hcp.CreateHealthCheck(hc.Out()) + hcp.CreateHealthCheck(hc.ToComputeHealthCheck()) // Create HTTPS HC for 1234) hc.Type = string(utils.ProtocolHTTPS) - hcp.CreateHealthCheck(hc.Out()) + hcp.CreateHealthCheck(hc.ToComputeHealthCheck()) // Delete only HTTP 1234 err := healthChecks.Delete(1234) @@ -139,7 +139,7 @@ func TestHealthCheckUpdate(t *testing.T) { hc := DefaultHealthCheck(3000, utils.ProtocolHTTP) hc.Name = namer.BeName(3000) hc.RequestPath = "/my-probes-health" - hcp.CreateHealthCheck(hc.Out()) + hcp.CreateHealthCheck(hc.ToComputeHealthCheck()) // Verify the health check exists _, err := healthChecks.Get(3000)