Feat: support private terraform registry #352

ramekris3163 · 2023-01-09T19:22:12Z

pull request to support volume mounting of the credentials.tfrc.json and .terraformrc files to be able to connect to private terraform registry.

codecov · 2023-01-09T19:26:29Z

Codecov Report

Base: 79.31% // Head: 80.36% // Increases project coverage by +1.04% 🎉

Coverage data is based on head (5b3fffb) compared to base (3a96f68).
Patch coverage: 89.84% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #352      +/-   ##
==========================================
+ Coverage   79.31%   80.36%   +1.04%     
==========================================
  Files          23       23              
  Lines        1750     1935     +185     
==========================================
+ Hits         1388     1555     +167     
- Misses        278      290      +12     
- Partials       84       90       +6

Flag	Coverage Δ
e2e	`?`
unit	`80.36% <89.84%> (+1.04%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
controllers/configuration_controller.go	`79.55% <89.84%> (+2.19%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

chivalryq

Thanks for contribution. These capabilities is important. Here are some suggestions about code optimization:

Consider the variable name, especially about Registry.
Reduce the duplication. make it more maintainable and easier for other contributors to understand and build upon.

chivalryq · 2023-01-13T06:28:57Z

api/types/state.go

+	TerraformInitError                                  ConfigurationState = "TerraformInitError"
+	InvalidGitCredentialsSecretReference                ConfigurationState = "InvalidGitCredentialsSecretReference"
+	InvalidTerraformCredentialsSecretReference          ConfigurationState = "InvalidTerraformCredentialsSecretReference"
+	InvalidTerraformRegistryConfigMapReference          ConfigurationState = "InvalidTerraformRegistryConfigMapReference"


If I understand correctly, all constant, variables, and functions named "TerraformRegistryConfigMapReference" refer to .terraformrc file stored in a ConfigMap). But we shouldn't refer it as simply Registry because this file contains more configuration more than just registry than registry information. It would be more appropriate to refer to it as "TerraformRC".

fixed as per review comments.

chivalryq · 2023-01-13T06:38:19Z

controllers/configuration_controller.go

@@ -571,6 +602,48 @@ func (r *ConfigurationReconciler) preCheck(ctx context.Context, configuration *v
 		}
 	}

