Skip to content

Bug 1547284 - Do not allow 'default' project to be isolated using 'oc adm pod-network' #18687

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.

Sign up for GitHub

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

Merged
openshift-merge-robot merged 3 commits into from
Feb 24, 2018
Merged

Bug 1547284 - Do not allow 'default' project to be isolated using 'oc adm pod-network' #18687

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
109cb89
SDN master controller should not allow isolation for 'default' project
Feb 21, 2018
d9649bc
oc adm client should forbid isolation for 'default' project
Feb 21, 2018
9780ac3
Boring change: Use lowercase for returning user errors
Feb 21, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pkg/network/common/dns.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ func (d *DNS) updateOne(dns string) (error, bool) {
// <domain-name>. <<ttl from authoritative ns> IN A <IP addr>
out, err := d.execer.Command(dig, "+nocmd", "+noall", "+answer", "+ttlid", "a", dns).CombinedOutput()
if err != nil || len(out) == 0 {
return fmt.Errorf("Failed to fetch IP addr and TTL value for domain: %q, err: %v", dns, err), false
return fmt.Errorf("failed to fetch IP addr and TTL value for domain: %q, err: %v", dns, err), false
}
outStr := strings.Trim(string(out[:]), "\n")

Expand Down
2 changes: 1 addition & 1 deletion pkg/network/master/subnets.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ func (master *OsdnMaster) SubnetStartMaster(clusterNetworks []common.ClusterNetw
glog.Infof("Found existing HostSubnet %s", common.HostSubnetToString(&sub))
_, subnetIP, err := net.ParseCIDR(sub.Subnet)
if err != nil {
return fmt.Errorf("Failed to parse network address: %q", sub.Subnet)
return fmt.Errorf("failed to parse network address: %q", sub.Subnet)
}

for _, cn := range clusterNetworks {
Expand Down
5 changes: 4 additions & 1 deletion pkg/network/master/vnids.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ func (vmap *masterVNIDMap) releaseNetID(nsName string) error {
// If not, then release the netid
if count := vmap.getVNIDCount(netid); count == 0 {
if err := vmap.netIDManager.Release(netid); err != nil {
return fmt.Errorf("Error while releasing netid %d for namespace %q, %v", netid, nsName, err)
return fmt.Errorf("error while releasing netid %d for namespace %q, %v", netid, nsName, err)
}
glog.Infof("Released netid %d for namespace %q", netid, nsName)
} else {
Expand Down Expand Up @@ -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 {
Copy link
Contributor

@enj enj Feb 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What makes this special? Do all reserved namespaces need this?

Copy link
Author

@pravisankar pravisankar Feb 22, 2018 •
edited by danwinship
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'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.

Copy link
Contributor

@danwinship danwinship Feb 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"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.

return 0, fmt.Errorf("network isolation for namespace %q is not allowed", nsName)
}
// Check if the given namespace is already isolated
if count := vmap.getVNIDCount(oldnetid); count == 1 {
return oldnetid, nil
Expand Down
6 changes: 3 additions & 3 deletions pkg/network/node/runtime.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ func (node *OsdnNode) getRuntimeService() (kubeletapi.RuntimeService, error) {
return true, nil
})
if err != nil {
return nil, fmt.Errorf("Failed to fetch runtime service: %v", err)
return nil, fmt.Errorf("failed to fetch runtime service: %v", err)
}
return node.runtimeService, nil
}
Expand All @@ -48,10 +48,10 @@ func (node *OsdnNode) getPodSandboxID(filter *kruntimeapi.PodSandboxFilter) (str

podSandboxList, err := runtimeService.ListPodSandbox(filter)
if err != nil {
return "", fmt.Errorf("Failed to list pod sandboxes: %v", err)
return "", fmt.Errorf("failed to list pod sandboxes: %v", err)
}
if len(podSandboxList) == 0 {
return "", fmt.Errorf("Pod sandbox not found for filter: %v", filter)
return "", fmt.Errorf("pod sandbox not found for filter: %v", filter)
}
return podSandboxList[0].Id, nil
}
7 changes: 6 additions & 1 deletion pkg/oc/admin/network/isolate_projects.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"github.com/spf13/cobra"

kerrors "k8s.io/apimachinery/pkg/util/errors"
kapi "k8s.io/kubernetes/pkg/apis/core"
"k8s.io/kubernetes/pkg/kubectl/cmd/templates"
kcmdutil "k8s.io/kubernetes/pkg/kubectl/cmd/util"

Expand Down Expand Up @@ -72,8 +73,12 @@ func (i *IsolateOptions) Run() error {

errList := []error{}
for _, project := range projects {
if project.Name == kapi.NamespaceDefault {
Copy link
Contributor

@enj enj Feb 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does this require a CLI change? The server should decide what is forbidden as the CLI can always be bypassed.

Copy link
Author

@pravisankar pravisankar Feb 22, 2018 •
edited by danwinship
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

errList = append(errList, fmt.Errorf("network isolation for project %q is forbidden", project.Name))
continue
}
if err = i.Options.UpdatePodNetwork(project.Name, network.IsolatePodNetwork, ""); err != nil {
errList = append(errList, fmt.Errorf("Network isolation for project %q failed, error: %v", project.Name, err))
errList = append(errList, fmt.Errorf("network isolation for project %q failed, error: %v", project.Name, err))
}
}
return kerrors.NewAggregate(errList)
Expand Down
2 changes: 1 addition & 1 deletion pkg/oc/admin/network/join_projects.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ func (j *JoinOptions) Run() error {
for _, project := range projects {
if project.Name != j.joinProjectName {
if err = j.Options.UpdatePodNetwork(project.Name, network.JoinPodNetwork, j.joinProjectName); err != nil {
errList = append(errList, fmt.Errorf("Project %q failed to join %q, error: %v", project.Name, j.joinProjectName, err))
errList = append(errList, fmt.Errorf("project %q failed to join %q, error: %v", project.Name, j.joinProjectName, err))
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/oc/admin/network/make_projects_global.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ func (m *MakeGlobalOptions) Run() error {
errList := []error{}
for _, project := range projects {
if err = m.Options.UpdatePodNetwork(project.Name, network.GlobalPodNetwork, ""); err != nil {
errList = append(errList, fmt.Errorf("Removing network isolation for project %q failed, error: %v", project.Name, err))
errList = append(errList, fmt.Errorf("removing network isolation for project %q failed, error: %v", project.Name, err))
}
}
return kerrors.NewAggregate(errList)
Expand Down
10 changes: 5 additions & 5 deletions pkg/oc/admin/network/project_options.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,12 +90,12 @@ func (p *ProjectOptions) Validate() error {
clusterNetwork, err := p.Oclient.Network().ClusterNetworks().Get(networkapi.ClusterNetworkDefault, metav1.GetOptions{})
if err != nil {
if kapierrors.IsNotFound(err) {
errList = append(errList, errors.New("Managing pod network is only supported for openshift multitenant network plugin"))
errList = append(errList, errors.New("managing pod network is only supported for openshift multitenant network plugin"))
} else {
errList = append(errList, errors.New("Failed to fetch current network plugin info"))
errList = append(errList, errors.New("failed to fetch current network plugin info"))
}
} else if !network.IsOpenShiftMultitenantNetworkPlugin(clusterNetwork.PluginName) {
errList = append(errList, fmt.Errorf("Using plugin: %q, managing pod network is only supported for openshift multitenant network plugin", clusterNetwork.PluginName))
errList = append(errList, fmt.Errorf("using plugin: %q, managing pod network is only supported for openshift multitenant network plugin", clusterNetwork.PluginName))
}

return kerrors.NewAggregate(errList)
Expand Down Expand Up @@ -140,7 +140,7 @@ func (p *ProjectOptions) GetProjects() ([]*projectapi.Project, error) {
}

if len(projectList) == 0 {
return projectList, fmt.Errorf("No projects found")
return projectList, fmt.Errorf("no projects found")
} else {
givenProjectNames := sets.NewString(p.ProjectNames...)
foundProjectNames := sets.String{}
Expand All @@ -149,7 +149,7 @@ func (p *ProjectOptions) GetProjects() ([]*projectapi.Project, error) {
}
skippedProjectNames := givenProjectNames.Difference(foundProjectNames)
if skippedProjectNames.Len() > 0 {
return projectList, fmt.Errorf("Projects %v not found", strings.Join(skippedProjectNames.List(), ", "))
return projectList, fmt.Errorf("projects %v not found", strings.Join(skippedProjectNames.List(), ", "))
}
}
return projectList, nil
Expand Down