[WIP] Add container cluster network policy addon #630

sebglon · 2017-10-25T14:51:35Z

sebglon · 2017-10-25T14:57:00Z

Actualy i open a case on google because with api, on create cluster param networkPolicyConfig disabled=false is not set on created cluster.
Case: 14016662

danawillow · 2017-11-01T23:17:36Z

Hey @sebglon, I see the WIP in the PR description. Do you have anything you're looking to get feedback on with things as they are now?

sebglon · 2017-11-02T07:55:29Z

Actualy, when i set disable NetworkPolicy tu fase, the network policy is not enabled on create.
For that i have open a case on google to understand why.

wmuizelaar · 2017-11-15T16:25:20Z

I just tried to use this, and when I looked at the webinterface of GKE, you it looked like you first need to enable it on the master-node, and then as well on the nodes. So probably there are two places in the API to look for?

sebglon · 2017-11-20T11:03:29Z

After many exchange with google team, i have changed network_policy on addons by the global network_policy.
Now the code only support global network_policy.
If the value change, it recreate the cluster.

sebglon · 2017-11-20T11:10:41Z

After many exchange with google team, i have changed network_policy on addons by the global network_policy.
Now the code only support global network_policy.
If the value change, it recreate the cluster.

@danawillow all is good for me. can you review it?

danawillow · 2017-11-20T23:54:48Z

google/resource_container_cluster.go

+							Type:     schema.TypeBool,
+							ForceNew: true,
+							Optional: true,
+							Default:  true,


Is this actually enabled by default?

danawillow · 2017-11-20T23:55:24Z

google/resource_container_cluster.go

+				Type:     schema.TypeList,
+				Optional: true,
+				Computed: true,
+				ForceNew: true,


Since you added update functionality, these doesn't need to be ForceNew

danawillow · 2017-11-21T00:00:16Z

google/resource_container_cluster.go

