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

Add a Nets field, deprecate the Net field. #470

Merged
merged 1 commit into from
Jul 13, 2017
Merged

Add a Nets field, deprecate the Net field. #470

merged 1 commit into from
Jul 13, 2017

Conversation

fasaxc
Copy link
Member

@fasaxc fasaxc commented Jul 6, 2017 •
edited
Loading

Description

Alternative approach to allowing multiple nets in a rule.

  • Deprecate the net field, add a new nets field.
  • Validate against specifying both.
  • On read, convert net to nets.

Todos

Release Note

Add new Source.Nets and Destination.Nets fields (and their negated couterparts) 
to rules, allowing multiple CIDRs to be matched in a single rule.  The Source.Net 
and Destination.Net fields are now deprecated; when reading back data that 
contains a Net field, it will be converted to a single-entry Nets field.  Felix (and 
Typha, if in use) should be upgraded before using the new Nets fields in a rule.

@fasaxc fasaxc force-pushed the multi-nets-2 branch 3 times, most recently from 89c84fd to d22bc15 Compare July 6, 2017 15:50
@fasaxc fasaxc requested a review from robbrockbank July 6, 2017 15:55
@fasaxc
Copy link
Member Author

fasaxc commented Jul 6, 2017

@robbrockbank think I prefer this. WDYT?

caseydavenport
caseydavenport reviewed Jul 6, 2017
View reviewed changes
lib/api/rule.go
@@ -128,3 +139,28 @@ type EntityRule struct {
// Protocol match in the Rule to be set to "tcp" or "udp".
NotPorts []numorstring.Port `json:"notPorts,omitempty" validate:"omitempty,dive"`
}

func combineNets(n *net.IPNet, nets []*net.IPNet) []*net.IPNet {
Copy link
Member

Choose a reason for hiding this comment

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

Is there a requirement to support both nets and net on the same object?

Maybe we could simplify this by just validating that only one of net and nets is provided - might also be useful as a way to get users to drop using the deprecated field (they need to stop using it in order to use the new feature) so that there's less pain when that field is dropped some day in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup I agree - I think I'd suggested similar on the other PR :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

@robbrockbank That was the approach I took, the validation function for the front-end does do that so a normal user shouldn't be able to hit this. I thought it was better to have felix tolerate having both in the backend though; no point dropping a policy on the floor if it has a fairly sensible interpretation.

robbrockbank
robbrockbank reviewed Jul 7, 2017
View reviewed changes
lib/api/rule.go Outdated
}

// GetNets returns either r.Nets or a slice containing r.Net. It is useful for unifying the
// two representations. If both values are specified, it joins them together.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think the "If both values are specified" sentence should be there since it shouldn't be possible to have that and it sounds like we are encouraging the use of both.

robbrockbank
robbrockbank reviewed Jul 7, 2017
View reviewed changes
lib/backend/model/rule.go Outdated
toParts = append(toParts, "cidr", joinNets(dstNets))
}
if len(r.NotDstPorts) > 0 {
NotDstPorts := make([]string, len(r.NotDstPorts))
Copy link
Contributor

Choose a reason for hiding this comment

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

NotDstPorts -> notDstPorts

robbrockbank
robbrockbank reviewed Jul 7, 2017
View reviewed changes
lib/validator/validator.go
structLevel.ReportError(reflect.ValueOf(rule.Source.Net), "Source.Net",
"", reason("rule contains an IP version that does not match src CIDR version"))
}
// Check that only one of the net or nets fields is specified.
Copy link
Contributor

Choose a reason for hiding this comment

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

Given our desire to ween people off the singular, I wonder if we should be a little more strict and reject if there is a mix of any of the Net and Nets fields (e.g. Source.Net && Dest.NotNets)

WDYT?

@robbrockbank
Copy link
Contributor

A couple of minor comments, and branch needs updating.

Otherwise LGTM.

robbrockbank
robbrockbank suggested changes Jul 7, 2017
View reviewed changes
Copy link
Contributor

@robbrockbank robbrockbank left a comment

Choose a reason for hiding this comment

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

See comments inline.

@fasaxc fasaxc mentioned this pull request Jul 10, 2017
Merged
robbrockbank
robbrockbank approved these changes Jul 11, 2017
View reviewed changes
Copy link
Contributor

@robbrockbank robbrockbank left a comment

Choose a reason for hiding this comment

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

Can you squash and rebase please and then we can merge.

caseydavenport
caseydavenport reviewed Jul 11, 2017
View reviewed changes
lib/api/rule.go
// NotNet is the negated version of the Net field.
// NotNet is an optional field that restricts the rule to only apply to traffic that
// does not originate from (or terminate at) an IP address in the given subnet.
// Deprecated: superseded by NotNets.
Copy link
Member

Choose a reason for hiding this comment

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

Spurred by the conversation in slack - is there a good place to add a warning log when users use Nets, so we can indicate that it's deprecated?

@caseydavenport caseydavenport added this to the Calico v2.4.0 milestone Jul 11, 2017
@fasaxc
Copy link
Member Author

fasaxc commented Jul 12, 2017

@caseydavenport How about this: 0886ade

@caseydavenport
Copy link
Member

@fasaxc that LGTM - sorry for the last minute comment :p

@caseydavenport caseydavenport mentioned this pull request Jul 13, 2017
Closed
@fasaxc fasaxc merged commit 02e76aa into projectcalico:master Jul 13, 2017
@fasaxc fasaxc deleted the multi-nets-2 branch July 13, 2017 08:50
@caseydavenport caseydavenport mentioned this pull request Jul 31, 2017
Merged
3 tasks
song-jiang pushed a commit to song-jiang/libcalico-go that referenced this pull request Jul 19, 2021
@asincu
[SAAS-1230] Allow licenses with non-default name to be created (proje…
e0e826f 
…ctcalico#470)

Co-authored-by: Alina Militaru <asincu@users.noreply.github.com>
song-jiang pushed a commit to song-jiang/libcalico-go that referenced this pull request Jul 19, 2021
@asincu
Revert "[SAAS-1230] Allow licenses with non-default name to be created (
b0b306b 
projectcalico#470)" (#477)

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

@caseydavenport caseydavenport caseydavenport left review comments

@robbrockbank robbrockbank robbrockbank approved these changes

Assignees

@fasaxc fasaxc

Labels
release-note-required
Projects
None yet
Milestone
Calico v2.4.0
Development

Successfully merging this pull request may close these issues.
3 participants
@fasaxc @robbrockbank @caseydavenport