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.

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

Egress support for Network Policy #366

Closed
cmluciano opened this Issue Jul 28, 2017 · 36 comments

Comments

@cmluciano
Member

cmluciano commented Jul 28, 2017

Feature Description

  • One-line feature description (can be used as a release note):
    • Enable Egress network policy filtering
  • Primary contact (assignee): @cmluciano
  • Responsible SIGs: SIG-Network
  • Design proposal link (community repo):
  • Reviewer(s) - (for LGTM) recommend having 2+ reviewers (at least one from code-area OWNERS file) agreed to review. Reviewers from multiple companies preferred: @caseydavenport @danwinship
  • Approver (likely from SIG/area to which feature belongs): @thockin
  • Feature target (which target equals to which milestone):
    • Beta release target (1.8)
    • Stable release target (1.12)
@idvoretskyi

This comment has been minimized.

Member

idvoretskyi commented Sep 5, 2017

@cmluciano @kubernetes/sig-network-feature-requests any updates for 1.8? Is this feature still on track for the release?

@cmluciano

This comment has been minimized.

Member

cmluciano commented Sep 5, 2017

Yes PR is approved and is currently running through the CI tests kubernetes/kubernetes#51351

@jdumars

This comment has been minimized.

Member

jdumars commented Sep 15, 2017

@cmluciano any update on missing docs? The PR is due today.

@jdumars

This comment has been minimized.

Member

jdumars commented Sep 18, 2017

@caseydavenport @cmluciano mentioned you as the contact for docs. We need them ASAP in order to make the 1.8 release docs.

@caseydavenport

This comment has been minimized.

Member

caseydavenport commented Sep 19, 2017

Docs PR is here: kubernetes/website#5529

@cmluciano

This comment has been minimized.

Member

cmluciano commented Oct 23, 2017

this was merged for 1.8

@cmluciano cmluciano closed this Oct 23, 2017

@idvoretskyi

This comment has been minimized.

Member

idvoretskyi commented Oct 23, 2017

@cmluciano any progress is planned for 1.9?

@cmluciano

This comment has been minimized.

Member

cmluciano commented Oct 23, 2017

We don't want to move to GA for 1.9 until the features have been battle-tested more. I would expert GA for this feature and ipBlock within the 1.10 timeframe.

@idvoretskyi

This comment has been minimized.

Member

idvoretskyi commented Oct 23, 2017

@cmluciano cmluciano reopened this Oct 23, 2017

@cmluciano cmluciano modified the milestones: 1.8, next-milestone Oct 23, 2017

@cmluciano cmluciano added stage/stable and removed stage/beta labels Oct 23, 2017

@cmluciano cmluciano changed the title from [Beta] Egress support for Network Policy to [GA] Egress support for Network Policy Oct 23, 2017

@idvoretskyi

This comment has been minimized.

Member

idvoretskyi commented Jan 15, 2018

@cmluciano

This comment has been minimized.

Member

cmluciano commented Jan 16, 2018

This is still under discussion in the mailing list.

@idvoretskyi

This comment has been minimized.

Member

idvoretskyi commented Jan 22, 2018

@cmluciano any progress?

@cmluciano

This comment has been minimized.

Member

cmluciano commented Jan 22, 2018

Yes, from our mailing list conversations this feature with not GA in 1.10. We will continue discussing potential for future releases on the sig-network mailing list.

@danwinship

This comment has been minimized.

danwinship commented Jul 31, 2018

IMHO egress (including ipBlock-based egress) is ready for GA, but ipBlock-based ingress is not, and in fact should be deprecated. (We still never defined whether it interacts with cluster ingress; if it does, then it's basically impossible to implement, and if it doesn't, then it's basically useless.)

@bjhaid

This comment has been minimized.

bjhaid commented Jul 31, 2018

but ipBlock-based ingress is not, and in fact should be deprecated

As a user I disagree with this, it matches what we have been doing prior to k8s and having this was necessary to move to k8s, preventing pods to talk to random pods in different namespaces is a requirement for us and we are required to enforce it more importantly for ingress

please ignore my comment, I responded and re-read the ipblock piece.

Follow up is how does deprecating this impact pod -> pod traffic and with more thought I only want to allow nodes(and known endpoints) ingress (which we explicitly have to configure today via some controller that generates NP), how does k8s help me prevent random hosts in my network from reaching out to the pods.

@cmluciano

This comment has been minimized.

Member

cmluciano commented Aug 1, 2018

/milestone v1.12
/stage ga

@stp-ip

This comment has been minimized.

Member

stp-ip commented Aug 1, 2018

@cmluciano this should work:
/remove-stage beta
/stage stable

Also milestones is not yet supported afaik.

@k8s-ci-robot k8s-ci-robot added stage/stable and removed stage/beta labels Aug 1, 2018

@justaugustus justaugustus added this to the v1.12 milestone Aug 4, 2018

@justaugustus

This comment has been minimized.

Member

justaugustus commented Aug 4, 2018

Thanks for the update. I've added this to the 1.12 tracking sheet.

cc: @kacole2 @wadadli @robertsandoval @rajendar38

@rtheis

This comment has been minimized.

rtheis commented Aug 6, 2018

@danwinship can you please expand on the problems with ipBlock-based ingress? If source IP is maintained to the pod from outside the cluster, wouldn't that give a reasonable use case for keeping the support? We often use Calico host-based policies for ipBlock-based ingress, but it seems that Kubernetes policies could work as well.

@danwinship

This comment has been minimized.

danwinship commented Aug 13, 2018

@rtheis yes, if the source IP is maintained then the network plugin can implement ipBlock-based ingress, but some cluster-ingress mechanisms aren't able to maintain the source IP, and the network plugin generally has no control over the behavior of the cluster ingress mechanisms anyway (and there are multiple different cluster ingress mechanisms that work in completely different ways, and people can invent new ones that old network plugins won't know about).

If we want ipBlock-based ingress rules to work with cluster ingress, then we need to define how they work. (Cluster ingress mechanisms are required to preserve source IP? Cluster ingress mechanisms are required to communicate source IP to the network plugin via some out-of-band mechanism? Cluster ingress mechanisms are responsible for implementing the ipBlock-ingress part of NetworkPolicy themselves, separately from the network plugin?)

@rtheis

This comment has been minimized.

rtheis commented Aug 14, 2018

@danwinship thank you. I agree, there are plenty of challenges with ipBlock-based ingress.

@zparnold

This comment has been minimized.

Member

zparnold commented Aug 20, 2018

Hey there! @cmluciano I'm the wrangler for the Docs this release. Is there any chance I could have you open up a docs PR against the release-1.12 branch as a placeholder? That gives us more confidence in the feature shipping in this release and gives me something to work with when we start doing reviews/edits. Thanks! If this feature does not require docs, could you please update the features tracking spreadsheet to reflect it?

@cmluciano

This comment has been minimized.

Member

cmluciano commented Aug 21, 2018

@zparnold Yes. Is this just bumping the CHANGELOG?

@zparnold

This comment has been minimized.

Member

zparnold commented Aug 25, 2018

That is a great question! I unfortunately don't know enough about this feature to know whether or not it's just a CHANGELOG bump.

@cmluciano

This comment has been minimized.

Member

cmluciano commented Aug 30, 2018

@thockin @caseydavenport @danwinship Aside from ample test coverage, do we need to add anything else for this feature to be GA?

@danwinship

This comment has been minimized.

danwinship commented Aug 30, 2018

I just filed kubernetes/website#10151 but while that came out of this discussion, it's not 1.12-specific; the clarifications (assuming everyone agrees we want them) apply to the 1.11 version of the features as well. I don't think we need any further new documentation. I think egress is already as well covered by e2e tests as ingress is. So... I think we're good?

@cmluciano

This comment has been minimized.

Member

cmluciano commented Aug 30, 2018

SGTM, so I just need to remove the notes on beta from the API docs and then ensure that it is displayed properly elsewhere

@zparnold

This comment has been minimized.

Member

zparnold commented Sep 1, 2018

justaugustus pushed a commit to justaugustus/features that referenced this issue Sep 3, 2018

Merge pull request kubernetes#366 from janetkuo/daemonset-upgrade-tem…
…plate-generation

Add template generation to DaemonSet upgrade proposal
@danwinship

This comment has been minimized.

danwinship commented Sep 4, 2018

1.11 is unchanged. The feature is GA in 1.12.

@justaugustus

This comment has been minimized.

Member

justaugustus commented Sep 5, 2018

@cmluciano @danwinship -- do we have the requisite docs, tests, etc. to graduate this for 1.12?

@cmluciano

This comment has been minimized.

Member

cmluciano commented Sep 6, 2018

The only missing thing would be to remove the note about it being beta in the code comments. I reviewed https://github.com/kubernetes/community/blob/master/contributors/devel/api_changes.md#alpha-field-in-existing-api-version and did not come across anything that indicates if we just remove them to indicate "GA-ness".

Can someone from @kubernetes/sig-api-machinery-api-reviews comment on this?

@kacole2

This comment has been minimized.

Contributor

kacole2 commented Sep 28, 2018

Seeing as how this has graduated to GA in 1.12 I'm going to close the issue. Nice work, all!

/close

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Sep 28, 2018

@kacole2: Closing this issue.

In response to this:

Seeing as how this has graduated to GA in 1.12 I'm going to close the issue. Nice work, all!

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

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