KIALI-660: Check Istio sidecars of services #188

israel-hdez · 2018-05-09T04:30:33Z

Add graph appender to provide information about missing sidecars in
the services.
Add checker to the istio_validations endpoint.
Fix a bug in the istio_validations endpoint: channel closed
early when more than one checker is available.

JSON returned for the istio_validations endpoint is like this:

{
   "reviews-dep-1009570596-rt794":{
      "name":"reviews-dep-1009570596-rt794",
      "object_type":"pod",
      "valid":false,
      "checks":[
         {
            "message":"Pod has no Istio sidecar",
            "severity":"warning",
            "path":""
         }
      ]
   },
   "reviews-v1-3758685241-qlqwf":{
      "name":"reviews-v1-3758685241-qlqwf",
      "object_type":"pod",
      "valid":true,
      "checks":null
   },
   "reviews-v2-29632839-xps5k":{
      "name":"reviews-v2-29632839-xps5k",
      "object_type":"pod",
      "valid":true,
      "checks":null
   },
   "reviews-v3-2285656649-2x8tm":{
      "name":"reviews-v3-2285656649-2x8tm",
      "object_type":"pod",
      "valid":true,
      "checks":null
   }
}

jshaughn

@israel-hdez , this looks pretty good, I do have a few comments for you so please take a look.

The main comment though is that while you were working on this @lucasponce consolidated some existing appenders into the new "IstioAppender". We did this to avoid querying k8s many times for the same information. Instead, that appender now calls IstioClient.GetServiceDetails one time for each service and annotates the graph in multiple ways in one pass. It currently adds circuit breaker and route rule information.

Because GetServiceDetails also calls GetServicePods under the covers, I think the sidecar check could be (and should be) integrated into the new appender.

If you like, we can move forward with this appender and do the merge as a follow-up task. Or, you can update this PR to do the merge now, it's up to you.

jshaughn · 2018-05-10T14:07:31Z

graph/appender/sidecards_check.go

+	"strings"
+)
+
+type SidecardsCheckAppender struct {


Is it Sidecard or Sidecar? I think it's the envoy Sidecar you are interested in, right? If I'm wrong just ignore this comment but otherwise please update everywhere.

Yes, wrong name. I'll rename in all places.

jshaughn · 2018-05-10T14:30:41Z

graph/cytoscape/cytoscape.go

+	IsGroup           string `json:"isGroup,omitempty"`           // set to the grouping type, current values: [ 'version' ]
+	IsRoot            string `json:"isRoot,omitempty"`            // true | false
+	IsUnused          string `json:"isUnused,omitempty"`          // true | false
+	PodsCount         int    `json:"podsCount,omitempty"`         // integer


Do we really need 3 new fields for the results of the sidecar check? I'm not sure of the client-side requirements for this PR but unless we have to have it I'd prefer that we stay with our general approach of using the graph to flag areas of interest, and let the user drill down into the details page. Maybe a single field like HasMissingSidecars with either a boolean value, or possibly a string with the number of missing sidecars (i.e. PodsCount-PodsWithSidecars)

I can use the single field HasMissingSidecars. I'll go that way.
Probably, the other data won't be too useful in the graph. This will simplify a little the code.

israel-hdez · 2018-05-11T15:02:03Z

@jshaughn I see GetIstioDetails currently isn't collecting the pods. I guess it was GetServiceDetails.
So, I can change GetIstioDetails to collect that data and consolidate this appender in the IstioAppender., if you think it's better.

jshaughn · 2018-05-11T16:01:25Z

@israel-hdez You are right, my mistake, the new Istio appender does things a little differently than I originally thought. Let's keep this appender independent for now, we can consolidate later if it makes sense.

israel-hdez · 2018-05-11T17:53:10Z

@jshaughn I've addressed your comments.
Not sure if you want to wait for @xeviknal's review, since this is touching some of his code.

xeviknal

Some comments about integration with the point of view I used to design this validation part.
Thank you for waiting for me. And sorry for the delay.

xeviknal · 2018-05-14T09:56:27Z

services/business/checkers/sidecars_checker.go

+const sidecarsCheckerType = "sidecar"
+const SidecarContainerImage = "docker.io/istio/proxy"
+
+func (checker *SidecarsChecker) Check() *models.IstioValidations {


@israel-hdez I would move the logic of the checker into a subfolder pods. I was thinking about grouping all the checkers by type. This way, we can validate pods on other terms and with no need to mix with the other checkers.
Another reason of why I did this subfolder is because we might want to run validations among objects (currently we only have individual validations). So we would have checkers validation how objects interfere among each others.

So I would suggest to follow same pattern of route_rule_checker.go. It is calling individual checkers and group checkers. The logic here, it would be into a file in this subfolder pods.

xeviknal · 2018-05-14T10:04:15Z

services/business/checkers/sidecars_checker.go

+		checker.K8sClient = k8sClient
+	}
+
+	pods, err := checker.K8sClient.GetServicePods(checker.Namespace, checker.ServiceName, checker.ServiceVersion, "")


When I thought about this solution, I didn't think about coupling connection with validation logic. Otherwise, you have one checker only for service pods, but not for every single pod. If you see the route rules checker, it can validate any kind of array of route rules independently where they are from.
So this logic would come either from istio_validations or an specific *_validations file. If you want to use same endpoint as istio_validations, then just fetch service pods from there and pass it to the checker.

xeviknal · 2018-05-14T10:07:39Z

services/business/istio_validations.go

 	return []ObjectChecker{
 		checkers.RouteRuleChecker{istioDetails.RouteRules},
+		&checkers.SidecarsChecker{Namespace: namespace, ServiceName: service, K8sClient: validationService.k8s},


Here is the point. Adding this, you are coupling the object fetching to its validations. If you see the previous line, we pass the objects to checker. This way, we prevent the coupling.

xeviknal · 2018-05-14T10:15:09Z

Perhaps you miss that, in the description of #180, you can find an high-level explanation of this validation part. If you see anything that could improve that, let me know and I will update it :)

jshaughn · 2018-05-14T12:50:35Z

@israel-hdez, I am fine with this PR, it can be merged when @xeviknal is happy.

israel-hdez · 2018-05-14T22:47:16Z

@xeviknal I've made changes. I hope to have solved your comments.

- Add graph appender to provide information about missing sidecars in the services. - Add checker to the istio_validations endpoint. - Fix a bug in the istio_validations endpoint: channel closed early when more than one checker is available.

xeviknal

LGTM! All comments addressed. After some discussion, now the implementation follows the architecture in the way it was designed.
Good approached @israel-hdez 🔝

israel-hdez added the do not merge A PR is not ready to merge label May 9, 2018

israel-hdez force-pushed the kiali-660 branch from 6cd2460 to 5d15329 Compare May 9, 2018 22:35

israel-hdez removed the do not merge A PR is not ready to merge label May 9, 2018

israel-hdez requested review from jshaughn and xeviknal May 9, 2018 22:36

israel-hdez force-pushed the kiali-660 branch from 5d15329 to 83b62a4 Compare May 9, 2018 22:38

israel-hdez changed the title ~~KIALI-660: Service graph appender to check sidecards in the pods~~ KIALI-660: Check Istio sidecards of services May 9, 2018

israel-hdez force-pushed the kiali-660 branch 3 times, most recently from 1016ec7 to 0c888f4 Compare May 9, 2018 22:58

jshaughn requested changes May 10, 2018

View reviewed changes

israel-hdez force-pushed the kiali-660 branch 4 times, most recently from 6624461 to 9ff3526 Compare May 11, 2018 17:47

israel-hdez changed the title ~~KIALI-660: Check Istio sidecards of services~~ KIALI-660: Check Istio sidecars of services May 11, 2018

israel-hdez mentioned this pull request May 11, 2018

KIALI-661: Graph badges for missing sidecars kiali/kiali-ui#330

Merged

xeviknal suggested changes May 14, 2018

View reviewed changes

israel-hdez force-pushed the kiali-660 branch 3 times, most recently from 2028509 to 359c05e Compare May 14, 2018 22:23

israel-hdez force-pushed the kiali-660 branch from 359c05e to 2d1acc3 Compare May 15, 2018 15:26

xeviknal approved these changes May 15, 2018

View reviewed changes

jshaughn approved these changes May 15, 2018

View reviewed changes

jshaughn merged commit b36195f into kiali:master May 15, 2018

israel-hdez deleted the kiali-660 branch May 15, 2018 19:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KIALI-660: Check Istio sidecars of services #188

KIALI-660: Check Istio sidecars of services #188

israel-hdez commented May 9, 2018 •

edited

Loading

jshaughn left a comment •

edited

Loading

jshaughn May 10, 2018

israel-hdez May 11, 2018

jshaughn May 10, 2018

israel-hdez May 11, 2018

israel-hdez commented May 11, 2018

jshaughn commented May 11, 2018

israel-hdez commented May 11, 2018

xeviknal left a comment

xeviknal May 14, 2018

xeviknal May 14, 2018

xeviknal May 14, 2018

xeviknal commented May 14, 2018

jshaughn commented May 14, 2018

israel-hdez commented May 14, 2018

xeviknal left a comment

KIALI-660: Check Istio sidecars of services #188

KIALI-660: Check Istio sidecars of services #188

Conversation

israel-hdez commented May 9, 2018 • edited Loading

jshaughn left a comment • edited Loading

Choose a reason for hiding this comment

jshaughn May 10, 2018

Choose a reason for hiding this comment

israel-hdez May 11, 2018

Choose a reason for hiding this comment

jshaughn May 10, 2018

Choose a reason for hiding this comment

israel-hdez May 11, 2018

Choose a reason for hiding this comment

israel-hdez commented May 11, 2018

jshaughn commented May 11, 2018

israel-hdez commented May 11, 2018

xeviknal left a comment

Choose a reason for hiding this comment

xeviknal May 14, 2018

Choose a reason for hiding this comment

xeviknal May 14, 2018

Choose a reason for hiding this comment

xeviknal May 14, 2018

Choose a reason for hiding this comment

xeviknal commented May 14, 2018

jshaughn commented May 14, 2018

israel-hdez commented May 14, 2018

xeviknal left a comment

Choose a reason for hiding this comment

israel-hdez commented May 9, 2018 •

edited

Loading

jshaughn left a comment •

edited

Loading