Convenience tool for converting v1alpha1 configs to v1alpha3 #4308

ijsnellf · 2018-03-15T22:33:34Z

From the command description:

Converts sets of v1alpha1 configs to v1alpha3 equivalents on a best effort basis. The output should be considered a starting point for your v1alpha3 configs and probably require some minor modification. Warnings will (hopefully) be generated where configs cannot be converted perfectly, or in certain edge cases. The input must be the set of configs that would be in place in an environment at a given time. This allows the command to attempt to create and merge output configs intelligently. Output configs are given the namespace and domain of the first input config so it is recommended that input configs be part of the same namespace and domain.

I had to recreate this PR after recreating my Istio fork. There are only minor differences between this and the old PR: config metadata is grabbed from the first config and a couple small fixes were made.

Note the following potential TODOs that may be handled in a future PR:

	// TODO: k8s ingress -> gateway?
	// TODO: create missing destination rules/subsets?

Contributes to #3621.

istio-merge-robot · 2018-03-15T22:34:17Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
We suggest the following additional approver: jmuk

Assign the PR to them by writing /assign @jmuk in a comment when ready.

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

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

istioctl/OWNERS
pilot/OWNERS

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

ijsnellf · 2018-03-15T22:40:46Z

pilot/pkg/config/kube/crd/types.go

 		object: &MockConfig{
 			TypeMeta: meta_v1.TypeMeta{
-				Kind:       model.MockConfig.Type,
+				Kind:       "MockConfig",


This is from a cherry-pick of a fix for an issue that was blocking me: #4305

I personally don't mind - I think it is far more readable to see the actual string (which is a constant) rather than navigate to another file. Been hurt many times by a 'constant' having the wrong value or changing.

costinm

I think it is a great step - haven't looked in detail at the new code but it should be low risk and relatively short lived, as long as the output is right I don't think optimizations or style are critical.

Will defer to Zack, but happy to see this PR !

frankbu · 2018-03-16T13:54:59Z

I great sanity test would be to run this tool with all the bookinfo rules (samples/bookinfo/kube/route-rule-*.yaml) and compare the output to the manually converted ones in https://github.com/istio/istio/tree/master/samples/bookinfo/routing.

…er-tool

codecov · 2018-03-16T22:10:41Z

Codecov Report

Merging #4308 into master will decrease coverage by 1%.
The diff coverage is 41%.

@@           Coverage Diff           @@
##           master   #4308    +/-   ##
=======================================
- Coverage      76%     76%   -<1%     
=======================================
  Files         297     298     +1     
  Lines       24207   24522   +315     
=======================================
+ Hits        18245   18439   +194     
- Misses       5230    5334   +104     
- Partials      732     749    +17

Impacted Files	Coverage Δ
istioctl/cmd/istioctl/convert/cmd.go	`41% <41%> (ø)`
mixer/adapter/stackdriver/helper/common.go	`63% <0%> (-15%)`	⬇️
pilot/pkg/serviceregistry/kube/queue.go	`86% <0%> (-3%)`	⬇️
mixer/adapter/opa/opa.go	`79% <0%> (-3%)`	⬇️
mixer/adapter/kubernetesenv/kubernetesenv.go	`68% <0%> (-2%)`	⬇️
mixer/adapter/statsd/statsd.go	`96% <0%> (-1%)`	⬇️
mixer/adapter/list/ipList.go	`100% <0%> (ø)`	⬆️
mixer/adapter/solarwinds/solarwinds.go	`0% <0%> (ø)`	⬆️
pilot/pkg/serviceregistry/kube/controller.go	`69% <0%> (ø)`	⬆️
mixer/adapter/stdio/stdio.go	`100% <0%> (ø)`	⬆️
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4ac60af...325986e. Read the comment docs.

ijsnellf · 2018-03-16T22:14:07Z

I did base the test cases off of equivalent v1alpha1 and v1alpha3 configs in the integration tests, but I can sanity check it against the bookinfo configs as well.

ijsnellf · 2018-03-21T17:59:48Z

/retest

…er-tool

ayj · 2018-03-21T19:52:39Z

istioctl/cmd/istioctl/convert/cmd.go

+			errs = multierror.Append(err, errs)
+		}
+	}
+	return


nit: can you used a named return here, e.g. return errs?

ayj · 2018-03-21T19:59:35Z

istioctl/cmd/istioctl/convert/cmd_test.go

@@ -0,0 +1,105 @@
+// Copyright 2017 Istio Authors.


