Subdomain User Operations #76

rejoshed · 2022-05-06T23:03:09Z

Issue #, if available: CTECH-214

Description of changes:
When account and domain are specified CAPC will find (or create) a user in that domain and perform operations as that user. 🎆

Testing performed:
Created a cluster w/Affinity groups in a subdomain using an account and user in that domain.

Deleted the machinedeployment and watched the deletion process. Recreated machinedeployment. Deleted the whole cluster.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

aws-paris-bot · 2022-05-06T23:03:11Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

maxdrib

Would it be possible to create an e2e test for this scenario? It seems fairly complex so I'd feel more comfortable if we were validating that the users are handled correctly

maxdrib · 2022-05-09T14:48:24Z

pkg/cloud/user_credentials.go

+// GetOrCreateUserWithKeys will search a domain and account for the first user that has api keys.
+// Creates a CAPC user if no such user is found.
+func (c *client) GetOrCreateUserWithKeys(user *User) error {
+	return nil
+}


can we remove these no-op and unused functions?

Well, this is a draft PR, so yes I'll remove them. : )

BTW, I do eventually want methods like this in the codebase to be able to create domains/accounts/users on the fly in testing.

But since we will probably never do that in production, I'll probably put them in the test suite itself.

controllers/utils/base_reconciler.go

wongni · 2022-05-09T14:56:53Z

controllers/utils/base_reconciler.go

