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

Enable service account authentication #1040

Merged
merged 30 commits into from
Apr 19, 2023
Merged

Conversation

adezxc
Copy link

@adezxc adezxc commented Apr 3, 2023

Original issue: #995

Depends on: vmware/go-vcloud-director#562

This PR adds a service_account field to the provider schema that allows providing a file containing a Service Account API token as described in the blog post.

For now it only supports using Service Accounts in Active Stage and the overall implementation is not set in stone, as maybe there are better solutions.

Adam Jasinski added 3 commits April 3, 2023 14:41
Signed-off-by: Adam Jasinski <jasinskia@vmware.com>
Signed-off-by: Adam Jasinski <jasinskia@vmware.com>
Signed-off-by: Adam Jasinski <jasinskia@vmware.com>
@adezxc adezxc marked this pull request as ready for review April 3, 2023 14:19
@adezxc adezxc removed the request for review from dataclouder April 3, 2023 14:19
Signed-off-by: Adam Jasinski <jasinskia@vmware.com>
Copy link
Author

@adezxc adezxc left a comment

Choose a reason for hiding this comment

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

Reminder to not forget about changelog entry, either in v3.9.0 or v3.10.0

Copy link
Collaborator

@lvirbalas lvirbalas left a comment

Choose a reason for hiding this comment

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

Unless, I missed it, we need more docs :)

@adezxc adezxc requested a review from dataclouder April 7, 2023 12:18
@Didainius
Copy link
Collaborator

It might be convenient to provide a similar script (https://registry.terraform.io/providers/vmware/vcd/latest/docs#shell-script-to-obtain-a-bearer-token) to get the Service Account active.

I also have a consideration that maybe it would be less friction to accept initial token as an attribute in provider section and do token rotation in file further.

Signed-off-by: Adam Jasinski <jasinskia@vmware.com>
@adezxc
Copy link
Author

adezxc commented Apr 11, 2023

It might be convenient to provide a similar script (https://registry.terraform.io/providers/vmware/vcd/latest/docs#shell-script-to-obtain-a-bearer-token) to get the Service Account active.

I created the script create_service_account.sh which goes from creation to active stage, but it's a 100 lines, so it's probably sub optimal to put it in the documentation and a link to the file could be given instead?

I also have a consideration that maybe it would be less friction to accept initial token as an attribute in provider section and do token rotation in file further.

I agree that it's less friction for the user, but is there a nice way of making it work? How would the provider know about the file the next time it is run? Also I feel like having files as the default just provides more flexibility to the user as to how they would want to make use multiple service account tokens

@Didainius
Copy link
Collaborator

I created the script create_service_account.sh which goes from creation to active stage, but it's a 100 lines, so it's probably sub optimal to put it in the documentation and a link to the file could be given instead?

Yes. It could probably live in examples subdirectory. We may think of a guide page in future how to use service accounts, but this is good for now.

The script itself is great - thanks. IMO this makes it so much easier to start with service accounts for those who don't have any infrastructure around them. My only wish (up to discussion) is probably to add -s to curl commands so that it doesn't spit out these:

nter VCD Role in URN format (e.g. urn:vcloud:role:System%20Administrator):
urn:vcloud:role:System%20Administrator
Enter software version (optional):
1
Enter client URI (optional):

Creating service account...
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100   483    0   313  100   170    345    187 --:--:-- --:--:-- --:--:--   536

I agree that it's less friction for the user, but is there a nice way of making it work? How would the provider know about the file the next time it is run? Also I feel like having files as the default just provides more flexibility to the user as to how they would want to make use multiple service account tokens

Agreed. Can live like that and then see if there is any demand to shape it in future

Adam Jasinski added 2 commits April 12, 2023 10:18
Signed-off-by: Adam Jasinski <jasinskia@vmware.com>
Signed-off-by: Adam Jasinski <jasinskia@vmware.com>
Adam Jasinski added 3 commits April 12, 2023 13:01
Signed-off-by: Adam Jasinski <jasinskia@vmware.com>
Signed-off-by: Adam Jasinski <jasinskia@vmware.com>
Signed-off-by: Adam Jasinski <jasinskia@vmware.com>
Signed-off-by: Adam Jasinski <jasinskia@vmware.com>
Signed-off-by: Adam Jasinski <jasinskia@vmware.com>
Copy link
Collaborator

@adambarreiro adambarreiro left a comment

Choose a reason for hiding this comment

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

Nice work!

Signed-off-by: Adam Jasinski <jasinskia@vmware.com>
Copy link
Collaborator

@Didainius Didainius left a comment

Choose a reason for hiding this comment

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

Token rotation and script to get service account worked in my testing.
Good to approve without an actual test because we do not have Go code to handle service account creation at the moment

Adam Jasinski added 6 commits April 17, 2023 14:24
Signed-off-by: Adam Jasinski <jasinskia@vmware.com>
Signed-off-by: Adam Jasinski <jasinskia@vmware.com>
Signed-off-by: Adam Jasinski <jasinskia@vmware.com>
Signed-off-by: Adam Jasinski <jasinskia@vmware.com>
Signed-off-by: Adam Jasinski <jasinskia@vmware.com>
Signed-off-by: Adam Jasinski <jasinskia@vmware.com>
Adam Jasinski added 6 commits April 18, 2023 16:00
Signed-off-by: Adam Jasinski <jasinskia@vmware.com>
Signed-off-by: Adam Jasinski <jasinskia@vmware.com>
Signed-off-by: Adam Jasinski <jasinskia@vmware.com>
Signed-off-by: Adam Jasinski <jasinskia@vmware.com>
Signed-off-by: Adam Jasinski <jasinskia@vmware.com>
Signed-off-by: Adam Jasinski <jasinskia@vmware.com>
Adam Jasinski added 5 commits April 19, 2023 09:50
Signed-off-by: Adam Jasinski <jasinskia@vmware.com>
nit
Signed-off-by: Adam Jasinski <jasinskia@vmware.com>
Signed-off-by: Adam Jasinski <jasinskia@vmware.com>
Signed-off-by: Adam Jasinski <jasinskia@vmware.com>
Signed-off-by: Adam Jasinski <jasinskia@vmware.com>
@adezxc adezxc merged commit 5ad2649 into vmware:main Apr 19, 2023
@adezxc adezxc deleted the service_accounts branch November 14, 2023 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants