Integrate ClusterServiceMapper into translator. #223
Conversation
eb595a7 to
a48f71b
Compare
|
/retest |
|
/assign @nicksardo |
a48f71b to
ad40872
Compare
|
/retest |
pkg/controller/controller.go
Outdated
| @@ -242,7 +248,14 @@ func (lbc *LoadBalancerController) sync(key string) (retErr error) { | |||
| } | |||
|
|
|||
| // gceNodePorts contains the ServicePorts used by only single-cluster ingress. | |||
| gceNodePorts := lbc.Translator.ToNodePorts(&gceIngresses) | |||
| gceNodePorts := make([]backends.ServicePort, 0) | |||
There was a problem hiding this comment.
nit: var gceNodePorts []backends.ServicePort
pkg/controller/controller.go
Outdated
| @@ -441,3 +465,11 @@ func updateAnnotations(client kubernetes.Interface, name, namespace string, anno | |||
| } | |||
| return nil | |||
| } | |||
|
|
|||
| func extractSvcPorts(svcPortMapping map[extensions.IngressBackend]backends.ServicePort) []backends.ServicePort { | |||
| svcPorts := make([]backends.ServicePort, 0) | |||
There was a problem hiding this comment.
same nit as above. Can also name the return and immediately append to it.
There was a problem hiding this comment.
Done, named the return.
pkg/controller/controller.go
Outdated
| @@ -282,7 +295,18 @@ func (lbc *LoadBalancerController) sync(key string) (retErr error) { | |||
| } | |||
|
|
|||
| func (lbc *LoadBalancerController) ensureIngress(key string, ing *extensions.Ingress, nodeNames []string, gceNodePorts []backends.ServicePort) error { | |||
| ingNodePorts := lbc.Translator.IngressToNodePorts(ing) | |||
| // svcPortMapping is reused by the Translator in ToURLMap() | |||
There was a problem hiding this comment.
Could you change this comment to explain what the mapping is instead of where else it's used.
i.e. Given an ingress, returns a mapping of K8s Service name/port -> backends.ServicePort.
pkg/controller/controller.go
Outdated
| msg := fmt.Sprintf("%v", e) | ||
| lbc.ctx.Recorder(ing.Namespace).Eventf(ing, apiv1.EventTypeWarning, "Service", msg) | ||
| } | ||
| } |
There was a problem hiding this comment.
IMO, would be nicer to show that code can handle non-multierror case. Maybe a type switch with a default case?
There was a problem hiding this comment.
Done. Left a TODO for myself to clean that error handling code up.
pkg/controller/controller.go
Outdated
| if err != nil { | ||
| return fmt.Errorf("convert to URL Map error %v", err) | ||
| return fmt.Errorf("convert to URL Map error: %v", err) |
There was a problem hiding this comment.
While you're at it, could you change this to "error converting to URLMap: %v"
There was a problem hiding this comment.
Done. I assume you want to change it for the ones below as well so I fixed those too.
| type ErrNodePortNotFound struct { | ||
| Backend v1beta1.IngressBackend | ||
| Err error | ||
| } |
There was a problem hiding this comment.
Why don't we want/need these anymore? Not necessary to make structs with internal data?
There was a problem hiding this comment.
After these changes, they won't be used anymore. I don't think they are necessary but I'm fine with leaving them around. Just figured we could delete code that isn't being used.
| return &GCE{ | ||
| recorders, | ||
| namer, | ||
| svcLister, | ||
| nodeLister, | ||
| podLister, | ||
| endpointLister, | ||
| svcMapper, |
There was a problem hiding this comment.
So you're making the mapper apart of controller context so we can update it from the MCI event handlers/goroutines, right?
There was a problem hiding this comment.
I haven't thought about how the best way to do it is yet but yes, something of that nature :)
| // So keep requeuing the l7 till all backends exist. | ||
| return utils.GCEURLMap{}, err | ||
| // Get the corresponding ServicePort for this backend. | ||
| svcPort, ok := svcPorts[p.Backend] |
| t.recorders.Recorder(ing.Namespace).Eventf(ing, api_v1.EventTypeNormal, "Service", msg) | ||
| svcPort, ok := svcPorts[*ing.Spec.Backend] | ||
| if ok { | ||
| defaultBackendName = t.namer.Backend(svcPort.NodePort) |
There was a problem hiding this comment.
Could you add a comment explaining what happens in the !ok case.
There was a problem hiding this comment.
Done. Not sure if the comment I added is 100% true so PTAL.
| func TestToURLMap(t *testing.T) { | ||
| ing, err := test.GetTestIngress("../../test/manifests/ing1.yaml") | ||
| if err != nil { | ||
| t.Errorf("Error occured when getting test Ingress: %v", err) |
pkg/mapper/mapper_test.go
Outdated
| if err != nil { | ||
| t.Errorf("Error occured when constructing test Ingress: %v", err) | ||
| t.Errorf("Error occured when getting test Ingress: %v", err) | ||
| return |
ad40872 to
d15f36f
Compare
|
/lgtm |
Extracts ServicePort logic out of translator using the new ClusterServiceMapper implementation. Also adds tests for untested code paths.