Skip to content

Conversation

@ijsnellf
Copy link
Contributor

@ijsnellf ijsnellf commented Feb 27, 2018

Converts multiple v1alpha1 configs into equivalent v1alpha2 configs.

edit:
From the command summary:

Converts sets of v1alpha1 configs to v1alpha2 equivalents on a best effort basis. The output should be considered a starting point for your v1alpha2 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.

@ijsnellf ijsnellf requested a review from a team February 27, 2018 22:53
@ijsnellf
Copy link
Contributor Author

#3621

@istio-merge-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

Assign the PR to them by writing /assign @gnirodi 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:

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

@ijsnellf
Copy link
Contributor Author

Potential things to expand on:

  1. Convert k8s ingress into gateways.
  2. Automatically create destination rules for route rule if there is not an existing destination policy.

@ijsnellf
Copy link
Contributor Author

@ZackButcher: I'd appreciate a quick scan if you have the time

@rshriram
Copy link
Member

@ijsnellf can you expand on the description please? This is actually pretty cool.
cc @louiscryan - this is the tool I was talking about.

@codecov
Copy link

codecov bot commented Feb 28, 2018

Codecov Report

Merging #3822 into master will decrease coverage by 1%.
The diff coverage is 44%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #3822    +/-   ##
=======================================
- Coverage      76%     76%   -<1%     
=======================================
  Files         291     295     +4     
  Lines       26939   27419   +480     
=======================================
+ Hits        20426   20630   +204     
- Misses       5205    5440   +235     
- Partials     1308    1349    +41
Impacted Files Coverage Δ
pilot/cmd/istioctl/convert/egress.go 17% <17%> (ø)
pilot/cmd/istioctl/convert/cmd.go 36% <36%> (ø)
pilot/cmd/istioctl/convert/routes.go 69% <69%> (ø)
pilot/cmd/istioctl/convert/destination.go 8% <8%> (ø)
pilot/pkg/serviceregistry/kube/queue.go 86% <0%> (-3%) ⬇️
mixer/adapter/opa/opa.go 79% <0%> (-3%) ⬇️
mixer/adapter/prometheus/server.go 95% <0%> (-1%) ⬇️
mixer/adapter/rbac/rbacStore.go 83% <0%> (-1%) ⬇️
pilot/pkg/proxy/envoy/v2/mesh.go 75% <0%> (ø) ⬇️
mixer/adapter/list/list.go 95% <0%> (ø) ⬇️
... and 14 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 6d53121...3c0f376. Read the comment docs.

@ijsnellf
Copy link
Contributor Author

ijsnellf commented Feb 28, 2018

@rshriram: I added a description. Please let me know if it makes sense.

@istio-testing
Copy link
Collaborator

istio-testing commented Feb 28, 2018

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

Test name Commit Details Rerun command
prow/e2e-simpleTests.sh 3c0f376 link /test e2e-simple

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
Copy link

@ijsnellf PR needs rebase

@istio-merge-robot istio-merge-robot added the needs-rebase Indicates a PR needs to be rebased before being merged label Mar 1, 2018
Copy link
Contributor

@ZackButcher ZackButcher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very cool, and exciting to have this tooling before we adopt the new APIs!

out = append(out, model.Config{
ConfigMeta: model.ConfigMeta{
Type: model.V1alpha2RouteRule.Type,
Name: "FIXME",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

host, since we're aggregating by host

Type: model.V1alpha2RouteRule.Type,
Name: "FIXME",
Namespace: "FIXME",
Domain: "FIXME",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think to be correct we'd have to create a route rule per (host, namespace, domain), instead of just host. However, given the nature of the tool, I think it'd also be fair to just pick the namespace + domain of one of the source DestinationPolicies (e.g. the first we touch for that host) and print a warning to STDERR saying it merged rules across namespaces and advising to only process files with objects belonging to a single namespace at a time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good suggestion, thanks

func convertConfigs(readers []io.Reader, writer io.Writer) error {
configDescriptor := model.ConfigDescriptor{
model.RouteRule,
model.V1alpha2RouteRule,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

update for VirtualService rename

func Command() *cobra.Command {
cmd := &cobra.Command{
Use: "convert",
Short: "Convert configs from v1alpha1 to v1alpha2",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Convert configs from v1alpha1 to v1alpha2" -> "Convert configs from v1alpha1 to v1alpha3"

cmd := &cobra.Command{
Use: "convert",
Short: "Convert configs from v1alpha1 to v1alpha2",
Long: "Converts sets of v1alpha1 configs to v1alpha2 equivalents on a best effort basis. " +
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

v1alpha3

Use: "convert",
Short: "Convert configs from v1alpha1 to v1alpha2",
Long: "Converts sets of v1alpha1 configs to v1alpha2 equivalents on a best effort basis. " +
"The output should be considered a starting point for your v1alpha2 configs and probably " +
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

v1alpha3

case model.RouteRule.Type:
err = model.ValidateRouteRule(config.Spec)
case model.V1alpha2RouteRule.Type:
err = model.ValidateRouteRuleV2(config.Spec)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VirtualService name change

"github.com/golang/protobuf/ptypes/duration"

routing "istio.io/api/routing/v1alpha1"
routingv2 "istio.io/api/routing/v1alpha2"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

import rename

@@ -0,0 +1,24 @@
apiVersion: config.istio.io/v1alpha2
kind: V1alpha2RouteRule
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VirtualService

@@ -0,0 +1,18 @@
apiVersion: config.istio.io/v1alpha2
kind: V1alpha2RouteRule
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VirtualService

@@ -0,0 +1,29 @@
apiVersion: config.istio.io/v1alpha2
kind: V1alpha2RouteRule
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VirtualService

@@ -0,0 +1,14 @@
apiVersion: config.istio.io/v1alpha2
kind: V1alpha2RouteRule
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VirtualService

readers := make([]io.Reader, 0)
for _, filename := range inFilenames {
if filename == "-" {
readers = append(readers, os.Stdin)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: set readers and break here

@ijsnellf
Copy link
Contributor Author

Closing in favor of #4308

@ijsnellf ijsnellf closed this Mar 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-rebase Indicates a PR needs to be rebased before being merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants