Add some more answers into PRR review in EndPort kep

[rikatz]

Signed-off-by: Ricardo Pchevuzinske Katz [email protected]

[rikatz]

/uncc @dcbw @caseydavenport

Will assign a PRR approver as soon as I get someone to approve this :)

[rikatz]

/assign @wojtek-t

[wojtek-t]

Thanks for filling this in - I added a couple relatively small comments.

[wojtek-t]

Increased 4xx/5xx http error count on network policies endpoints maybe?

[rikatz]

Hum, seems a reasonable metric for me. We can have 5xx errors against netpol endpoints if some validation fails yeap. I'm afraid that 4xx http error can represent something not quite a metric error (like forbidden access to netpol object per RBAC, or maybe a not found object when getting it). but 5xx seems a good metric to me

[rikatz]

@wojtek-t thank you for the kind review :)

I've added your suggestions, and left two items to deal later:

• Make some unit tests with fg enable/disable/enable again (will see how to achieve this)
• Make some manual tests with the PR to provide further information on the KEP Perf review.

Don't know if we can move forward with those changes, otherwise I compromise myself to review this weekend, or not later then next tuesday.

[wojtek-t]

Just some nits - will approve once addressed.

[rikatz]

@wojtek-t Thank you again, corrected the small nits as requested, squashed commits so we don't polute the repo with small commits.

Thanks!

[wojtek-t]

/lgtm
/approve

[wojtek-t]

@rikatz - now you also need approval from SIG Network leads

/assign @thockin

[rikatz]

Already pinged them in slack :) Thanks!

[rikatz]

@caseydavenport @thockin made some changes, but left the question about what should happen when the feature gate is disabled, the old object differs from the new one only on the EndPort value :)

Thanks

[rikatz]

@wojtek-t so I've added in a2e29d29 the Feature Enable/Disable/Enable test.

Hopefully this is what you expect :)

I think I'll need to maybe refactor that and create a helper function instead of keep doing Update and verify in everything (it's pretty ugly now, I think) but let me know if this works as a test.

EDIT: I've just refactored and created a helper function for this operation!

@thockin @caseydavenport I've also added the check of if the field is already present, do not drop it.

If this is OK to you folks, maybe the KEP can be updated and we can continue working on the EndPort PR :)

Thank you!

[wojtek-t]

@wojtek-t so I've added in a2e29d29 the Feature Enable/Disable/Enable test.
Hopefully this is what you expect :)

Yes - this is great, thanks!
Eventually, we would like to have those kind of tests to test more behavior, but it's already huge step in that direction.

[wojtek-t]

It's probably because of my english, but I think I'm not fully parsing this sentence.

Maybe let's say:
Yes. One caveat here is that NetworkPolicy objects create with EndPort field set when the features was enabled will continue to have that field set when the feature is disabled, but will not have any effect.

WDYT?

[i.e. change this whole paragraph to what I wrote]

[rikatz]

hum, my concern here is that it actually will have effect, if the CNI knows the field.

As an example, let's say Calico is using the last client-go which knows the EndPort field. Once I disable the Feature Gate, per PrepareForCreate and PrepareForUpdate I will not be able to add a new EndPort field (if the EndPort field is not in use in that object), but existing EndPort fields can be changed or removed. So, disabling the FeatureGate only blocks users to add/change on objects that does not have this field, but does not block the CNI to consume this, or even the user to Update or Delete it.

Before reaching your comment here, I've re-written the paragraph as the following:

Yes, fields created before feature gate being disabled will not be dropped unless the user removes it from the object.
  If the user removes the field from the object it will be dropped and the 
  field can only be re-inserted if feature gate is enabled again.

How does this sounds?

[rikatz]

@wojtek-t @thockin @caseydavenport added a new commit with some more corrections :)

Thanks

[wojtek-t]

This isn't exactly true - if the feature is disabled there shouldn't be a way to drop the field even...

[rikatz]

Hum, okay, thinking here loudly (sorry for being too verbose!)

By the impl, when we edit an object that have the NetPol EndPort, but with the FG disabled, the EndPort field appears there (like in kubectl edit)

When I remove the field, PrepareForUpdate will run, and skip the dropEndPort because the field is in use, so it will follow its normal flow. (https://github.com/kubernetes/kubernetes/blob/f0601ac64d39c601a66f8f6e6f69d55995c2b32a/pkg/registry/networking/networkpolicy/strategy.go#L62-L64)

The normal flow is to take the old object, and if it's changed add +1 into Generation and then apply the desired changes.

So probably I'll need to also add into something after the PrepareForUpdate to not drop the EndPort field, even if the user has removed it?

[wojtek-t]

This is a tricky topic - I think I didn't realize one thing when writing this comment.

Looking into other examples across the codebase, the only thing we're doing is we're preventing from setting the field when the FG is switched on and silently dropping it if it was set. We allow to explicitly dropping it at any time.

So, I just actually suggest a simple nit:

Yes. One caveat here is that NetworkPolicies created with EndPort fiel set when the feature was enabled will continue to have that field set when the feature is disabled unless user removes it from the object.

[The sentence below kind-of duplicates the message]

[rikatz]

Great :)

I've improved the sentence here consolidating your suggestion and improving mine (I don't know where was my head when I wrote the next sentence, but it took me 3 reads to understand what I wanted to say).

Made another commit here

[wojtek-t]

/lgtm
/approve

@thockin @caseydavenport for SIG approval

[wojtek-t]

@rikatz - also please squash commits

[rikatz]

Squashed, pinged Tim on slack so he can take a look when gets some time :)

[wojtek-t]

/lgtm

[thockin]

Maybe clarify: The net impact of a rollback is that a port-range reverts to a single port. This may break users, but that is inevitable in such a case. It satisfies the fail-closed requirement.

[thockin]

ping me when small change is done.

[rikatz]

@thockin The change is done. I've already rebased but the only change was what you've suggested on lines 243:249.

Thanks!

[thockin]

Thanks!

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rikatz, thockin, wojtek-t

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:

• keps/prod-readiness/OWNERS [wojtek-t]
• keps/sig-network/OWNERS [thockin]

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