Bug 1547284 - Do not allow 'default' project to be isolated using 'oc adm pod-network' #18687
Conversation
openshift-ci-robot
added
the
size/M
pravisankar
added
component/networking
kind/bug
| @@ -72,8 +73,12 @@ func (i *IsolateOptions) Run() error { | |||
|
|
|||
| errList := []error{} | |||
| for _, project := range projects { | |||
| if project.Name == kapi.NamespaceDefault { | |||
There was a problem hiding this comment.
Why does this require a CLI change? The server should decide what is forbidden as the CLI can always be bypassed.
There was a problem hiding this comment.
SDN controller handles isolating project networks. As controllers run asynchronously, we will see forbidden error in the master log but the client only sees timeout error after few seconds (unfortunately there is no status field to report messages). This CLI change will not make a request to server when it knows that the server is going to reject for the given namespace and can also throw meaningful error message in this case.
| @@ -170,6 +170,9 @@ func (vmap *masterVNIDMap) updateNetID(nsName string, action network.PodNetworkA | |||
| return 0, fmt.Errorf("netid not found for namespace %q", joinNsName) | |||
| } | |||
| case network.IsolatePodNetwork: | |||
| if nsName == kapi.NamespaceDefault { | |||
There was a problem hiding this comment.
What makes this special? Do all reserved namespaces need this?
There was a problem hiding this comment.
'default' namespace is special because openshift deploys kube service in 'default' namespace by default and by allowing the namespace to be isolated, other namespaces can not access the kube api.
There was a problem hiding this comment.
"default" is treated specially in the SDN code; there are places that just assume that it has VNID 0 without checking. The fact that it was possible to renumber it at all was an accident.
|
Just some comments, not familiar enough with the area to attest if the change makes sense. |
|
/lgtm |
|
Sounds reasonable. /approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danwinship, enj, pravisankar The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
openshift-ci-robot
added
the
approved
|
/retest |
|
/retest |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
1 similar comment
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/test all [submit-queue is verifying that this PR is safe to merge] |
|
Automatic merge from submit-queue. |
No description provided.