+
+func flattenNetworkPolicy(c *container.NetworkPolicy) []map[string]interface{} {
+	if c == nil {
+		c = &container.NetworkPolicy{}


I don't like reassigning pointers this way. Instead, how about something like:

result := []map[string]interface{}{} if c != nil { result = append(result, map[string]interface{}{ "enabled": c.Enabled, "provider": c.Provider, }) } return result

danawillow · 2017-11-21T00:01:50Z

google/resource_container_cluster_test.go

+	}`, acctest.RandString(10))
+
+var testAccContainerCluster_updateNetworkPolicyEnabled = fmt.Sprintf(`
+		resource "google_container_cluster" "with_network_policy_enabled" {


nit: can you indent this block one more to the left to match the previous var?

danawillow · 2017-11-21T00:02:30Z

google/resource_container_cluster_test.go

+
+var testAccContainerCluster_updateNetworkPolicyEnabled = fmt.Sprintf(`
+		resource "google_container_cluster" "with_network_policy_enabled" {
+			name = "cluster-test-%s"


Since this config and the previous one both use random strings as part of the cluster name, this will destroy+recreate the cluster so it's not actually testing update.

danawillow · 2017-11-21T00:03:31Z

google/resource_container_cluster_test.go

+			zone = "us-central1-a"
+			initial_node_count = 1
+
+			// remove network_policy is equal than enabled=false


Instead of this comment, what about renaming the test var to testAccContainerCluster_removeNetworkPolicy? Right now it's a bit hard to understand.

sebglon · 2017-11-21T15:03:13Z

@danawillow thanks for your review. All changes are made

danawillow · 2017-11-21T20:50:14Z

google/resource_container_cluster.go

+					Schema: map[string]*schema.Schema{
+						"enabled": {
+							Type:     schema.TypeBool,
+							ForceNew: true,


I'm confused why you've decided to allow adding/removing the network policy but not changing the values within it. Was that intentional? If so, can you add a comment explaining why?

sorry @danawillow i don't understand exactly what do you say about 'changing the value".
if you do not put network_policy block, it's the same than disable it.
if you put network_policy block, you can choice to enable or diable it.

danawillow · 2017-11-21T20:50:27Z

google/resource_container_cluster_test.go

@@ -911,7 +949,7 @@ resource "google_container_cluster" "primary" {

 var testAccContainerCluster_withMasterAuth = fmt.Sprintf(`
 resource "google_container_cluster" "with_master_auth" {
-	name = "cluster-test-%s"
+	name = "%s"


I think you changed the wrong test here

sebglon · 2017-11-24T21:53:17Z

@danawillow Can i have a feedback

danawillow · 2017-11-27T20:39:54Z

Just added a few small things, but looks good overall, thanks!

* replalce TypeList by TypeSet * Add network policy * test improvement * correct test * Add cluster network polocy enabled * Replalce network_policy addons by global network_policy enabled * Update node_config.go * Update resource_container_cluster.go * clean * clean * Correct PR * COrrect PR * pr * fix test to use same name * add more documentation

Signed-off-by: Modular Magician <[email protected]>

ghost · 2020-03-30T14:11:45Z

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. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!

sebglon changed the title ~~Add container cluster network policy addon~~ [WIP] Add container cluster network policy addon Oct 25, 2017

sebglon force-pushed the networkpolicy branch 2 times, most recently from 8f07134 to 75d8672 Compare October 26, 2017 08:38

danawillow self-requested a review October 26, 2017 22:13

sebglon force-pushed the networkpolicy branch from 75d8672 to 666ccf8 Compare November 2, 2017 07:29

Sébastien GLON added 3 commits November 2, 2017 21:59

replalce TypeList by TypeSet

d1ccbb8

Add network policy

f0e705f

test improvement

1fb2e80

sebglon force-pushed the networkpolicy branch from 1b0c52f to 1fb2e80 Compare November 2, 2017 21:02

paddycarver added upstream waiting-response labels Nov 7, 2017

correct test

55492f0

Sébastien GLON added 2 commits November 16, 2017 23:00

Add cluster network polocy enabled

63e38be

Replalce network_policy addons by global network_policy enabled

22758f7

Sébastien GLON added 4 commits November 20, 2017 12:03

Update node_config.go

ca4e019

Update resource_container_cluster.go

fa8eb30

clean

730005b

clean

ec664d4

danawillow removed the waiting-response label Nov 20, 2017

danawillow suggested changes Nov 21, 2017

View reviewed changes

Sébastien GLON added 2 commits November 21, 2017 16:02

Correct PR

89e014a

COrrect PR

571dd8d

danawillow reviewed Nov 21, 2017

View reviewed changes

pr

93e7e07

danawillow added 2 commits November 27, 2017 12:33

fix test to use same name

b0c787e

add more documentation

de10560

danawillow approved these changes Nov 27, 2017

View reviewed changes

danawillow merged commit 39f22ef into hashicorp:master Nov 27, 2017

sebglon deleted the networkpolicy branch November 27, 2017 21:56

paddycarver mentioned this pull request Mar 1, 2018

Network Policy addon for container cluster is not implemented #583

Closed

luis-silva pushed a commit to luis-silva/terraform-provider-google that referenced this pull request May 21, 2019

Add support for network endpoint groups (hashicorp#630)

71a7aa6

Signed-off-by: Modular Magician <[email protected]>

ghost locked and limited conversation to collaborators Mar 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Add container cluster network policy addon #630

[WIP] Add container cluster network policy addon #630

sebglon commented Oct 25, 2017

sebglon commented Oct 25, 2017 •

edited

Loading

danawillow commented Nov 1, 2017

sebglon commented Nov 2, 2017

wmuizelaar commented Nov 15, 2017

sebglon commented Nov 20, 2017

sebglon commented Nov 20, 2017

danawillow Nov 20, 2017

danawillow Nov 20, 2017

danawillow Nov 21, 2017

danawillow Nov 21, 2017

danawillow Nov 21, 2017

danawillow Nov 21, 2017

sebglon commented Nov 21, 2017 •

edited

Loading

danawillow Nov 21, 2017

sebglon Nov 22, 2017

danawillow Nov 21, 2017

sebglon commented Nov 24, 2017

danawillow commented Nov 27, 2017

ghost commented Mar 30, 2020

[WIP] Add container cluster network policy addon #630

[WIP] Add container cluster network policy addon #630

Conversation

sebglon commented Oct 25, 2017

sebglon commented Oct 25, 2017 • edited Loading

danawillow commented Nov 1, 2017

sebglon commented Nov 2, 2017

wmuizelaar commented Nov 15, 2017

sebglon commented Nov 20, 2017

sebglon commented Nov 20, 2017

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

sebglon commented Nov 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebglon commented Nov 24, 2017

danawillow commented Nov 27, 2017

ghost commented Mar 30, 2020

sebglon commented Oct 25, 2017 •

edited

Loading

sebglon commented Nov 21, 2017 •

edited

Loading