Disallow invalid VLAN/VNI value during network creation by kahou82 · Pull Request #1069 · contiv/netplugin · GitHub
Skip to content

Disallow invalid VLAN/VNI value during network creation#1069

Merged
unclejack merged 2 commits into
contiv:masterfrom
kahou82:issue-183-new
Nov 17, 2017
Merged

Disallow invalid VLAN/VNI value during network creation#1069
unclejack merged 2 commits into
contiv:masterfrom
kahou82:issue-183-new

Conversation

@kahou82

@kahou82 kahou82 commented Nov 15, 2017

Copy link
Copy Markdown
Member

Currently, netctl parse the pkgTag input as an integer directly.
Therefore when user put a garbage in pkgTag, the conversion will
default it to 0 instead.

This patchset is to cast the input to string and perform further
validation logic before we create network

Signed-off-by: Kahou Lei kalei@cisco.com

Currently, netctl parse the pkgTag input as an integer directly.
Therefore when user put a garbage in pkgTag, the conversion will
default it to 0 instead.

This patchset is to cast the input to string and perform further
validation logic before we create network

Signed-off-by: Kahou Lei <kalei@cisco.com>
Comment thread netctl/netctl.go Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit, drop the declaration of err on 538 and use
pktTag, err := strconv.Atoi(pktTagInString)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I can't as pktTag need to be used in line 560.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

just the err, pktTag still decalred on 537, not a blocker

@kahou82

kahou82 commented Nov 15, 2017

Copy link
Copy Markdown
Member Author

This is the issue refer to contiv/install#182

chrisplo
chrisplo previously approved these changes Nov 15, 2017
@chrisplo

Copy link
Copy Markdown
Contributor

build PR

@rchirakk

Copy link
Copy Markdown
Contributor

isn't it more appropriate to change to cli.Intflag ?https://github.com/contiv/netplugin/blob/master/netctl/commands.go#L219

@kahou82

kahou82 commented Nov 16, 2017

Copy link
Copy Markdown
Member Author

@rchirakk i thought pkt-tag can represent multiple VLAN IDs and VNIs and therefore it was using String for the command input?

@chrisplo chrisplo dismissed their stale review November 16, 2017 03:51

there seems to be a mismatch with expected input values and implementation

@kahou82

kahou82 commented Nov 16, 2017

Copy link
Copy Markdown
Member Author

@chrisplo please review again

@kahou82

kahou82 commented Nov 16, 2017

Copy link
Copy Markdown
Member Author

build PR

1 similar comment
@kahou82

kahou82 commented Nov 16, 2017

Copy link
Copy Markdown
Member Author

@unclejack unclejack merged commit b6cffca into contiv:master Nov 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants