Change the installation files for cluster wide by andraxylia · Pull Request #860 · istio/istio

andraxylia · 2017-09-19T05:33:43Z

This is needed to switch to cluster-wide installation files for istio.yaml and istio-auth.yaml.
Installation flow will ask the user to:

Install one of istio.yaml or istio-auth.yaml
Optionally install the istio-initializer.yaml, if cluster has alpha enabled and transparent injection of proxy is desired.

The namespace restricted files used for testing on a shared cluster have been renamed to istio-one-namespace.yaml, respectively istio-one-namespace-auth.yaml.

Updated e2e tests to use the istio-one-namespace* files.

Fixes #146

istio-merge-robot · 2017-09-19T05:34:06Z

@andraxylia PR needs rebase

ldemailly · 2017-09-19T06:15:54Z

install/kubernetes/BUILD

    name = "kubernetes",
    srcs = [
        "istio.yaml",
-        "istio-auth.yaml",


As mentioned over email I think we should keep a single file installing everything in 1 step (with auth)

ldemailly · 2017-09-19T06:21:11Z

install/kubernetes/templates/istio-ca-one-namespace.yaml.tmpl

              apiVersion: v1
              fieldPath: metadata.namespace
---
+---


make sure all files end properly with a newline (side note we need a linter for that, I keep repeating it in PR after PR)

why is that? it actually creates ugly resulting concatenated files.

@andraxylia no it's the only way to be able to concatenate 2 files properly actually
if you have file A

line1 line2

and file B

line3

if you cat A B and A doesn't have the last newline you'll get

line1 line2line3

which is usually not what you want

it's also a sane convention for a files of X lines to have exactly X newlines, not X-1 and most editors are set by default to, or can be set to, add the missing newlines for you

ldemailly · 2017-09-19T06:24:18Z

tests/e2e/framework/kubernetes.go


-	content = []byte(strings.Replace(string(content),
-		`args: ["discovery", "-v", "2", "--admission-service", "istio-pilot-external"]`,
-		`args: ["discovery", "-v", "2", "--admission-service", "istio-pilot-external", "-a", "`+k.Namespace+`"]`,


this was almost ready to work with 2 namespaces in shared env

ldemailly · 2017-09-19T06:26:48Z

install/updateVersion.sh

-  cat $SRC/istio-namespace-ca.yaml.tmpl >> $ISTIO
+  cp $ISTIO $ISTIO_ONE_NAMESPACE
+  # restrict pilot controllers to a single namespace in the test file
+  sed -i=.bak "s|args: \[\"discovery\", \"-v\", \"2\"|args: \[\"discovery\", \"-v\", \"2\", \"-a\", \"${ISTIO_NAMESPACE}\"|" $ISTIO_ONE_NAMESPACE


it'd be nice to support in shared env 1 namespace for the app and 1 namespace for istio; but I guess we can change that later

ldemailly · 2017-09-19T06:28:38Z

install/kubernetes/istio-one-namespace.yaml

        image: gcr.io/istio-testing/pilot:aca4710958367c0ac39e0a890cce599690680414
        imagePullPolicy: IfNotPresent
-        args: ["discovery", "-v", "2", "--admission-service", "istio-pilot-external"]
+        args: ["discovery", "-v", "2", "-a", "istio-system", "--admission-service", "istio-pilot-external"]


it's really weird to watch istio-system - this should be default or app-namespace no ?
(unless we move that file to be for testing only maybe)

that file is mainly for testing.

If you want to use a different ns, do a sed istio-system.

let's set it to something else to be replaced so it's not confusing

we can generate a file with default, but then we cannot sed properly, default is used in many places.
maybe call it istio-sandbox or istio-app?

that's not ok, because then the user would not be able to generate with the chosen name. Maybe in a separate commit.

call it APPNAMESPACE or whatever makes it both easy to replace and clear it needs replacement

I will think about it, but it is not a priority.

ldemailly · 2017-09-19T06:28:52Z

install/kubernetes/istio-rbac-beta.yaml

@@ -1,3 +1,5 @@
+# GENERATED FILE. Use with Kubernetes 1.7+


cool ! thx!

I think that the comments will be removed if I generate this file again? How about updating the README here a bit to reflect this?

No, they will not be removed.
I will remove this file in a separate commit, it's no longer useful.

I see, thanks @andraxylia

andraxylia · 2017-09-19T06:35:13Z

/retest

andraxylia · 2017-09-19T07:12:31Z

/retest all

ldemailly · 2017-09-19T07:17:10Z

install/kubernetes/istio.yaml

        image: gcr.io/istio-testing/istio-ca:87901f278dd40e864b083409dfee9ca72e6a27e9
        imagePullPolicy: IfNotPresent
-        env:
-        - name: NAMESPACE


did that version somehow get accidentally deployed to the shared test cluster ?
(trying to understand how come we have errors in the bad CA sha PR that appear to be bleeding on other PRs)

No, it has not. Just checked the cluster, there is nothing there.

andraxylia · 2017-09-19T15:43:03Z

/test e2e-suite-rbac-auth

andraxylia · 2017-09-19T16:00:50Z

/test e2e-suite-rbac-no_auth

andraxylia · 2017-09-19T17:08:23Z

/test e2e-suite-rbac-auth

andraxylia · 2017-09-19T17:08:31Z

/test e2e-suite-rbac-no_auth

ldemailly · 2017-09-19T18:42:24Z

can you update the summary and including adding a rationale for why have 2 files that could stay combined in 1 is an improvement

Alternatively if a pr isn't ready maybe put WIP in the summary until it's ready

rshriram · 2017-09-19T19:57:24Z

Please provide a description for context.

istio-merge-robot · 2017-09-19T23:53:35Z

@andraxylia PR needs rebase

ldemailly · 2017-09-19T23:56:20Z

install/kubernetes/istio-auth.yaml

 data:
  mesh: |-
-    # Uncomment the following line to enable mutual TLS between proxies
+    # Uncomment the following line to enable mutual encryption


leave it as mTLS or as "encryption" but not "mutual encryption" that's meaningless

this should be left alone as mTLS. we don't support regular TLS which is also encryption.

ldemailly · 2017-09-19T23:56:49Z

install/kubernetes/istio-auth.yaml

    #
    # Along with discoveryRefreshDelay, this setting determines how
-    # frequently should Envoy fetch and update its internal configuration
+    # frequently should Envoy fetch and update its ntiernal configuration


spellcheck/typo (review diff)

ldemailly · 2017-09-19T23:57:26Z

install/kubernetes/istio-one-namespace-auth.yaml

        image: gcr.io/istio-testing/pilot:aca4710958367c0ac39e0a890cce599690680414
        imagePullPolicy: IfNotPresent
-        args: ["discovery", "-v", "2", "--admission-service", "istio-pilot-external"]
+        args: ["discovery", "-v", "2", "-a", "istio-system", "--admission-service", "istio-pilot-external"]


same comment as before

andraxylia · 2017-09-20T01:12:52Z

/test istio-presubmit

andraxylia · 2017-09-20T02:12:42Z

Addressed the comments, PTAL.

gyliu513 · 2017-09-20T03:53:21Z

install/updateVersion.sh

  cat $SRC/istio-ns.yaml.tmpl >> $ISTIO
  cat $SRC/istio-rbac-beta.yaml.tmpl >> $ISTIO
  cat $SRC/istio-mixer.yaml.tmpl >> $ISTIO
+  cat $SRC/istio-config.yaml.tmpl >> $ISTIO


Do we also need to update those templates as well by adding the info of GENERATED FILE. Use with Kubernetes 1.7+, @andraxylia

Not sure what you mean... the templates are not generated.

@gyliu513 see line 134,135 ?

thanks, got it, then seems this is missing for the $ISTIO_INITIALIZER?

Added it, ptal.

ldemailly · 2017-09-20T04:14:03Z

install/kubernetes/istio-one-namespace-auth.yaml

        image: gcr.io/istio-testing/pilot:e32cdc4b77175ccc800231e4103ab2b0182ea25d
        imagePullPolicy: IfNotPresent
-        args: ["discovery", "-v", "2", "--admission-service", "istio-pilot-external"]
+        args: ["discovery", "-v", "2", "-a", "istio-system", "--admission-service", "istio-pilot-external"]


it doesn't make sense to watch istio-system (3rd time)

ldemailly · 2017-09-20T04:19:18Z

install/kubernetes/templates/istio-config.yaml.tmpl

@@ -0,0 +1,66 @@
+apiVersion: v1


what does the moving from pilot to this new file do ? (it is probably cleaner if that config isn't just about pilot - is it? - but it's not necessary for 0.2?)

Yes, it's much cleaner.

ldemailly · 2017-09-20T04:20:18Z

install/updateVersion.sh

  cat $SRC/istio-ns.yaml.tmpl >> $ISTIO
  cat $SRC/istio-rbac-beta.yaml.tmpl >> $ISTIO
  cat $SRC/istio-mixer.yaml.tmpl >> $ISTIO
+  cat $SRC/istio-config.yaml.tmpl >> $ISTIO


@gyliu513 see line 134,135 ?

gyliu513

/lgtm

istio-merge-robot · 2017-09-20T16:29:01Z

@andraxylia PR needs rebase

ldemailly · 2017-09-20T16:54:34Z

/retest

rshriram

/lgtm

istio-merge-robot · 2017-09-20T17:15:02Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gyliu513, rshriram

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

Details

Needs approval from an approver in each of these OWNERS Files:

~~OWNERS~~ [rshriram]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

Former-commit-id: 388289074b98e51f23ef07d8373634e9ee187594

* Change the installation files Former-commit-id: 1c04856

Former-commit-id: 49bde5caa5fb136cb3295f01882b1050408759c6

* Change the installation files Former-commit-id: 1c04856

* remove istioctl * remove remainder * fix template

* Change the installation files Former-commit-id: 1c04856

* OSSM-2378: Use Maistra proxy CentOS Stream 8 img (istio#713) * OSSM-2378: Use Maistra proxy CentOS Stream 8 img For integration tests. Based on (istio#609) * Build container images with docker Signed-off-by: Jacek Ewertowski <jewertow@redhat.com> * Use maistra proxy and CentOS Stream 8 as a base image for proxyv2 Signed-off-by: Jacek Ewertowski <jewertow@redhat.com> * Test VMs using CentOS Stream 8 Signed-off-by: Jacek Ewertowski <jewertow@redhat.com> * Temporarily skip failing security tests Signed-off-by: Jacek Ewertowski <jewertow@redhat.com> * Temporarily skip tests suite telemetry.prometheus.wasm Signed-off-by: Jacek Ewertowski <jewertow@redhat.com> Signed-off-by: Jacek Ewertowski <jewertow@redhat.com> * fix(tests): enables fixed integration tests Signed-off-by: Jacek Ewertowski <jewertow@redhat.com> Co-authored-by: Jacek Ewertowski <jewertow@redhat.com> * Use our proxy (istio#801) Signed-off-by: Jacek Ewertowski <jewertow@redhat.com> * OSSM-5958: Remove wasm extensions (istio#937) * Do not rely on WASM extensions Since maistra/proxy#253 * Remove wasm extensions from EnvoyFilters Signed-off-by: Jacek Ewertowski <jewertow@redhat.com> * Disable tests for WASM extensions Signed-off-by: Jacek Ewertowski <jewertow@redhat.com> * Remove commented code Signed-off-by: Jacek Ewertowski <jewertow@redhat.com> --------- Signed-off-by: Jacek Ewertowski <jewertow@redhat.com> Co-authored-by: Jonh Wendell <jwendell@redhat.com> * Disable failing tests Signed-off-by: Jacek Ewertowski <jewertow@redhat.com> * Fix TestTrustDomainValidation - change expected TLS error to make it work with OpenSSL Signed-off-by: Jacek Ewertowski <jewertow@redhat.com> --------- Signed-off-by: Jacek Ewertowski <jewertow@redhat.com> Co-authored-by: Bartosz Majsak <bartosz.majsak@gmail.com> Co-authored-by: Jonh Wendell <jonh.wendell@redhat.com> Co-authored-by: Jonh Wendell <jwendell@redhat.com>

Change the installation files

c86f263

googlebot added the cla: yes label Sep 19, 2017

istio-merge-robot added needs-rebase Indicates a PR needs to be rebased before being merged release-note labels Sep 19, 2017

Merge remote-tracking branch 'origin' into yaml

da47bce

istio-merge-robot removed the needs-rebase Indicates a PR needs to be rebased before being merged label Sep 19, 2017

andraxylia added 2 commits September 18, 2017 22:58

Put back ns

31bc537

Fix ns

0543524

ldemailly reviewed Sep 19, 2017

View reviewed changes

Run update

68122d7

Add newline

693d1e6

andraxylia self-assigned this Sep 19, 2017

ldemailly reviewed Sep 19, 2017

View reviewed changes

andraxylia requested a review from costinm September 19, 2017 17:07

Use two files, one without auth, one with auth

f5a0fd4

istio-merge-robot added the needs-rebase Indicates a PR needs to be rebased before being merged label Sep 19, 2017

andraxylia added 2 commits September 19, 2017 16:54

Remove ns from configmap template

32b87eb

updateVersion

8a555c7

ldemailly reviewed Sep 19, 2017

View reviewed changes

Merge remote-tracking branch 'origin' into yaml

0b4af6b

andraxylia added 2 commits September 19, 2017 18:53

Disable temporarily the check

2db856a

Newline

fba96d8

gyliu513 reviewed Sep 20, 2017

View reviewed changes

ldemailly reviewed Sep 20, 2017

View reviewed changes

Added generated for the initializer

e202894

gyliu513 reviewed Sep 20, 2017

View reviewed changes

Merge remote-tracking branch 'origin' into yaml

48d29ae

istio-merge-robot added the needs-rebase Indicates a PR needs to be rebased before being merged label Sep 20, 2017

Merge remote-tracking branch 'origin' into yaml

916d0ff

istio-merge-robot removed the needs-rebase Indicates a PR needs to be rebased before being merged label Sep 20, 2017

Fix the merge error

0a7a945

rshriram approved these changes Sep 20, 2017

View reviewed changes

istio-merge-robot added the approved label Sep 20, 2017

andraxylia merged commit 1c04856 into master Sep 20, 2017

andraxylia mentioned this pull request Sep 20, 2017

Update installation files to deploy cluster-wide in istio-system namespace #146

Closed

andraxylia changed the title ~~Change the installation files~~ Change the installation files for cluster wide Sep 20, 2017

andraxylia deleted the yaml branch September 21, 2017 17:26

mandarjog pushed a commit to mandarjog/istio that referenced this pull request Oct 30, 2017

Fix linter warnings. (istio#860)

e5b942a

Former-commit-id: 388289074b98e51f23ef07d8373634e9ee187594

rshriram pushed a commit that referenced this pull request Oct 30, 2017

Change the installation files (#860)

9df862a

* Change the installation files Former-commit-id: 1c04856

mandarjog pushed a commit that referenced this pull request Oct 31, 2017

Fix linter warnings. (#860)

7b72a26

Former-commit-id: 49bde5caa5fb136cb3295f01882b1050408759c6

vbatts pushed a commit to vbatts/istio that referenced this pull request Oct 31, 2017

Change the installation files (istio#860)

0fe4f21

* Change the installation files Former-commit-id: 1c04856

mandarjog pushed a commit that referenced this pull request Oct 31, 2017

Move istioctl out of pilot to istio/config (#860)

0fcc3b9

* remove istioctl * remove remainder * fix template

mandarjog pushed a commit that referenced this pull request Nov 2, 2017

Change the installation files (#860)

c6b88c8

* Change the installation files Former-commit-id: 1c04856

0x01001011 pushed a commit to thedemodrive/istio that referenced this pull request Jul 16, 2020

Add support for inline role definition (istio#860)

8b4ca3e

Conversation

andraxylia commented Sep 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

istio-merge-robot commented Sep 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ldemailly Sep 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andraxylia commented Sep 19, 2017

Uh oh!

andraxylia commented Sep 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andraxylia commented Sep 19, 2017

Uh oh!

andraxylia commented Sep 19, 2017

Uh oh!

andraxylia commented Sep 19, 2017

Uh oh!

andraxylia commented Sep 19, 2017

Uh oh!

ldemailly commented Sep 19, 2017

Uh oh!

rshriram commented Sep 19, 2017

Uh oh!

istio-merge-robot commented Sep 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andraxylia commented Sep 20, 2017

Uh oh!

andraxylia commented Sep 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andraxylia commented Sep 19, 2017 •

edited

Loading

ldemailly Sep 19, 2017 •

edited

Loading

ldemailly Sep 20, 2017 •

edited

Loading