+	if meta.TerraformCredentialsSecretReference != nil {


Here we have four similar logics fragment. It would be better to organize the content with a table like:

secretConfigMapToCheck:=struct{ ref *SecretConfigMapReference notFoundState ConfigurationState }{ {gitRef, InvalidGitCredentialsSecretReference}, {credsRef, InvalidTerraformCredentialsSecretReference } for _,check:=range secretConfigMapToCheck{ // check the secret or configMap }

fixed as per review comments.

chivalryq · 2023-01-13T06:41:28Z

controllers/configuration_controller.go

@@ -929,6 +1038,36 @@ func (meta *TFConfigurationMeta) createGitAuthConfigVolume() v1.Volume {
 	return gitAuthSecretVolume
 }

+func (meta *TFConfigurationMeta) createTerraformCredentialsConfigVolume() v1.Volume {


The four (three new added and createGitAuthConfigVolume) similar function can be unified. Please implement them with a generator function.

fixed as per review comments.

chivalryq · 2023-01-13T06:42:20Z

controllers/configuration_controller.go

@@ -1358,3 +1497,61 @@ func GetGitCredentialsSecret(ctx context.Context, k8sClient client.Client, secre

 	return secret, nil
 }
+
+// GetTerraformCredentialsSecret will get the secret containing the terraform credentials
+func GetTerraformCredentialsSecret(ctx context.Context, k8sClient client.Client, secretRef *v1.SecretReference) (*v1.Secret, error) {


Also, these GetFooSecret/GetFooConfigMap can be unified with a generator function.

fixed as per review comment.

removed GetFooSecret/GetFooConfigMap functions and we call GetSecretOrConfigMap to retrieve the secret/configmap

chivalryq · 2023-01-13T06:49:14Z

You can use git commit -s to sign-off the DCO.

chivalryq · 2023-01-17T06:50:44Z

controllers/configuration_controller.go

+	// validate secretreferences and config maps. 1. GitCredentialsSecretReference 2. TerraformCredentialsSecretReference 3. TerraformRCConfigMapReference
+	// 4. TerraformCredentialsHelperConfigMapReference


Please format the comment.

chivalryq · 2023-01-17T06:57:11Z

controllers/configuration_controller.go

+	secretConfigMapToCheck := []struct {
+		ref           *v1.SecretReference
+		notFoundState types.ConfigurationState
+		refType       string


You can replace the refType with a function and use this function to check the existence. No need to match by type.

[]struct { ref *v1.SecretReference notFoundState types.ConfigurationState checkFn func(ctx context.Context, k8sClient client.Client, ref *Reference) (metav1.Object, error) }

There is another thing to do: define a Reference struct which has all field of SecretReference and ConfigMapReference. You can cast the type to Reference when you call check

See #352 (comment). And checkFn will be assigned with GetObjectByReferenceFn(ctx, client, ref, needKey, errMsg)

removed the function call and changed the code to call a single method to retrieve secret or configmap.

chivalryq · 2023-01-17T06:58:43Z

controllers/configuration_controller.go

+			case "GitCredentialsSecretReference":
+				gitCreds, err := GetGitCredentialsSecret(ctx, k8sClient, check.ref)
+				if gitCreds == nil {
+					checkErr = err
+					checkErrFlag = true
+				}
+			case "TerraformCredentialsSecretReference":
+				terraformCreds, err := GetTerraformCredentialsSecret(ctx, k8sClient, check.ref)
+				if terraformCreds == nil {
+					checkErr = err
+					checkErrFlag = true
+				}
+			case "TerraformRCConfigMapReference":
+				terraformRegistryConfig, err := GetTerraformRCConfigMap(ctx, k8sClient, check.ref)
+				if terraformRegistryConfig == nil {
+					checkErr = err
+					checkErrFlag = true
+				}
+			case "TerraformCredentialsHelperConfigMapReference":
+				terraformCredentialsHelper, err := GetTerraformCredentialsHelperConfigMap(ctx, k8sClient, check.ref)
+				if terraformCredentialsHelper == nil {
+					checkErr = err
+					checkErrFlag = true
+				}


And this become

checkedObj,err:= check.checkFn(ctx, k8sObject, (*Reference)check.ref) // check err and objects...

removed the function call.

chivalryq · 2023-01-17T07:01:28Z

controllers/configuration_controller.go

@@ -898,9 +1020,21 @@ func (meta *TFConfigurationMeta) assembleExecutorVolumes() []v1.Volume {
 	tfBackendVolume := meta.createTFBackendVolume()
 	executorVolumes := []v1.Volume{workingVolume, inputTFConfigurationVolume, tfBackendVolume}
 	if meta.GitCredentialsSecretReference != nil {
-		gitAuthConfigVolume := meta.createGitAuthConfigVolume()
+		gitAuthConfigVolume := meta.createSecretOrConfigMapVolume(true, meta.GitCredentialsSecretReference.Name, GitAuthConfigVolumeName)


This is the same. Embed a function in a table and loop it instead of execute respectively。

chivalryq · 2023-01-17T07:11:50Z

controllers/configuration_controller.go

+	if configMap != nil {
+		return configMap, nil
+	} else {
+		return nil, err


Well, we're used to check the err first. Sometimes you can assume that if configMap returned isn't nil, then no error happens. But it depends. My suggestion is to check it first.

chivalryq · 2023-01-17T07:25:06Z

controllers/configuration_controller.go

+	errMsg := "Failed to get the terraform credentials helper configmap"
+	neededKeys := []string{}
+	_, configMap, err := GetSecretOrConfigMap(ctx, k8sClient, false, configMapRef, neededKeys, errMsg, "")
+	if configMap != nil {
+		return configMap, nil
+	} else {
+		return nil, err
+	}
+}


Great, seems you have made some abstraction using GetSecretOrConfigMap. As I mentioned in the last review, we can make GetFooConfigMap/GetBarSecret function more general.

You can use a function to generate these function like

type ObjectGetter func (ctx context.Context, k8sClient client.Client, ref *Reference) (metav1.Object,error) func GetObjectByReferenceFn(ctx context.Context, k8sClient client.Client, ref *Reference, ,needKeys []string, errMsg string) ObjectGetter { return func(ctx context.Context, k8sClient client.Client, ref *Reference)(metav1.Object,error){ // get configMap or Secret // check err // return metav1.Object } }

Returned metav1.Object is an option. You can also use unstructrured.Unstructured. They are type/interface that can represent both Secret and ConfigMap from k8s.io/apimechinary.

controllers/configuration_controller.go

chivalryq · 2023-01-18T03:53:58Z

controllers/configuration_controller_test.go

@@ -1639,6 +1639,118 @@ func TestAssembleTerraformJobWithGitCredentialsSecretRef(t *testing.T) {
 	assert.Contains(t, spec.Volumes, gitAuthSecretVolume)
 }

+func TestAssembleTerraformJobWithTerraformRegistryConfigAndCredentialsSecretRef(t *testing.T) {


Suggested change

func TestAssembleTerraformJobWithTerraformRegistryConfigAndCredentialsSecretRef(t *testing.T) {

func TestAssembleTerraformJobWithTerraformRCAndCredentials(t *testing.T) {

chivalryq · 2023-01-18T03:56:10Z

controllers/configuration_controller_test.go

+
+}
+
+func TestAssembleTerraformJobWithTerraformRegistryConfigAndCredentialsHelperConfigMapRef(t *testing.T) {


Why not merge this with the last test?

merged the two tests.

chivalryq · 2023-01-18T03:59:26Z

controllers/configuration_controller_test.go

+				k8sClient: k8sClient,
+				TerraformCredentialsSecretReference: &corev1.SecretReference{
+					Namespace: "default",
+					Name:      "terraform-credentials",


Suggested change

Name: "terraform-credentials",

Name: "secret-not-exist",

chivalryq

Generally LGTM. Please add some e2e test.

controllers/configuration_controller.go

…rm a kubernetes secret. Signed-off-by: raradhakrishnan <raradhakrishnan@guidewire.com>

Signed-off-by: raradhakrishnan <raradhakrishnan@guidewire.com>

…tials-helper form configmaps. Signed-off-by: raradhakrishnan <raradhakrishnan@guidewire.com>

…ry as per review commments. Signed-off-by: raradhakrishnan <raradhakrishnan@guidewire.com>

… or configMap. Review comment fixed. Signed-off-by: raradhakrishnan <raradhakrishnan@guidewire.com>

…fixed review comment. Signed-off-by: raradhakrishnan <raradhakrishnan@guidewire.com>

…tent for secret and configmaps check. Fixed review comments. Signed-off-by: raradhakrishnan <raradhakrishnan@guidewire.com>

Signed-off-by: raradhakrishnan <raradhakrishnan@guidewire.com>

Co-authored-by: qiaozp <47812250+chivalryq@users.noreply.github.com> Signed-off-by: raradhakrishnan <raradhakrishnan@guidewire.com>

ramekris3163 mentioned this pull request Jan 9, 2023

Support customized .terraformrc and credentials.tfrc.json #350

Closed

chivalryq reviewed Jan 13, 2023

View reviewed changes

ramekris3163 force-pushed the feat/supportprivateterraformregistry branch from 62520e5 to 6a1232f Compare January 16, 2023 21:02

chivalryq reviewed Jan 17, 2023

View reviewed changes

chivalryq reviewed Jan 18, 2023

View reviewed changes

chivalryq reviewed Jan 19, 2023

View reviewed changes

controllers/configuration_controller.go Outdated Show resolved Hide resolved

controllers/configuration_controller.go Outdated Show resolved Hide resolved

ramekris3163 and others added 15 commits January 18, 2023 22:14

feat: Added ability to load terraform registry url and credentials fo…

9086660

…rm a kubernetes secret. Signed-off-by: raradhakrishnan <raradhakrishnan@guidewire.com>

feat: fixed review comments.

2ac8852

Signed-off-by: raradhakrishnan <raradhakrishnan@guidewire.com>

feat: added required unit tests.

c9fff27

Signed-off-by: raradhakrishnan <raradhakrishnan@guidewire.com>

feat: autogenerated files during test.

57bb6cd

Signed-off-by: raradhakrishnan <raradhakrishnan@guidewire.com>

feat: fixed intendation.

52b3651

Signed-off-by: raradhakrishnan <raradhakrishnan@guidewire.com>

feat: added ability to volume mount .terraformrc and terraform-creden…

ee99851

…tials-helper form configmaps. Signed-off-by: raradhakrishnan <raradhakrishnan@guidewire.com>

feat: updated variable name to TerraformRC instead of TerraformRegist…

5578811

…ry as per review commments. Signed-off-by: raradhakrishnan <raradhakrishnan@guidewire.com>

feat: unified similar functions to create a volume with either secret…

fcb2537

… or configMap. Review comment fixed. Signed-off-by: raradhakrishnan <raradhakrishnan@guidewire.com>

feat: GetFooSecret/GetFooConfigMap unified with a generator function.…

fbf828b

…fixed review comment. Signed-off-by: raradhakrishnan <raradhakrishnan@guidewire.com>

feat: reduced cylomatic complexity of precheck func and organized con…

c8efbee

…tent for secret and configmaps check. Fixed review comments. Signed-off-by: raradhakrishnan <raradhakrishnan@guidewire.com>

feat: added unit test.

b3a2a4b

Signed-off-by: raradhakrishnan <raradhakrishnan@guidewire.com>

feat: fixed review comments.

77e583a

Signed-off-by: raradhakrishnan <raradhakrishnan@guidewire.com>

feat: fixed review comments.

61f1aaa

Signed-off-by: raradhakrishnan <raradhakrishnan@guidewire.com>

Update controllers/configuration_controller.go

07d4854

Co-authored-by: qiaozp <47812250+chivalryq@users.noreply.github.com> Signed-off-by: raradhakrishnan <raradhakrishnan@guidewire.com>

Update controllers/configuration_controller.go

5b3fffb

Co-authored-by: qiaozp <47812250+chivalryq@users.noreply.github.com> Signed-off-by: raradhakrishnan <raradhakrishnan@guidewire.com>

ramekris3163 force-pushed the feat/supportprivateterraformregistry branch from 6a6cb4c to 5b3fffb Compare January 19, 2023 03:15

chivalryq changed the title ~~Feat/supportprivateterraformregistry~~ Jan 19, 2023

chivalryq changed the title ~~Feat/support private terraform registry~~ Jan 19, 2023

chivalryq approved these changes Jan 19, 2023

View reviewed changes

chivalryq merged commit c6d69a8 into kubevela:master Jan 19, 2023

Somefive mentioned this pull request Feb 1, 2023

Nominate anoop2811 as a maintainer of KubeVela Org kubevela/community#110

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat: support private terraform registry #352

Feat: support private terraform registry #352

ramekris3163 commented Jan 9, 2023

codecov bot commented Jan 9, 2023 •

edited

Loading

chivalryq left a comment •

edited

Loading

chivalryq Jan 13, 2023

ramekris3163 Jan 16, 2023

chivalryq Jan 13, 2023

ramekris3163 Jan 16, 2023

chivalryq Jan 13, 2023

ramekris3163 Jan 16, 2023

chivalryq Jan 13, 2023

ramekris3163 Jan 16, 2023

ramekris3163 Jan 17, 2023

chivalryq commented Jan 13, 2023

chivalryq Jan 17, 2023

ramekris3163 Jan 17, 2023

chivalryq Jan 17, 2023

chivalryq Jan 17, 2023

ramekris3163 Jan 17, 2023

chivalryq Jan 17, 2023 •

edited

Loading

ramekris3163 Jan 17, 2023

chivalryq Jan 17, 2023

ramekris3163 Jan 17, 2023

chivalryq Jan 17, 2023

ramekris3163 Jan 18, 2023

chivalryq Jan 17, 2023 •

edited

Loading

ramekris3163 Jan 18, 2023

chivalryq Jan 18, 2023

ramekris3163 Jan 18, 2023

chivalryq Jan 18, 2023

ramekris3163 Jan 18, 2023

chivalryq Jan 18, 2023

ramekris3163 Jan 18, 2023

chivalryq left a comment

		// validate secretreferences and config maps. 1. GitCredentialsSecretReference 2. TerraformCredentialsSecretReference 3. TerraformRCConfigMapReference
		// 4. TerraformCredentialsHelperConfigMapReference

	func TestAssembleTerraformJobWithTerraformRegistryConfigAndCredentialsSecretRef(t *testing.T) {
	func TestAssembleTerraformJobWithTerraformRCAndCredentials(t *testing.T) {

Feat: support private terraform registry #352

Feat: support private terraform registry #352

Conversation

ramekris3163 commented Jan 9, 2023

codecov bot commented Jan 9, 2023 • edited Loading

Codecov Report

chivalryq left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chivalryq commented Jan 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chivalryq Jan 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chivalryq Jan 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chivalryq left a comment

Choose a reason for hiding this comment

codecov bot commented Jan 9, 2023 •

edited

Loading

chivalryq left a comment •

edited

Loading

chivalryq Jan 17, 2023 •

edited

Loading

chivalryq Jan 17, 2023 •

edited

Loading