Skip to content

Commit 9f8a13f

Browse files
jujubotjujubot
authored
Merge pull request juju#10737 from babbageclunk/2.6-vsphere-permission-precheck
juju#10737 ## Description of change If a machine has a non-default root-disk constraint, we'll need the global System.Read privilege to wait for the extend task to complete. Check that we have it in PrecheckInstance so that we can give early feedback to the user, rather than failing and retrying in the provisioner. ## QA steps * Ensure the juju user doesn't have System.Read for the vsphere root (i.e. it hasn't been granted Administrator or ReadOnly) * Deploy an application without a root-disk constraint - it works. * Deploy one with root-disk - it's rejected with an error explaining what privilege should be granted. * Grant ReadOnly to the juju user. * Try the root-disk deploy again - it's accepted and the unit deploys successfully with the expanded disk. ## Documentation changes * We should add a note to the vsphere provider docs explaining the privilege needed. ## Bug reference Rest of the fix for: https://bugs.launchpad.net/juju/+bug/1847245
2 parents 55478b8 + c9eeea1 commit 9f8a13f

File tree

8 files changed

+176
-3
lines changed

8 files changed

+176
-3
lines changed

‎provider/vsphere/client.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@ type Client interface {
3333
RemoveVirtualMachines(context.Context, string) error
3434
UpdateVirtualMachineExtraConfig(context.Context, *mo.VirtualMachine, map[string]string) error
3535
VirtualMachines(context.Context, string) ([]*mo.VirtualMachine, error)
36+
UserHasRootLevelPrivilege(context.Context, string) (bool, error)
3637
}
3738

3839
func dialClient(

‎provider/vsphere/environ_policy.go

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,10 @@ func (env *sessionEnviron) PrecheckInstance(ctx context.ProviderCallContext, arg
3030
if err := env.checkZones(ctx, args.Constraints.Zones); err != nil {
3131
return errors.Trace(err)
3232
}
33-
err := env.checkDatastore(ctx, args.Constraints.RootDiskSource)
33+
if err := env.checkDatastore(ctx, args.Constraints.RootDiskSource); err != nil {
34+
return errors.Trace(err)
35+
}
36+
err := env.checkExtendPermissions(ctx, args.Constraints.RootDisk)
3437
return errors.Trace(err)
3538
}
3639

@@ -71,6 +74,25 @@ func (env *sessionEnviron) checkDatastore(ctx context.ProviderCallContext, datas
7174
return errors.NotFoundf("datastore %q", name)
7275
}
7376

77+
func (env *sessionEnviron) checkExtendPermissions(ctx context.ProviderCallContext, rootDisk *uint64) error {
78+
if rootDisk == nil || *rootDisk == 0 {
79+
return nil
80+
}
81+
// If we're going to need to resize the root disk we need to have
82+
// the System.Read privilege on the root level folder - this seems
83+
// to be because the extend disk task doesn't get created with a
84+
// target, so the root-level permissions are applied.
85+
ok, err := env.client.UserHasRootLevelPrivilege(env.ctx, "System.Read")
86+
if err != nil {
87+
return errors.Trace(err)
88+
}
89+
if !ok {
90+
user := env.cloud.Credential.Attributes()[credAttrUser]
91+
return errors.Errorf("the System.Read privilege is required at the root level to extend disks - please grant the ReadOnly role to %q", user)
92+
}
93+
return nil
94+
}
95+
7496
var unsupportedConstraints = []string{
7597
constraints.Tags,
7698
constraints.VirtType,

‎provider/vsphere/environ_policy_test.go

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,10 +4,12 @@
44
package vsphere_test
55

66
import (
7+
"github.com/juju/errors"
78
jc "github.com/juju/testing/checkers"
89
"github.com/vmware/govmomi/object"
910
"github.com/vmware/govmomi/vim25/mo"
1011
"github.com/vmware/govmomi/vim25/types"
12+
"golang.org/x/net/context"
1113
gc "gopkg.in/check.v1"
1214

1315
"github.com/juju/juju/core/constraints"
@@ -136,3 +138,24 @@ func (s *environPolSuite) TestPrecheckInstanceChecksConstraintDatastore(c *gc.C)
136138
})
137139
c.Assert(err, jc.ErrorIsNil)
138140
}
141+
142+
func (s *environPolSuite) TestPrecheckInstanceChecksForPrivIfRootDiskSet(c *gc.C) {
143+
err := s.env.PrecheckInstance(s.callCtx, environs.PrecheckInstanceParams{
144+
Constraints: constraints.MustParse("root-disk=30G"),
145+
})
146+
c.Assert(err, gc.ErrorMatches, `the System.Read privilege is required at the root level to extend disks - please grant the ReadOnly role to "user1"`)
147+
148+
s.client.SetErrors(errors.New("oops"))
149+
err = s.env.PrecheckInstance(s.callCtx, environs.PrecheckInstanceParams{
150+
Constraints: constraints.MustParse("root-disk=30G"),
151+
})
152+
c.Assert(err, gc.ErrorMatches, "oops")
153+
154+
s.client.hasPrivilege = true
155+
err = s.env.PrecheckInstance(s.callCtx, environs.PrecheckInstanceParams{
156+
Constraints: constraints.MustParse("root-disk=30G"),
157+
})
158+
c.Assert(err, jc.ErrorIsNil)
159+
160+
s.client.CheckCall(c, 0, "UserHasRootLevelPrivilege", context.Background(), "System.Read")
161+
}

‎provider/vsphere/internal/vsphereclient/client.go

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -648,3 +648,49 @@ func isManagedObjectNotFound(err error) bool {
648648
}
649649
return false
650650
}
651+
652+
// UserHasRootLevelPrivilege returns whether the connected user has the
653+
// specified privilege on the root-level object.
654+
func (c *Client) UserHasRootLevelPrivilege(ctx context.Context, privilege string) (bool, error) {
655+
session, err := c.client.SessionManager.UserSession(ctx)
656+
if err != nil {
657+
return false, errors.Annotate(err, "getting user session")
658+
}
659+
vimClient := c.client.Client
660+
req := types.HasPrivilegeOnEntities{
661+
This: *vimClient.ServiceContent.AuthorizationManager,
662+
Entity: []types.ManagedObjectReference{vimClient.ServiceContent.RootFolder},
663+
SessionId: session.Key,
664+
PrivId: []string{privilege},
665+
}
666+
667+
resp, err := methods.HasPrivilegeOnEntities(ctx, vimClient, &req)
668+
if privilege == "System.Read" && isPermissionError(err) {
669+
// This is a special case - for System.Read you need the
670+
// privilege to check whether you have the privilege.
671+
return false, nil
672+
} else if err != nil {
673+
return false, errors.Annotatef(err, "checking for %q privilege", privilege)
674+
}
675+
676+
if count := len(resp.Returnval); count != 1 {
677+
return false, errors.Errorf("expected 1 privilege response, got %d", count)
678+
}
679+
entityPriv := resp.Returnval[0]
680+
if count := len(entityPriv.PrivAvailability); count != 1 {
681+
return false, errors.Errorf("expected 1 privilege availability, got %d", count)
682+
}
683+
684+
return entityPriv.PrivAvailability[0].IsGranted, nil
685+
}
686+
687+
func isPermissionError(err error) bool {
688+
if err == nil || !soap.IsSoapFault(err) {
689+
return false
690+
}
691+
switch soap.ToSoapFault(err).VimFault().(type) {
692+
case types.NoPermission:
693+
return true
694+
}
695+
return false
696+
}