Thanks for separating this out into a new convert package. Could you also move the package under istioctl/pkg so we can mirror the organizational structure of other components? The user facing convert cobra.Command in cmd.go could be kept under istioctl/cmd subdirectory, e.g. istioctl/cmd/convert.go.

Sure. I didn't because none of the other components in istioctl follow that pattern.

ayj · 2018-03-21T20:06:09Z

istioctl/cmd/istioctl/convert/cmd.go

+// Command for converting v1alpha1 configs to v1alpha3
+func Command() *cobra.Command {
+	cmd := &cobra.Command{
+		Use:   "convert",


convert is a pretty general term for a command that only converts a subset of Istio config from one alpha version to a newer alpha version. Maybe something like convert-networking? Given that this should hopefully be short lived, relative to lifespan of Istio, we should also consider introduce an experimental or alpha subcommand group which has relaxed backwards compat. guarantees. This is analogous to cloud alpha e.g.

istioctl alpha convert-networking-apis -f <in.yaml> -o <out.yaml>

ayj · 2018-03-21T20:08:48Z

istioctl/cmd/istioctl/convert/cmd.go

+		log.Warnf("Output config(s) are invalid: %v", err)
+	}
+
+	return nil


Why don't we want to propagate the sanity check error back to the user?

ayj · 2018-03-21T20:15:46Z

istioctl/cmd/istioctl/convert/cmd_test.go

+				t.Fatal(err)
+			}
+			// FIXME: close before CompareYAML?
+			defer out.Close() // nolint: errcheck


re: FIXME, this defer should be fine.

ayj

/lgtm

Thanks for shuffling the code around!

ayj · 2018-03-22T22:56:55Z

