-
Notifications
You must be signed in to change notification settings - Fork 8.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Revendor godep #994
Revendor godep #994
Conversation
expNodes := sets.NewString(nodeNames...) | ||
//expNodes := sets.NewString() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will clean this up in next push
/assign nicksardo |
if cm.fakeIGs.Ports[0] != testPort { | ||
t.Errorf("Expected the same node port on all igs, got ports %+v", cm.fakeIGs.Ports) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original test and this change look funky. We should be testing each instance group for the testPort
, but this only checks the first port and happens outside the instance group loop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my other comment
controllers/gce/firewalls/fakes.go
Outdated
namer *utils.Namer | ||
fw map[string]*compute.Firewall | ||
namer *utils.Namer | ||
networkUrl string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think namer
has to be removed from the fakeFirewallsProvider. Cloudprovider should no longer be generating the name given a suffix - it should just be given the name.
controllers/gce/firewalls/fakes.go
Outdated
func (f *fakeFirewallsProvider) GetFirewall(prefixedName string) (*compute.Firewall, error) { | ||
rule, exists := f.fw[prefixedName] | ||
func (ff *fakeFirewallsProvider) GetFirewall(prefixedName string) (*compute.Firewall, error) { | ||
rule, exists := ff.fw[prefixedName] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please rename to just name
controllers/gce/firewalls/fakes.go
Outdated
prefixedName := f.namer.FrName(name) | ||
_, exists := f.fw[prefixedName] | ||
prefixedName := ff.namer.FrName(name) | ||
_, exists := ff.fw[prefixedName] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't need namer
here, right?
firewall, err := fr.createFirewallObject(suffix, "GCE L7 firewall rule", nodePorts, nodeNames) | ||
if err != nil { | ||
return err | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we just call this createFirewallObject once earlier in the function and re-use it here and below?
url := instanceRefs[ix].Instance | ||
parts := strings.Split(url, "/") | ||
instanceNames[ix] = parts[len(parts)-1] | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could instead change to storing URLs instead of names. Up to you.
for _, port := range namedPorts { | ||
f.Ports = append(f.Ports, port.Port) | ||
} | ||
return nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either add it now or add a TODO. We should assert existence of the instance group and return an error if missing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
@@ -63,13 +63,14 @@ func (i *Instances) Init(zl zoneLister) { | |||
// all of which have the exact same named port. | |||
func (i *Instances) AddInstanceGroup(name string, port int64) ([]*compute.InstanceGroup, *compute.NamedPort, error) { | |||
igs := []*compute.InstanceGroup{} | |||
namedPort := &compute.NamedPort{} | |||
namedPort := &compute.NamedPort{Name: fmt.Sprintf("port%v", port), Port: port} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either now or in a TODO, the port name should be moved to namer
.
func (f *FakeLoadBalancers) ReserveGlobalAddress(addr *compute.Address) error { | ||
f.calls = append(f.calls, "ReserveGlobalAddress") | ||
f.IP = append(f.IP, addr) | ||
return nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the future, we should just refactor this to use a map. Nothing to do now.
I didn't see any show stoppers, but would appreciate some minor changes in areas. |
return fmt.Errorf("Failed to find instance group %q in zone %q", igName, zone) | ||
} | ||
|
||
f.Ports = f.Ports[:0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nicksardo The interface and fakeInstanceGroup changed. Only store the namedports, not append. So there is only one port.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems odd to me. Can we change the FakeInstanceGroup.Ports []int64
to FakeInstanceGroup.Port int64
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope. There can be multiple named port passing in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. I ignored the code below :)
7d5605b
to
92efa39
Compare
@nicksardo @freehan this is ready to be merged? |
lgtm at a quick glance :) |
I'm kind of confused looking at the commits, why are there commits from someone else in the list? |
@bowei I started the update until I found the gce controller require changes because of the firewall api changes. |
@@ -60,9 +60,15 @@ func (fr *FirewallRules) Sync(nodePorts []int64, nodeNames []string) error { | |||
// instead of the whole name. | |||
name := fr.namer.FrName(suffix) | |||
rule, _ := fr.cloud.GetFirewall(name) | |||
|
|||
firewall, err := fr.createFirewallObject(suffix, "GCE L7 firewall rule", nodePorts, nodeNames) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We pass in suffix
though we just computed the name above. Could we either delete line 61 and change uses to "firewall.Name" or pass in the name?
err := fr.cloud.DeleteFirewall(fr.namer.FrSuffix()) | ||
name := fr.namer.FrName(fr.namer.FrSuffix()) | ||
glog.Infof("Deleting firewall %v", name) | ||
err := fr.cloud.DeleteFirewall(name) | ||
if err != nil && utils.IsHTTPErrorCode(err, 404) { | ||
glog.Infof("Firewall with suffix %v didn't exist at Shutdown", fr.namer.FrSuffix()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can change this line to print the firewall name computed above.
ports := make([]string, len(nodePorts)) | ||
for ix := range nodePorts { | ||
ports[ix] = strconv.Itoa(int(nodePorts[ix])) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we de-duping ports anywhere? Checking
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
func (f *FakeLoadBalancers) DeleteGlobalStaticIP(name string) error { | ||
f.calls = append(f.calls, "DeleteGlobalStaticIP") | ||
// DeleteGlobalAddress fakes out static IP deletion. | ||
func (f *FakeLoadBalancers) DeleteGlobalAddress(name string) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note for future, we should return fake 404 if address wasn't found.
My last two comments can be done in another PR. I think it's good to go, but will ping @bowei for last check. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@freehan I lack the depth that @nicksardo has in this code base. I have left a few minor comments. LGTM otherwise.
controllers/gce/backends/backends.go
Outdated
@@ -26,10 +26,10 @@ import ( | |||
"github.com/golang/glog" | |||
|
|||
compute "google.golang.org/api/compute/v1" | |||
api_v1 "k8s.io/api/core/v1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason to merge these two blocks? I really like the separation between the k8s API imports and other imports
return fmt.Errorf("Failed to find instance group %q in zone %q", igName, zone) | ||
} | ||
|
||
f.Ports = f.Ports[:0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems odd to me. Can we change the FakeInstanceGroup.Ports []int64
to FakeInstanceGroup.Port int64
?
core/pkg/ingress/status/status.go
Outdated
"sort" | ||
"sync" | ||
"time" | ||
|
||
"github.com/golang/glog" | ||
|
||
api "k8s.io/api/core/v1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
call this v1
or corev1
. It gets pretty confusing when reading the code in these packages. For example, see https://github.com/kubernetes/ingress/pull/994/files#diff-636822cdb1e125937cd0afa244e18161R33.
Addressed the comments. |
looks ok to me |
Revendor Godep and fix gce controller
This includes a lot of changes. Need careful review.
@nicksardo @nikhiljindal @bowei @madhusudancs