-
Notifications
You must be signed in to change notification settings - Fork 97
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
Adding support for terraform private module source for git #7167
Adding support for terraform private module source for git #7167
Conversation
Signed-off-by: Vishwanath Hiremath <[email protected]>
Signed-off-by: Vishwanath Hiremath <[email protected]>
Signed-off-by: Vishwanath Hiremath <[email protected]>
Signed-off-by: Vishwanath Hiremath <[email protected]>
recipeConfig := src.Properties.RecipeConfig | ||
if recipeConfig != nil { | ||
if recipeConfig.Terraform != nil { | ||
if recipeConfig.Terraform.Authentication != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can these conditions be collapsed? Or do we expect that there may be future logic that's unique to each config level?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In any case i think we still need to check nil at every level to avoid null pointer. Let me know if you think otherwise
@@ -63,6 +64,24 @@ func (src *EnvironmentResource) ConvertTo() (v1.DataModelInterface, error) { | |||
} | |||
converted.Properties.Compute = *envCompute | |||
|
|||
recipeConfig := src.Properties.RecipeConfig | |||
if recipeConfig != nil { | |||
if recipeConfig.Terraform != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since these lines are specific to Terraform, maybe we should move this logic to a helper function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @sk593. We can have a function that takes care of the recipeConfig conversion and that function can make a call to another function which takes care of terraform related stuff conversion. With this approach we could keep things simple. It would be more readable, easier to debug and write unit tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -151,6 +170,27 @@ func (dst *EnvironmentResource) ConvertFrom(src v1.DataModelInterface) error { | |||
dst.Properties.Recipes = recipes | |||
} | |||
|
|||
if !reflect.DeepEqual(env.Properties.RecipeConfig, datamodel.RecipeConfigProperties{}) { | |||
recipeConfig := &RecipeConfigProperties{} | |||
if !reflect.DeepEqual(env.Properties.RecipeConfig.Terraform, datamodel.TerraformConfigProperties{}) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above, since this logic is specific to Terraform, maybe we can move it to a helper function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can apply the same thing as I mentioned above but this time in reverse.
This function -> RecipeConfig conversion function -> Terraform related conversion function
In future, it would be very easy to add new types of provider conversions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
created a separate func
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with the suggestion of creating a Terraform related conversion function. @vishwahiremat I know you have updated it to recipe config specific conversion, have you considered adding Terraform specific function to make it easy to extend the recipe config function in the future?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Vishwa and I discussed offline, we will abstract it out into Terraform specific function once we add support for other types.
Signed-off-by: Vishwanath Hiremath <[email protected]>
@@ -63,6 +64,24 @@ func (src *EnvironmentResource) ConvertTo() (v1.DataModelInterface, error) { | |||
} | |||
converted.Properties.Compute = *envCompute | |||
|
|||
recipeConfig := src.Properties.RecipeConfig | |||
if recipeConfig != nil { | |||
if recipeConfig.Terraform != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @sk593. We can have a function that takes care of the recipeConfig conversion and that function can make a call to another function which takes care of terraform related stuff conversion. With this approach we could keep things simple. It would be more readable, easier to debug and write unit tests.
@@ -151,6 +170,27 @@ func (dst *EnvironmentResource) ConvertFrom(src v1.DataModelInterface) error { | |||
dst.Properties.Recipes = recipes | |||
} | |||
|
|||
if !reflect.DeepEqual(env.Properties.RecipeConfig, datamodel.RecipeConfigProperties{}) { | |||
recipeConfig := &RecipeConfigProperties{} | |||
if !reflect.DeepEqual(env.Properties.RecipeConfig.Terraform, datamodel.TerraformConfigProperties{}) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can apply the same thing as I mentioned above but this time in reverse.
This function -> RecipeConfig conversion function -> Terraform related conversion function
In future, it would be very easy to add new types of provider conversions.
@@ -117,6 +117,19 @@ func TestConvertVersionedToDataModel(t *testing.T) { | |||
Scope: "/planes/aws/aws/accounts/140313373712/regions/us-west-2", | |||
}, | |||
}, | |||
RecipeConfig: datamodel.RecipeConfigProperties{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs a few different layers and we could run into nil pointer exception easily. We should test it thoroughly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we remove the PAT map from this test, we can see if we run into nil pointer exception above in the original conversion logic.
"pat": { | ||
"dev.azure.com":{ | ||
"secretStore":"/planes/radius/local/resourcegroups/default/providers/Applications.Core/secretStores/github" | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if this part doesn't exist (by mistake of the operator), would this cause a nil pointer exception?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont think it will nil pointer if pat property doesnt exist
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a unit test for this?
Compute rpv1.EnvironmentCompute `json:"compute,omitempty"` | ||
Recipes map[string]map[string]EnvironmentRecipeProperties `json:"recipes,omitempty"` | ||
Providers Providers `json:"providers,omitempty"` | ||
RecipeConfig RecipeConfigProperties `json:"recipeConfig,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am a fan of adding new properties to the end. Would the order matter here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not really but i added before recipes because recipeConfig is usually configured before recipes.
// Getting the module source path with credentials information if its a private source. | ||
path, _ := getModuleSource(ctx, envConfig, envRecipe.TemplatePath, armOptions) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we ignoring the error here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch! i handled it my latest pr
if secretStore != "" { | ||
secretStoreID, err := resources.ParseResource(secretStore) | ||
if err != nil { | ||
return "", err | ||
} | ||
client, err := v20231001preview.NewSecretStoresClient(secretStoreID.RootScope(), &aztoken.AnonymousCredential{}, armOptions) | ||
if err != nil { | ||
return "", err | ||
} | ||
secrets, err := client.ListSecrets(ctx, secretStoreID.Name(), map[string]any{}, nil) | ||
if err != nil { | ||
return "", err | ||
} | ||
url, err := getGitURL(templatePath) | ||
if err != nil { | ||
return "", err | ||
} | ||
var username, pat *string | ||
path := "git::https://" | ||
user, ok := secrets.Data["username"] | ||
if ok { | ||
username = user.Value | ||
path += fmt.Sprintf("%s:", *username) | ||
} | ||
token, ok := secrets.Data["pat"] | ||
if ok { | ||
pat = token.Value | ||
path += *pat | ||
} | ||
path += fmt.Sprintf("@%s", strings.TrimPrefix(url.String(), "https://")) | ||
return path, err | ||
} | ||
|
||
return templatePath, err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could do the other case first: if secretStore != "" { return templatePath, err }
. Then the rest doesn't need to be within an if case. I find this more readable: first exit cases then the actual logic.
|
||
err := tfconfig.Save(testcontext.New(t), testDir) | ||
require.Error(t, err) | ||
require.Equal(t, fmt.Sprintf("error creating file: open %s/main.tf.json: no such file or directory", testDir), err.Error()) | ||
} | ||
|
||
func Test_getSecretStoreID(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should add unit tests for all the new functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont if we can add unit for getGitSource
func but i added for the other ones
pkg/recipes/terraform/execute.go
Outdated
@@ -268,6 +271,8 @@ func downloadAndInspect(ctx context.Context, tf *tfexec.Terraform, options Optio | |||
// Download the Terraform module to the working directory. | |||
logger.Info(fmt.Sprintf("Downloading Terraform module: %s", options.EnvRecipe.TemplatePath)) | |||
downloadStartTime := time.Now() | |||
|
|||
//path := options.EnvRecipe.TemplatePath |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can be removed.
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
func getModuleSource(ctx context.Context, envConfig *recipes.Configuration, templatePath string, armOptions *arm.ClientOptions) (string, error) { | ||
var source string | ||
var err error | ||
if strings.HasPrefix(templatePath, "git::") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we only supporting private registries that are accessed over HTTPS? Terraform supports other Git URLS (just looking at this: https://developer.hashicorp.com/terraform/language/modules/sources#github) so I'm wondering what the expected behavior would be if a user passed one of these formats
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thats correct, today we don't support private registries that are accessed over ssh. And for private git repository we expect user to provide the source in generic git format so all git platforms are supported.
Signed-off-by: Vishwanath Hiremath <[email protected]>
...tion/applications/resource-manager/Applications.Core/preview/2023-10-01-preview/openapi.json
Outdated
Show resolved
Hide resolved
...tion/applications/resource-manager/Applications.Core/preview/2023-10-01-preview/openapi.json
Outdated
Show resolved
Hide resolved
Signed-off-by: Vishwanath Hiremath <[email protected]>
Signed-off-by: Vishwanath Hiremath <[email protected]>
Signed-off-by: Vishwanath Hiremath <[email protected]>
} | ||
} | ||
} | ||
for k, v := range config.Terraform.Authentication.Git.PAT { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this throw a nil pointer exception if config doesn't have Terraform map? Or Terraform map doesn't have Authentication and so on. We can add unit tests for those cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this for loop should move to the last if in above. See: if config.Terraform.Authentication.Git.PAT != nil.
@@ -117,6 +117,19 @@ func TestConvertVersionedToDataModel(t *testing.T) { | |||
Scope: "/planes/aws/aws/accounts/140313373712/regions/us-west-2", | |||
}, | |||
}, | |||
RecipeConfig: datamodel.RecipeConfigProperties{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we remove the PAT map from this test, we can see if we run into nil pointer exception above in the original conversion logic.
if providers.Azure != nil { | ||
config.Providers.Azure.Scope = to.String(providers.Azure.Scope) | ||
} | ||
env, err := environment.ConvertTo() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is ConvertTo a new function? It doesn't make a lot of sense to me. We want to convert environment to what specifically? Not in the scope of this PR, but we should check this function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i am using the existing convertTo() function from environment_conversions
RecipeConfig: &model.RecipeConfigProperties{ | ||
Terraform: &model.TerraformConfigProperties{ | ||
Authentication: &model.AuthConfig{ | ||
Git: &model.GitAuthConfig{ | ||
Pat: map[string]*model.Secret{ | ||
"dev.azure.com": &model.Secret{ | ||
SecretStore: to.Ptr("secretStoreID"), | ||
}, | ||
}, | ||
}, | ||
}, | ||
}, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should have a few cases that would cause errors. Like what happens if we remove the PAT?
Signed-off-by: Vishwanath Hiremath <[email protected]>
|
||
@doc("Specifies authentication information needed to access private terraform modules from Git repository sources.") | ||
model GitAuthConfig{ | ||
@doc("Specifies the secret details of type personal access token for each different git platforms") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@doc("Specifies the secret details of type personal access token for each different git platforms") | |
@doc("Personal Access Token (PAT) configuration used to authenticate to Git platforms.") |
pat?: Record<SecretConfig>; | ||
} | ||
|
||
@doc("Specifies the secret details of type personal access token for each different git platforms") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@doc("Specifies the secret details of type personal access token for each different git platforms") | |
@doc("Personal Access Token (PAT) configuration used to authenticate to Git platforms.") |
|
||
@doc("Specifies the secret details of type personal access token for each different git platforms") | ||
model SecretConfig { | ||
@doc("The resource id for the Applications.Core/SecretStore resource containing credentials.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add some information about what the secret name is required to be in the secret store? Do we expect that there needs to be a secret with a specific name?
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
Signed-off-by: Vishwanath Hiremath <[email protected]>
Signed-off-by: Vishwanath Hiremath <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall. A few minor comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the example be updated as well https://github.com/radius-project/radius/blob/main/typespec/Applications.Core/examples/2023-10-01-preview/Environments_CreateOrUpdate.json?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated with examples
Signed-off-by: Vishwanath Hiremath <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Type descriptions look good to me!
Signed-off-by: Vishwanath Hiremath <[email protected]>
Signed-off-by: Vishwanath Hiremath <[email protected]>
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
Signed-off-by: Vishwanath Hiremath <[email protected]>
Signed-off-by: Vishwanath Hiremath <[email protected]>
Signed-off-by: Vishwanath Hiremath <[email protected]>
Signed-off-by: Vishwanath Hiremath <[email protected]>
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
…oject#7167) - Added a RecipeConfig property to the environment - Typesspec changes - Conversion and unit tests - Datamodel changes - Adding changes to the config to append the template path with credential informaiton - changes to query list secret to get secret information - creating credential appended template path Design Doc: radius-project/design-notes#37 <!-- Please select **one** of the following options that describes your change and delete the others. Clearly identifying the type of change you are making will help us review your PR faster, and is used in authoring release notes. If you are making a bug fix or functionality change to Radius and do not have an associated issue link please create one now. --> - This pull request adds or changes features of Radius and has an approved issue (issue link required). - This pull request is a minor refactor, code cleanup, test improvement, or other maintenance task and doesn't change the functionality of Radius (issue link optional). <!-- Please update the following to link the associated issue. This is required for some kinds of changes (see above). --> Fixes: radius-project#6911 --------- Signed-off-by: Vishwanath Hiremath <[email protected]>
…oject#7167) - Added a RecipeConfig property to the environment - Typesspec changes - Conversion and unit tests - Datamodel changes - Adding changes to the config to append the template path with credential informaiton - changes to query list secret to get secret information - creating credential appended template path Design Doc: radius-project/design-notes#37 <!-- Please select **one** of the following options that describes your change and delete the others. Clearly identifying the type of change you are making will help us review your PR faster, and is used in authoring release notes. If you are making a bug fix or functionality change to Radius and do not have an associated issue link please create one now. --> - This pull request adds or changes features of Radius and has an approved issue (issue link required). - This pull request is a minor refactor, code cleanup, test improvement, or other maintenance task and doesn't change the functionality of Radius (issue link optional). <!-- Please update the following to link the associated issue. This is required for some kinds of changes (see above). --> Fixes: radius-project#6911 --------- Signed-off-by: Vishwanath Hiremath <[email protected]>
…oject#7167) - Added a RecipeConfig property to the environment - Typesspec changes - Conversion and unit tests - Datamodel changes - Adding changes to the config to append the template path with credential informaiton - changes to query list secret to get secret information - creating credential appended template path Design Doc: radius-project/design-notes#37 <!-- Please select **one** of the following options that describes your change and delete the others. Clearly identifying the type of change you are making will help us review your PR faster, and is used in authoring release notes. If you are making a bug fix or functionality change to Radius and do not have an associated issue link please create one now. --> - This pull request adds or changes features of Radius and has an approved issue (issue link required). - This pull request is a minor refactor, code cleanup, test improvement, or other maintenance task and doesn't change the functionality of Radius (issue link optional). <!-- Please update the following to link the associated issue. This is required for some kinds of changes (see above). --> Fixes: radius-project#6911 --------- Signed-off-by: Vishwanath Hiremath <[email protected]>
…oject#7167) - Added a RecipeConfig property to the environment - Typesspec changes - Conversion and unit tests - Datamodel changes - Adding changes to the config to append the template path with credential informaiton - changes to query list secret to get secret information - creating credential appended template path Design Doc: radius-project/design-notes#37 <!-- Please select **one** of the following options that describes your change and delete the others. Clearly identifying the type of change you are making will help us review your PR faster, and is used in authoring release notes. If you are making a bug fix or functionality change to Radius and do not have an associated issue link please create one now. --> - This pull request adds or changes features of Radius and has an approved issue (issue link required). - This pull request is a minor refactor, code cleanup, test improvement, or other maintenance task and doesn't change the functionality of Radius (issue link optional). <!-- Please update the following to link the associated issue. This is required for some kinds of changes (see above). --> Fixes: radius-project#6911 --------- Signed-off-by: Vishwanath Hiremath <[email protected]>
…oject#7167) - Added a RecipeConfig property to the environment - Typesspec changes - Conversion and unit tests - Datamodel changes - Adding changes to the config to append the template path with credential informaiton - changes to query list secret to get secret information - creating credential appended template path Design Doc: radius-project/design-notes#37 <!-- Please select **one** of the following options that describes your change and delete the others. Clearly identifying the type of change you are making will help us review your PR faster, and is used in authoring release notes. If you are making a bug fix or functionality change to Radius and do not have an associated issue link please create one now. --> - This pull request adds or changes features of Radius and has an approved issue (issue link required). - This pull request is a minor refactor, code cleanup, test improvement, or other maintenance task and doesn't change the functionality of Radius (issue link optional). <!-- Please update the following to link the associated issue. This is required for some kinds of changes (see above). --> Fixes: radius-project#6911 --------- Signed-off-by: Vishwanath Hiremath <[email protected]>
…oject#7167) # Description - Added a RecipeConfig property to the environment - Typesspec changes - Conversion and unit tests - Datamodel changes - Adding changes to the config to append the template path with credential informaiton - changes to query list secret to get secret information - creating credential appended template path Design Doc: radius-project/design-notes#37 ## Type of change <!-- Please select **one** of the following options that describes your change and delete the others. Clearly identifying the type of change you are making will help us review your PR faster, and is used in authoring release notes. If you are making a bug fix or functionality change to Radius and do not have an associated issue link please create one now. --> - This pull request adds or changes features of Radius and has an approved issue (issue link required). - This pull request is a minor refactor, code cleanup, test improvement, or other maintenance task and doesn't change the functionality of Radius (issue link optional). <!-- Please update the following to link the associated issue. This is required for some kinds of changes (see above). --> Fixes: radius-project#6911 --------- Signed-off-by: Vishwanath Hiremath <[email protected]>
…oject#7167) # Description - Added a RecipeConfig property to the environment - Typesspec changes - Conversion and unit tests - Datamodel changes - Adding changes to the config to append the template path with credential informaiton - changes to query list secret to get secret information - creating credential appended template path Design Doc: radius-project/design-notes#37 ## Type of change <!-- Please select **one** of the following options that describes your change and delete the others. Clearly identifying the type of change you are making will help us review your PR faster, and is used in authoring release notes. If you are making a bug fix or functionality change to Radius and do not have an associated issue link please create one now. --> - This pull request adds or changes features of Radius and has an approved issue (issue link required). - This pull request is a minor refactor, code cleanup, test improvement, or other maintenance task and doesn't change the functionality of Radius (issue link optional). <!-- Please update the following to link the associated issue. This is required for some kinds of changes (see above). --> Fixes: radius-project#6911 --------- Signed-off-by: Vishwanath Hiremath <[email protected]> Signed-off-by: willdavsmith <[email protected]>
Description
Design Doc: radius-project/design-notes#37
Type of change
Fixes: #6911