‎provider/vsphere/internal/vsphereclient/client_test.go

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,10 @@ func (s *clientSuite) SetUpTest(c *gc.C) {
7373
Type: "SearchIndex",
7474
Value: "FakeSearchIndex",
7575
},
76+
AuthorizationManager: &types.ManagedObjectReference{
77+
Type: "AuthorizationManager",
78+
Value: "FakeAuthorizationManager",
79+
},
7680
}
7781
s.roundTripper = mockRoundTripper{
7882
collectors: make(map[string]*collector),
@@ -89,6 +93,21 @@ func (s *clientSuite) SetUpTest(c *gc.C) {
8993
{Name: "name", Val: "dc0"},
9094
},
9195
}},
96+
"FakeSessionManager": {{
97+
Obj: types.ManagedObjectReference{
98+
Type: "SessionManager",
99+
Value: "FakeSessionManager",
100+
},
101+
PropSet: []types.DynamicProperty{
102+
{Name: "name", Val: "sm"},
103+
{
104+
Name: "currentSession",
105+
Val: types.UserSession{
106+
Key: "session-key",
107+
},
108+
},
109+
},
110+
}},
92111
"FakeDatacenter": {{
93112
Obj: types.ManagedObjectReference{
94113
Type: "Datacenter",
@@ -849,6 +868,47 @@ func (s *clientSuite) TestResourcePools(c *gc.C) {
849868
c.Check(result[3].InventoryPath, gc.Equals, "/z0/Resources/other")
850869
}
851870

871+
func (s *clientSuite) TestUserHasRootLevelPrivilege(c *gc.C) {
872+
client := s.newFakeClient(&s.roundTripper, "dc0")
873+
result, err := client.UserHasRootLevelPrivilege(context.Background(), "Some.Privilege")
874+
c.Assert(err, jc.ErrorIsNil)
875+
c.Assert(result, gc.Equals, true)
876+
877+
s.roundTripper.CheckCalls(c, []testing.StubCall{
878+
retrievePropertiesStubCall("FakeSessionManager"),
879+
{"HasPrivilegeOnEntities", []interface{}{
880+
"FakeAuthorizationManager",
881+
[]types.ManagedObjectReference{s.serviceContent.RootFolder},
882+
"session-key",
883+
[]string{"Some.Privilege"},
884+
}},
885+
})
886+
887+
s.roundTripper.SetErrors(nil, permissionError)
888+
result, err = client.UserHasRootLevelPrivilege(context.Background(), "System.Read")
889+
c.Assert(err, jc.ErrorIsNil)
890+
c.Assert(result, gc.Equals, false)
891+
892+
s.roundTripper.SetErrors(nil, permissionError)
893+
result, err = client.UserHasRootLevelPrivilege(context.Background(), "Other.Privilege")
894+
c.Assert(err, gc.ErrorMatches, `checking for "Other.Privilege" privilege: ServerFaultCode: Permission to perform this operation was denied.`)
895+
}
896+
897+
var permissionError = soap.WrapSoapFault(&soap.Fault{
898+
XMLName: xml.Name{Space: "http://schemas.xmlsoap.org/soap/envelope/", Local: "Fault"},
899+
Code: "ServerFaultCode",
900+
String: "Permission to perform this operation was denied.",
901+
Detail: struct {
902+
Fault types.AnyType "xml:\",any,typeattr\""
903+
}{
904+
Fault: types.NoPermission{
905+
SecurityError: types.SecurityError{},
906+
Object: types.ManagedObjectReference{Type: "Folder", Value: "group-d1"},
907+
PrivilegeId: "System.Read",
908+
},
909+
},
910+
})
911+
852912
func fakeAcquire(spec mutex.Spec) (func(), error) {
853913
return func() {}, nil
854914
}

‎provider/vsphere/internal/vsphereclient/createvm_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -114,7 +114,7 @@ func (s *clientSuite) TestCreateVirtualMachine(c *gc.C) {
114114
{"CreateFilter", nil},
115115
{"WaitForUpdatesEx", nil},
116116
{"HttpNfcLeaseComplete", []interface{}{"FakeLease"}},
117-
{"MarkAsTemplateBody", []interface{}{"FakeVm0"}},
117+
{"MarkAsTemplate", []interface{}{"FakeVm0"}},
118118
retrievePropertiesStubCall("network-0", "network-1"),
119119
retrievePropertiesStubCall("onetwork-0"),
120120
retrievePropertiesStubCall("dvportgroup-0"),

‎provider/vsphere/internal/vsphereclient/mock_test.go

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -260,9 +260,22 @@ func (r *mockRoundTripper) RoundTrip(ctx context.Context, req, res soap.HasFault
260260
}
261261
case *methods.MarkAsTemplateBody:
262262
req := req.(*methods.MarkAsTemplateBody).Req
263-
r.MethodCall(r, "MarkAsTemplateBody", req.This.Value)
263+
r.MethodCall(r, "MarkAsTemplate", req.This.Value)
264264
res.Res = &types.MarkAsTemplateResponse{}
265265

266+
case *methods.HasPrivilegeOnEntitiesBody:
267+
req := req.(*methods.HasPrivilegeOnEntitiesBody).Req
268+
r.MethodCall(r, "HasPrivilegeOnEntities", req.This.Value, req.Entity, req.SessionId, req.PrivId)
269+
res.Res = &types.HasPrivilegeOnEntitiesResponse{
270+
Returnval: []types.EntityPrivilege{{
271+
Entity: req.Entity[0],
272+
PrivAvailability: []types.PrivilegeAvailability{{
273+
PrivId: req.PrivId[0],
274+
IsGranted: true,
275+
}},
276+
}},
277+
}
278+
266279
default:
267280
logger.Debugf("mockRoundTripper: unknown res type %T", res)
268281
panic(fmt.Sprintf("unknown type %T", res))

‎provider/vsphere/mock_test.go

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@ type mockClient struct {
3939
virtualMachines []*mo.VirtualMachine
4040
datastores []*mo.Datastore
4141
vmFolder *object.Folder
42+
hasPrivilege bool
4243
}
4344

4445
func (c *mockClient) Close(ctx context.Context) error {
@@ -125,6 +126,13 @@ func (c *mockClient) UpdateVirtualMachineExtraConfig(ctx context.Context, vm *mo
125126
return c.NextErr()
126127
}
127128

129+
func (c *mockClient) UserHasRootLevelPrivilege(ctx context.Context, privilege string) (bool, error) {
130+
c.mu.Lock()
131+
defer c.mu.Unlock()
132+
c.MethodCall(c, "UserHasRootLevelPrivilege", ctx, privilege)
133+
return c.hasPrivilege, c.NextErr()
134+
}
135+
128136
func (c *mockClient) VirtualMachines(ctx context.Context, path string) ([]*mo.VirtualMachine, error) {
129137
c.mu.Lock()
130138
defer c.mu.Unlock()

0 commit comments

Comments
 (0)