[WIP] Sagemaker Notebook DirectInternetAccess support

[bcatubig]

Enhancement #2999

Changes proposed in this pull request:

• Allow user to disable internet access to notebook

Community Note

• Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
• Please do not leave "+1" comments, they generate extra noise for pull request followers and do not help prioritize the request

Output from acceptance testing:

Currently dealing with destroy issues...

make testacc TESTARGS='-run=TestAccAWSSagemakerNotebookInstance_directInternetAccess'

==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -parallel 20 -run=TestAccAWSSagemakerNotebookInstance_directInternetAccess -timeout 120m
=== RUN   TestAccAWSSagemakerNotebookInstance_directInternetAccess
=== PAUSE TestAccAWSSagemakerNotebookInstance_directInternetAccess
=== CONT  TestAccAWSSagemakerNotebookInstance_directInternetAccess
--- FAIL: TestAccAWSSagemakerNotebookInstance_directInternetAccess (225.45s)
    testing.go:599: Error destroying resource! WARNING: Dangling resources
        may exist. The full state and error is shown below.
        
        Error: Error applying: 1 error occurred:
                * aws_sagemaker_notebook_instance.foo (destroy): 1 error occurred:
                * aws_sagemaker_notebook_instance.foo: error waiting for sagemaker notebook instance (terraform-testacc-20190311022405872400000001) to stop: RequestError: send request failed
        caused by: Post https://api.sagemaker.us-east-1.amazonaws.com/: read tcp 192.168.1.201:57866->34.201.234.253:443: read: connection reset by peer

[bflad]

Hi @bcatubig 👋 This is looking pretty good. Is there anything else that needs to happen here? Acceptance testing seems to be passing just fine for me, e.g.

--- PASS: TestAccAWSSagemakerNotebookInstance_basic (220.51s)
--- PASS: TestAccAWSSagemakerNotebookInstance_directInternetAccess (295.28s)

[bflad]

Nit: To prevent confusion for anyone running the acceptance testing in their own accounts it might be good to make this Name tags more descriptive for the test configurations in case the testing leaves dangling resources, e.g.

Suggested change

	Name = "Main"
	Name = "tf-acc-test-sagemaker-notebook-instance-direct-internet-access"

[bcatubig]

Thanks @bflad -- will make requested changes

[bcatubig]

Hey @bflad

Feedback implemented

[bflad]

This is almost there, @bcatubig! Two little fixes and should be ready to go. 😄

[bflad]

It looks like this acceptance test is failing while the others pass:

--- FAIL: TestAccAWSSagemakerNotebookInstance_directInternetAccess_enabled (255.12s)
    testing.go:538: Step 0 error: Check failed: Check 3/5 error: DirectInternetAccess not configured correctly: want Enabled got Disabled
--- PASS: TestAccAWSSagemakerNotebookInstance_basic (259.02s)
--- PASS: TestAccAWSSagemakerNotebookInstance_directInternetAccess_disabled (292.10s)
--- PASS: TestAccAWSSagemakerNotebookInstance_tags (296.42s)
--- PASS: TestAccAWSSagemakerNotebookInstance_disappears (314.73s)
--- PASS: TestAccAWSSagemakerNotebookInstance_update (443.07s)

Did you mean to point it to the same direct_internet_access = "Disabled" configuration? The easiest fix might be to parameterize that argument for the configuration:

func testAccAWSSagemakerNotebookInstanceConfigDirectInternetAccess(notebookName, directInternetAccess string) string {
	return fmt.Sprintf(`
resource "aws_vpc" "main" {
  cidr_block = "10.0.0.0/16"
}
resource "aws_subnet" "main" {
  vpc_id     = "${aws_vpc.main.id}"
  cidr_block = "10.0.1.0/24"
  tags = {
    Name = "tf-acc-test-sagemaker-notebook-instance-direct-internet-access"
  }
}
resource "aws_security_group" "foo" {
  name        = "foo"
  vpc_id      = "${aws_vpc.main.id}"
  description = "foo bar baz"
  tags = {
    Name = "foo_sg"
  }
}
resource "aws_sagemaker_notebook_instance" "foo" {
  name                   = %[1]q
  role_arn               = "${aws_iam_role.foo.arn}"
  instance_type          = "ml.t2.medium"
  subnet_id              = "${aws_subnet.main.id}"
  security_groups        = ["${aws_security_group.foo.id}"]
  direct_internet_access = %[2]q
}
resource "aws_iam_role" "foo" {
  name               = %[1]q
  path               = "/"
  assume_role_policy = "${data.aws_iam_policy_document.assume_role.json}"
}
data "aws_iam_policy_document" "assume_role" {
  statement {
    actions = ["sts:AssumeRole"]
    principals {
      type        = "Service"
      identifiers = ["sagemaker.amazonaws.com"]
    }
  }
}
`, notebookName, directInternetAccess)
}

Then updating the two Config calls:

// in TestAccAWSSagemakerNotebookInstance_directInternetAccess_disabled
Config: testAccAWSSagemakerNotebookInstanceConfigDirectInternetAccess(notebookName, "Disabled"),

// in TestAccAWSSagemakerNotebookInstance_directInternetAccess_enabled
Config: testAccAWSSagemakerNotebookInstanceConfigDirectInternetAccess(notebookName, "Enabled"),

[bcatubig]

I'll refactor this

[bflad]

We should mention this supports Enabled as well (for Terraform modules, etc.)

Suggested change

	* `direct_internet_access` - (Optional) Set to `Disabled` to disable internet access to notebook. Requires `subnet_id` to be set.
	* `direct_internet_access` - (Optional) Set to `Disabled` to disable internet access to notebook. Requires `subnet_id` to be set. Valid values: `Disabled`, `Enabled`.

[bcatubig]

Hey @bflad, is there specific format for parameters that support multiple values?

Ex:

• foo - (Optional). Foo does a thing. Supported values: Enabled or Disabled

[bflad]

I lean towards Valid values: one, two but I'm not sure if there is a specific "format". Its more important they are listed in some way.

[bcatubig]

Totally nuked this by accident. Will fix tonight when I have access to my other computer.

[rainmanh]

What is the status for this @bcatubig ?

[chrispruitt]

@bcatubig is the direct_internet_access option available in any specific version yet? many thanks

[bcatubig]

Hello, I still need to fix this pr. Will get it sorted out as there are only a few items left. Thanks

…

On Thu, May 9, 2019 at 12:22 PM, chrispruitt ***@***.***> wrote: ***@***.***(https://github.com/bcatubig) is the direct_internet_access option available in any specific version yet? many thanks — You are receiving this because you were mentioned. Reply to this email directly, [view it on GitHub](#7884 (comment)), or [mute the thread](https://github.com/notifications/unsubscribe-auth/AAKR3YBLOK4KWMYXYQG4BXLPURMW5ANCNFSM4G45YS6Q).

[sworisbreathing]

hi @bcatubig - has there been any movement on fixing up this PR? I'm pretty keen on seeing this config option exposed through terraform.

[bcatubig]

Hey @sworisbreathing -- this has been moved to #8618

[ghost]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!