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

Istio namespace overlay #102

Merged
merged 39 commits into from
Jun 7, 2019

Conversation

kkasravi
Copy link
Contributor

@kkasravi kkasravi commented May 22, 2019

  • Define a kustomize manifest to install ISTIO on GCP gcp/{istio,istio-crds,istio-install}
  • Rearrange some kustomize manifests as a work around for the fact that kustomize does not preserve install order (i.e. kustomize re-orders config causing fail when crd or admission webhook used kubernetes-sigs/kustomize#821) ?
  • Change some kustomize manifests to properly set the namespace due to ingress and istio needing to be installed in the ISTIO namespace
  • Add virtual service resources to a bunch of the kustomize packages for webapps.
  • remove useIstio: false componentParams

This change is Reviewable

@jlewi
Copy link
Contributor

jlewi commented Jun 4, 2019

@hougangliu Can you review this so I'm not the blocker?

@kkasravi how come this PR is modifying 120 files?

@kkasravi
Copy link
Contributor Author

kkasravi commented Jun 4, 2019

@jlewi this has been in the queue for a while. Most changes are minor - moving virtual-service.yaml to overlays/istio for example. @gabrielwen is the best choice to review given his familiarity with what the kustomize targets should look like

@jlewi
Copy link
Contributor

jlewi commented Jun 6, 2019

Is the PR description accurate?

if useIstio == true, adds bootstrap/config/overlays/istio which will
add components {istio,istio-install} to app.yaml
add istio as an overlay in componentParams for all components which have a virtual-service.yaml

Is that referring to your other PR to modify kfctl?

It looks to me like the changes in this PR are

@jlewi
Copy link
Contributor

jlewi commented Jun 6, 2019

Actually is this PR in sync with master?

It looks like most kustomize packages already have virtual-service.yaml file defined inside base. It looks like this PR is defining a virtual service inside the ISTIO overlay.

But the diff doesn't show deleting any deletion of files in base?

What were your reasons for defining virtual service inside ISTIO overlays? I think that makes sense because with the goal of making each application like TFJob/pipelines installed standalone. In standalone mode we don't have a reverse proxy and may not need ISTIO so no need to add virtual services for it.

@jlewi
Copy link
Contributor

jlewi commented Jun 6, 2019

Looks like you are pushing commits to this PR. Let me know when its ready for another look.

@kkasravi kkasravi force-pushed the istio-namespace-overlay branch from 49c4389 to 7f7e40f Compare June 6, 2019 21:06
@kkasravi kkasravi force-pushed the istio-namespace-overlay branch from 28e1e14 to 217c87c Compare June 7, 2019 04:43
@jlewi
Copy link
Contributor

jlewi commented Jun 7, 2019

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jlewi

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@jlewi
Copy link
Contributor

jlewi commented Jun 7, 2019

/hold

@k8s-ci-robot k8s-ci-robot merged commit 91c2238 into kubeflow:master Jun 7, 2019
@kkasravi kkasravi deleted the istio-namespace-overlay branch June 25, 2019 02:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants