-
Notifications
You must be signed in to change notification settings - Fork 68
Basic --validate flag: Checks that NodePort is the same in each cluster. #148
Conversation
app/kubemci/cmd/create.go
Outdated
@@ -95,6 +97,7 @@ func addCreateFlags(cmd *cobra.Command, options *CreateOptions) error { | |||
// TODO(nikhiljindal): Add a short flag "-p" if it seems useful. | |||
cmd.Flags().StringVarP(&options.GCPProject, "gcp-project", "", options.GCPProject, "[optional] name of the gcp project. Is fetched using gcloud config get-value project if unset here") | |||
cmd.Flags().BoolVarP(&options.ForceUpdate, "force", "f", options.ForceUpdate, "[optional] overwrite existing settings if they are different") | |||
cmd.Flags().BoolVarP(&options.Validate, "validate", "", options.Validate, "[optional] Enable/disable some validation steps.") |
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.
Also explain what happens on enabling validation?
Like "Performs extra validation steps and throws an error before creating the load balancer"?
func (l *LoadBalancerSyncer) ServicesNodePortsSame(clients map[string]kubeclient.Interface, ing *v1beta1.Ingress) error { | ||
for _, rule := range ing.Spec.Rules { | ||
if rule.HTTP == nil { | ||
glog.Errorf("ignoring non http Ingress rule") |
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.
use fmt.Errorf or make it V(2).
Also print the rule?
continue | ||
} | ||
for _, path := range rule.HTTP.Paths { | ||
glog.Infof("Validating path:%s", path.Path) |
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.
nit: Add a space after ":"
And same comment as above. make it fmt.Printf or make this V(2)? (making this V(2) seems better to me)
for _, path := range rule.HTTP.Paths { | ||
glog.Infof("Validating path:%s", path.Path) | ||
if err := l.NodePortSameInAllClusters(path.Backend, ing.Namespace); err != nil { | ||
return fmt.Errorf("NodePort validation error for service '%s/%s': %s", ing.Namespace, path.Backend.ServiceName, 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.
Start error message with small letter: https://github.com/golang/go/wiki/CodeReviewComments#error-strings.
|
||
} | ||
} | ||
glog.Infof("Checking default backend's nodeports.") |
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.
fmt.Errorf
@@ -105,14 +105,85 @@ func NewLoadBalancerSyncer(lbName string, clients map[string]kubeclient.Interfac | |||
}, nil | |||
} | |||
|
|||
// ServicesNodePortsSame checks that for each backend/service, the services are | |||
// all listening on the same NodePort. | |||
func (l *LoadBalancerSyncer) ServicesNodePortsSame(clients map[string]kubeclient.Interface, ing *v1beta1.Ingress) 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.
Both this and the method below can be private? (Can remain private when moved to the validations package)
// 1x GET-Service for path=/foo. | ||
// 1x GET-Service for default backend. | ||
// Start of setup (using a random cluster's client): | ||
// 2x GET-Service. |
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.
umm why do we fetch it twice again? To determine the node port? We should reuse the nodeport computed during validation?
Not to be done in this PR. Feel free to add a TODO and/or file an issue.
t.Fatalf("%s", err) | ||
} | ||
if err := lbc.CreateLoadBalancer(ing, true /*forceUpdate*/, true /*validate*/, clusters); err == nil { | ||
t.Errorf("should've gotten an 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.
Add more detail on what error should it have been
} | ||
} | ||
} | ||
} |
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.
Add a line break
} | ||
glog.Infof("Default backend's nodeports passed validation.") | ||
} else { | ||
return fmt.Errorf("Ingress Spec is missing default backend") |
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.
fwiw, This is what we use today: "unexpected: ing.spec.backend is nil. Multicluster ingress needs a user specified default backend". Seems more meaningful :)
Thanks for sending this @G-Harmon Most comments are generic code style comments that apply all through:
|
b9a66de
to
8ec4c78
Compare
Reviewed 5 of 5 files at r1, 5 of 5 files at r2, 2 of 2 files at r3. app/kubemci/cmd/create.go, line 100 at r1 (raw file): Previously, nikhiljindal (Nikhil Jindal) wrote…
Done. app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer.go, line 110 at r1 (raw file): Previously, nikhiljindal (Nikhil Jindal) wrote…
Done. app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer.go, line 113 at r1 (raw file): Previously, nikhiljindal (Nikhil Jindal) wrote…
Well, what I have here is just copied from ingToNodePort. Made them both Warningf(), seems unlikely to trigger? app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer.go, line 117 at r1 (raw file): Previously, nikhiljindal (Nikhil Jindal) wrote…
added space. But I'm inclined to keep it in the default log file... Why do you suggest either promoting OR demoting? app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer.go, line 119 at r1 (raw file): Previously, nikhiljindal (Nikhil Jindal) wrote…
done. app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer.go, line 124 at r1 (raw file): Previously, nikhiljindal (Nikhil Jindal) wrote…
I don't follow- this isn't returning an error here. app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer.go, line 128 at r1 (raw file): Previously, nikhiljindal (Nikhil Jindal) wrote…
Done. app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer.go, line 129 at r1 (raw file): Previously, nikhiljindal (Nikhil Jindal) wrote…
Done. (This required updating the unit test, since now more GETs are done and errors returned.) app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer.go, line 133 at r1 (raw file):
app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer.go, line 141 at r1 (raw file):
app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer.go, line 143 at r1 (raw file): Previously, nikhiljindal (Nikhil Jindal) wrote…
oops. fixed all the variables. app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer.go, line 144 at r1 (raw file): Previously, nikhiljindal (Nikhil Jindal) wrote…
I think it's valuable to add the cluster name because it gives context to logs inside the loop, like for the "ServicePort" Log. But okay, we can make it a Vlog. app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer.go, line 161 at r1 (raw file): Previously, nikhiljindal (Nikhil Jindal) wrote…
Done. app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer.go, line 178 at r1 (raw file): Previously, nikhiljindal (Nikhil Jindal) wrote…
okay, in addition to moving these 2 functions I added, I also had to move GetServiceNodePorts and getSvc. app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer.go, line 183 at r1 (raw file): Previously, nikhiljindal (Nikhil Jindal) wrote…
oh right. I set the default to 'true' in create.go. app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer.go, line 596 at r1 (raw file): Previously, nikhiljindal (Nikhil Jindal) wrote…
done and done and done. app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer_test.go, line 99 at r1 (raw file): Previously, nikhiljindal (Nikhil Jindal) wrote…
thanks for poking at this. I checked again, and it's not duplicating work. it's 1 time for the default backend and 1 time for the path rule. app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer_test.go, line 222 at r1 (raw file): Previously, nikhiljindal (Nikhil Jindal) wrote…
Done. app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer_test.go, line 237 at r1 (raw file): Previously, nikhiljindal (Nikhil Jindal) wrote…
Done. Comments from Reviewable |
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.
Thanks for the refactoring.
Most of the comments are for using glog vs fmt.Printf because I wasnt clear the last time.
Feel free to ping for any questions.
app/kubemci/cmd/create.go
Outdated
@@ -95,6 +99,7 @@ func addCreateFlags(cmd *cobra.Command, options *CreateOptions) error { | |||
// TODO(nikhiljindal): Add a short flag "-p" if it seems useful. | |||
cmd.Flags().StringVarP(&options.GCPProject, "gcp-project", "", options.GCPProject, "[optional] name of the gcp project. Is fetched using gcloud config get-value project if unset here") | |||
cmd.Flags().BoolVarP(&options.ForceUpdate, "force", "f", options.ForceUpdate, "[optional] overwrite existing settings if they are different") | |||
cmd.Flags().BoolVarP(&options.Validate, "validate", "", options.Validate, "[optional] If enabled, do some validation checks and potentially return an error, before creating load balancer") |
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.
Also say that it is true by default
client, cErr := getAnyClient(l.clients) | ||
if cErr != nil { | ||
// No point in continuing without a client. | ||
return cErr | ||
} | ||
|
||
if validate { | ||
|
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.
remove line break
if validate { | ||
|
||
if err := validations.Validate(l.clients, ing); err != nil { | ||
glog.Errorf("Validation failed: %s", 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.
Sorry should have been clearer before.
We do not use glog unless user explicitly uses verbose flag. Hence we dont use glog.Errorf directly. Using glog.V(2).Errorf is fine.
For messages that we want to print without verbosity, we use fmt.Printf().
glog.Errorf("Validation failed: %s", err) | ||
return err | ||
} | ||
glog.Infof("Validation passed.") |
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.
same add V(2) or use Printf
@@ -549,6 +511,7 @@ func getAnyClient(clients map[string]kubeclient.Interface) (kubeclient.Interface | |||
} | |||
// Return the client for any cluster. | |||
for k := range clients { | |||
glog.Infof("getAnyClient: using client for cluster %s", k) |
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.
use V(2)
continue | ||
} | ||
for _, path := range rule.HTTP.Paths { | ||
glog.Infof("Validating path: %s", path.Path) |
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.
glog.V(4)
} | ||
} | ||
} | ||
glog.Infof("Checking default backend's nodeports.") |
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.
V(4)
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.
fwiw, printing "checking" is less meaningful than printing the result (failed/passed).
Right now, printing checking is at a lower verbosity than printing the result
|
||
if ing.Spec.Backend != nil { | ||
if err := nodePortSameInAllClusters(*ing.Spec.Backend, ing.Namespace, clients); err != nil { | ||
glog.Errorf("nodePort validation error for default backend service '%s/%s': %s", *ing.Spec.Backend, ing.Namespace, 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.
fmt.Printf.
Also keep this consistent with error for non backend service. I see we are not printing the error there
return err | ||
} | ||
glog.Infof("cluster %s: Service's servicePort: %+v", clientName, servicePort) | ||
// The NodePort is stored in 'Port' by getServiceNodePort. |
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 got renamed to NodePort, so the comment is not required now :)
continue | ||
} | ||
if clusterNodePort != nodePort { | ||
return fmt.Errorf("some instances of the '%s/%s' Service (e.g. in '%s') are on NodePort %v, but '%s' is on %v. All clusters must use same NodePort", |
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.
How about collecting all the clusters where nodePort is not same as firstCluster's node Port.
For ex: "Clusters A, B and C have a different node ports for service foo than cluster D. All are expected to have same ....".
Review status: all files reviewed at latest revision, 19 unresolved discussions, some commit checks failed. app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer.go, line 117 at r1 (raw file): Previously, G-Harmon wrote…
Sorry should have been clearer before. Meant either use V() if you want to use glog. Or use fmt.Printf if you want to print by default app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer.go, line 124 at r1 (raw file): Previously, G-Harmon wrote…
sorry meant fmt.Printf or use V() with glog app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer.go, line 144 at r1 (raw file): Previously, G-Harmon wrote…
Also remove "client" in "client/cluster"? We use that as variable name in code. Comments from Reviewable |
5664363
to
8d14992
Compare
Reviewed 6 of 6 files at r4, 4 of 4 files at r5. app/kubemci/cmd/create.go, line 102 at r3 (raw file): Previously, nikhiljindal (Nikhil Jindal) wrote…
Done. app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer.go, line 144 at r1 (raw file): Previously, nikhiljindal (Nikhil Jindal) wrote…
Done. app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer.go, line 116 at r3 (raw file): Previously, nikhiljindal (Nikhil Jindal) wrote…
Done. app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer.go, line 118 at r3 (raw file): Previously, nikhiljindal (Nikhil Jindal) wrote…
I removed some other glog.Errorf's I found. app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer.go, line 514 at r3 (raw file): Previously, nikhiljindal (Nikhil Jindal) wrote…
Done. app/kubemci/pkg/kubeutils/utils.go, line 271 at r3 (raw file): Previously, nikhiljindal (Nikhil Jindal) wrote…
Done. app/kubemci/pkg/validations/validations.go, line 32 at r3 (raw file): Previously, nikhiljindal (Nikhil Jindal) wrote…
Done. (s/ServicesNodePortsSame/servicesNodePortsSame/, assuming that's what you meant) app/kubemci/pkg/validations/validations.go, line 33 at r3 (raw file): Previously, nikhiljindal (Nikhil Jindal) wrote…
Done. app/kubemci/pkg/validations/validations.go, line 42 at r3 (raw file): Previously, nikhiljindal (Nikhil Jindal) wrote…
Done. app/kubemci/pkg/validations/validations.go, line 48 at r3 (raw file): Previously, nikhiljindal (Nikhil Jindal) wrote…
Done. app/kubemci/pkg/validations/validations.go, line 52 at r3 (raw file): Previously, nikhiljindal (Nikhil Jindal) wrote…
changed them to both be fmt.Errorf (and not print) app/kubemci/pkg/validations/validations.go, line 76 at r3 (raw file): Previously, nikhiljindal (Nikhil Jindal) wrote…
nice! app/kubemci/pkg/validations/validations.go, line 85 at r3 (raw file): Previously, nikhiljindal (Nikhil Jindal) wrote…
discussed offline. This returns an error at the first mismatch. I added a TODO at the top of this loop for improving it, if we ever get a request for it. Comments from Reviewable |
/test pull-kubernetes-multicluster-ingress-test |
I rebased my patch, and that should fix the test failure we were seeing. |
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.
Thanks for the fixes @G-Harmon
Looks great mostly.
Few minor comments mostly about printing info/debug messages
|
||
if validate { | ||
if err := validations.Validate(l.clients, ing); err != nil { | ||
fmt.Println("error: validation failed:", 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.
This will result in printing the error twice? The caller also prints the 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.
good catch. I often forget/don't worry about errors being printed multiple times. I fixed up 2 other spots where we were printing it extra.
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.
Great! Thanks for fixing those other places as well :)
if err != nil { | ||
glog.Errorf("%v", err) | ||
fmt.Println("error getting service nodeport:", err, "Ignoring.") |
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.
Add ". " before ignoring as you do for the error below?
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.
done.
app/kubemci/pkg/kubeutils/utils.go
Outdated
|
||
proto := annotations.ProtocolHTTP | ||
if protoStr, exists := appProtocols[port.Name]; exists { | ||
glog.V(2).Infof("service %s/%s: using protocol to %q", namespace, be.ServiceName, protoStr) |
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.
nit: Also add port name or number?
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.
done.
} | ||
} | ||
} | ||
glog.Infof("Checking default backend's nodeports.") |
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.
fwiw, printing "checking" is less meaningful than printing the result (failed/passed).
Right now, printing checking is at a lower verbosity than printing the result
|
||
servicePort, err := kubeutils.GetServiceNodePort(backend, namespace, client) | ||
if err != nil { | ||
glog.Errorf("Could not get service NodePort in cluster %s: %s", clientName, 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.
as discussed add V() or change to fmt.Printf for glog.Errorf() (Infof and Warningf are fine)
For every Backend/Service in the Ingress, validates that each cluster is using the same port number for its NodePort. Still todo: check that the server version is at least 1.8.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.
validations,go:
-I rationalized the V() levels a bit.
-I changed the glog.Errorf to return fmt.Errorf.
|
||
if validate { | ||
if err := validations.Validate(l.clients, ing); err != nil { | ||
fmt.Println("error: validation failed:", 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.
good catch. I often forget/don't worry about errors being printed multiple times. I fixed up 2 other spots where we were printing it extra.
if err != nil { | ||
glog.Errorf("%v", err) | ||
fmt.Println("error getting service nodeport:", err, "Ignoring.") |
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.
done.
app/kubemci/pkg/kubeutils/utils.go
Outdated
|
||
proto := annotations.ProtocolHTTP | ||
if protoStr, exists := appProtocols[port.Name]; exists { | ||
glog.V(2).Infof("service %s/%s: using protocol to %q", namespace, be.ServiceName, protoStr) |
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.
done.
app/kubemci/pkg/kubeutils/utils.go
Outdated
} | ||
glog.Infof("Found ServicePort: %+v", p) | ||
return p, 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.
done. (go fmt enforces 1 newline between functions, it appears. I put the newline before the brace.)
Thanks for all the fixes @G-Harmon |
For every Backend/Service in the Ingress, validates that each cluster
is using the same port number for its NodePort.
This covers some of the work needed for #53.
cc @nikhiljindal @csbell @bowei
This change is![Reviewable](https://camo.githubusercontent.com/1541c4039185914e83657d3683ec25920c672c6c5c7ab4240ee7bff601adec0b/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)