Skip to content
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

Kiali 812 - Check if route rule versions labels has pods running #233

Merged
merged 3 commits into from May 31, 2018

Conversation

@xeviknal
Copy link
Member

commented May 30, 2018

Given a route rule routing traffic to v1 and v2, add a new validation if v1 or v2 doesn't have any pod running.

@xeviknal xeviknal requested a review from lucasponce May 30, 2018
objectCheckers := []ObjectChecker{
checkers.RouteRuleChecker{istioDetails.RouteRules},
checkers.RouteRuleChecker{Namespace: namespace, PodList: pods.Items, RouteRules: istioDetails.RouteRules},

This comment has been minimized.

Copy link
@lucasponce

lucasponce May 30, 2018

Contributor

I am not sure if this is correct.
For the GetServiceValidations, the pods are filtered by the service, or it should be if I read it correctly.
For the namespace, you are fetching all pods and this is checking against versions without matching per labels.
So, if I am not wrong we might have a bad-pod-1234 with labels version=v1, version=v2 and matches correctly.

The idea to add the RouteRuleChecker was to enable validations that are independent of the service, to not replicate in Istio Config what we are showing in Service Details.

Then, I would leave in GetNamespaceValidations validations that are per namespace, but not attached to the services, I think.

Perhaps, that means to split the RouteRuleChecker, or if PodList is not provided, to not use the pod checker on this.

Or, if we want to maintain the consistency, the pods should be narrowed/filtered per service in the evaluation.

If I didn't miss anything in the code, I couldn't review it entirely yet.

This comment has been minimized.

Copy link
@xeviknal

xeviknal May 30, 2018

Author Member

For the GetServiceValidations, the pods are filtered by the service, or it should be if I read it correctly.

You are right, they are filtered by app label (service level).

For the namespace, you are fetching all pods and this is checking against versions without matching per labels.

You are right, for the GetNamespaceValidations I am not filtering per service. This solution then it could trigger some false positive.

The idea to add the RouteRuleChecker was to enable validations that are independent of the service, to not replicate in Istio Config what we are showing in Service Details.

I don't know if I fully follow you. I thought that we wanted to show same validations for each item within IstioConfig view. If we are using RouteRuleChecker, then it means that we will show weight/precedence validations as well.
IMO, it might be somewhat confusing to have some validations in one page but not to the other.

Or, if we want to maintain the consistency, the pods should be narrowed/filtered per service in the evaluation.

IMO, I would go this way. But perhaps I am missing something.

This comment has been minimized.

Copy link
@lucasponce

lucasponce May 31, 2018

Contributor

No, it is good, I wanted to maintain consistency between both, that's the correct way, but I didn't want to pull all data related to services in the Istio Config logic.

I guess filtering pods ver labels "app x version" is what it is necessary.

I am also working actively in these files too and I was also thinking loud.

routeLabels[key] = value.(string)
}

routeLabels[config.Get().ServiceFilterLabelName] = getServiceName(routeRule)

This comment has been minimized.

Copy link
@xeviknal

xeviknal May 31, 2018

Author Member

@lucasponce now it is filtering by service label. This way, it is removed the false positive we talked before.

This comment has been minimized.

Copy link
@lucasponce

lucasponce May 31, 2018

Contributor

It is good, as we know app x version label should exist on pods, so, this looks better now.
Thanks.

@@ -105,6 +105,12 @@ func (in *IstioClient) GetPods(namespace string, labelsSet labels.Set) (*v1.PodL
return in.k8s.CoreV1().Pods(namespace).List(meta_v1.ListOptions{LabelSelector: formatted})
}

// GetPods returns the pods definitions for a given namespace

This comment has been minimized.

Copy link
@lucasponce

lucasponce May 31, 2018

Contributor

Copy/paste typo, it should be "GetNamespacePods"

@@ -4,11 +4,14 @@ import (
"github.com/kiali/kiali/kubernetes"
"github.com/kiali/kiali/services/business/checkers/route_rules"
"github.com/kiali/kiali/services/models"
"k8s.io/api/core/v1"

This comment has been minimized.

Copy link
@lucasponce

lucasponce May 31, 2018

Contributor

Bad import order, it should be
standard imports
LINE
third pary
LINE
kiali imports

If not @jmazzitelli will chase you :-)

This comment has been minimized.

Copy link
@xeviknal

xeviknal May 31, 2018

Author Member

Upsie! Just forgot that rule here.

@@ -42,15 +45,16 @@ func (in RouteRuleChecker) runGroupChecks() models.IstioValidations {
}

// enabledCheckersFor returns the list of all individual enabled checkers.
func enabledCheckersFor(object kubernetes.IstioObject) []Checker {
func (in RouteRuleChecker) enabledCheckersFor(object kubernetes.IstioObject) []Checker {

This comment has been minimized.

Copy link
@lucasponce

lucasponce May 31, 2018

Contributor

I am not sure if I like to extract functions for one-line assign.
Indeed I am adding this lines without the enabledCheckersFor, sorry, I guess a comment is enough in assignment :-).

}

for i := 0; i < slice.Len(); i++ {
route := slice.Index(i).Interface().(map[string]interface{})

This comment has been minimized.

Copy link
@lucasponce

lucasponce May 31, 2018

Contributor

I think this cast should be protected, as a typo in the routerule yaml is possible and you might not cast this, so you would get then a nice panic.
A simple route, ok := ... should help.

podFound := false

for _, pod := range checker.PodList {
podFound = podFound || selector.Matches(labels.Set(pod.ObjectMeta.Labels))

This comment has been minimized.

Copy link
@lucasponce

lucasponce May 31, 2018

Contributor

podFound = podFound || selector.Matches(labels.Set(pod.ObjectMeta.Labels)) ?

Not better just

podFound = selector.Matches(labels.Set(pod.ObjectMeta.Labels))

As you break if true below ?


func getSelector(routeRule kubernetes.IstioObject, rawLabels interface{}) labels.Selector {
routeLabels := map[string]string{}
for key, value := range rawLabels.(map[string]interface{}) {

This comment has been minimized.

Copy link
@lucasponce

lucasponce May 31, 2018

Contributor

Cast should be protected, as this comes from some yaml with potential malformed types.


func getServiceName(routeRule kubernetes.IstioObject) string {
serviceName := ""
if destination, ok := routeRule.GetSpec()["destination"]; ok {

This comment has been minimized.

Copy link
@lucasponce

lucasponce May 31, 2018

Contributor

This assumes Spec exists. This could not pass some test with nil objects. I started to add them as I introduced a bug without notice for a similar condition like this, so I recommend to check it out first.

"github.com/kiali/kiali/kubernetes"
)

func TestCheckerWithPodsMatching(t *testing.T) {

This comment has been minimized.

Copy link
@lucasponce

lucasponce May 31, 2018

Contributor

I suggest to add some basic test with inputs with nil, or empty objects to ensure things don't crash.


// Setup mocks
podList := []v1.Pod{
fakePodsForLabels(map[string]string{"app": "reviews", "version": "v1", "stage": "production"}),

This comment has been minimized.

Copy link
@lucasponce

lucasponce May 31, 2018

Contributor

Just to double check, a map[string]string it could be different than map[string]interface{}, so are you sure that k8s cast labels.Set as a map[string]string vs map[string]interface{} ?

If not in runtime it can skip some cast.

This comment has been minimized.

Copy link
@xeviknal

xeviknal May 31, 2018

Author Member

labels.Set is a map[string]string by definition. So no problem in casting.
Dealing with IstioObjects is where most of the pain is. The definition of the struct is basically map[string]interface{}

v1.Pod{
ObjectMeta: meta_v1.ObjectMeta{
Name: "reviews-12345-hello",
Labels: map[string]string{

This comment has been minimized.

Copy link
@lucasponce

lucasponce May 31, 2018

Contributor

Commented out, just to be sure that the mock pod has the correct type used by the real pod, map[string]string vs map[string]interface{}.

It just to double check this.

This comment has been minimized.

Copy link
@xeviknal

xeviknal May 31, 2018

Author Member

Yes, k8s uses map[string]string for labels whereas IstioObject uses map[string]interface{}. I had to deal with that before and had to add those castings instead.

Copy link
Contributor

left a comment

In general it looks good, some comments that could be easily addressed.

@xeviknal xeviknal force-pushed the xeviknal:KIALI-812 branch from 855801e to a07b6a1 May 31, 2018
@xeviknal

This comment has been minimized.

Copy link
Member Author

commented May 31, 2018

@lucasponce comments approached!

@lucasponce lucasponce merged commit c034af5 into kiali:master May 31, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@xeviknal xeviknal deleted the xeviknal:KIALI-812 branch May 31, 2018
@xeviknal

This comment has been minimized.

Copy link
Member Author

commented May 31, 2018

Thanks for the exhaustive review lucas!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.