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

Implementation on Network Policy Status #107963

Merged
merged 3 commits into from Mar 29, 2022
Merged

Implementation on Network Policy Status #107963

merged 3 commits into from Mar 29, 2022

Conversation

rikatz
Copy link
Contributor

@rikatz rikatz commented Feb 5, 2022 •
edited

What type of PR is this?

/kind feature
/kind api-change

TODO:

  • Validation of Status field
  • Implement the FG and test behavior
  • Unit test
  • e2e tests

What this PR does / why we need it:

Implement the NetworkPolicy Status subresource per KEP https://github.com/kubernetes/enhancements/tree/master/keps/sig-network/2943-networkpolicy-status

Which issue(s) this PR fixes:

Fixes Partially kubernetes/enhancements#2943

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Adds a new Status subresource in Network Policy objects

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

Please use the following format for linking documentation:

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/feature Categorizes issue or PR as related to a new feature. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/S Denotes a PR that changes 10-29 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Feb 5, 2022
@k8s-ci-robot k8s-ci-robot added sig/network Categorizes an issue or PR as relevant to SIG Network. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Feb 5, 2022
@rikatz
Copy link
Contributor Author

rikatz commented Feb 5, 2022

reviewers: this is a WIP. I will do some first pass in our KEP + eventually required API strategy validations, etc.

Right now, I think we can already discuss about the Type polarity and some well known reasons to add as constants, wdyt? :)

@rikatz rikatz closed this Feb 16, 2022
@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 16, 2022
@rikatz
Copy link
Contributor Author

rikatz commented Feb 16, 2022

/reopen

@k8s-ci-robot k8s-ci-robot reopened this Feb 16, 2022
@k8s-ci-robot
Copy link
Contributor

@rikatz: Reopened this PR.

In response to this:

/reopen

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.

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 16, 2022
rikatz
rikatz commented Feb 18, 2022
View reviewed changes
pkg/apis/networking/types.go Outdated Show resolved Hide resolved
rikatz
rikatz commented Feb 18, 2022
View reviewed changes
pkg/apis/networking/types.go Outdated Show resolved Hide resolved
rikatz
rikatz commented Feb 18, 2022
View reviewed changes
pkg/registry/networking/networkpolicy/strategy.go Outdated

// GetResetFields returns the set of fields that get reset by the strategy
// and should not be modified by the user.
// TODO: Am I doing right here? Netpol didn't had a ResetFields on the specStrategu so I'm assuming this only applies for status here?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was confused here. NetPol does not have a GetResetFields method on the regular strategy, but apparently it's a good idea to add it here as status shouldn't be modified by users.

Is this right? Should this also be added in netpolStrategy?

Copy link
Member

Choose a reason for hiding this comment

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

I don't know about this method or what it is used for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just copied this from ingress status, but TBH I have no idea what it does neither. Will drop then, and re-add as required per reviews

Copy link
Contributor Author

Choose a reason for hiding this comment

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

per Slack conversation: https://kubernetes.slack.com/archives/C0EG7JC6T/p1648138134222819

"if you don't have the reset set-up properly, someone could own spec fields on a status change"

rikatz
rikatz commented Feb 18, 2022
View reviewed changes
pkg/registry/networking/networkpolicy/strategy.go Outdated
if len(oldNetworkPolicy.Status.Conditions) < 1 {
newNetworkPolicy.Status = networking.NetworkPolicyStatus{}
}
// TODO: What to do when status field is in usage and we revert FG? Drop? Keep the old value? Allow updates?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Question to be answered: What if the feature gate is being disabled, so an old status existed but a new one no?

Should we only keep the old status? This may lead to users confused by status that doesn't reflect to reality.

Should we still allow NPPs to update it? Maybe the FG is being disabled due to some NPP error and this way we will keep accepting wrong status updates.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, we generally keep data in this case, but no updates except to clear it. It's imperfect but rare and complicated to get right (arguably impossible since the implications of resetting the data could be arbitrary)

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 29, 2022
@k8s-ci-robot k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Mar 29, 2022
@rikatz
Copy link
Contributor Author

rikatz commented Mar 29, 2022

/test pull-kubernetes-e2e-kind

@aojea
Copy link
Member

aojea commented Mar 29, 2022

/retest
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 29, 2022
@pacoxu
Copy link
Member

pacoxu commented Mar 29, 2022

/retest
/skip

@k8s-ci-robot k8s-ci-robot merged commit 42a1201 into kubernetes:master Mar 29, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.24 milestone Mar 29, 2022
@rikatz rikatz mentioned this pull request Mar 29, 2022
Closed
4 tasks
@cici37
Copy link
Contributor

cici37 commented Mar 29, 2022

/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Mar 29, 2022
@Riaankl Riaankl mentioned this pull request Apr 4, 2022
Merged
@github-actions github-actions bot mentioned this pull request Apr 5, 2022
Open
This was referenced Apr 9, 2022
Closed
Merged
@Riaankl
Copy link
Contributor

Riaankl commented Jun 29, 2022

@rikatz if I remember correctly the plan is to implement conformance test for the Network policy Status endpoint in 1.25.
I this still the case?
There is 3 weeks remaining for e2e test to be merged inorder to get to the 2 week flake free requiement before test freeze. Let me know if we could support in anyway.

muyangren2 pushed a commit to muyangren2/kubernetes that referenced this pull request Jul 14, 2022
@rikatz @muyangren2
Implementation on Network Policy Status (kubernetes#107963)
be95cfe 
* Implement status subresource in NetworkPolicy

* add NetworkPolicyStatus generated files

* Fix comments in netpol status review
@rikatz
Copy link
Contributor Author

rikatz commented Jan 23, 2023

@Riaankl just because I owe you for a long time, we are going to revert this PR as sig-net decided to withdrawn it for now :)

@rikatz rikatz mentioned this pull request Feb 16, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rahulkjoshi rahulkjoshi rahulkjoshi left review comments

@thockin thockin thockin left review comments

@aojea aojea aojea left review comments

@caseydavenport caseydavenport Awaiting requested review from caseydavenport

Assignees

@thockin thockin

@aojea aojea

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/apiserver area/code-generation area/network-policy Issues or PRs related to Network Policy subproject area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/network Categorizes an issue or PR as relevant to SIG Network. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Milestone
v1.24
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@rikatz @k8s-ci-robot @aojea @thockin @k8s-triage-robot @pacoxu @cici37 @Riaankl @rahulkjoshi