istioctl/cmd/istioctl/convert/cmd.go

 	// sanity check that the outputs are valid
 	if err := validateConfigs(out); err != nil {
-		log.Warnf("Output config(s) are invalid: %v", err)
+		return multierror.Prefix(err, "output config(s) are invalid:")


nit: I don't think the : is required since multierror already wraps errors?

GregHanson · 2018-03-23T19:18:06Z

/lgtm

GregHanson · 2018-03-23T19:18:53Z

/lgtm

istio-testing · 2018-03-23T19:18:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ayj, GregHanson

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:

~~istioctl/OWNERS~~ [ayj]

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

istio-merge-robot · 2018-03-23T19:19:27Z

/test all [submit-queue is verifying that this PR is safe to merge]

istio-testing · 2018-03-23T19:52:16Z

@ijsnellf: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
prow/istio-pilot-e2e.sh	`325986e`	link	`/test istio-pilot-e2e`
prow/istio-pilot-e2e-v1alpha3.sh	`325986e`	link	`/test istio-pilot-e2e-v1alpha3`

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

istio-merge-robot · 2018-03-23T19:55:00Z

Automatic merge from submit-queue.

Pulling the following changes from github.com/istio/proxy: f498337 Fix a bug in origin authenticator that wrongly treats empty origin methods as pass (istio#1962) c352de0 Mixer Client: Add support for TCP local attributes (istio#1967) 2c563c6 remove not used path patcher functions (istio#1966) 490d26f Update Envoy SHA to latest with TCP proxy fixes. (istio#1964) 4cc4b7c Mixer Client uses Node metadata to populate Mixer attributes (istio#1961) cf23357 Fix the peerIsOptional and originIsOptional for authn filter. (istio#1959) cc6e58e support per-path JWT validation. (istio#1928) a5dd1aa mixer: clear route cache on header update (istio#1946) Pulling the following changes from github.com/envoyproxy/envoy: f936fc60f ssl: serialize accesses to SSL socket factory contexts (istio#4345) e34dcd62a Fix crash in tcp_proxy (istio#4323) ae6a25222 router: fix matching when all domains have wildcards (istio#4326) aa06142ff test: Stop fake_upstream methods from accidentally succeeding (istio#4232) 5d731878f rbac: update the authenticated.user to a StringMatcher. (istio#4250) c6bfc7d9a time: Event::TimeSystem abstraction to make it feasible to inject time with simulated timers (istio#4257) 752483ea9 Fixing the fix (istio#4333) 83487f6f3 tls: update BoringSSL to ab36a84b (3497). (istio#4338) 7bc210e02 test: fixing interactions between waitFor and ignore_spurious_events (istio#4309) 69474b398 admin: order stats in clusters json admin (istio#4306) 2d155f901 ppc64le build (istio#4183) 07efc6dc6 fix static initialization fiasco problem (istio#4314) 0b7e3b5e0 test: Remove declared but undefined class methods (istio#4297) 1485a1304 lua: make sure resetting dynamic metadata wrapper when request info is marked dead d243cd62e test: set to zero when start_time exceeds limit (istio#4328) 0a1e92acc test: fix heap use-after-free in ~IntegrationTestServer. (istio#4319) cddc732c7 CONTRIBUTING: Document 'kick-ci' trick. (istio#4335) f13ef2464 docs: remove reference to deprecated value field (istio#4322) e947a2766 router: minor doc fixes in stream idle timeout (istio#4329) 0c2e998af tcp-proxy: fixing a TCP proxy bug where we attempted to readDisable a closed connection (istio#4296) 00ffe44a2 utility: fix strftime overflow handling. (istio#4321) af1183c28 Re-enable TcpProxySslIntegrationTest and make the tests pass again. (istio#4318) 35534617b fuzz: fix H2 codec fuzzer post istio#4262. (istio#4311) 42f604853 Proto string issue fix (istio#4320) 9c492a01d Support Envoy to fetch secrets using SDS service. (istio#4256) a8572192f ratelimit: revert `revert rate limit failure mode config` and add tests (istio#4303) 1d34172bd dns: fix exception unsafe behavior in c-ares callbacks. (istio#4307) 121242340 alts: add gRPC TSI socket (istio#4153) f0363ae63 fuzz: detect client-side resets in H2 codec fuzzer. (istio#4300) 01aa3f820 test: hopefully deflaking echo integration test (istio#4304) 1fc0f4ba2 ratelimit: link legacy proto when message is being used (istio#4308) aa4481e6b fix rare List::remove(&target) segfault (istio#4244) 89e0f23ba headers: fixing fast fail of size-validation (istio#4269) 97eba5918 build: bump googletest version. (istio#4293) 0057e22d9 fuzz: avoid false positives in HCM fuzzer. (istio#4262) 9d094e590 Revert ac0bd74f6f9716e3a44d1412f795317c30ca770a (istio#4295) ddb28a4a1 Add validation context provider (istio#4264) 3b47cbabb added histogram latency information to Hystrix dashboard stream (istio#3986) cf87d50cd docs: update SNI FAQ. (istio#4285) f952033a4 config: fix update empty stat for eds (istio#4276) 329e591d3 router: Add ability of custom headers to rely on per-request data (istio#4219) 68d20b46c thrift: refactor build files and imports (istio#4271) 5fa8192a3 access_log: log requested_server_name in tcp proxy (istio#4144) fa45bb48f fuzz: libc++ clocks don't like nanos. (istio#4282) 53f8944f7 stats: add symbol table for future stat name encoding (istio#3927) c987b425b test infra: Remove timeSource() from the ClusterManager api (istio#4247) cd171d9a9 websocket: tunneling websockets (and upgrades in general) over H2 (istio#4188) b9dc5d9a0 router: disallow :path/host rewriting in request_headers_to_add. (istio#4220) 0c9101127 network: skip socket options and source address for UDS client connections (istio#4252) da1857d59 build: fixing a downstream compile error by noting explicit fallthrough (istio#4265) 9857cfe2a fuzz: cleanup per-test environment after each fuzz case. (istio#4253) 52beb067d test: Wrap proto string in std::string before comparison (istio#4238) f5e219edc extensions/thrift_proxy: Add header matching to thrift router (istio#4239) c9ce5d2b1 fuzz: track read_disable_count bidirectionally in codec_impl_fuzz_test. (istio#4260) 35103b353 fuzz: use nanoseconds for SystemTime in RequestInfo. (istio#4255) ba6ba9883 fuzz: make runtime root hermetic in server_fuzz_test. (istio#4258) b0a901480 time: Add 'format' test to ensure no one directly instantiates Prod*Time from source. (istio#4248) 85674603b access_log: support beginning of epoch in START_TIME. (istio#4254) 28d5f4118 proto: unify envoy_proto_library/api_proto_library. (istio#4233) f7d3cb638 http: fix allocation bug introduced in istio#4211. (istio#4245) Fixes istio#8310. Signed-off-by: Piotr Sikora <piotrsikora@google.com>

* Update Proxy SHA to latest with TCP proxy fixes. Pulling the following changes from github.com/istio/proxy: f498337 Fix a bug in origin authenticator that wrongly treats empty origin methods as pass (#1962) c352de0 Mixer Client: Add support for TCP local attributes (#1967) 2c563c6 remove not used path patcher functions (#1966) 490d26f Update Envoy SHA to latest with TCP proxy fixes. (#1964) 4cc4b7c Mixer Client uses Node metadata to populate Mixer attributes (#1961) cf23357 Fix the peerIsOptional and originIsOptional for authn filter. (#1959) cc6e58e support per-path JWT validation. (#1928) a5dd1aa mixer: clear route cache on header update (#1946) Pulling the following changes from github.com/envoyproxy/envoy: f936fc60f ssl: serialize accesses to SSL socket factory contexts (#4345) e34dcd62a Fix crash in tcp_proxy (#4323) ae6a25222 router: fix matching when all domains have wildcards (#4326) aa06142ff test: Stop fake_upstream methods from accidentally succeeding (#4232) 5d731878f rbac: update the authenticated.user to a StringMatcher. (#4250) c6bfc7d9a time: Event::TimeSystem abstraction to make it feasible to inject time with simulated timers (#4257) 752483ea9 Fixing the fix (#4333) 83487f6f3 tls: update BoringSSL to ab36a84b (3497). (#4338) 7bc210e02 test: fixing interactions between waitFor and ignore_spurious_events (#4309) 69474b398 admin: order stats in clusters json admin (#4306) 2d155f901 ppc64le build (#4183) 07efc6dc6 fix static initialization fiasco problem (#4314) 0b7e3b5e0 test: Remove declared but undefined class methods (#4297) 1485a1304 lua: make sure resetting dynamic metadata wrapper when request info is marked dead d243cd62e test: set to zero when start_time exceeds limit (#4328) 0a1e92acc test: fix heap use-after-free in ~IntegrationTestServer. (#4319) cddc732c7 CONTRIBUTING: Document 'kick-ci' trick. (#4335) f13ef2464 docs: remove reference to deprecated value field (#4322) e947a2766 router: minor doc fixes in stream idle timeout (#4329) 0c2e998af tcp-proxy: fixing a TCP proxy bug where we attempted to readDisable a closed connection (#4296) 00ffe44a2 utility: fix strftime overflow handling. (#4321) af1183c28 Re-enable TcpProxySslIntegrationTest and make the tests pass again. (#4318) 35534617b fuzz: fix H2 codec fuzzer post #4262. (#4311) 42f604853 Proto string issue fix (#4320) 9c492a01d Support Envoy to fetch secrets using SDS service. (#4256) a8572192f ratelimit: revert `revert rate limit failure mode config` and add tests (#4303) 1d34172bd dns: fix exception unsafe behavior in c-ares callbacks. (#4307) 121242340 alts: add gRPC TSI socket (#4153) f0363ae63 fuzz: detect client-side resets in H2 codec fuzzer. (#4300) 01aa3f820 test: hopefully deflaking echo integration test (#4304) 1fc0f4ba2 ratelimit: link legacy proto when message is being used (#4308) aa4481e6b fix rare List::remove(&target) segfault (#4244) 89e0f23ba headers: fixing fast fail of size-validation (#4269) 97eba5918 build: bump googletest version. (#4293) 0057e22d9 fuzz: avoid false positives in HCM fuzzer. (#4262) 9d094e590 Revert ac0bd74f6f9716e3a44d1412f795317c30ca770a (#4295) ddb28a4a1 Add validation context provider (#4264) 3b47cbabb added histogram latency information to Hystrix dashboard stream (#3986) cf87d50cd docs: update SNI FAQ. (#4285) f952033a4 config: fix update empty stat for eds (#4276) 329e591d3 router: Add ability of custom headers to rely on per-request data (#4219) 68d20b46c thrift: refactor build files and imports (#4271) 5fa8192a3 access_log: log requested_server_name in tcp proxy (#4144) fa45bb48f fuzz: libc++ clocks don't like nanos. (#4282) 53f8944f7 stats: add symbol table for future stat name encoding (#3927) c987b425b test infra: Remove timeSource() from the ClusterManager api (#4247) cd171d9a9 websocket: tunneling websockets (and upgrades in general) over H2 (#4188) b9dc5d9a0 router: disallow :path/host rewriting in request_headers_to_add. (#4220) 0c9101127 network: skip socket options and source address for UDS client connections (#4252) da1857d59 build: fixing a downstream compile error by noting explicit fallthrough (#4265) 9857cfe2a fuzz: cleanup per-test environment after each fuzz case. (#4253) 52beb067d test: Wrap proto string in std::string before comparison (#4238) f5e219edc extensions/thrift_proxy: Add header matching to thrift router (#4239) c9ce5d2b1 fuzz: track read_disable_count bidirectionally in codec_impl_fuzz_test. (#4260) 35103b353 fuzz: use nanoseconds for SystemTime in RequestInfo. (#4255) ba6ba9883 fuzz: make runtime root hermetic in server_fuzz_test. (#4258) b0a901480 time: Add 'format' test to ensure no one directly instantiates Prod*Time from source. (#4248) 85674603b access_log: support beginning of epoch in START_TIME. (#4254) 28d5f4118 proto: unify envoy_proto_library/api_proto_library. (#4233) f7d3cb638 http: fix allocation bug introduced in #4211. (#4245) Fixes #8310. Signed-off-by: Piotr Sikora <piotrsikora@google.com> * Update go-control-plane API and fix test fails * Fixed the pilot rbac build fail due to envoyproxy/envoy#4250 * Fixed the istioctl test fail due to envoyproxy/envoy#4306 Signed-off-by: Piotr Sikora <piotrsikora@google.com>

Isaiah Snell-Feikema added 4 commits March 15, 2018 16:58

convenience tool for converting v1alpha1 configs to v1alpha3 configs

3d81232

fix generated crd kind

15fff7e

fix kinds

85ec3a7

tweak output for default routing + redirects

7c0ee1d

ijsnellf requested review from a team, GregHanson, ZackButcher, frankbu, ldemailly and rshriram March 15, 2018 22:33

googlebot added the cla: yes label Mar 15, 2018

ijsnellf changed the title ~~V1alpha3 converter tool~~ Convenience tool for converting v1alpha1 configs to v1alpha3 Mar 15, 2018

ijsnellf commented Mar 15, 2018

View reviewed changes

costinm reviewed Mar 16, 2018

View reviewed changes

ldemailly requested review from kyessenov and removed request for ldemailly March 16, 2018 04:53

ijsnellf mentioned this pull request Mar 16, 2018

[WIP] v1alpha2 conversion tool #3822

Closed

Isaiah Snell-Feikema added 4 commits March 16, 2018 15:27

Merge branch 'master' of github.com:istio/istio into v1alpha3-convert…

0a47b5d

…er-tool

Merge branch 'master' of github.com:istio/istio into v1alpha3-convert…

e4e7324

…er-tool

accomodate gogo api changes

7cb1b75

fmt

bfb41a0

ijsnellf removed the request for review from kyessenov March 16, 2018 22:14

Merge branch 'master' of github.com:istio/istio into v1alpha3-convert…

cfc7042

…er-tool

ayj reviewed Mar 21, 2018

View reviewed changes

fixed broken fault percentage conversion

f3581ff

Isaiah Snell-Feikema added 2 commits March 22, 2018 17:13

address comments

68d3194

missing cmd name

478091d

ayj approved these changes Mar 22, 2018

View reviewed changes

istio-testing assigned ayj Mar 22, 2018

istio-testing added lgtm labels Mar 22, 2018

fmt

325986e

istio-testing removed the lgtm label Mar 23, 2018

istio-testing assigned GregHanson Mar 23, 2018

istio-testing added the lgtm label Mar 23, 2018

istio-merge-robot merged commit 3988cfe into istio:master Mar 23, 2018

ijsnellf deleted the v1alpha3-converter-tool branch March 23, 2018 19:58

PiotrSikora mentioned this pull request Sep 11, 2018

Update Proxy SHA to latest with TCP proxy fixes. #8606

Merged

Convenience tool for converting v1alpha1 configs to v1alpha3 #4308

Convenience tool for converting v1alpha1 configs to v1alpha3 #4308

Uh oh!

Conversation

ijsnellf commented Mar 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

istio-merge-robot commented Mar 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

costinm left a comment

Choose a reason for hiding this comment

Uh oh!

frankbu commented Mar 16, 2018

Uh oh!

codecov bot commented Mar 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ijsnellf commented Mar 16, 2018

Uh oh!

ijsnellf commented Mar 21, 2018

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!

ayj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GregHanson commented Mar 23, 2018

Uh oh!

GregHanson commented Mar 23, 2018

Uh oh!

istio-testing commented Mar 23, 2018

Uh oh!

istio-merge-robot commented Mar 23, 2018

Uh oh!

istio-testing commented Mar 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

istio-merge-robot commented Mar 23, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

ijsnellf commented Mar 15, 2018 •

edited

Loading

codecov bot commented Mar 16, 2018 •

edited

Loading

istio-testing commented Mar 23, 2018 •

edited

Loading