🐛 allNodesSecurityGroupRules: relax remote fields#2077
🐛 allNodesSecurityGroupRules: relax remote fields#2077k8s-ci-robot merged 1 commit intokubernetes-sigs:mainfrom
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/L
✅ Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
/cc mdbooth jnummelin |
|
@EmilienM: GitHub didn't allow me to request PR reviews from the following users: jnummelin. Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs. DetailsIn response to this:
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-sigs/prow repository. |
|
/cherry-pick release-0.10 |
|
@EmilienM: once the present PR merges, I will cherry-pick it on top of release-0.10 in a new PR and assign it to you. DetailsIn response to this:
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-sigs/prow repository. |
| } | ||
| } else { | ||
| if rule.RemoteGroupID == nil && rule.RemoteIPPrefix == nil { | ||
| return nil, fmt.Errorf("remoteManagedGroups or remoteGroupID or remoteIPPrefix must be set for allNodes security group rules") |
There was a problem hiding this comment.
I'm missing something here. Why can't we just add the rule without either remote group id or remote ip prefix?
There was a problem hiding this comment.
This was confusing to me because:
openstackclientsets a default IP prefix to0.0.0.0/0if no remote parameter is set. Source.- Gophercloud doesn't have a default for that string.
- When creating the Security Group Rule with Gophercloud (reproducer), the returned object has no Remote IP prefix, however
openstackclientwould show it to0.0.0.0/0(logs).
So I think we can disable this rule since apparently there is a default provided by Neutron.
|
/lgtm Nice catch :) |
k8s-ci-robot
added
the
lgtm
|
/hold I'll fix what we commented with Matt first. |
k8s-ci-robot
added
the
do-not-merge/hold
k8s-ci-robot
removed
the
lgtm
EmilienM
changed the title
|
/hold cancel |
k8s-ci-robot
removed
the
do-not-merge/hold
Don't make `remoteManagedGroups` required, since a user can use `remoteIPPrefix` instead or even no remote parameter at all. It's fine because Neutron will set it to an fully-open CIDR if no remote field is provided.
k8s-ci-robot
added
size/M
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mdbooth The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
|
This behavior is even better :) |
k8s-ci-robot
added
the
lgtm
|
@EmilienM: new pull request created: #2080 DetailsIn response to this:
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-sigs/prow repository. |
5 participants
What this PR does / why we need it:
This does the following:
Don't make
remoteManagedGroupsrequired, since a user can useremoteIPPrefixinstead or even no remote parameter at all.It's fine because Neutron will set it to an fully-open CIDR if no remote field is provided.
We made wrong assumptions when writing this and we're now relaxing it.
Which issue(s) this PR fixes:
Fixes #2075