Add satellite features and add support to the user can direct which security groups are added to their workers #4953

Blintmester · 2023-11-27T13:40:22Z

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" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Relates OR Closes #0000

Output from acceptance testing:

$ make testacc TEST=./ibm/service/satellite TESTARGS='-run=TestAccSatelliteLocation_PodAndServiceSubnet'

--- PASS: TestAccSatelliteLocation_PodAndServiceSubnet (1187.34s)
PASS
ok      command-line-arguments  1187.409s

z0za · 2023-12-14T14:50:42Z

go.mod

@@ -237,8 +237,12 @@ replace github.com/dgrijalva/jwt-go v3.2.0+incompatible => github.com/golang-jwt

 replace github.com/portworx/sched-ops v0.0.0-20200831185134-3e8010dc7056 => github.com/portworx/sched-ops v0.20.4-openstorage-rc3 // required by rook v1.7

+replace github.com/IBM-Cloud/container-services-go-sdk v0.0.0-20231106114255-c50117860a3c => github.com/Blintmester/container-services-go-sdk v0.0.0-20231116135145-fae5e6201d4c // for development only


I believe these can go, right?

sure, my mistake, fixed it

dawall1mn · 2023-12-15T14:59:35Z

I see the update to allow security groups on vpc create cluster. It was not obvious this PR also includes support to specify security groups on vpc worker pool create. Is that included?

hkantare · 2023-12-18T02:44:46Z

ibm/service/kubernetes/resource_ibm_container_vpc_cluster.go

@@ -322,6 +322,14 @@ func ResourceIBMContainerVpcCluster() *schema.Resource {
 				RequiredWith:     []string{"kms_instance_id", "crk"},
 			},

+			"cluster_security_groups": {


can we rename this argument security_groups because the resource already indicates its a cluster

sure, I renamed it

hkantare · 2023-12-18T02:45:33Z

ibm/service/satellite/data_source_ibm_satellite_location.go

@@ -138,6 +138,18 @@ func DataSourceIBMSatelliteLocation() *schema.Resource {
 					},
 				},
 			},
