instances: Patch CNS processing during instance creation #32

jwreagor · 2017-07-28T19:45:48Z

CNS was not respected when creating a new instance. This addresses that concern, adds a thorough acceptance test for instance creation, and includes an example for others.

jen20

One minor nit, but other than that this looks correct to me. 👍 for the test.

jen20 · 2017-07-28T19:57:39Z

compute/instances.go

@@ -963,8 +974,7 @@ func (api *_Instance) toNative() (*Instance, error) {
 // submit an API call to the vmapi machine endpoint
 func (mcns *InstanceCNS) toTags(m map[string]interface{}) {
 	if mcns.Disable != nil {
-		s := fmt.Sprintf("%t", mcns.Disable)
-		m[CNSTagDisable] = &s
+		m[CNSTagDisable] = fmt.Sprintf("%t", *mcns.Disable)


This should likely use strconv.FormatBool from the standard library instead of fmt.Sprintf

Fair enough, I think I'm going to remove the pointers here as well. I'm not sure why they're used by the InstanceCNS struct values.

jwreagor · 2017-07-28T20:21:55Z

Actually, looks like there are some changes for supporting CNS updates through the ReplaceTags endpoint. I'll include them here as well.

jwreagor · 2017-07-29T00:15:30Z

compute/instances.go

-	Disable    *bool
-	ReversePTR *string
+	Disable    bool
+	ReversePTR string


We should be fine changing these semantics considering this struct was renamed under the new API.

jwreagor · 2017-07-29T00:38:20Z

Nevermind... found another last minute change.

jwreagor · 2017-07-29T00:49:32Z

compute/instances.go

-		m[CNSTagDisable] = &s
+func (cns *InstanceCNS) toTags(m map[string]interface{}) {
+	if cns.Disable {
+		m[CNSTagDisable] = cns.Disable


Just confirmed that this actually needs to be a boolean and posted as such through the tags API (as JSON).

A bool or a "string masquerading as a bool" ? Regardless, this may be worth calling out as a comment in the code:

// NOTE(justinwr@): The API requires the CNSTagDisable attribute to be a boolean, not a bool string.

That comment probably needs to chase into a Validator() function, actually.

A bool bool since the JSON encoder will keep it a boolean. Doesn't look like we had any other examples before this.

I'll add the comment here as well.

jwreagor · 2017-07-29T02:43:17Z

Ok, this is ready for review again.

sean-

A few misc items and comments.

sean- · 2017-07-30T18:55:00Z

compute/instances.go

 	input.CNS.toTags(result)
+	for key, value := range result {
+		if key == CNSTagServices {
+			result[fmt.Sprintf("tag.%s", key)] = value


This is actually one of those cases where I'd skip fmt.Sprintf() and would just use + to concatenate the two strings.

That said, can you sanitize the input for key to make sure that it conforms to what we're expecting?

I can definitely clean this up a bit and rely more on CNSTagServices. That is the only tag we need to prepend with tag. here, anything else slips through.

sean- · 2017-07-30T18:57:01Z

compute/instances.go

+}
+
+// toAPI() merges both Tags and CNS tags into the same map of strings used as
+// the body of our ReplaceTags request.


Append something like:

Used to join Tags and CNS tags into the same JSON object before sending an API request to the API gateway.

So that in the future we know we should be able to remove this method if/when CNS and Tags become first-class API citizens with their own API endpoints.

sean- · 2017-07-30T18:57:25Z

compute/instances.go

+func (input ReplaceTagsInput) toAPI() map[string]interface{} {
+	result := map[string]interface{}{}
+	for key, value := range input.Tags {
+		result[key] = value


These values have been sanitized at this point, correct?

I think we're alright since we're liberal with what can be a tag and we're also relying on the JSON encoder to marshal types going into CloudAPI. But I can see how the use of interface{} would normally allow other types through in cases like this.

sean- · 2017-07-30T19:00:17Z

compute/instances.go

-		m[CNSTagDisable] = &s
+func (cns *InstanceCNS) toTags(m map[string]interface{}) {
+	if cns.Disable {
+		m[CNSTagDisable] = cns.Disable


A bool or a "string masquerading as a bool" ? Regardless, this may be worth calling out as a comment in the code:

// NOTE(justinwr@): The API requires the CNSTagDisable attribute to be a boolean, not a bool string.

That comment probably needs to chase into a Validator() function, actually.

sean- · 2017-07-30T19:01:16Z

compute/instances.go

-	if len(mcns.Services) > 0 {
-		m[CNSTagServices] = strings.Join(mcns.Services, ",")
+	if len(cns.Services) > 0 {
+		m[CNSTagServices] = strings.Join(cns.Services, ",")


In order to get a stable comparison, in both the *Read call and here, do we need to sort cns.Services? I don't know but seem to recall there wasn't a sorting guarantee here (I also don't think we should depend on it if there is one currently).

And we specifically spoke about this when I was in town too... I haven't seen a single issue but I'll test more variations.

Correct me if I'm wrong but we'd want to handle this on the Terraform/consumer side (here and here)?

I'd say this is something Terraform should handle rather than the library since that is where the sorted version is required.

sean- · 2017-07-30T19:03:17Z

compute/instances_test.go

@@ -30,6 +35,202 @@ func getAnyInstanceID(t *testing.T, client *compute.ComputeClient) (string, erro
 	return "", errors.New("no machines configured")
 }

+func RandInt() int {
+	reseed()
+	return rand.New(rand.NewSource(time.Now().UnixNano())).Int()


I'm going to stump the seeding library here, either for explicit use or to crib an example from: https://github.com/sean-/seed

Given we actually need cryptographically secure random number generation in the authentication package (ref https://github.com/joyent/triton-go/blob/5f7872646922b73632f434afa5e45fcd18aa9d5b/authentication/private_key_signer.go#L77) which is imported into the client, which is in turn imported everywhere, maybe we should put any seeding there?

👍 I agree and I'll fit this into a secondary PR.

Ref: #33 ✔️

sean- · 2017-07-30T19:05:04Z

examples/compute/create_instance.go

+
+func main() {
+	keyID := os.Getenv("SDC_KEY_ID")
+	accountName := os.Getenv("SDC_ACCOUNT")


It may be worth using os.LookupEnv here instead. This is just an example, but these values can't be empty strings without blowing up the downstream callers.

sean- · 2017-07-30T19:05:42Z

examples/compute/create_instance.go

+	createInput := &compute.CreateInstanceInput{
+		Name:     "go-test1",
+		Package:  "g4-highcpu-512M",
+		Image:    "1f32508c-e6e9-11e6-bc05-8fea9e979940",


What datacenter is this instance out of? It may be worth noting the image ID is tied to a DC atm.

This was sloppy. I had written out the example for someone else and refactored into the test. I just need to pull the image ID dynamically as I did there.

sean- · 2017-07-30T19:10:18Z

examples/compute/create_instance.go

+
+	select {
+	case instance := <-state:
+		fmt.Println("\nDuration:", time.Since(startTime).String())


fmt.Printf("\nDuration: %s\n", time.Since(startTime))

sean- · 2017-07-30T19:11:36Z

examples/compute/create_instance.go

+		log.Fatalf("Create(): %v", err)
+	}
+
+	// Wait for provisioning to complete...


This should probably be a helper function that we write and expose to consumers because most API consumers are going to want a synchronous API and semantics (including Terraform), not async.

Yes, I agree. I'll push a PR separately for that so we can define the API.

Seems reasonable. We likely want to define a uniform way of dealing with this for as many resources as possible in order to reduce the amount of API internals which need to be understood to consume this.

Ref: #34 ✔️

jwreagor · 2017-07-31T18:36:13Z

Think we're good to merge here.

jen20 · 2017-07-31T18:53:03Z

Looks good to me!

instances: Patch CNS processing during instance creation

c662e5d

jen20 approved these changes Jul 28, 2017

View reviewed changes

jwreagor commented Jul 29, 2017

View reviewed changes

jwreagor requested a review from jen20 July 29, 2017 00:19

jwreagor removed the request for review from jen20 July 29, 2017 00:38

instances: Refactor and simplify ReplaceTags and InstanceCNS

a792af0

jwreagor force-pushed the f-create-cns branch from 72e14b0 to a792af0 Compare July 29, 2017 00:48

jwreagor commented Jul 29, 2017

View reviewed changes

jwreagor requested a review from sean- July 29, 2017 02:43

sean- reviewed Jul 30, 2017

View reviewed changes

instances: Refactor CNS input, adjust comments and create example

2759844

jwreagor mentioned this pull request Jul 31, 2017

Introduce Wait function that blocks on backend API actions #34

Open

jwreagor merged commit e48f42a into TritonDataCenter:master Jul 31, 2017

instances: Patch CNS processing during instance creation #32

instances: Patch CNS processing during instance creation #32

Conversation

jwreagor commented Jul 28, 2017

jen20 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwreagor commented Jul 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwreagor commented Jul 29, 2017

jwreagor Jul 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwreagor commented Jul 29, 2017

sean- 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

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

jwreagor commented Jul 31, 2017

jen20 commented Jul 31, 2017

jwreagor Jul 29, 2017 •

edited

Loading