Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

azurerm_postgresql_flexible_server - fix a potential panic #20052

Closed
wants to merge 1 commit into from

Conversation

ziyeqf
Copy link
Contributor

@ziyeqf ziyeqf commented Jan 17, 2023

It will panic when authentication is specified with empty block. add a runtime check for it

Copy link
Member

@jackofallops jackofallops left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @ziyeqf - Thanks for this PR.
I think we could approach this a different way to assist users by catching the problem at plan time, and ensuring that the block cannot be empty by using schema constraints, for example:

							AtLeastOneOf: []string{
								"authentication.0.active_directory_auth_enabled",
								"authentication.0.password_auth_enabled",
								"authentication.0.tenant_id",
							},

on each item in authentication, WDYT?

@ziyeqf
Copy link
Contributor Author

ziyeqf commented Jan 18, 2023

Hi @ziyeqf - Thanks for this PR. I think we could approach this a different way to assist users by catching the problem at plan time, and ensuring that the block cannot be empty by using schema constraints, for example:

							AtLeastOneOf: []string{
								"authentication.0.active_directory_auth_enabled",
								"authentication.0.password_auth_enabled",
								"authentication.0.tenant_id",
							},

on each item in authentication, WDYT?

Make sense, code updated, thanks!

jackofallops
jackofallops previously approved these changes Jan 18, 2023
Copy link
Member

@jackofallops jackofallops left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @ziyeqf
Thanks for making those changes, LGTM 👍

@jackofallops jackofallops dismissed their stale review January 18, 2023 06:03

Misread changes.

Copy link
Member

@jackofallops jackofallops left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ziyeqf
Apologies for the mistaken approval earlier, my mistake, hadn't had coffee yet 🙈 .
wrt combining with #20054, only one of these fixes should be required, hence asking to consider rolling them up / closing one. Both would result in guarding users against providing an invalid config. The simplest would be to set default: true on password_auth_enabled, so I think we should close this one, and run with/merge 20054?

Comment on lines +84 to +87
AtLeastOneOf: []string{
"authentication.0.active_directory_auth_enabled",
"authentication.0.password_auth_enabled",
},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be on each of the properties inside the block, so below would look like:

						"active_directory_auth_enabled": {
							Type:     pluginsdk.TypeBool,
							Optional: true,
							AtLeastOneOf: []string{
								"authentication.0.active_directory_auth_enabled",
								"authentication.0.password_auth_enabled",
							},
						},
						"password_auth_enabled": {
							Type:     pluginsdk.TypeBool,
							Optional: true,
							AtLeastOneOf: []string{
								"authentication.0.active_directory_auth_enabled",
								"authentication.0.password_auth_enabled",
							},
						},
						"tenant_id": {
							Type:         pluginsdk.TypeString,
							Optional:     true,
							ValidateFunc: validation.IsUUID,
							RequiredWith: []string{
								"authentication.0.active_directory_auth_enabled",
							},
						},
					},

@github-actions
Copy link

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 github-actions bot locked as resolved and limited conversation to collaborators Feb 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants