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-1578 KIALI-1136 Show WARN on all DRs with duplicate hosts #521

Merged
merged 2 commits into from Oct 1, 2018

Conversation

@burmanm
Copy link
Contributor

commented Sep 26, 2018

All offending DestinationRules should be flagged with WARN, not just the last one. Also, fix a typo that caused the warnings to be filtered and add the warns to the config listing also.

…, not just the latter one. Also, fix a type that caused the warnings to be filtered
@rhqci

This comment has been minimized.

Copy link
Collaborator

commented Sep 26, 2018

✔️ Jenkins CI: kiali-core-pr-e2e-test #100

KIALI_NAME=kialicorepr521-istio-system

@hhovsepy

This comment has been minimized.

Copy link

commented Sep 27, 2018

Warn Icon is shown on Istio Config list and details page.
But it is missing in Service Detail's "Destination Rules" tab:
screenshot from 2018-09-27 09-17-14

@burmanm

This comment has been minimized.

Copy link
Contributor Author

commented Sep 27, 2018

As I commented on JIRA, the Service Details is not intended to display other than pod-type validations at this point.

@lucasponce

This comment has been minimized.

Copy link
Contributor

commented Sep 28, 2018

Sorry for my delay to jump on the review.
@hhovsepy is right that in the service validations used by the UI:

    let promiseValidations = API.getServiceValidations(
      authentication(),
      this.props.match.params.namespace,
      this.props.match.params.service
    );

We invoke the ServiceValidations which includes the VR validations from the backend:

// GetServiceValidations returns an IstioValidations object with all the checks found when running
// all the enabled checkers.
func (in *IstioValidationsService) GetServiceValidations(namespace, service string) (models.IstioValidations, error) {
	// Get Gateways and ServiceEntries to validate VirtualServices
	var err error
	wg := sync.WaitGroup{}
	errChan := make(chan error, 2)

	vs := make([]kubernetes.IstioObject, 0)
	dr := make([]kubernetes.IstioObject, 0)

	wg.Add(2)
	go fetch(&vs, namespace, service, in.k8s.GetVirtualServices, &wg, errChan)
	go fetch(&dr, namespace, service, in.k8s.GetDestinationRules, &wg, errChan)
	wg.Wait()
	if len(errChan) != 0 {
		err = <-errChan
		return nil, err
	}
	objectCheckers := []ObjectChecker{
		checkers.VirtualServiceChecker{namespace, dr, vs},
	}

	// Get group validations for same kind istio objects
	return runObjectCheckers(objectCheckers), nil
}

So, I think DR should also apply here as well, and potentially the UI should also shown same scheme of icons as happens in the Istio Config.

Also, I am not sure if perhaps we can change this to avoid inconsistencies between the Istio Config validations vs the Service Validations and perhaps the Service Details UI should get the list of services and invoke the same logic used in the Istio Config (I see some duplication here we introduced in the past, not on this PR).

Anyway, let me know what do you think.

Perhaps it's a good moment to refactor this area, clean the duplication between validation and simplify the endpoints used.

I.e. instead to have a validation for istio_config and other for service details; service details can provide the vs/dr and UI invoke the istio_config validations (as similar way it happens with the health).
Then validation would happen from only a single endpoint.

@burmanm

This comment has been minimized.

Copy link
Contributor Author

commented Sep 28, 2018

Yes, I think the validation should happen in a refactored way. That serviceValidations is missing multiple validations, including noServiceChecker for VirtualServices since these are all defined in different places. Thus, the whole code is quite spaghetti with duplication of data.

@lucasponce

This comment has been minimized.

Copy link
Contributor

commented Sep 28, 2018

That serviceValidations is missing multiple validations, including noServiceChecker for VirtualServices since these are all defined in different places.

+1

Thus, the whole code is quite spaghetti with duplication of data.

This is a known technical debt because original design was to validate per object, at the moment we introduced two views for same object (VS can be shown under ServiceDetails and Istio Config) we maintained the backend endpoint for validations trying to sync on the backend.

But we don't have goto sentences in the code :-) jumping from one place to another without logic.

So, now it is good moment to refactor and simplify this.

@rhqci

This comment has been minimized.

Copy link
Collaborator

commented Sep 28, 2018

!!! Couldn't read commit file !!!

@gbaufake

This comment has been minimized.

Copy link
Member

commented Sep 28, 2018

kialiqeci

@rhqci

This comment has been minimized.

Copy link
Collaborator

commented Sep 28, 2018

✔️ Jenkins CI: kiali-core-pr-e2e-test #117

KIALI_NAME=kialicorepr521-istio-system

@hhovsepy

This comment has been minimized.

Copy link

commented Oct 1, 2018

PR verified:
kiali-ui 0.8.0 (45044cb29f7fbb5e1cf9d3e9f3d6d328dde3515c)
kiali v0.8.0-SNAPSHOT (4dc40b2)

@lucasponce lucasponce merged commit ad863f6 into kiali:master Oct 1, 2018
2 checks passed
2 checks passed
Jenkins-CI Test PASSed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@lucasponce

This comment has been minimized.

Copy link
Contributor

commented Oct 1, 2018

Thanks @burmanm and @hhovsepy

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