Add calicoctl convert command for manifest offline conversions #1782

gunjan5 · 2017-12-06T02:00:18Z

Description

example multi-resource manifest conversion:
Original:

cat ../test-data/v1/multi-resource.yaml
- apiVersion: v1
  kind: bgpPeer
  metadata:
    node: Node1
    peerIP: aa:bb::ff
    scope: node
  spec:
    asNumber: 64514
- apiVersion: v1
  kind: bgpPeer
  metadata:
    node: Node2
    peerIP: 1.2.3.4
    scope: node
  spec:
    asNumber: 6455
---
apiVersion: v1
kind: bgpPeer
metadata:
  scope: global
  peerIP: 192.20.30.40
spec:
  asNumber: 64567

---
- apiVersion: v1
  kind: ipPool
  metadata:
    cidr: 192.168.0.0/16
  spec:
    ipip:
      enabled: true

- apiVersion: v1
  kind: ipPool
  metadata:
    cidr: 2001::00/120
  spec:

Conversion command:

calicoctl convert -f ../test-data/v1/multi-resource.yaml -o yaml

- apiVersion: projectcalico.org/v3
  kind: BGPPeer
  metadata:
    creationTimestamp: null
    name: node1.00aa-00bb-0000-0000-0000-0000-0000-00ff
  spec:
    asNumber: 64514
    node: node1
    peerIP: aa:bb::ff
- apiVersion: projectcalico.org/v3
  kind: BGPPeer
  metadata:
    creationTimestamp: null
    name: node2.1-2-3-4
  spec:
    asNumber: 6455
    node: node2
    peerIP: 1.2.3.4
- apiVersion: projectcalico.org/v3
  kind: BGPPeer
  metadata:
    creationTimestamp: null
    name: 192-20-30-40
  spec:
    asNumber: 64567
    peerIP: 192.20.30.40
- apiVersion: projectcalico.org/v3
  kind: IPPool
  metadata:
    creationTimestamp: null
    name: 192-168-0-0-16
  spec:
    cidr: 192.168.0.0/16
    ipipMode: Always
- apiVersion: projectcalico.org/v3
  kind: IPPool
  metadata:
    creationTimestamp: null
    name: 2001---120
  spec:
    cidr: 2001::/120
    ipipMode: Never

Another example:

original yaml file:

- apiVersion: v1
  kind: bgpPeer
  metadata:
    node: Node1
    peerIP: aa:bb::ff
    scope: node
  spec:
    asNumber: 64514
- apiVersion: v1
  kind: bgpPeer
  metadata:
    node: Node2
    peerIP: 1.2.3.4
    scope: node
  spec:
    asNumber: 6455

conversion:

calicoctl convert -f ./bgpplist.yaml -o yaml
- apiVersion: projectcalico.org/v3
  kind: BGPPeer
  metadata:
    creationTimestamp: null
    name: node1.00aa-00bb-0000-0000-0000-0000-0000-00ff
  spec:
    asNumber: 64514
    node: node1
    peerIP: aa:bb::ff
- apiVersion: projectcalico.org/v3
  kind: BGPPeer
  metadata:
    creationTimestamp: null
    name: node2.1-2-3-4
  spec:
    asNumber: 6455
    node: node2
    peerIP: 1.2.3.4

Todos

Tests
Documentation
Release note

Release Note

Add calicoctl convert command for manifest offline conversions

robbrockbank · 2017-12-07T07:10:13Z

calicoctl/commands/convert.go

+
+
+Description:
+  Convert config files between different API versions. Both YAML and JSON formats are accepted.


between different implies this is more flexible than it is. I think it's just that it converts v1 manifests to v3.

robbrockbank · 2017-12-07T07:11:29Z

calicoctl/commands/convert.go

+	}
+}
+
+func convertResource(v1resource unversioned.Resource) (runtime.Object, error) {


missing a fn description. Perhaps should make clear it's converting v1 to v3.

robbrockbank · 2017-12-07T07:12:28Z

calicoctl/commands/convert.go

+	}
+}
+
+func convert(convRes conversion.Converter, v1resource unversioned.Resource) (conversion.Resource, error) {


missing a fn description.

Not sure you gain much from this extra method, other than making it difficult to explain the difference between convertResource() and convert().

I'm not too fussed if this stays as it is, but another way to structure this would be to have the resource-specific switch statement select a converter to use and then just join two methods together, or have a method that returns the converter for a specific resource type (which is easier to explain than the subtle different between convertResource and convert).

Anyways, it's a nit and I'm talking more than I should.

robbrockbank · 2017-12-07T07:13:14Z

calicoctl/commands/convert.go

+	if err != nil {
+		return nil, err
+	}
+


We should probably validate the final result resource too. WDYT?

I think it would be a good idea, that would catch some of the problems I've seen in testing when taking converted data and applying it.

hmmm, not sure if it makes sense to fail validation on something that user can't control, I'd say it's better to fail when a user tries to apply the config. Failing validation on the config we generated seems a bit weird (from the UX PoV)

Agree with @gunjan5 on this - our bad v3 manifest will be caught when the user applies it. As G5 mentions, there's nothing the user can do about our botched conversion (other than edit and try to apply again).

robbrockbank · 2017-12-07T07:17:29Z

calicoctl/commands/convert.go

+		rp = nil
+	}
+
+	if rp == nil {


nit: can the following two lines just be put in the default: branch above and then we can do away with this if statement.

robbrockbank · 2017-12-07T07:19:14Z

calicoctl/commands/convert.go

+
+	err = rp.print(nil, results)
+	if err != nil {
+		fmt.Println(err)


should exit with non-zero RC here.

robbrockbank

Some really minor feedback - but otherwise this looks good.

lwr20 · 2017-12-07T09:22:59Z

Please add STs :)

tmjd · 2017-12-07T16:19:21Z

I've got a PR against gunjan's branch gunjan5#2 to add some STs for this.

bcreane

a few nits, overall looks quite good.

bcreane · 2017-12-07T17:56:56Z

calicoctl/commands/convert.go

+                [--output=<OUTPUT>]
+
+Examples:
+  # Create a policy using the data in policy.yaml.


Prefer "convert" to "create" here. Something like: "Convert the contents of policy.yaml to v3 policy."

bcreane · 2017-12-07T18:18:23Z

calicoctl/commands/convert.go

+  # Create a policy using the data in policy.yaml.
+  calicoctl convert -f ./policy.yaml -o yaml
+
+  # Create a policy based on the JSON passed into stdin.


Probably "Create" -> "Convert"

bcreane · 2017-12-07T18:18:37Z

calicoctl/commands/convert.go

+
+Options:
+  -h --help                     Show this screen.
+  -f --filename=<FILENAME>      Filename to use to create the resource.  If set to


nit, single space after period.

hehe, i know i know i complain about this and now i have this in my code too. I guess blind copy paste :em

bcreane · 2017-12-07T18:19:11Z

calicoctl/commands/convert.go

+
+
+Description:
+  Convert config files between different API versions. Both YAML and JSON formats are accepted.


bcreane · 2017-12-07T18:21:34Z

calicoctl/commands/convert.go

+		rp = nil
+	}
+
+	if rp == nil {


bcreane · 2017-12-07T18:22:14Z

calicoctl/commands/convert.go

+	}
+}
+
+func convertResource(v1resource unversioned.Resource) (runtime.Object, error) {


bcreane · 2017-12-07T18:23:31Z

calicoctl/commands/v1resourceloader/v1resourceloader.go

+	if !ok {
+		return nil, errors.New(fmt.Sprintf("Unknown resource type (%s) and/or version (%s)", tm.Kind, tm.APIVersion))
+	}
+	log.Infof("Found resource helper: %s", rh)


debug level log?

hmm I've used Info to be consistent with the other calicoctl commands, but I think you're right, it/they should be debug

o nvm i thought you were talking about the log.Infof("results: %+v", results) line in convert.go, but this makes more sense, I'll change it to debug

bcreane · 2017-12-07T18:36:26Z

calicoctl/commands/convert.go

+Description:
+  Convert config files between different API versions. Both YAML and JSON formats are accepted.
+
+  The default output will be printed to stdout in YAML format.


suggest: "calicoctl sends YAML to standard output (stdout) by default."

"will be printed" is future tense/passive voice.

I mostly copied the description from kubectl convert command help to be consistent, I can change it if you like.

gunjan5 · 2017-12-08T20:24:05Z

@robbrockbank @bcreane @tmjd @lwr20 I've added validation for the converted resources as well as a new --ignore-validation flag. It's in the last commit. I could use some help around the wording of the help text and the error message when we fail validation.

robbrockbank · 2017-12-08T23:02:50Z

I assume we'll look into fixing the creationTimestamp: null or just live-with? I think it'll be a pain to get rid of - we'd need to convert the struct to a map[string]interface{} and then remove the field from the map.

gunjan5 · 2017-12-08T23:08:54Z

yeah it's hard to get rid of, we also print that with --export flag. Would've been easier to add omitempty if we didn't import the ObjectMeta field from k8s. I think we've already adjusted our tests to deal with it

bcreane

lgtm, just one potential nit.

bcreane · 2017-12-08T23:21:58Z

calicoctl/commands/convert.go

+
+
+Description:
+  Convert config files from Calico V1 to V3 API versions. Both YAML and JSON formats are accepted.


nit: is it v[13] or V[13]? Thought the former ...

bcreane · 2017-12-08T23:27:13Z

calicoctl/commands/convert.go

+	if err != nil {
+		return nil, err
+	}
+


Agree with @gunjan5 on this - our bad v3 manifest will be caught when the user applies it. As G5 mentions, there's nothing the user can do about our botched conversion (other than edit and try to apply again).

gunjan5 added the PR/WIP label Dec 6, 2017

gunjan5 added this to the Calico v3.0.0 milestone Dec 6, 2017

gunjan5 self-assigned this Dec 6, 2017

gunjan5 added the release-note-required label Dec 6, 2017

gunjan5 force-pushed the offline-migration-tool branch from a41c635 to 9043fa8 Compare December 6, 2017 23:30

gunjan5 removed the PR/WIP label Dec 6, 2017

gunjan5 changed the title ~~[WIP] Add calicoctl convert command for manifest offline conversions~~ Add calicoctl convert command for manifest offline conversions Dec 6, 2017

gunjan5 requested review from robbrockbank and bcreane December 7, 2017 00:10

robbrockbank reviewed Dec 7, 2017

View reviewed changes

robbrockbank suggested changes Dec 7, 2017

View reviewed changes

bcreane reviewed Dec 7, 2017

View reviewed changes

gunjan5 force-pushed the offline-migration-tool branch from 050bd8d to e08dd2d Compare December 8, 2017 20:24

robbrockbank approved these changes Dec 8, 2017

View reviewed changes

bcreane approved these changes Dec 8, 2017

View reviewed changes

Add calicoctl convert command for manifest offline conversions

8c31bfb

gunjan5 force-pushed the offline-migration-tool branch from 85a37e3 to 8c31bfb Compare December 8, 2017 23:37

gunjan5 merged commit 628abe6 into projectcalico:master Dec 11, 2017



		Description:
		Convert config files between different API versions. Both YAML and JSON formats are accepted.



		Description:
		Convert config files from Calico V1 to V3 API versions. Both YAML and JSON formats are accepted.

Add calicoctl convert command for manifest offline conversions #1782

Add calicoctl convert command for manifest offline conversions #1782

Conversation

gunjan5 commented Dec 6, 2017 • edited Loading

Description

Todos

Release Note

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robbrockbank Dec 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robbrockbank left a comment

Choose a reason for hiding this comment

lwr20 commented Dec 7, 2017

tmjd commented Dec 7, 2017

bcreane left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gunjan5 commented Dec 8, 2017

robbrockbank commented Dec 8, 2017

gunjan5 commented Dec 8, 2017

bcreane left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gunjan5 commented Dec 6, 2017 •

edited

Loading

robbrockbank Dec 7, 2017 •

edited

Loading