cloudfoundry / cf-k8s-networking

building a cloud foundry without gorouter....
Apache License 2.0
32 stars 17 forks source link

Transform istio ingressgateway into daemonset using a ytt overlay #35

Closed christianang closed 4 years ago

cf-gitbot commented 4 years ago

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/171909022

The labels on this github issue will be updated when the story is started.

mike1808 commented 4 years ago

I think, we also should disable HorizontalPodAutoscaler to ensure Istio doesn't try to do anything funny with IngressGateway and receive a bunch of errors.

We can do it by settings gateways.istio-ingressgateway.autoscaleEnabled to false in istio-values.yaml

mike1808 commented 4 years ago

Also, in my opinion, we should try to use Istio ways to do such things. In this case, I'd avoid using ytt and rather use IstioOperator (renamed to this in 1.5). It will be easier to upgrade to the new major version in the future.

For example, for Istio 1.4, to change the type to DeamonSet you can do:

apiVersion: install.istio.io/v1alpha2
kind: IstioControlPlane
spec:
  gateways:
    components:
      ingressGateway:
        k8s:
          overlays:
          - apiVersion: apps/v1
            kind: Deployment
            name: istio-ingressgateway
            patches:
            - path: kind
              value: DeamonSet
tcdowney commented 4 years ago

Also, in my opinion, we should try to Istio configs to do such things. In this case, I'd avoid using ytt and rather use IstioOperator (renamed to this in 1.5). It will be easier to upgrade to the new major version in the future.

For example, for Istio 1.4, to change the type to DeamonSet you can do:

apiVersion: install.istio.io/v1alpha2
kind: IstioControlPlane
spec:
  gateways:
    components:
      ingressGateway:
        k8s:
          overlays:
          - apiVersion: apps/v1
            kind: Deployment
            name: istio-ingressgateway
            patches:
            - path: kind
              value: DeamonSet

I wonder how IstioOperator would work in the kapp lifecycle that cf-for-k8s uses. This might be worth an explore story... 🤔 cc @ndhanushkodi

christianang commented 4 years ago

The IstioOperator overlay usage is interesting. Does it support modifying specific array values though like I had to do to add the hostPort fields to the specific containerPorts. One thing I wouldn't want is to split the changes between ytt'ing some and using istio configuration for others.

Good point on the autoscaling.

mike1808 commented 4 years ago

@christianang yes, it does (why we need ytt, lol, jk)

apiVersion: install.istio.io/v1alpha2
 kind: IstioControlPlane
 spec:
   gateways:
     components:
       ingressGateway:
         k8s:
           overlays:
           - apiVersion: apps/v1
             kind: Deployment
             name: istio-ingressgateway
             patches:
             - path: kind
               value: DeamonSet
             - path: spec.template.spec.containers.[name:istio-proxy].ports.[containerPort:80].hostPort
               value: 80
             - path: spec.template.spec.containers.[name:istio-proxy].ports.[containerPort:443].hostPort
               value: 443
christianang commented 4 years ago

Cool, thanks for sharing that out. Seems like a good idea to use the IstioOperator config when we can.

XanderStrike commented 4 years ago

This doesn't seem like the story to introduce the operator pattern, but we'll make a future explore to look into that. For now we'll merge the PR as is.

mike1808 commented 4 years ago

@XanderStrike what about autoscaler change? Have you tested that it doesn't have conflicts with DeamonSet?