search

GoogleCloudPlatform / k8s-multicluster-ingress

kubemci: Command line tool to configure L7 load balancers using multiple kubernetes clusters
Apache License 2.0
377 stars 68 forks source link

Adding code to set the right network name while creating firewall rules #137

Closed nikhiljindal closed 6 years ago

nikhiljindal commented 6 years ago

Fixes https://github.com/GoogleCloudPlatform/k8s-multicluster-ingress/issues/121

Renamed NetworkTagsGetter to InstanceGetter so that FirewallRuleSyncer can use it to fetch instances and then use the fetched instances to get both network tags and network name. This way we dont fetch instances twice.

cc @G-Harmon @nicksardo @csbell

This change is Reviewable

nikhiljindal commented 6 years ago

cc @brndnmtthws and @kinghrothgar who reported this issue

brndnmtthws commented 6 years ago

Looks like this only supports a single network, which is a good start.

nikhiljindal commented 6 years ago

Looks like this only supports a single network, which is a good start.

Thats interesting, hadnt thought of that.

Whats the use case for multiple networks? Do you require support for multiple networks to be able to use kubemci? I assume I will have to create a firewall rule for each network?

cc @nicksardo, our expert here :)

brndnmtthws commented 6 years ago

An example use case might be to expose a certain set of instances to another service via firewall rules, while making sure the other set of instances can't reach it. I can imagine there might be a few environments where this might make sense if additional isolation is warranted.

In my particular case I don't need more than one network, but it seems logical to me that you'd just create a list of all the networks for all pools and add those to the firewall rule (assuming that's possible).

nikhiljindal commented 6 years ago

FirewallRule.Network is a string field and not an []string, so cant specify more than one network there.

I briefly chatted with @nicksardo as well and multiple networks setup seems reasonable but I dont know of anyone doing it. I think its fine to start with one network here and we can revisit with updated requirements? Also longer term, we want to delegate this to ingress-gce: https://github.com/GoogleCloudPlatform/k8s-multicluster-ingress/issues/122

G-Harmon commented 6 years ago

lgtm. very minor comments.

Reviewed 9 of 9 files at r1. Review status: all files reviewed at latest revision, 4 unresolved discussions.

app/kubemci/pkg/gcp/firewallrule/firewallrulesyncer.go, line 189 at r1 (raw file):

  sort.Strings(targetTags)

  // We assume that all instances are in the same network, so we just fetch the network of the first instance.

make this a TODO(?)

app/kubemci/pkg/gcp/firewallrule/firewallrulesyncer.go, line 213 at r1 (raw file):

  for _, v := range igLinks {
      // Return an instance from the first instance group of each cluster.
      // TODO(nikhiljindal): Make this more resilient. If we fail to fetch an instance from one instance group, try the next.

should this read: "if we fail to fetch one instance from an IG, try the next" ? The current wording makes it sound like we should try the next instance group. Or, it's at least ambiguous.

app/kubemci/pkg/gcp/firewallrule/firewallrulesyncer.go, line 224 at r1 (raw file):


// getTargetTags returns the required network tags to target all instances.
// It assumes that the input contains an instance from each cluster.

s/cluster/instance group/ ? Not sure.

app/kubemci/pkg/gcp/instances/fake_instancegetter.go, line 1 at r1 (raw file):

// Copyright 2017 Google Inc.

s/2017/2018/

Comments from Reviewable

nikhiljindal commented 6 years ago

Review status: all files reviewed at latest revision, 4 unresolved discussions.

app/kubemci/pkg/gcp/firewallrule/firewallrulesyncer.go, line 189 at r1 (raw file):

Previously, G-Harmon wrote…
make this a TODO(?)

Done.

app/kubemci/pkg/gcp/firewallrule/firewallrulesyncer.go, line 213 at r1 (raw file):

Previously, G-Harmon wrote…
should this read: "if we fail to fetch one instance from an IG, try the next" ? The current wording makes it sound like we should try the next instance group. Or, it's at least ambiguous.

Done.

app/kubemci/pkg/gcp/firewallrule/firewallrulesyncer.go, line 224 at r1 (raw file):

Previously, G-Harmon wrote…
s/cluster/instance group/ ? Not sure.

Cluster is right

app/kubemci/pkg/gcp/instances/fake_instancegetter.go, line 1 at r1 (raw file):

Previously, G-Harmon wrote…
s/2017/2018/

Its a file rename so we retain its old year :)

Comments from Reviewable

nikhiljindal commented 6 years ago

Thanks @G-Harmon for the review. Updated code as per comments