Add v1beta2 support #67

nkinkade · 2024-09-26T22:37:11Z

This PR attempts to add v1beta2 support to this repository. A container image built with these changes is currently running in our sandbox cluster and appears to be working as intended. That said, I know little to nothing about how kubernetes APIs are written or managed, nor necessarily best practices. It is entirely possible that while these changes function, that the implementation is way off. If so, any guidance would be appreciated.

Though this PR touches 49 files, I only manually touched a small handful of them. All the other changes are the result of running go mod tidy and go mod vendor. I essentially did these few things:

Import github.com/k8snetworkplumbingwg/multi-networkpolicy/pkg/apis/k8s.cni.cncf.io/v1beta2, instead of v1beta1 (and renamed the import alias to multiv1beta2), and updated all references to it.
Import github.com/k8snetworkplumbingwg/multi-networkpolicy/pkg/client/informers/externalversions/k8s.cni.cncf.io/v1beta2, instead of v1beta1 (and renamed the import alias to multiinformerv1beta2), and updated all references to it.
Made one small code change to pkg/server/policyrules.go which modifies the --dport flag of the iptables command when port.EndPort is not nil.
Implemented a few small suggestions from the Go linter.

Are there any other changes that are necessary or that you would like to see to consider accepting this PR?

coveralls · 2024-09-30T09:18:26Z

Pull Request Test Coverage Report for Build 11165520063

Details

26 of 37 (70.27%) changed or added relevant lines in 3 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.2%) to 58.799%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/controllers/networkpolicy.go	8	9	88.89%
pkg/server/policyrules.go	16	17	94.12%
pkg/server/server.go	2	11	18.18%

Totals
Change from base Build 10792446610:	0.2%
Covered Lines:	1106
Relevant Lines:	1881

💛 - Coveralls

zeeke · 2024-09-30T09:23:56Z

@nkinkade, thanks for contributing to this. do you mind splitting the commit 33d115a into:

one for go.mod, vendor and import renaming
one with the change to support the EndPort field

Though the current commit division is good, the new one would help review the gist of the PR

nkinkade · 2024-10-01T18:50:32Z

@zeeke: Thank you for the reply. I have modified this PR such that it should only contain changes to go.mod, go.sum, vendor/, and the import renaming for v1beta2.

I realized as I was putting this together that I don't know how to handle stacked PRs from a forked repository back to the upstream in Github, to the extent that Github can even natively handle them. I have the changes that should go on top of this PR for v1beta2 support queued up in my fork:

https://github.com/m-lab/multi-networkpolicy-iptables/compare/v1beta2...m-lab:multi-networkpolicy-iptables:v1beta2-core-changes?expand=1

If I create a PR with that against this repository, the Github UI compares it to master of this repo, which contains both the changes in the PR, and the newer changes.

I can submit the other PR, and the Github UI should do the right thing once/if this PR gets merged.

Do you have any suggestions on how to proceed?

Thanks!

Nathan

nkinkade · 2024-10-01T20:00:44Z

Oh, I also noted that one of the e2e tests that were triggered by my PR failed. I will try to take a look at that to see if it was the result of my changes.

maiqueb · 2024-10-02T06:57:10Z

/uncc

zeeke · 2024-10-02T07:30:41Z

hi @nkinkade,

I suggest you to proceed as following:

# create a backup branch of your work
git branch -c v1beta2-backup

# completely discard the latest two commits
git reset --hard HEAD~2

# discard the other three commits preserving the working tree
git reset --mixed HEAD~3

# <commit your changes as you prefer>

# force push your changes to your repository
git push -f <your remote> v1beta2

Otherwise, you can deep dive into the mechanism of git rebase, but things can get complicated if you're not familiar with that
https://git-scm.com/docs/git-rebase

nkinkade · 2024-10-02T15:10:56Z

@zeeke: I apologize, I'm a little confused by your suggestions. What if I back up and start from the beginning? When you requested that I split the original PR into modules/vendor/imports and policyrules.go, that left the the implementation of actually supporting v1beta2 split between two PRs, which seemed odd to me, but I tried to do it since that is what you requested. It seems to me that the v1beta1->v1beta2 module/alias import changes should go be in the same PR as the changes to policyrules.go, since neither one works without the other.

To clarify, I would normally not touch Go modules at all, nor vendoring, but I only did so because the current go.mod imports a version of github.com/k8snetworkplumbingwg/multi-networkpolicy from before that module supported v1beta2 (v0.0.0-20200903074708-7b3ce95ae804). Perhaps it would have been better for me to manually update that single module import in go.mod rather than just running go mod tidy and go mod vendor?

What do you think about me closing #69, and backuping up on this PR, removing almost all modifications to go.mod/go.sum and vendor/, except for the one specific update necessary in go.mod for github.com/k8snetworkplumbingwg/multi-networkpolicy? Then this PR could consist of nothing but the precise changes necessary to support v1beta2.

I apologize if I've made this harder than it needs to have been by originally including a bunch of changes to Go modules and vendoring that didn't necessarily have anything to do with the functionality introduced the PR. When working on repos that I own, I'll generally pull in all changes to modules while I'm at it, just so that modules do get to stale, but I can see that this strategy doesn't work well when contributing to other projects.

zeeke · 2024-10-02T15:36:31Z

hi @nkinkade, and thanks for putting effort into this work. I really appreciate it.

What I meant in my request is to split the commit, not the PR. I think having a single PR for bumping the dependency (go.mod + vendor + other changes) is good.
What I'd like to see is a separation of the commits, so I can go over them one by one and review them (tab "Commits" on the PR).

In the first push, this PR had 3 commits:

33d115a : (go.mod + go.sum) changes, vendor folder updates, v1beta1 to v1beta2 import update, a move of a defer file.Close() statement, the port.EndPort handle logic
51b1db4 remove a debug log line
aed8c82 : unit tests

The first commit (33d115a) is a little bit hard to review, as it contains a log of change. My request is to have 1 PR (you can use this by using git push --force) with 7 commits:

commit 1: go.mod + go.sum
commit 2: vendor changes
commit 3: v1beta1 to v1beta2 import update
commit 4: a move of a defer file.Close() statement
commit 5: the port.EndPort handle logic (where we'll focus the review process, this is the most important one)
commit 6: remove a debug log line
commit 7: unit tests

Ok to close #69

Please let me know if you need guidance on this

nkinkade · 2024-10-02T15:57:22Z

Ah, I apologize for the confusion on my part! Now your previous suggestions make more sense. I will refactor this PR into discrete commits that are easier to review.

This is purely the results of running `go mod tidy` and `go mod vendor`. The only reason for doing this is to update the module github.com/k8snetworkplumbingwg/multi-networkpolicy to a version in which v1beta2 is supported.

This commit updates all references to the import to use the new import alias of "v1beta2" and "multiinformerv1beta2". Additionally, there are 5 or 6 small changes suggested by the Go linter in vscode.

This commit simply adds a small conditional checking whether port.EndPort is not nil, in which case it writes the iptable rule with flag `--dport N:N`, else it write the same iptables rule as before.

Verfies that a port specification which includes endPort writes the expected iptables rule with `--dport N:N`, and that one that doesn't include endPort writes the normal iptables rule with `--dport N`.

nkinkade · 2024-10-02T20:56:30Z

@zeeke: The commit history of this PR show now be much cleaner and easy to inspect.

There is still, at least, one outstanding issue that will probably need to be resolved before this PR can be accepted. I noticed on my first PR that most of the Kind e2e tests were failing with my changes. I suspect that this is because the e2e tests deploy the MultiNetworkPolicy CRD using scheme.yml from the master branch of the repository github.com/k8snetworkplumbingwg/multi-networkpolicy. That CRD has v1beta2 disabled. There was already some discussion of this issue not that long ago:

k8snetworkplumbingwg/multi-networkpolicy#23

Do you have any advice on how to handle this dependency, as well as the Kind e2e tests?

zeeke

Do you have any advice on how to handle this dependency, as well as the Kind e2e tests?

I don't have a clear resolution path at the moment. Let's discuss on

When will v1beta2 be fixed? multi-networkpolicy#23

I'd like to have the iptables implementation work with the v1beta1 API version for the moment.

zeeke · 2024-10-03T07:35:04Z

pkg/server/policyrules.go

+			if port.EndPort != nil {
+				writeLine(ipt.ingressPorts, "-A", chainName,
+					"-i", podIntf.InterfaceName,
+					"-m", proto, "-p", proto, "--dport", fmt.Sprintf("%s:%d", port.Port.String(), *port.EndPort),
+					"-j", "MARK", "--set-xmark", "0x10000/0x10000")
+			} else {
+				writeLine(ipt.ingressPorts, "-A", chainName,
+					"-i", podIntf.InterfaceName,
+					"-m", proto, "-p", proto, "--dport", port.Port.String(),
+					"-j", "MARK", "--set-xmark", "0x10000/0x10000")
+			}


what do you think about this form?

Suggested change

if port.EndPort != nil {

writeLine(ipt.ingressPorts, "-A", chainName,

"-i", podIntf.InterfaceName,

"-m", proto, "-p", proto, "--dport", fmt.Sprintf("%s:%d", port.Port.String(), *port.EndPort),

"-j", "MARK", "--set-xmark", "0x10000/0x10000")

} else {

writeLine(ipt.ingressPorts, "-A", chainName,

"-i", podIntf.InterfaceName,

"-m", proto, "-p", proto, "--dport", port.Port.String(),

"-j", "MARK", "--set-xmark", "0x10000/0x10000")

}

dport := port.Port.String()

if port.EndPort != nil {

dport = fmt.Sprintf("%s:%d", port.Port.String(), *port.EndPort)

}

writeLine(ipt.ingressPorts, "-A", chainName,

"-i", podIntf.InterfaceName,

"-m", proto, "-p", proto, "--dport", dport,

"-j", "MARK", "--set-xmark", "0x10000/0x10000")

Might read a little bit simpler, WDYT?

Also, do you mind adding a similar logic to renderEgressPorts(...)?

Your suggested change makes sense and is better and more readable... eliminating a duplication of writeLine().

I also added the same functionality to renderEgressPorts(), along with a basic unit test. This was an oversight on my part. Our use case only requires ingress rules, but clearly this functionality needs to exist for both. Thanks for catching that.

This was an oversight on my part, as our use case only requires ingress rules, but the functionality needs to be available for both ingress and egress rules. Additionally, I reformated the logic, per PR reviewer suggestions, to eliminate some redudnacy and make the code cleaner and more readable.

nkinkade force-pushed the v1beta2 branch from 85f9f28 to cc2b002 Compare October 1, 2024 18:17

nkinkade changed the title ~~Add v1beta2 support~~ Updates imports for v1beta2 + updates go modules and vendoring Oct 1, 2024

nkinkade changed the title ~~Updates imports for v1beta2 + updates go modules and vendoring~~ Update imports for v1beta2 + update go modules and vendoring Oct 1, 2024

nkinkade mentioned this pull request Oct 1, 2024

Add v1beta2 support to policyrules.go + basic unit test #69

Closed

nkinkade added 4 commits October 2, 2024 14:22

Updates Go modules and vendoring

f4cbd1f

This is purely the results of running `go mod tidy` and `go mod vendor`. The only reason for doing this is to update the module github.com/k8snetworkplumbingwg/multi-networkpolicy to a version in which v1beta2 is supported.

Changes import of module multi-networkpolicy from v1beta1 to v1beta2

eeda9a0

This commit updates all references to the import to use the new import alias of "v1beta2" and "multiinformerv1beta2". Additionally, there are 5 or 6 small changes suggested by the Go linter in vscode.

Adds support for endPort to policyrules.go

6f8c441

This commit simply adds a small conditional checking whether port.EndPort is not nil, in which case it writes the iptable rule with flag `--dport N:N`, else it write the same iptables rule as before.

Adds a basic unit test for endPort in the port specification

eb4a7db

Verfies that a port specification which includes endPort writes the expected iptables rule with `--dport N:N`, and that one that doesn't include endPort writes the normal iptables rule with `--dport N`.

nkinkade force-pushed the v1beta2 branch from cc2b002 to eb4a7db Compare October 2, 2024 20:40

nkinkade changed the title ~~Update imports for v1beta2 + update go modules and vendoring~~ Add v1beta2 support Oct 2, 2024

zeeke reviewed Oct 3, 2024

View reviewed changes

nkinkade added 2 commits October 3, 2024 10:01

Add basic unit test for endPort usage in egress rules

f486531

nkinkade mentioned this pull request Oct 24, 2024

Add support for API version v1beta2 m-lab/multi-networkpolicy-iptables#1

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add v1beta2 support #67

Add v1beta2 support #67

nkinkade commented Sep 26, 2024 •

edited

Loading

coveralls commented Sep 30, 2024 •

edited

Loading

zeeke commented Sep 30, 2024

nkinkade commented Oct 1, 2024

nkinkade commented Oct 1, 2024

maiqueb commented Oct 2, 2024

zeeke commented Oct 2, 2024

nkinkade commented Oct 2, 2024

zeeke commented Oct 2, 2024

nkinkade commented Oct 2, 2024

nkinkade commented Oct 2, 2024

zeeke left a comment

zeeke Oct 3, 2024

nkinkade Oct 3, 2024

Add v1beta2 support #67

Are you sure you want to change the base?

Add v1beta2 support #67

Conversation

nkinkade commented Sep 26, 2024 • edited Loading

coveralls commented Sep 30, 2024 • edited Loading

Pull Request Test Coverage Report for Build 11165520063

Details

💛 - Coveralls

zeeke commented Sep 30, 2024

nkinkade commented Oct 1, 2024

nkinkade commented Oct 1, 2024

maiqueb commented Oct 2, 2024

zeeke commented Oct 2, 2024

nkinkade commented Oct 2, 2024

zeeke commented Oct 2, 2024

nkinkade commented Oct 2, 2024

nkinkade commented Oct 2, 2024

zeeke left a comment

Choose a reason for hiding this comment

zeeke Oct 3, 2024

Choose a reason for hiding this comment

nkinkade Oct 3, 2024

Choose a reason for hiding this comment

nkinkade commented Sep 26, 2024 •

edited

Loading

coveralls commented Sep 30, 2024 •

edited

Loading