+				r.CSCluster.Spec.Account, r.CSCluster.Spec.Domain)
+		}
+		cfg := cloud.Config{APIKey: user.APIKey, SecretKey: user.SecretKey}
+		if client, err := r.CSClient.NewClientFromSpec(cfg); err != nil {


Suggested change

if client, err := r.CSClient.NewClientFromSpec(cfg); err != nil {

if r.CSUser, err := r.CSClient.NewClientFromSpec(cfg); err != nil {

Can we remove the else statement with this?

Not without a more verbose variable declaration to bring both vars into scope. : )

wongni · 2022-05-09T14:59:45Z

pkg/cloud/client.go

+	if err != nil && strings.Contains(strings.ToLower(err.Error()), "i/o timeout") {
+		return newC, errors.Wrap(err, "timeout while checking CloudStack API Client connectivity")
+	}
+	return newC, errors.Wrap(err, "checking CloudStack API Client connectivity:")


Looks like this function always returns an error. When should it return nil for error?

It doesn't. errors.Wrap will return nil if the passed err is nil.

pkg/cloud/user_credentials.go

wongni · 2022-05-09T15:07:33Z

pkg/cloud/user_credentials.go

+
+// GetUserWithKeys will search a domain and account for the first user that has api keys.
+// Returns true if a user is found and false otherwise.
+func (c *client) GetUserWithKeys(user *User) (bool, error) {


How does this function work differently from calling ResolveUserKeys directly?

Well, ResolveUserKeys works if a user is already specified--for example you've already got a user's name or ID.

In all of our current cases we don't know of any sub-domain users until we run. As the method comment mentions, this will look at all users in a domain and resolve the passed user pointer to the first user that has sufficient API keys.

wongni · 2022-05-09T15:08:15Z

pkg/cloud/user_credentials.go

+
+// CreateUserWithKeys will create a CloudStack user in the specified domain and account and generate API keyes for
+// said user.
+func (c *client) CreateUserWithKeys(user *User) error {


Please remove the functions not used and not implemented.

Well, this is a draft PR, so yes I'll remove them. : )

BTW, I do eventually want methods like this in the codebase to be able to create domains/accounts/users on the fly in testing.

But since we will probably never do that in production, I'll probably put them in the test suite itself.

rejoshed · 2022-05-09T19:04:11Z

pkg/cloud/instance.go

@@ -109,7 +109,7 @@ func (c *client) ResolveTemplate(
 	zoneID string,
 ) (templateID string, retErr error) {
 	if len(csMachine.Spec.Template.ID) > 0 {
-		csTemplate, count, err := c.cs.Template.GetTemplateByID(csMachine.Spec.Template.ID, "all")
+		csTemplate, count, err := c.cs.Template.GetTemplateByID(csMachine.Spec.Template.ID, "executable")


"all" is only availalble to admin users.

rejoshed · 2022-05-09T19:05:05Z

pkg/cloud/instance.go

 	// Attempt deletion regardless of machine state.
-	p := c.cs.VirtualMachine.NewDestroyVirtualMachineParams(*csMachine.Spec.InstanceID)
+	p := c.csAsync.VirtualMachine.NewDestroyVirtualMachineParams(*csMachine.Spec.InstanceID)


This shouldn't make any difference, but I was checking that.

We use the Async client here anyway, so felt better to match.

rejoshed · 2022-05-09T19:05:43Z

pkg/cloud/instance_test.go

-				return fmt.Sprintf(`"%s" and "%s", %s`, account, domainID, desc)
-			}
-		}
-		DescribeTable("DomainID and Account test table.",


We no longer need to set domain and account, so all these extra tests are for code that doesn't exist.

rejoshed · 2022-05-09T19:05:59Z

pkg/cloud/isolated_network_test.go

@@ -229,7 +229,6 @@ var _ = Describe("Network", func() {
 			dummies.SetDummyIsoNetToNameOnly()
 			dummies.SetClusterSpecToNet(&dummies.ISONet1)
 			dummies.CSCluster.Spec.ControlPlaneEndpoint.Host = ""
-			Ω(client.ResolveZones(dummies.CSCluster)).Should(Succeed())


No such thing anymore!

rejoshed · 2022-05-09T19:43:46Z

@maxdrib

Would it be possible to create an e2e test for this scenario? It seems fairly complex so I'd feel more comfortable if we were validating that the users are handled correctly

Yes, it is possible. I will need to add an account and domain to our EC2 instance that has full perms, and then we'd just add that account and domain to our normal runs.

That said, I probably can't get to this today.

maxdrib · 2022-05-09T19:48:54Z

The code looks good to me. I think it'd be worth calling out the specific behavior that is implemented when using subdomains w.r.t. users expectations in the README

maxdrib · 2022-05-09T19:50:07Z

pkg/cloud/user_credentials_test.go

+	})
+
+	Context("UserCred Semi-Integ Tests", func() {
+		client, connectionErr := cloud.NewClient("../../cloud-config")


should we be hardcoding this path in the tests? I'd prefer to look at the secrets environment variable being set instead

Completely, we should not.

This and other issues are why I want to turn my attention towards our CI/CD this week.

The semi-integ unit tests only work locally at the moment. They do pass for me, but there are other configuration hurdles besides this one that is preventing the semi-integs from working. For example, the name of the sub domain user should be configurable.

The E2E tests have some of this configuration, but not enough to run all tests. So, yeah. Working to get to this this week. : )

maxdrib · 2022-05-09T19:51:16Z

pkg/cloud/user_credentials_test.go

+			}
+
+			// Settup dummies.
+			// TODO: move these to the test dummies package.


could you create an issue to track this?

I don't think the overhead of an issue for this one todo is necessary, but I can if you really want. I'll be in this file this week too.

maxdrib · 2022-05-09T19:51:37Z

pkg/cloud/user_credentials_test.go

+
+			// Settup dummies.
+			// TODO: move these to the test dummies package.
+			domain = cloud.Domain{Path: "ROOT/blah/blah/subsub"}


I'd also like to see a test case where domain is not provided and we assume it to be root

I will see about adding unit test for that soon, but this is handled via our E2Es.

maxdrib · 2022-05-09T19:52:45Z

Yes, it is possible. I will need to add an account and domain to our EC2 instance that has full perms, and then we'd just add that account and domain to our normal runs.

@rejoshed would you mind creating an issue to track this?

aws-paris-bot · 2022-05-09T19:56:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: maxdrib, rejoshed, wongni

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~~ [maxdrib,rejoshed,wongni]

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

rejoshed · 2022-05-09T19:56:43Z

@maxdrib

E2E test issue for tracking.
#78

Yes, it is possible. I will need to add an account and domain to our EC2 instance that has full perms, and then we'd just add that account and domain to our normal runs.

@rejoshed would you mind creating an issue to track this?

maxdrib · 2022-05-09T20:03:30Z

/lgtm

Init.

f3690c0

aws-paris-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 6, 2022

aws-paris-bot requested review from jweite-amazon and maxdrib May 6, 2022 23:03

aws-paris-bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 6, 2022

rejoshed changed the title ~~Init.~~ Subdomain User Operations May 6, 2022

Joshua Reed added 3 commits May 6, 2022 16:13

Bump CloudStack Go API version.

41c4b76

Can switch users now.

5bb23ef

Switch users by default now.

9aa26dc

maxdrib reviewed May 9, 2022

View reviewed changes

wongni suggested changes May 9, 2022

View reviewed changes

Joshua Reed added 2 commits May 9, 2022 11:47

Fixup deletion and ag owner refs.

9ec6d9c

Small lint fixup.

4807b42

aws-paris-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 9, 2022

rejoshed commented May 9, 2022

View reviewed changes

Joshua Reed added 2 commits May 9, 2022 12:11

Addressed PR comment concerns.

71d594c

Merge branch 'main' into feature/subdomain-user-operations

51b0661

rejoshed marked this pull request as ready for review May 9, 2022 19:28

aws-paris-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 9, 2022

aws-paris-bot requested review from pmotyka and vignesh-goutham May 9, 2022 19:28

rejoshed requested review from maxdrib and wongni May 9, 2022 19:35

maxdrib reviewed May 9, 2022

View reviewed changes

wongni approved these changes May 9, 2022

View reviewed changes

maxdrib approved these changes May 9, 2022

View reviewed changes

aws-paris-bot assigned maxdrib May 9, 2022

aws-paris-bot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 9, 2022

rejoshed added /lgtm and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels May 9, 2022

kubernetes-sigs deleted a comment from aws-paris-bot May 9, 2022

rejoshed merged commit 382c908 into main May 9, 2022

rejoshed deleted the feature/subdomain-user-operations branch May 9, 2022 20:06

rejoshed mentioned this pull request Jun 8, 2022

REQUEST: New membership for @rejoshed kubernetes/org#3490

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Subdomain User Operations #76

Subdomain User Operations #76

rejoshed commented May 6, 2022 •

edited

Loading

aws-paris-bot commented May 6, 2022

maxdrib left a comment

maxdrib May 9, 2022

rejoshed May 9, 2022

rejoshed May 9, 2022

wongni May 9, 2022

rejoshed May 9, 2022

wongni May 9, 2022

rejoshed May 9, 2022

wongni May 9, 2022

rejoshed May 9, 2022

wongni May 9, 2022

rejoshed May 9, 2022

rejoshed May 9, 2022

rejoshed May 9, 2022

rejoshed May 9, 2022

rejoshed May 9, 2022

rejoshed May 9, 2022

rejoshed commented May 9, 2022 •

edited

Loading

maxdrib commented May 9, 2022

maxdrib May 9, 2022 •

edited

Loading

rejoshed May 9, 2022

maxdrib May 9, 2022

rejoshed May 9, 2022 •

edited

Loading

maxdrib May 9, 2022

rejoshed May 9, 2022

maxdrib commented May 9, 2022

aws-paris-bot commented May 9, 2022

rejoshed commented May 9, 2022

maxdrib commented May 9, 2022

	if client, err := r.CSClient.NewClientFromSpec(cfg); err != nil {
	if r.CSUser, err := r.CSClient.NewClientFromSpec(cfg); err != nil {

Subdomain User Operations #76

Subdomain User Operations #76

Conversation

rejoshed commented May 6, 2022 • edited Loading

aws-paris-bot commented May 6, 2022

maxdrib 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

Choose a reason for hiding this comment

rejoshed commented May 9, 2022 • edited Loading

maxdrib commented May 9, 2022

maxdrib May 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rejoshed May 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxdrib commented May 9, 2022

aws-paris-bot commented May 9, 2022

rejoshed commented May 9, 2022

maxdrib commented May 9, 2022

rejoshed commented May 6, 2022 •

edited

Loading

rejoshed commented May 9, 2022 •

edited

Loading

maxdrib May 9, 2022 •

edited

Loading

rejoshed May 9, 2022 •

edited

Loading