`kubernetes_cluster` -`gmsa` #16437

qiqingzhang · 2022-04-19T04:44:50Z

create a public pull request for `k8s_cluster` supports `gmsa`

test result:

stephybun

Thanks for this PR @heiazuo.

In the AKS resource as well as throughout the provider we typically don't expose an enabled field for blocks, since the behaviour should be if block is present -> enabled, if block is absent -> disabled.

Since both the DNS server and root domain name can be left empty/omitted I think these might be better as their own individual properties
gmsa_enabled
gmsa_dns_server
gmsa_root_domain_name

stephybun · 2022-05-12T10:11:02Z

internal/services/containers/kubernetes_cluster_resource.go

+							Optional: true,
+							Elem: &pluginsdk.Resource{
+								Schema: map[string]*pluginsdk.Schema{
+									"enabled": {


Can we not expose this field for toggling GMSA, as with the other blocks in the AKS resource and across most of the provider the behaviour should be if block is present -> enabled, if block is absent -> disabled.

stephybun · 2022-05-12T10:11:49Z

internal/services/containers/kubernetes_cluster_resource.go

@@ -852,6 +852,28 @@ func resourceKubernetesCluster() *pluginsdk.Resource {
 								string(containerservice.LicenseTypeWindowsServer),
 							}, false),
 						},
+						"gmsa_profile": {


_profile doesn't add any informational value, so i'd suggest removing it.

Suggested change

"gmsa_profile": {

"gmsa": {

qiqingzhang · 2022-05-16T08:03:16Z

Hi @stephybun , would you please take another look? Thanks!

ms-henglu · 2022-05-30T02:52:20Z

Hi @stephybun , would you please take a look at this PR? Thanks!

stephybun · 2022-05-30T09:43:47Z

internal/services/containers/kubernetes_cluster_resource.go

+						"gmsa": {
+							Type:     pluginsdk.TypeList,
+							Optional: true,
+							Elem: &pluginsdk.Resource{
+								Schema: map[string]*pluginsdk.Schema{
+									"gmsa_dns_server": {
+										Type:         pluginsdk.TypeString,
+										Optional:     true,
+										ValidateFunc: validation.StringIsNotEmpty,
+									},
+									"gmsa_root_domain_name": {
+										Type:         pluginsdk.TypeString,
+										Optional:     true,
+										ValidateFunc: validation.StringIsNotEmpty,
+									},


If both gmsa_dns_server and gmsa_root_domain_name are optional, then if a user wants to enable gmsa without specifying either of those properties they would have to add an empty block to their config like below

gmsa {}

Is this a valid use case? If so then I think we should flatten this block and create three separate properties like was suggested initially. This would be more consistent with the resource and the provider.

qiqingzhang · 2022-06-01T02:23:59Z

Hi @stephybun, I modified it according to the suggestion, please take a look again.

stephybun · 2022-06-02T08:13:50Z

internal/services/containers/kubernetes_cluster_resource.go

+						"gmsa_dns_server": {
+							Type:         pluginsdk.TypeString,
+							Optional:     true,
+							ValidateFunc: validation.StringIsNotEmpty,
+						},
+						"gmsa_root_domain_name": {
+							Type:         pluginsdk.TypeString,
+							Optional:     true,
+							ValidateFunc: validation.StringIsNotEmpty,
+						},


Given the changes last made I'm assuming even though gmsa can be enabled without specifying a dns server and root domain it isn't a functional configuration. In that case we can use a block, but should specify AtLeastOneOf so at least one or the other property must be specified for the block.

Suggested change

"gmsa_dns_server": {

Type: pluginsdk.TypeString,

Optional: true,

ValidateFunc: validation.StringIsNotEmpty,

},

"gmsa_root_domain_name": {

Type: pluginsdk.TypeString,

Optional: true,

ValidateFunc: validation.StringIsNotEmpty,

},

"gmsa": {

Type: pluginsdk.TypeList,

Optional: true,

MaxItems: 1,

Elem: &pluginsdk.Resource{

Schema: map[string]*pluginsdk.Schema{

"dns_server": {

Type: pluginsdk.TypeString,

Optional: true,

ValidateFunc: validation.StringIsNotEmpty,

AtLeastOneOf: []string{"windows_profile.0.gmsa.0.dns_server"},

},

"root_domain": {

Type: pluginsdk.TypeString,

Optional: true,

ValidateFunc: validation.StringIsNotEmpty,

AtLeastOneOf: []string{"windows_profile.0.gmsa.0.root_domain"},

},

},

},

},

qiqingzhang · 2022-06-02T09:12:56Z

Hi @stephybun, changes have been made as suggested, would you please take a look at this PR? Thanks!

…OneOf`

mrowken · 2022-08-10T11:15:08Z

Is it gonna to be finished soon ? I'm waiting for this to be able rollout Windows Nodes. Any ETA will be appreciated.

stephybun

Hi @qiqingzhang, sorry for the delayed re-review here.

While playing around with this I noticed that once gmsa is enabled for a cluster it isn't possible to disable it. We should add some logic to CustomizeDiff that will ForceNew the resource if a user tries to remove the gmsa block.

Once that's done we can take another look through.

Thanks!

ms-henglu · 2022-08-29T07:48:57Z

Hi @stephybun ,

I've added a commit to fix it, please take another look, thanks!

stephybun · 2022-08-30T06:41:22Z

website/docs/r/kubernetes_cluster.html.markdown

+
+A `gmsa` block supports the following:
+
+* `dns_server` - (Optional) Specifies the DNS server for Windows gMSA. Set it to empty if you have configured the DNS server in the vnet which is used to create the managed cluster.


Can you please confirm with the service team whether setting dns_server and root_domain to empty is a valid configuration for the cluster? This isn't mentioned in the documentation anywhere?

stephybun · 2022-08-30T06:43:27Z

internal/services/containers/kubernetes_cluster_resource.go

+									"dns_server": {
+										Type:         pluginsdk.TypeString,
+										Optional:     true,
+										ValidateFunc: validation.StringIsNotEmpty,
+										AtLeastOneOf: []string{"windows_profile.0.gmsa.0.dns_server", "windows_profile.0.gmsa.0.root_domain"},
+									},
+									"root_domain": {
+										Type:         pluginsdk.TypeString,
+										Optional:     true,
+										ValidateFunc: validation.StringIsNotEmpty,
+										AtLeastOneOf: []string{"windows_profile.0.gmsa.0.dns_server", "windows_profile.0.gmsa.0.root_domain"},
+									},


It looks like the API has changed since it now throws an error if only one or the other is set. Depending on the answer to my question in the other comment left in-line, these should either both become Required or RequiredWith.

Suggested change

"dns_server": {

Type: pluginsdk.TypeString,

Optional: true,

ValidateFunc: validation.StringIsNotEmpty,

AtLeastOneOf: []string{"windows_profile.0.gmsa.0.dns_server", "windows_profile.0.gmsa.0.root_domain"},

},

"root_domain": {

Type: pluginsdk.TypeString,

Optional: true,

ValidateFunc: validation.StringIsNotEmpty,

AtLeastOneOf: []string{"windows_profile.0.gmsa.0.dns_server", "windows_profile.0.gmsa.0.root_domain"},

},

"dns_server": {

Type: pluginsdk.TypeString,

Optional: true,

ValidateFunc: validation.StringIsNotEmpty,

RequiredWith: []string{"windows_profile.0.gmsa.0.dns_server", "windows_profile.0.gmsa.0.root_domain"},

},

"root_domain": {

Type: pluginsdk.TypeString,

Optional: true,

ValidateFunc: validation.StringIsNotEmpty,

RequiredWith: []string{"windows_profile.0.gmsa.0.dns_server", "windows_profile.0.gmsa.0.root_domain"},

},

ms-henglu · 2022-08-31T03:01:58Z

Hi @stephybun ,

I've confirmed with service team, if both dns_server and root_domain are omitted, it's still a valid config to enable gmsa. I've updated the PR, thanks!

stephybun · 2022-09-05T11:34:36Z

Thanks for clarifying @ms-henglu.

To avoid the case where a user would need to add in an empty gmsa block in their config just to enable it I think we need to make the following adjustments to the schema

						"gmsa": {
							Type:     pluginsdk.TypeList,
							Optional: true,
							MaxItems: 1,
							Elem: &pluginsdk.Resource{
								Schema: map[string]*pluginsdk.Schema{
									// empty values for these are valid and required to enable gmsa with defaults
									"dns_server": {
										Type:     pluginsdk.TypeString,
										Required: true,
									},
									"root_domain": {
										Type:     pluginsdk.TypeString,
										Required: true,
									},
								},
							},
						},

Once that's done this should be good to go!

ms-henglu · 2022-09-06T03:17:34Z

Hi @stephybun ,

Thanks for the suggestion, I've updated the schema, would you please take another look? Thanks!

stephybun

Thanks @qiqingzhang @ms-henglu LGTM 🍕

github-actions · 2022-09-09T03:17:30Z

This functionality has been released in v3.22.0 of the Terraform Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

github-actions · 2022-10-10T02:42:00Z

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

github-actions bot added documentation size/M labels Apr 19, 2022

qiqingzhang changed the title ~~add gmsaprofile to k8s cluster~~ k8s_cluster supports gmsa_profile Apr 19, 2022

stephybun added enhancement service/kubernetes-cluster labels Apr 19, 2022

stephybun self-assigned this Apr 19, 2022

qiqingzhang changed the title ~~k8s_cluster supports gmsa_profile~~ kubernetes_cluster supports gmsa_profile May 6, 2022

qiqingzhang changed the title ~~kubernetes_cluster supports gmsa_profile~~ kubernetes_cluster -gmsa_profile May 11, 2022

stephybun requested changes May 12, 2022

View reviewed changes

qiqingzhang force-pushed the branch-add_gmsaProfile_to_k8s_cluster branch from 77a67f6 to 75040b8 Compare May 16, 2022 07:05

stephybun requested changes May 30, 2022

View reviewed changes

stephybun added the waiting-response label May 30, 2022

github-actions bot added size/S and removed size/M labels May 31, 2022

github-actions bot removed the waiting-response label Jun 1, 2022

stephybun requested changes Jun 2, 2022

View reviewed changes

qiqingzhang force-pushed the branch-add_gmsaProfile_to_k8s_cluster branch from 05f536d to 835d8a8 Compare June 2, 2022 09:11

github-actions bot added size/M and removed size/S labels Jun 2, 2022

qiqingzhang changed the title ~~kubernetes_cluster -gmsa_profile~~ kubernetes_cluster -gmsa Jun 2, 2022

qiqingzhang force-pushed the branch-add_gmsaProfile_to_k8s_cluster branch from 835d8a8 to 21d6288 Compare June 6, 2022 05:12

qiqingzhang added 5 commits June 6, 2022 15:01

k8s_cluster supports gmsa_profile

56c8610

modify dns/domain optional add valiadatefunc

af4acf9

remove unnecessary sapce

f70a949

[fix] remove enabled and rename gmsa

b353aa0

[sytle] clean up code

eb5302d

[fix](kubernetes_cluster):change to gmsa{...} styleand add `AtLeast…

40f1bb8

…OneOf`

qiqingzhang force-pushed the branch-add_gmsaProfile_to_k8s_cluster branch from 21d6288 to 40f1bb8 Compare June 6, 2022 07:23

[fix](kubernetes_cluster):fix testProvider

b32e0b0

stephybun requested changes Aug 25, 2022

View reviewed changes

github-actions bot added the waiting-response label Aug 25, 2022

add customize diff func to force new resource

4f5aa7c

github-actions bot removed the waiting-response label Aug 29, 2022

stephybun requested changes Aug 30, 2022

View reviewed changes

github-actions bot added the waiting-response label Aug 30, 2022

update

de99a72

github-actions bot removed the waiting-response label Aug 31, 2022

update schema

4a0505e

add note to docs on setting dns_server and root_domain

ec0fcb1

stephybun approved these changes Sep 6, 2022

View reviewed changes

stephybun merged commit 2596209 into hashicorp:main Sep 6, 2022

github-actions bot added this to the v3.22.0 milestone Sep 6, 2022

stephybun added a commit that referenced this pull request Sep 6, 2022

Update for #16437

56575bf

github-actions bot locked as resolved and limited conversation to collaborators Oct 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`kubernetes_cluster` -`gmsa` #16437

`kubernetes_cluster` -`gmsa` #16437

qiqingzhang commented Apr 19, 2022 •

edited by stephybun

Loading

stephybun left a comment

stephybun May 12, 2022

stephybun May 12, 2022

qiqingzhang commented May 16, 2022

ms-henglu commented May 30, 2022

stephybun May 30, 2022

qiqingzhang commented Jun 1, 2022

stephybun Jun 2, 2022

qiqingzhang commented Jun 2, 2022

mrowken commented Aug 10, 2022

stephybun left a comment

ms-henglu commented Aug 29, 2022

stephybun Aug 30, 2022

stephybun Aug 30, 2022

ms-henglu commented Aug 31, 2022

stephybun commented Sep 5, 2022

ms-henglu commented Sep 6, 2022

stephybun left a comment

github-actions bot commented Sep 9, 2022

github-actions bot commented Oct 10, 2022


		A `gmsa` block supports the following:

		* `dns_server` - (Optional) Specifies the DNS server for Windows gMSA. Set it to empty if you have configured the DNS server in the vnet which is used to create the managed cluster.

kubernetes_cluster -gmsa #16437

kubernetes_cluster -gmsa #16437

Conversation

qiqingzhang commented Apr 19, 2022 • edited by stephybun Loading

create a public pull request for k8s_cluster supports gmsa

stephybun left a comment

Choose a reason for hiding this comment

stephybun May 12, 2022

Choose a reason for hiding this comment

stephybun May 12, 2022

Choose a reason for hiding this comment

qiqingzhang commented May 16, 2022

ms-henglu commented May 30, 2022

stephybun May 30, 2022

Choose a reason for hiding this comment

qiqingzhang commented Jun 1, 2022

stephybun Jun 2, 2022

Choose a reason for hiding this comment

qiqingzhang commented Jun 2, 2022

mrowken commented Aug 10, 2022

stephybun left a comment

Choose a reason for hiding this comment

ms-henglu commented Aug 29, 2022

stephybun Aug 30, 2022

Choose a reason for hiding this comment

stephybun Aug 30, 2022

Choose a reason for hiding this comment

ms-henglu commented Aug 31, 2022

stephybun commented Sep 5, 2022

ms-henglu commented Sep 6, 2022

stephybun left a comment

Choose a reason for hiding this comment

github-actions bot commented Sep 9, 2022

github-actions bot commented Oct 10, 2022

`kubernetes_cluster` -`gmsa` #16437

`kubernetes_cluster` -`gmsa` #16437

qiqingzhang commented Apr 19, 2022 •

edited by stephybun

Loading

create a public pull request for `k8s_cluster` supports `gmsa`