adding new way to apply firewall #55
Conversation
aabughosh
added
the
do-not-merge/work-in-progress
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: aabughosh The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-merge-robot
added
the
needs-rebase
aabughosh
force-pushed
the
UpdateFirewallImp
branch
from
767da83
to
c57dce2
Compare
openshift-merge-robot
removed
the
needs-rebase
aabughosh
added
the
do-not-merge/work-in-progress
aabughosh
removed
the
do-not-merge/work-in-progress
| @@ -238,3 +240,13 @@ func (u *utils) IsBMInfra() (bool, error) { | |||
|
|
|||
| return infra.Status.PlatformStatus.Type == configv1.BareMetalPlatformType, nil | |||
| } | |||
|
|
|||
| func (u *utils) GetClusterVersion() (string, error) { | |||
There was a problem hiding this comment.
This is only used in the test, not sure if you miss a use of this func in main or you can use this function that's already in the test files
test/e2e/nftables_test.go
Outdated
| versionMajorMinor, err := utilsHelpers.GetClusterVersion() | ||
| Expect(err).ToNot(HaveOccurred()) | ||
|
|
||
| if firewall.IsVersionGreaterThan(versionMajorMinor, "4.16") { // if version more than 4.16 need to change cluster MachineConfiguration. |
There was a problem hiding this comment.
The communication matrix is only supported since 4.16
There was a problem hiding this comment.
Yah yah
The meaning is from 4.17+
And for 4.16 we are not doing that
test/pkg/firewall/firewall.go
Outdated
| if len(output) == 0 { | ||
| return nil, fmt.Errorf("no nft rules on node %s: ", debugPod.Spec.NodeName) | ||
| } | ||
| func UpdateMachineConfiguration(c *client.ClientSet) error { |
There was a problem hiding this comment.
I don't think this func belongs to firewall package
There was a problem hiding this comment.
From my understanding this is not updating a MC, it's creating a new one.
Can you just use a MC object and apply it with the client?
You already added the scheme here https://github.com/openshift-kni/commatrix/pull/55/files#diff-2895f30116602d8fe6b0545c186d4f81247fa4eaab049dab174c6c91c0036e08R49 no need to create an additional client
Also maybe use a more meaningful name for the MC than "cluster"
test/pkg/firewall/firewall.go
Outdated
| allReady := true | ||
|
|
||
| for _, mcp := range mcpList.Items { | ||
| fmt.Printf("MCP: %s\n", mcp.Name) |
There was a problem hiding this comment.
These are checked next I don't think there is an extra value in printing them
openshift-merge-robot
added
the
needs-rebase
openshift-merge-robot
removed
the
needs-rebase
No description provided.