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

Panic when calling LoadModuleFromFile #61

Open
zzxwill opened this issue Apr 18, 2021 · 3 comments
Open

Panic when calling LoadModuleFromFile #61

zzxwill opened this issue Apr 18, 2021 · 3 comments

Comments

@zzxwill
Copy link

zzxwill commented Apr 18, 2021

Terraform Configuration rds.tf is as below.

module "rds" {
  source = "terraform-alicloud-modules/rds/alicloud"
  engine = "MySQL"
  engine_version = "8.0"
  instance_type = "rds.mysql.c1.large"
  instance_storage = "20"
  instance_name = var.instance_name
  account_name = var.account_name
  password = var.password
}

output "DB_NAME" {
  value = module.rds.this_db_instance_name
}
output "DB_USER" {
  value = module.rds.this_db_database_account
}
output "DB_PORT" {
  value = module.rds.this_db_instance_port
}
output "DB_HOST" {
  value = module.rds.this_db_instance_connection_string
}
output "DB_PASSWORD" {
  value = module.rds.this_db_instance_port
}


variable "instance_name" {
  default = "poc"
  type = string
}

variable "account_name" {
  default = "oam"
}

variable "password" {
  default = "Xyfff83jfewGGfaked"
}

Try to get all variable information but panic happended.

func main()  {
	p := hclparse.NewParser()
	hclFile, _ := p.ParseHCLFile("./rds.tf")
	mod := tfconfig.Module{Variables: map[string]*tfconfig.Variable{}}
	tfconfig.LoadModuleFromFile(hclFile, &mod)
	for _, v := range mod.Variables {
		fmt.Println(v.Name, v.Type)
	}
}

It seems that all fields of tfconfig.Module has to be set even though I just variable from it, or panic will happen.

panic: assignment to entry in nil map

goroutine 1 [running]:
github.com/hashicorp/terraform-config-inspect/tfconfig.LoadModuleFromFile(0xc00010bd00, 0xc0001b7f00, 0x0, 0x0, 0x0)
	/Users/zhouzhengxi/go/pkg/mod/github.com/hashicorp/terraform-config-inspect@v0.0.0-20210318070130-9a80970d6b34/tfconfig/load_hcl.go:336 +0x1d12
main.main()
	/Users/zhouzhengxi/Programming/golang/src/github.com/zzxwill/try-cloudnative/homework/hcl.go:66 +0x246

zzxwill added a commit to zzxwill/terraform-config-inspect that referenced this issue Apr 18, 2021
In function ParseHCL, not all fields of tfconfig.Module is checked,
so panic will happen.

Fix #hashicorp#61
zzxwill added a commit to zzxwill/terraform-config-inspect that referenced this issue Apr 18, 2021
In function ParseHCL, not all fields of tfconfig.Module is checked,
so panic will happen.

Fix #hashicorp#61
@radeksimko
Copy link
Member

If you replace this

mod := tfconfig.Module{Variables: map[string]*tfconfig.Variable{}}

with

mod := tfconfig.NewModule()

then your example should not panic.

@zzxwill
Copy link
Author

zzxwill commented Apr 20, 2021

@radeksimko Thanks for the reply. But there is no path for .tf files as the configuration file is from an API and I don't want to write it to a local path in order to speed the process.

I think nil checking won't do anything bad in the PR, right?

func NewModule(path string) *Module {

@radeksimko
Copy link
Member

I don't know how does your API work, but I believe that LoadModuleFromFilesystem(fs, dir) is the function you are looking for.

Assuming that you can expose the directory through that interface you should be able to make it work:

// FS represents a minimal filesystem implementation
// See io/fs.FS in http://golang.org/s/draft-iofs-design
type FS interface {
Open(name string) (File, error)
ReadFile(name string) ([]byte, error)
ReadDir(dirname string) ([]os.FileInfo, error)
}
// File represents an open file in FS
// See io/fs.File in http://golang.org/s/draft-iofs-design
type File interface {
Stat() (os.FileInfo, error)
Read([]byte) (int, error)
Close() error
}

The FS interface is basically a superset of the Go 1.16 io/fs.FS and File is an exact copy of io/fs.File. This is partially because these interfaces were introduced prior to Go 1.16 being released.

We use that exact function inside Terraform LS where files (and their content) also come via API.

I think nil checking won't do anything bad in the PR, right?

AFAICT with the break statements as-is it would effectively prevent parsing anything when the Module has uninitialised fields - which - again should not happen as long as you use that constructor. I admit that panic is not ideal, but IMO it's a better side effect than silently not parsing anything.

In Go constructors usually follow a particular naming convention - so I'd expect people to look for New* functions before attempting to create the struct on their own and running into similar problems. I'm not aware of any decent/easy way of communicating that convention however.

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

No branches or pull requests

2 participants