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

Add options for specifying the subscription and object IDs with the Azure Provisioner #642

Merged
merged 9 commits into from Mar 3, 2022

Conversation

vijayjt
Copy link
Contributor

@vijayjt vijayjt commented Feb 22, 2022

This PR is related to this issue in the certificates repo:

It adds command line options for the Azure Provisioner to allow the user to specify the subscription IDs and/or object IDs that can request certificates.

Here are the corresponding PRs in the certificates repo and the linked ca repo.

Copy link
Collaborator

@maraino maraino left a comment

Choose a reason for hiding this comment

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

Hi @vijayjt, the beta commands flags are set, but they are not implemented, can you add them to the createAzureDetails and updateAzureDetails functions.

command/ca/provisioner/add.go Outdated Show resolved Hide resolved
command/ca/provisioner/add.go Outdated Show resolved Hide resolved
command/ca/provisionerbeta/add.go Show resolved Hide resolved
Comment on lines +195 to +198
azureSubscriptionIDFlag,
removeAzureSubscriptionIDFlag,
azureObjectIDFlag,
removeAzureObjectIDFlag,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see these flags taken into account in the updateAzureDetails function.

Copy link
Collaborator

@maraino maraino left a comment

Choose a reason for hiding this comment

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

Hi @vijayjt, I think there's a small problem with the flags to update parameters, they should not include the add- prefix.

I'm going to merge the changes and create another PR to fix all of those, because it looks like GCP, AWS and existing Azure flags have the same problem.

Comment on lines 818 to 832
if ctx.IsSet("azure-resource-group") {
details.ResourceGroups = append(details.ResourceGroups, ctx.StringSlice("add-azure-resource-group")...)
}
if ctx.IsSet("remove-azure-subscription-id") {
details.SubscriptionIds = removeElements(details.SubscriptionIds, ctx.StringSlice("remove-azure-subscription-id"))
}
if ctx.IsSet("azure-subscription-id") {
details.SubscriptionIds = append(details.SubscriptionIds, ctx.StringSlice("add-azure-subscription-id")...)
}
if ctx.IsSet("remove-azure-object-id") {
details.ObjectIds = removeElements(details.ObjectIds, ctx.StringSlice("remove-azure-object-id"))
}
if ctx.IsSet("azure-object-id") {
details.ObjectIds = append(details.ObjectIds, ctx.StringSlice("add-azure-object-id")...)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the flags are:

  • azure-subscription-id instead of add-azure-subscription-id
  • azure-object-id instead of add-azure-object-id

And the same might happen in add-azure-resource-group.

Copy link
Collaborator

Choose a reason for hiding this comment

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

In fact, after looking at the code, the same happens for:

  • add-gcp-service-account
  • add-gcp-project
  • add-aws-account
  • and of course add-azure-resource-group

@maraino
Copy link
Collaborator

maraino commented Mar 3, 2022

I'll push directly to your repo and merge everything together.

@maraino maraino merged commit 57b7033 into smallstep:master Mar 3, 2022
maraino added a commit that referenced this pull request Mar 3, 2022
@maraino maraino mentioned this pull request Mar 3, 2022
maraino added a commit that referenced this pull request Mar 3, 2022
@vijayjt vijayjt deleted the new-azure-token-authz-options branch March 4, 2022 00:32
@vijayjt
Copy link
Contributor Author

vijayjt commented Mar 4, 2022

@maraino Awesome - thanks for making those changes for me and for reviewing and getting my PRs merged in - much appreciated!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs triage Waiting for discussion / prioritization by team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants