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

Proposal for networkpolicy for multus interface (i.e. net-attach-def) #430

Merged
merged 1 commit into from
Jan 13, 2021
Merged

Proposal for networkpolicy for multus interface (i.e. net-attach-def) #430

merged 1 commit into from
Jan 13, 2021

Conversation

s1061123
Copy link
Contributor

No description provided.

@s1061123
Copy link
Contributor Author

/assign @jwmatthews

dougbtv
dougbtv suggested changes Aug 17, 2020
View reviewed changes
enhancements/network/multi-networkpolicy.md Outdated Show resolved Hide resolved
enhancements/network/multi-networkpolicy.md Outdated Show resolved Hide resolved
enhancements/network/multi-networkpolicy.md Outdated Show resolved Hide resolved
enhancements/network/multi-networkpolicy.md Show resolved Hide resolved
@s1061123 s1061123 force-pushed the macvlan-networkpolicy branch 2 times, most recently from 25740f7 to 4341aa4 Compare August 18, 2020 03:21
danwinship
danwinship reviewed Aug 21, 2020
View reviewed changes
enhancements/network/multi-networkpolicy.md Outdated Show resolved Hide resolved
enhancements/network/multi-networkpolicy.md Show resolved Hide resolved
enhancements/network/multi-networkpolicy.md Show resolved Hide resolved
enhancements/network/multi-networkpolicy.md Outdated Show resolved Hide resolved
enhancements/network/multi-networkpolicy.md

MultiNetworkPolicy CRD has pretty similar schema to Kubernetes NetworkPolicy and its
semantics is same as well as Kubernetes other than target interface. Target interface
is specified annotation, `k8s.v1.cni.cncf.io/policy-for`, as net-attach-def name.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this an annotation rather than a CRD field?

Copy link
Contributor Author

@s1061123 s1061123 Aug 25, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

It is to provide same usability to the users as user uses net-attach-def/Pod for multus interface, as we discussed in networking plumbing working group. In addition, it makes identical between k8s network policy and multi network policy (except for 'kind' and 'apiVersion').

Does it make sense?

@s1061123 s1061123 force-pushed the macvlan-networkpolicy branch 3 times, most recently from ae106b5 to 6871b86 Compare August 26, 2020 13:55
danwinship
danwinship reviewed Sep 11, 2020
View reviewed changes
Copy link
Contributor

@danwinship danwinship left a comment

Choose a reason for hiding this comment

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

OK so this is basically just documenting the addition of a feature that has already been almost fully implemented so it doesn't make sense to nitpick the details at this point.

/lgtm

enhancements/network/multi-networkpolicy.md Show resolved Hide resolved
@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 11, 2020
@danwinship
Copy link
Contributor

/assign @knobunc

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Sep 16, 2020
@s1061123
Copy link
Contributor Author

@danwinship I've added upstream repo info. Could you please double check and give LGTM again?

@danwinship
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 16, 2020
@openshift-bot
Copy link

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

@openshift-ci-robot openshift-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 15, 2020
@danwinship
Copy link
Contributor

/remove-lifecycle stale
/assign @squeed
(This has been ready to merge forever. The feature it describes was already implemented.)

@openshift-ci-robot openshift-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 13, 2021
@squeed
Copy link
Contributor

squeed commented Jan 13, 2021

@s1061123 would love to see a followup PR that fills in some of the missing sections.
/approve

@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: squeed

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 13, 2021
@openshift-merge-robot openshift-merge-robot merged commit 758309d into openshift:master Jan 13, 2021
@s1061123 s1061123 deleted the macvlan-networkpolicy branch March 17, 2021 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danwinship danwinship danwinship left review comments

@derekwaynecarr derekwaynecarr derekwaynecarr left review comments

@zshi-redhat zshi-redhat zshi-redhat left review comments

@dougbtv dougbtv dougbtv requested changes

@adambkaplan adambkaplan Awaiting requested review from adambkaplan

@mfojtik mfojtik Awaiting requested review from mfojtik

Assignees

@danwinship danwinship

@jwmatthews jwmatthews

@squeed squeed

@knobunc knobunc

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.