@leseb

Thank you for your review. I will make the specified changes to the variables.

Also, this is how I would get the istio version with kubectl.
kubectl get deployment istiod -o yaml -n istio-system | grep image:

But I am unclear on where this should be executed by the ceph operator. Can you please point me in the right direction?

Seems like an easy fix to conditionally set the variable if istio is detected. Is this what you had in mind?

As much as I'd like to support the new Istio version, adding new CRs setting to allow that and maintain backward compatibility seems overkill as it complexities the CR.

Is there a way we can detect Istio internally and change the service name accordingly maybe?

I would even go a step further and say there is no need for these names in the CR or to change them if istio is installed. It should be fine just to follow the istio convention. Nothing should really rely on these names.

Although it does seem like an odd requirement that the name has to be in a certain format since there is also a protocol field that serves the purpose of defining the protocol.

Besides adding the http- prefix to the one service, do we even need to change the msgr port names? If istio defaults to tcp it should be fine, right?

I don't really understand why istio/kiali rely on the port name prefix either, but they sure do complain/not work when they are not set "correctly". Can I trash all of this variable nonsense and just prefix each of these with their protocols and be done with it? Follow on question, if yes, is should they be http or tcp?

I don't really understand why istio/kiali rely on the port name prefix either, but they sure do complain/not work when they are not set "correctly". Can I trash all of this variable nonsense and just prefix each of these with their protocols and be done with it?

Sounds good to revert the variable changes.

Follow on question, if yes, is should they be http or tcp?

I understood from the istio doc that tcp will be the default protocol if they don't understand it. So the mon ports don't need to change and we just need the http port on the other one, right?

@travisn
https://github.com/kiali/kiali/blob/a5a1dcd7948796725eea5314b57af61ed002ab43/kubernetes/istio_details_service_test.go#L45 through L87 show their port name validations. From what I see, the name must either be the protocol or be prefixed by the protocol. "msgr1" would be invalid by itself, but tcp-msgr1 would be fine, unless it is a udp protocol, and udp-msgr1 would also work... From my understanding.

In this doc, I was interpreting that the tcp- prefix was optional. Are you seeing failures without the tcp prefix?

If the naming does not match this form (or is undefined), Istio treats all the traffic TCP instead of the defined protocol in the definition. 

Yes, without a prefix, kiali displays the service as unhealthy and communication between services becomes disrupted.

sounds like my next stop is going to be with the istio docs....

Yes, without a prefix, kiali displays the service as unhealthy and communication between services becomes disrupted.

Ok sounds good to add the tcp prefix in that case

This screenshot provides a little more info with what we are seeing.

LGTM, just please squash the commits before we merge.

I squashed but it's still showing 3 commits, did I do something wrong?

This pull request has merge conflicts that must be resolved before it can be merged. @afflom please rebase it. https://rook.io/docs/rook/master/development-flow.html#updating-your-fork

Thank you! Do I owe anymore work on this?

@leseb Great catch!

Looking at https://github.com/afflom/rook/blob/9a4d535f09c08a39470bdcc3b3bb37ee2e053d65/pkg/operator/ceph/object/spec.go#L213 The port names set in generateService are compatible with istio naming conventions (as per https://github.com/kiali/kiali/blob/a5a1dcd7948796725eea5314b57af61ed002ab43/kubernetes/istio_details_service_test.go#L45).

Do I need to change the port name anywhere else?

func (c *clusterConfig) generateService(cephObjectStore *cephv1.CephObjectStore) *v1.Service {
	labels := getLabels(cephObjectStore.Name, cephObjectStore.Namespace)
	svc := &v1.Service{
		ObjectMeta: metav1.ObjectMeta{
			Name:      instanceName(cephObjectStore.Name),
			Namespace: cephObjectStore.Namespace,
			Labels:    labels,
		},
		Spec: v1.ServiceSpec{
			Selector: labels,
		},
	}

	if c.clusterSpec.Network.IsHost() {
		svc.Spec.ClusterIP = v1.ClusterIPNone
	}

	destPort := c.generateLiveProbePort()
	addPort(svc, "http", cephObjectStore.Spec.Gateway.Port, destPort.IntVal)
	addPort(svc, "https", cephObjectStore.Spec.Gateway.SecurePort, cephObjectStore.Spec.Gateway.SecurePort)

	return svc
}

...<truncated>...

func addPort(service *v1.Service, name string, port, destPort int32) {
	if port == 0 || destPort == 0 {
		return
	}
	service.Spec.Ports = append(service.Spec.Ports, v1.ServicePort{
		Name:       name,
		Port:       port,
		TargetPort: intstr.FromInt(int(destPort)),
		Protocol:   v1.ProtocolTCP,
	})
}

@leseb Great catch!

Looking at https://github.com/afflom/rook/blob/9a4d535f09c08a39470bdcc3b3bb37ee2e053d65/pkg/operator/ceph/object/spec.go#L213 The port names set in generateService are compatible with istio naming conventions (as per https://github.com/kiali/kiali/blob/a5a1dcd7948796725eea5314b57af61ed002ab43/kubernetes/istio_details_service_test.go#L45).

Do I need to change the port name anywhere else?

func (c *clusterConfig) generateService(cephObjectStore *cephv1.CephObjectStore) *v1.Service {
	labels := getLabels(cephObjectStore.Name, cephObjectStore.Namespace)
	svc := &v1.Service{
		ObjectMeta: metav1.ObjectMeta{
			Name:      instanceName(cephObjectStore.Name),
			Namespace: cephObjectStore.Namespace,
			Labels:    labels,
		},
		Spec: v1.ServiceSpec{
			Selector: labels,
		},
	}

	if c.clusterSpec.Network.IsHost() {
		svc.Spec.ClusterIP = v1.ClusterIPNone
	}

	destPort := c.generateLiveProbePort()
	addPort(svc, "http", cephObjectStore.Spec.Gateway.Port, destPort.IntVal)
	addPort(svc, "https", cephObjectStore.Spec.Gateway.SecurePort, cephObjectStore.Spec.Gateway.SecurePort)

	return svc
}

...<truncated>...

func addPort(service *v1.Service, name string, port, destPort int32) {
	if port == 0 || destPort == 0 {
		return
	}
	service.Spec.Ports = append(service.Spec.Ports, v1.ServicePort{
		Name:       name,
		Port:       port,
		TargetPort: intstr.FromInt(int(destPort)),
		Protocol:   v1.ProtocolTCP,
	})
}

Thanks for checking, no it's fine keep them as is.

Thank you so much!