+			"service_subnet": {
+				Type:        schema.TypeString,


In datasource these should be computed only remove optional and forceNew.
Same for pod_subnet also

hkantare · 2023-12-18T02:48:47Z

Fix conflicts in go.mod and go.sum files
Update the respective docs files for new argumets added
security_groups were added only for vpc_cluster should we added for worker_pool resource also

Blintmester · 2024-01-03T14:48:15Z

Fix conflicts in go.mod and go.sum files Update the respective docs files for new argumets added security_groups were added only for vpc_cluster should we added for worker_pool resource also

If I can fix everything, go.mod and go.sum files will be fixed.

Currently I try to add security_groups to worker_pool resource, will check in if its done!

Blintmester · 2024-01-05T14:30:51Z

If everything is fine, I'll fix the go.mod conflicts too.

Blintmester · 2024-01-10T13:55:32Z

@hkantare I pushed the docs files too, please can you review it?

z0za · 2024-01-11T09:12:13Z

ibm/flex/structures.go

+	elems := make([]string, 0)
+	for _, elem := range set.List() {
+		elems = append(elems, elem.(string))
+	}
+	return elems


this can be optimised a bit. if you set the capacity of the slice in the make, you can minimise the memory allocations during the appends ( https://stackoverflow.com/questions/57100525/how-does-go-slice-capacity-change-on-append )

ps: you can achieve similar results by setting the length as in FlattenZonesv2

Suggested change

elems := make([]string, 0)

for _, elem := range set.List() {

elems = append(elems, elem.(string))

}

return elems

setList := set.List()

elems := make([]string, 0, len(setList))

for _, elem := range setList {

elems = append(elems, elem.(string))

}

return elems

z0za · 2024-01-11T10:22:01Z

ibm/service/kubernetes/resource_ibm_container_vpc_cluster.go

@@ -322,6 +322,14 @@ func ResourceIBMContainerVpcCluster() *schema.Resource {
 				RequiredWith:     []string{"kms_instance_id", "crk"},
 			},

+			"security_groups": {


it looks like this parameter can only be set at create
you can use DiffSuppressFunc: flex.ApplyOnce to ignore any changes to the parameter

or

it can be marked with forcenew to recreate the cluster if this param is changed. this means the original cluster will be deleted, so this should be handled with extra care

I think we should go with ApplyOnce rather than ForceNew.

At the moment, these values can not be changed in the API. If the ability to change the security groups was added in the future and customers update the security groups outside of TF, TF would replace the cluster.

If we go with ApplyOnce, the changes would be ignored until the TF provider supported that functionality at which point we could provide guidance to customers on importing the updated changes and avoid cluster deletion.

z0za · 2024-01-11T10:23:51Z

ibm/service/kubernetes/resource_ibm_container_worker_pool.go

@@ -42,7 +44,7 @@ func ResourceIBMContainerWorkerPool() *schema.Resource {
 					"cluster"),
 			},

-			"machine_type": {
+			"flavor": {


please don't convert this resource to vpc, there is already a vpc_worker_pool one

z0za · 2024-01-11T10:26:31Z

ibm/service/kubernetes/resource_ibm_container_vpc_cluster.go

+				Elem:        &schema.Schema{Type: schema.TypeString},
+				Set:         flex.ResourceIBMVPCHash,
+			},
+


it looks like it is missing from the API, but...
should we be able to set SecurityGroupIDs to the default workerpool too?

z0za · 2024-01-11T10:28:02Z

website/docs/r/container_worker_pool.html.markdown

+
+  zones {
+      name = "us-south-1"
+      subnet_id = "0716-2389b003-7968-41b1-833c-4d63d7165899"


please remove the real ids from the example. you can add variables instead

z0za · 2024-01-11T10:29:15Z

ibm/service/satellite/resource_ibm_satellite_location.go

+				Optional:    true,
+				ForceNew:    true,
+				Description: "Custom subnet CIDR to provide private IP addresses for services",
+				//Computed:    true,


if it is not needed, please remove it

z0za · 2024-01-11T10:41:02Z

ibm/flex/structures.go

-// flattenHostLabels ..
-func FlattenHostLabels(hostLabels []interface{}) map[string]string {
+// flatten the provided key-value pairs
+func FlattenKeyValues(keyValues []interface{}) map[string]string {


why did you rename this function?

previously it was used only for flatten Host Labels, but I renamed it and we can use it for everything when we should flatten key-values (map[string]string), and I used it too (and it was not host labels)

z0za · 2024-01-11T10:46:38Z

ibm/service/satellite/resource_ibm_satellite_cluster.go

@@ -280,6 +280,12 @@ func ResourceIBMSatelliteCluster() *schema.Resource {
 				Sensitive:   true,
 				Description: "The IBM Cloud Identity and Access Management (IAM) service CRN token for the service that creates the cluster.",
 			},
+			"calico_ip_autodetection": {


is it only for create too? maybe ApplyOnce or forcenew?

yes, it is only for create, so I'll add ApplyOnce

z0za · 2024-01-12T14:01:39Z

ibm/service/kubernetes/resource_ibm_container_vpc_worker_pool.go

@@ -212,6 +212,14 @@ func ResourceIBMContainerVpcWorkerPool() *schema.Resource {
 				Computed:    true,
 				Description: "Autoscaling is enabled on the workerpool",
 			},
+
+			"security_groups": {


can you please add the ApplyOnce diff suppress here too?

sure, will do

z0za · 2024-01-12T14:01:54Z

ibm/service/kubernetes/resource_ibm_container_vpc_cluster_test.go

+	resource.Test(t, resource.TestCase{
+		PreCheck:  func() { acc.TestAccPreCheck(t) },
+		Providers: acc.TestAccProviders,
+		// CheckDestroy: testAccCheckIBMContainerVpcClusterDestroy,


why is it commented out?

the CheckDestroy function fails on every run, because it doesn't wait enough, or something else, I don't really know, but the resource (after a failed run) was destroyed successfully

z0za · 2024-01-12T14:02:20Z

ibm/service/kubernetes/resource_ibm_container_vpc_worker_pool.go

@@ -212,6 +212,14 @@ func ResourceIBMContainerVpcWorkerPool() *schema.Resource {
 				Computed:    true,
 				Description: "Autoscaling is enabled on the workerpool",
 			},
+
+			"security_groups": {


can you please add a test for this too?

z0za · 2024-01-12T14:15:45Z

website/docs/r/satellite_location.html.markdown

+  zones             = var.location_zones
+  managed_from      = var.managed_from
+  resource_group_id = data.ibm_resource_group.group.id
+  pod_subnet = var.pod_subnet // "10.42.0.0/16"


can you please fix the formatting in the example?

…ecurity groups are added to their workers

Blintmester marked this pull request as ready for review December 14, 2023 14:20

Blintmester force-pushed the satellite-features branch 2 times, most recently from dcae8af to 6ebdc51 Compare December 14, 2023 14:48

z0za reviewed Dec 14, 2023

View reviewed changes

Blintmester force-pushed the satellite-features branch 2 times, most recently from cfc7b9e to b0beebd Compare December 15, 2023 07:23

hkantare reviewed Dec 18, 2023

View reviewed changes

Blintmester force-pushed the satellite-features branch from b566455 to be48bc7 Compare January 3, 2024 16:15

Blintmester force-pushed the satellite-features branch 3 times, most recently from 936af91 to 1c72212 Compare January 10, 2024 13:54

Blintmester force-pushed the satellite-features branch 2 times, most recently from db74ef3 to e007250 Compare January 10, 2024 14:08

z0za suggested changes Jan 11, 2024

View reviewed changes

Blintmester force-pushed the satellite-features branch 2 times, most recently from 3798d04 to 2348b4c Compare January 12, 2024 14:13

z0za suggested changes Jan 12, 2024

View reviewed changes

Blintmester force-pushed the satellite-features branch 2 times, most recently from 7a9e876 to 5a1368e Compare January 15, 2024 09:58

Add satellite features and add support to the user can direct which s…

da3a879

…ecurity groups are added to their workers

Blintmester force-pushed the satellite-features branch from 5a1368e to da3a879 Compare January 15, 2024 11:55

z0za approved these changes Jan 15, 2024

View reviewed changes

lewiseevans approved these changes Jan 15, 2024

View reviewed changes

Blintmester requested a review from hkantare January 15, 2024 16:14

hkantare approved these changes Jan 16, 2024

View reviewed changes

hkantare merged commit 7fc2c5a into IBM-Cloud:master Jan 16, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add satellite features and add support to the user can direct which security groups are added to their workers #4953

Add satellite features and add support to the user can direct which security groups are added to their workers #4953

Blintmester commented Nov 27, 2023 •

edited

Loading

z0za Dec 14, 2023

Blintmester Dec 14, 2023

dawall1mn commented Dec 15, 2023

hkantare Dec 18, 2023

Blintmester Jan 3, 2024

hkantare Dec 18, 2023

Blintmester Jan 3, 2024

hkantare commented Dec 18, 2023 •

edited

Loading

Blintmester commented Jan 3, 2024

Blintmester commented Jan 5, 2024

Blintmester commented Jan 10, 2024

z0za Jan 11, 2024

z0za Jan 11, 2024

lewiseevans Jan 11, 2024

z0za Jan 11, 2024

z0za Jan 11, 2024

z0za Jan 11, 2024

z0za Jan 11, 2024

z0za Jan 11, 2024

Blintmester Jan 11, 2024

z0za Jan 11, 2024

Blintmester Jan 11, 2024

z0za Jan 12, 2024

Blintmester Jan 12, 2024

z0za Jan 12, 2024

Blintmester Jan 12, 2024

z0za Jan 12, 2024

z0za Jan 12, 2024

		@@ -237,8 +237,12 @@ replace github.com/dgrijalva/jwt-go v3.2.0+incompatible => github.com/golang-jwt

		replace github.com/portworx/sched-ops v0.0.0-20200831185134-3e8010dc7056 => github.com/portworx/sched-ops v0.20.4-openstorage-rc3 // required by rook v1.7

		replace github.com/IBM-Cloud/container-services-go-sdk v0.0.0-20231106114255-c50117860a3c => github.com/Blintmester/container-services-go-sdk v0.0.0-20231116135145-fae5e6201d4c // for development only

Add satellite features and add support to the user can direct which security groups are added to their workers #4953

Add satellite features and add support to the user can direct which security groups are added to their workers #4953

Conversation

Blintmester commented Nov 27, 2023 • edited Loading

Community Note

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dawall1mn commented Dec 15, 2023

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

hkantare commented Dec 18, 2023 • edited Loading

Blintmester commented Jan 3, 2024

Blintmester commented Jan 5, 2024

Blintmester commented Jan 10, 2024

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

Blintmester commented Nov 27, 2023 •

edited

Loading

hkantare commented Dec 18, 2023 •

edited

Loading