Fix OpsWorks 'set permissions' failing to set ssh access. #1038
Conversation
When the permissions being changed in OpsWorks were only the flags
for the sudo and ssh permissions of the current user, terraform
would send a 'level' parameter containing an empty string. This is
not valid, and AWS will respond to the request saying you cannot
change your own privilege level. Clearly an empty string ("") is
not taken as being 'unchanged', but nil is.
This change detects that an empty string would be sent to AWS and
if so, sends 'nil' in its place (which makes the parameter unset,
and allows the operation). Consequently, this allows the OpsWorks
SetPermissions operation to complete, changing the Sudo or SSH
permissions.
Fixes: hashicorp#458
The ticket is commonly referenced in the CHANGELOG.md file, but this had not been done.
There was a problem hiding this comment.
Thank you for sending this PR @charles-at-geospock
This looks functionally good, I just left you some rather stylish and nitpicky comments there. Let me know what you think.
btw. We don't usually touch the Changelog in PRs to avoid conflicts. It is a job of the person merging the PR to do that.
| level_aws := aws.String(level_string) | ||
| if level_string == "" { | ||
| level_aws = nil | ||
| } |
There was a problem hiding this comment.
We have a more idiomatic way of dealing with this exact problem, take a look at d.GetOk()
There was a problem hiding this comment.
Ta; will fix.
| req := &opsworks.SetPermissionInput{ | ||
| AllowSudo: aws.Bool(d.Get("allow_sudo").(bool)), | ||
| AllowSsh: aws.Bool(d.Get("allow_ssh").(bool)), | ||
| Level: aws.String(d.Get("level").(string)), | ||
| Level: level_aws, | ||
| IamUserArn: aws.String(d.Get("user_arn").(string)), | ||
| StackId: aws.String(d.Get("stack_id").(string)), | ||
| } |
There was a problem hiding this comment.
The common pattern you can spot elsewhere in the codebase would 1st build the basic request with all required params and then set any optional ones if specified, e.g.
if v, ok := d.GetOk("level"); ok {
req.Level = aws.String(v.(string))
}There was a problem hiding this comment.
That's a lot cleaner, thanks!
radeksimko
added
bug
The 'GetOk' method makes the assignment of these optional variables a lot easier.
|
Changes to the code made as requested; makes things a bit nicer and now I know a little more. I'll look more at other examples of the code in the future to make sure that I can make it more similar. Did you want the ChangeLog change removing as well? I can strip that out as well, if that would help. And thanks for your very quick response! |
|
Awesome, thanks very much. I expect I will be submitting more changes in the future, so I'll try to get those things right in the future and make it smoother all around. |
ghost
mentioned this pull request
|
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! |
ghost
locked and limited conversation to collaborators
breathingdust
removed
the
waiting-response
When the permissions being changed in OpsWorks were only the flags
for the sudo and ssh permissions of the current user, terraform
would send a 'level' parameter containing an empty string. This is
not valid, and AWS will respond to the request saying you cannot
change your own privilege level. Clearly an empty string ("") is
not taken as being 'unchanged', but nil is.
This change detects that an empty string would be sent to AWS and
if so, sends 'nil' in its place (which makes the parameter unset,
and allows the operation). Consequently, this allows the OpsWorks
SetPermissions operation to complete, changing the Sudo or SSH
permissions.
Fixes: #458