-
Notifications
You must be signed in to change notification settings - Fork 70
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
Fix compilation errors and test and add random-fully to the masquerade rule #108
Conversation
Welcome @Ramkumar-K! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Ramkumar-K for addressing this long standing issue as well as the new test breakage, much appreciated.
I left some comments around the fake iptables change but the fix itself looks good to me. It might be good to see if we can clean up the commits a bit by squashing the relevant ones :)
@@ -312,7 +312,7 @@ func (f *FakeIPTables) RestoreAll(data []byte, flush iptables.FlushFlag, counter | |||
return err | |||
} | |||
} | |||
f.Dump = dump | |||
// f.Dump = dump |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see, does it mean if we do SaveInto()
instead then we won't need to carry an internal patch here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I looked at other usage and by using the SaveInto, we don't need this change.
Also, changing this line causes the fake_test to break. Looking at the code, I thought that this line was not required as this function seems to update the right places.
go.mod
Outdated
@@ -6,7 +6,7 @@ require ( | |||
github.com/golang/glog v1.0.0 | |||
k8s.io/apimachinery v0.26.1 | |||
k8s.io/component-base v0.26.1 | |||
k8s.io/kubernetes v1.15.0-alpha.0 | |||
k8s.io/kubernetes v1.26.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ha, looks like this addresses a previous comment that I made - it would be nice to clean up the commits I think to avoid confusion when people is reading the codes commit by commit :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies. Was not sure how the PR will look. Have now squashed all the commits.
0f3451f
to
1d21f1d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
/lgtm
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: MrHohn, Ramkumar-K 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 |
To address a customer issue, the --random-fully flag is being added to the masquerade rule in IPtables.