feat: Make registry work with Istio mTLS

[funkypenguin]

Hey guys,

I have Harbor 2.5.2 working nicely with Istio mTLS, using a VirtualService instead of nginx, except for this one issue.. (which is resolved by this PR).

So Istio uses service port names as a "hint" re protocol selection.. if a service is named http-<something> or https-<something>, Istio will treat the traffic a certain way. If there's no name, or a name which doesn't match one of the pre-defined prefixes, Istio will treat the traffic as "tcp", and not try to do any clever HTTP parsing on it.

Now presumable registry's service on port 5000 is actually HTTP/S, but the envoy sidecar proxy doesn't particularly like the way core talks to registry (due to the host header), and so when using mTLS with the helm chart as-is, we see errors like this when pushing into the registry:

ERRO[0002] pushing image failed: pushing tag(s): GET https://registry.elpenguino.net/v2/: unexpected status code 503 Service Unavailable: upstream connect error or disconnect/reset before headers. reset reason: connection termination 

However, change the name of the registry service from http-registry or https-registry to simply registry (or bacon if you like, provided it's not an Istio prefix), and istio treats the traffic as TCP, and doesn't worry about parsing the HTTP.

So... this PR simply changes the name of the registry service port, which seems harmless to me, for the sake of mTLS compatibility. I'm happy to take input / suggestions from anyone more knowledgeable than me re the internals of either Harbor or Istio :)

For the record, I'm using the following VirtualService to mimic what nginx or the ingress does:

apiVersion: networking.istio.io/v1beta1
kind: VirtualService
metadata:
  creationTimestamp: "2022-06-29T22:53:41Z"
  generation: 17
  labels:
    kustomize.toolkit.fluxcd.io/name: istio-system-elpenguino-net
    kustomize.toolkit.fluxcd.io/namespace: flux-system
  name: harbor
  namespace: harbor
  resourceVersion: "10596813"
  uid: 601d5907-e352-4604-9210-a67a7ab31485
spec:
  gateways:
  - istio-ingressgateway.istio-system.svc.cluster.local
  hosts:
  - registry.elpenguino.net
  http:
  - match:
    - uri:
        prefix: /api/
    route:
    - destination:
        host: harbor-core
        port:
          number: 80
  - match:
    - uri:
        prefix: /service/
    route:
    - destination:
        host: harbor-core
        port:
          number: 80
  - match:
    - uri:
        prefix: /chartrepo
    route:
    - destination:
        host: harbor-core
        port:
          number: 80
  - match:
    - uri:
        prefix: /c/
    route:
    - destination:
        host: harbor-core
        port:
          number: 80
  - match:
    - uri:
        prefix: /v1/
    route:
    - destination:
        host: harbor-core
        port:
          number: 80
  - match:
    - uri:
        prefix: /v2/
    route:
    - destination:
        host: harbor-core
        port:
          number: 80
  - name: portal
    route:
    - destination:
        host: harbor-portal
        port:
          number: 80
    timeout: 30s

[funkypenguin]

Maintainers, could you please activate workflows on this PR, to progress it? There are at least 3 of us looking forward to using the fix ;)

[sylvainOL]

Hello, doing that would break part of istio recognition system no?

I'm trying to make it work on istio mTLS but so far I wasn't able to do it either :(

[sylvainOL]

Hey, so I managed to arrive at the same point as you and yes it doesn't work with http-registry but I can't understand why :(

[funkypenguin]

IIRC, the reason why (counter-intuitively) using an http- prefix doesn't work, is because when core talks to registry, it preserves the original host header, which confuses Istio, which happily ignores the destination hostname / IP, and tries to route the request according to the Host header instead.

"Getting rid" of the http- prefix makes Istio give up on header analysis, and just treat the traffic as TCP, honoring the intended destination (core -> registry)

[sylvainOL]

But with the "correct" service entry and virtual service, shouldn't it make it work even with http-registry? I think it's too bad that it doesn't (although we managed to make it without http part still :) )

[sylvainOL]

and sorry to spam but did you try here https://github.com/goharbor/harbor-helm/blob/master/templates/core/core-cm.yaml#L43 to set harbor-registry.harbor.svc.cluster.local:5000 instead of harbor-registry:5000? In some componetns deployments, it made istio "happy"

[jabbors]

and sorry to spam but did you try here https://github.com/goharbor/harbor-helm/blob/master/templates/core/core-cm.yaml#L43 to set harbor-registry.harbor.svc.cluster.local:5000 instead of harbor-registry:5000? In some componetns deployments, it made istio "happy"

I just tried this and it didtn't have any effect. But when stripping the http- prefix from the service name it works. So we can conclude that it doesn't matter if the short service name or the full service name is specified.

[ChrisJBurns]

Any traction on this? We have had to disable MTLS in our Harbor namespace because of the fact we can't modify the service port names and this PR would be valuable for us.

[GreenCappuccino]

In my lab cluster I have a kustomization.yaml with the following patch (using the new appProtocol key as opposed to using port name prefixes)

patchesJson6902:
  - target: # See https://github.com/goharbor/harbor-helm/pull/1228
      version: v1
      kind: Service
      name: harbor-registry
    patch: |
      - op: replace
        path: "/spec/ports/0"
        value:
          name: http-registry
          port: 5000
          appProtocol: tcp

[namenmalkav]

Hi is there any plan on this?

[tomaszkrzyzanowski]

Issue is still happening on the 1.12.2 chart while Istio's mTls is enabled and the patch mentioned by @GreenCappuccino is fixing it

[github-actions[bot]]

This PR is being marked stale due to a period of inactivty. If this PR is still relevant, please comment or remove the stale label. Otherwise, this PR will close in 30 days.

[github-actions[bot]]

This PR was closed because it has been stalled for 30 days with no activity. If this PR is still relevant, please re-open a new PR against main.