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

API missing method to walk through providers #118

Closed
jonathansp opened this issue May 20, 2021 · 10 comments
Closed

API missing method to walk through providers #118

jonathansp opened this issue May 20, 2021 · 10 comments

Comments

@jonathansp
Copy link
Contributor

jonathansp commented May 20, 2021

Given the following providers.tf file:

provider "azuread" {
    tenant_id = "00000000-0000-0000-0000-000000000000"
}

provider "databricks" {
}

I'm working on a tflint plugin to validate the requirements in our declared providers. I can see the providers were parsed correctly but there are no public methods to expose them.

Screenshot 2021-05-20 at 13 30 05

Is there any way to walk through providers in order to validate their attributes?

@jonathansp
Copy link
Contributor Author

jonathansp commented May 20, 2021

Thanks for replying @wata727

I guess the issue here is that tflint.Runner interface does not provide any public method to list data from providers.

 ... Check(runner tflint.Runner) error { ... 

config, _ := runner.Config()

call len(config.Module.ProviderMetas)
0
call len(config.Module.ProviderLocalNames)
0
call len(config.Module.ProviderConfigs)
0

The test case for this is:

func Test_DummyTestCase(t *testing.T) {
	cases := []struct {
		Name     string
		Content  string
		Expected helper.Issues
	}{
		{
			Name: "dummy test case",
			Content: `
provider "azuread" {
	tenant_id = "00000000-0000-0000-0000-000000000000"
}
provider "databricks" {}`,
			Expected: helper.Issues{
				{
					Rule: NewDummyRule(),
					Range: hcl.Range{
						Filename: "providers.tf",
						Start:    hcl.Pos{Line: 3, Column: 21},
						End:      hcl.Pos{Line: 3, Column: 31},
					},
				},
			},
		},
	}
	rule := NewDummyRule()
	for _, tc := range cases {
		runner := helper.TestRunner(t, map[string]string{"providers.tf": tc.Content})
		if err := rule.Check(runner); err != nil {
			t.Fatalf("Unexpected error occurred: %s", err)
		}
		helper.AssertIssues(t, tc.Expected, runner.Issues)
	}
}

What is not clear to me is, how should I access these providers inside the Check() method?

// Check checks whether ...
func (r *DummyRule) Check(runner tflint.Runner) error {

	... 


	return runner.EmitIssue(
		r,
		fmt.Sprintf("issue"),
		hcl.Range{},
	)
}

Thanks once again.

@wata727
Copy link
Member

wata727 commented May 20, 2021

Ah, understood. This is a helper.TestRunner issue. The runner provided by tflint-plugin-sdk is different from the real Runner and does not support some features. For instance, provider is not supported:

case "provider":
// TODO

The issue is mentioned in #64. There should be no problem in the real use case.

@jonathansp
Copy link
Contributor Author

ah, I see. Thanks. Do you think we can have the Files() method implemented in the fake runner like in https://github.com/terraform-linters/tflint/blob/master/tflint/runner.go#L218? It would be a quick workaround to make both tests and the rule itself working

@wata727
Copy link
Member

wata727 commented May 21, 2021

I think it is difficult to add a workaround for the fake runner because the Runner interface doesn't have the Files() method. The only workaround is to add an implementation like the following to the fake runner.
https://github.com/hashicorp/terraform/blob/cebbc8b246fd143fe161f01d2a96dac506224dbe/internal/configs/provider.go#L37-L131

@jonathansp
Copy link
Contributor Author

jonathansp commented May 21, 2021

Hi @wata727 I may have misunderstood what you explained. Runner interface is part of tflint-plugin-sdk, (not from tflint or terraform) and this interface does not implement all the public methods from the original Runner struct. Runner interface must have implemented Files() method (and all the public ones) in order to mimic the expected behavior, even if these methods just raise an error when not supported yet. Some of them are:

func (r *Runner) TFConfigPath() string 
func (r *Runner) LookupIssues(files ...string) Issues
func (r *Runner) Files() map[string]*hcl.File

Can you please confirm if my understanding is correct? Or I'm confused due the name Runner 😅

Many thanks

@wata727
Copy link
Member

wata727 commented May 22, 2021

Runner interface is part of tflint-plugin-sdk, (not from tflint or terraform) and this interface does not implement all the public methods from the original Runner struct.

Correct. To be precise, this interface is not related to the tflint's Runner struct.

Originally, all rules were designed to take a Runner struct as an argument, but we needed to abstract implementation to support the plugin system. That is the Runner interface.

In the plugin system, the implementation that satisfies the Runner interface is an RPC client, not the Runner struct. This is because we cannot decode complex structures over RPC. The RPC client makes a request to tflint's host process server. The server process returns the result to the RPC client using the Runner struct. See also Architecture.

For testing, it's difficult to have an RPC client, so we provide a fake Runner.

Runner interface must have implemented Files() method (and all the public ones) in order to mimic the expected behavior

No, as mentioned above, tflint's Runner struct and the Runner interface are irrelevant, and the Files() method is the logic of the Runner struct, so the interface doesn't need to implement this.

I'm also worried about the complexity of this architecture... 😖
In the future, tflint's Runner struct will be a more server-specific implementation (now, some built-in rules use the Runner directly) and maybe improved.

@jonathansp
Copy link
Contributor Author

Many thanks for digging into, @wata727. Now I got it. Would be correct, based on the RootProvider(name) function, extending tflint, adding a Providers() function, as the following snippet:

// Providers returns the providers configuration on the module as tfplugin.Provider
func (s *Server) Providers(req *tfplugin.ProvidersRequest, resp *tfplugin.ProvidersResponse) error {
	var providers []*tfplugin.Provider

	for _, provider := range s.rootRunner.TFConfig.Module.ProviderConfigs {
		providers = append(providers, s.encodeProvider(provider))
	}

	for _, provider := range s.runner.TFConfig.Module.ProviderConfigs {
		providers = append(providers, s.encodeProvider(provider))
	}

	*resp = tfplugin.ProvidersResponse{
		Providers: providers,
	}
	return nil
}

If not, do you think we currently have any other alternative to listing the providers?

@wata727
Copy link
Member

wata727 commented May 24, 2021

You can get providers as follows by using Config() API. However, as mentioned above, you cannot get providers in tests.

import "github.com/terraform-linters/tflint-plugin-sdk/tflint"

...

func (r *ExampleRule) Check(runner tflint.Runner) error {
	runner.Config().Module. ProviderConfigs // => map[string]*Provider
}

On the other hand, adding an API like Providers() seems like a good idea. If you are interested, I'm happy to review it.

@wata727
Copy link
Member

wata727 commented Jul 3, 2021

TFLint v0.30 / Plugin SDK v0.9 has been released.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants