Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Bump libcalico go #1814

Merged
merged 2 commits into from
Mar 1, 2018
Merged

Bump libcalico go #1814

merged 2 commits into from
Mar 1, 2018

Conversation

ozdanborne
Copy link
Member

@ozdanborne ozdanborne commented Mar 1, 2018 •
edited
Loading

Description

  • Bumps libcalico-go to latest version
  • fixes some sample policies that violated new icmp validation

Todos

  • Tests
  • Documentation
  • Release note

Release Note

Fix bug in icmp validation where ipVersion was required for all icmp rules.

@CLAassistant
Copy link

CLAassistant commented Mar 1, 2018 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@ozdanborne
bump libcalico-go
96a204d
@ozdanborne
Make dummy rules pass icmp validation
46bed62 
New icmp validation is more strict. This commit updates dummy rules so
they pass validation.
@ozdanborne ozdanborne changed the title [wip] Bump libcalico go Bump libcalico go Mar 1, 2018
caseydavenport
caseydavenport reviewed Mar 1, 2018
View reviewed changes
tests/st/utils/data.py
@@ -260,7 +258,6 @@
],
'ingress': [
{
'notICMP': {'type': 19, 'code': 255},
Copy link
Member

Choose a reason for hiding this comment

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

Why are we removing these? Which part of validation does this fail?

Copy link
Member

Choose a reason for hiding this comment

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

There's a proto: TCP just below.

Copy link
Member Author

Choose a reason for hiding this comment

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

These rules have both ICMP rules and Ports rules, which is invalid now:

  • If includes port rules, protocol needs to be TCP.
  • If it includes ICMP rules, protocol needs to be ICMP

@caseydavenport caseydavenport added this to the Calico v3.1.0 milestone Mar 1, 2018
@caseydavenport caseydavenport merged commit 1412e34 into projectcalico:master Mar 1, 2018
@caseydavenport
Copy link
Member

@ozdanborne could you write a release note?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthewdupre matthewdupre matthewdupre left review comments

@caseydavenport caseydavenport caseydavenport approved these changes

Assignees
No one assigned
Labels
release-note-required
Projects
None yet
Milestone
Calico v3.1.0
Development

Successfully merging this pull request may close these issues.
4 participants
@ozdanborne @CLAassistant @caseydavenport @matthewdupre