Feature: Implement vcpctl create solution user for CCM (Stage 2) #44

fanzhangio · 2018-08-23T00:57:43Z

Add Client for delegating vim25 and ssoadmin
Add Credential for client, session and sso
Add CreateUserFunc for creating solution user and person user
Implement creating default solution user and granting WSTrust
permission and Administrator role.

fixes #15

Special notes for your reviewer:
Stage 2 for vcpctl implementation

Release note:
NONE

none

fanzhangio · 2018-08-23T05:57:21Z

/assign

frapposelli

just a couple of comments to be addressed.

@dougm PTAL at the solution user create process

frapposelli · 2018-08-23T12:59:31Z

pkg/cli/cli.go

+			u.solution.Certificate = cert
+		}
+		if u.solution.Certificate == "" {
+			return fmt.Errorf("Need solution user certificate (--cert) to create solution user\n")


What are the steps to create/fetch this certificate?

A vCenter Solution User can only use public key authentication via SAML, a Solution User does not have a plain text password. So the SSO_TOKEN needs to be provided (by ENV currently). This solution.Certificate contains the cert data decoded from crt file.
In command line, the cert file need to be passed to --cert flag.

--cert k8s-vcp.crt

It stores the raw data in ClientOption, and decoded into User.solution.Certificate

We could add an option to generate the key pair too, can wait for a later PR though.

Trailing \n should be removed from the error message.

frapposelli · 2018-08-23T12:59:35Z

pkg/cli/util.go

+			return "https://username:password@" + ip.String() + "/sdk"
+		}
+	}
+	return "https://username:password@host/sdk"


I'm a little puzzled on why you need to hardcode username:password in the URL here.

Also, seems like url.Parse would be more suitable for the type of parsing you're doing here.

The basic idea is to initialize UserInfo with username and password. The following
processCredential() call will overwrite the actual credentials based on the Credential strategy.

type Credential struct { username string password string cert string Secrete VCCMSecret }

declarative username and password

Secrete in k8s

soap.ParseURL can be used here instead? ok to initialize UserInfo, but it should be with empty "" username and password.

fanzhangio

@frapposelli
Thanks for your comments !

fanzhangio · 2018-08-23T21:32:41Z

pkg/cli/util.go

+		if v == "https" || v == "http" {
+			continue
+		}
+		ip = net.ParseIP(v)


There is a small bug in for range loop, where token v should be trimmed by //

v = strings.Trim(v, "//")

dougm

A few minor suggestions, otherwise looking good @fanzhangio

dougm · 2018-08-23T22:41:35Z

cmd/vcpctl/provision/provision.go

-	provisionCmd.Flags().StringVar(&vcport, "vcport", "", "specify vCenter Port ")
-	provisionCmd.Flags().StringVar(&vcUser, "vcuser", "", "specify vCenter user ")
-	provisionCmd.Flags().StringVar(&vcPassword, "vcpassword", "", "specify vCenter Password ")
+	provisionCmd.Flags().StringVar(&vchost, "host", "", "specify vCenter IP ")


Flag descriptions should start with an uppercase letter, and not have a trailing space.

dougm · 2018-08-23T22:42:57Z

pkg/cli/cli.go

+			u.solution.Certificate = cert
+		}
+		if u.solution.Certificate == "" {
+			return fmt.Errorf("Need solution user certificate (--cert) to create solution user\n")


We could add an option to generate the key pair too, can wait for a later PR though.

dougm · 2018-08-23T22:43:15Z

pkg/cli/cli.go

+			u.solution.Certificate = cert
+		}
+		if u.solution.Certificate == "" {
+			return fmt.Errorf("Need solution user certificate (--cert) to create solution user\n")


Trailing \n should be removed from the error message.

dougm · 2018-08-23T22:45:06Z

pkg/cli/cli.go

+		}
+		p := types.PrincipalId{Name: "k8s-vcp", Domain: c.Domain}
+
+		if _, err := c.SetRole(ctx, p, "Administrator"); err != nil {


Role should probably be a flag, we can default to Administrator. A separate feature for vcpctl could be to create a role with the minimum set of privileges required by VCP.

Yes, added a flag for role (default is Administrator).
Create Role could be in separate PR.

dougm · 2018-08-23T22:46:14Z

pkg/cli/client.go

+	username string
+	password string
+	cert     string
+	Secrete  VCCMSecret


Secrete -> Secret

dougm · 2018-08-23T22:46:42Z

pkg/cli/client.go

+	o.credential = c
+}
+
+func (c Credential) setPassowrd(p string) {


setPassowrd -> setPassword

Why the getters and setters here, rather than just use the fields directly?

Removed setters and getters.

dougm · 2018-08-23T22:47:48Z

pkg/cli/config.go

@@ -22,6 +22,10 @@ import (
 	"k8s.io/cloud-provider-vsphere/pkg/cloudprovider/vsphere"
 )

+// Config represents configuration of cpv


cpv -> vcp

dougm · 2018-08-23T22:48:56Z

pkg/cli/user.go

+		return err
+	}
+
+	// SSO admin server has its own session manager. There is no SSO token persistence yet, so just use an env


The persistence comment only applied to govc - sessions are persisted by default by govc in ~/.govmomi/sessions

dougm · 2018-08-23T22:52:20Z

pkg/cli/util.go

+			return "https://username:password@" + ip.String() + "/sdk"
+		}
+	}
+	return "https://username:password@host/sdk"


soap.ParseURL can be used here instead? ok to initialize UserInfo, but it should be with empty "" username and password.

- Add Client for delegating vim25 and ssoadmin - Add Credential for client, session and sso - Add CreateUserFunc for creating solution user and person user - Implement creating default solution user and granting WSTrust permission and Administrator role.

dougm

/lgtm

frapposelli · 2018-09-04T19:11:19Z

/lgtm
/approve

👍

k8s-ci-robot · 2018-09-04T19:11:27Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: frapposelli

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [frapposelli]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 23, 2018

k8s-ci-robot requested review from BaluDontu and frapposelli August 23, 2018 00:57

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 23, 2018

fanzhangio force-pushed the vcpctl branch from 3cc9513 to fcc5f47 Compare August 23, 2018 01:17

k8s-ci-robot assigned fanzhangio Aug 23, 2018

frapposelli reviewed Aug 23, 2018

View reviewed changes

fanzhangio commented Aug 23, 2018

View reviewed changes

dougm reviewed Aug 23, 2018

View reviewed changes

fanzhangio force-pushed the vcpctl branch from fcc5f47 to 0dc18b3 Compare September 4, 2018 17:22

fanzhangio force-pushed the vcpctl branch from 0dc18b3 to e85d903 Compare September 4, 2018 17:23

dougm approved these changes Sep 4, 2018

View reviewed changes

k8s-ci-robot assigned dougm Sep 4, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 4, 2018

k8s-ci-robot assigned frapposelli Sep 4, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 4, 2018

k8s-ci-robot merged commit 5347344 into kubernetes:master Sep 4, 2018

fanzhangio changed the title ~~Feature: Implement vcpctl create solution user for CCM~~ Feature: Implement vcpctl create solution user for CCM (Stage 2) Sep 11, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: Implement vcpctl create solution user for CCM (Stage 2) #44

Feature: Implement vcpctl create solution user for CCM (Stage 2) #44

fanzhangio commented Aug 23, 2018 •

edited

Loading

fanzhangio commented Aug 23, 2018

frapposelli left a comment

frapposelli Aug 23, 2018

fanzhangio Aug 23, 2018

dougm Aug 23, 2018

dougm Aug 23, 2018

frapposelli Aug 23, 2018

fanzhangio Aug 23, 2018

dougm Aug 23, 2018

fanzhangio left a comment

fanzhangio Aug 23, 2018

dougm left a comment

dougm Aug 23, 2018

fanzhangio Sep 4, 2018

dougm Aug 23, 2018

dougm Aug 23, 2018

dougm Aug 23, 2018

fanzhangio Sep 4, 2018

dougm Aug 23, 2018

fanzhangio Sep 4, 2018

dougm Aug 23, 2018

dougm Aug 23, 2018

fanzhangio Sep 4, 2018

dougm Aug 23, 2018

fanzhangio Sep 4, 2018

dougm Aug 23, 2018

dougm Aug 23, 2018

dougm left a comment

frapposelli commented Sep 4, 2018

k8s-ci-robot commented Sep 4, 2018

Feature: Implement vcpctl create solution user for CCM (Stage 2) #44

Feature: Implement vcpctl create solution user for CCM (Stage 2) #44

Conversation

fanzhangio commented Aug 23, 2018 • edited Loading

fanzhangio commented Aug 23, 2018

frapposelli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fanzhangio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dougm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dougm left a comment

Choose a reason for hiding this comment

frapposelli commented Sep 4, 2018

k8s-ci-robot commented Sep 4, 2018

fanzhangio commented Aug 23, 2018 •

edited

Loading