Skip to content

add "ca_file" option to the elasticstack provider #35

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

Merged
merged 4 commits into from
Jan 20, 2022

Conversation

utilitynerd
Copy link

@utilitynerd utilitynerd commented Jan 11, 2022

allows connecting to an elasticsearch cluster using a custom certificate authority.

I know just enough go to get myself in trouble, but figured I would try to solve my own problem anyways. resolves #34

It adds the "ca_file" parameter to the elasticstack provider. If ca_file is set, the certificate is read and its contents are added to the CACert field in the elasticsearch.Config struct.

@utilitynerd utilitynerd requested a review from olksdr as a code owner January 11, 2022 18:20
@elasticmachine
Copy link

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@cla-checker-service
Copy link

cla-checker-service bot commented Jan 11, 2022

💚 CLA has been signed

@Kushmaro
Copy link
Contributor

Thanks so much for putting up the first community PR @utilitynerd , I have to ask though - is there a specific reason for not implementing "insecure=true" like mentioned in the issue? that would be the direction we'd like to take to resolve this one.

Specifying a root_ca file is often quite cumbersome, and doesn't add much value in TF as it does on browsers for example (where browsers will show if a cert is trusted or not)

Copy link
Contributor

@olksdr olksdr left a comment

Choose a reason for hiding this comment

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

Hey @utilitynerd,
Thanks for your contribution!

There are few more steps to get this over and get merged:

  • things we're also support the per elasticsearch-resource elasticsearch_connection block, we will need to add ca_file in there as well, see
    Elem: &schema.Resource{
    Schema: map[string]*schema.Schema{
    "username": {
    Description: "A username to use for API authentication to Elasticsearch.",
    Type: schema.TypeString,
    Optional: true,
    RequiredWith: []string{"elasticsearch_connection.0.password"},
    },
    "password": {
    Description: "A password to use for API authentication to Elasticsearch.",
    Type: schema.TypeString,
    Optional: true,
    Sensitive: true,
    RequiredWith: []string{"elasticsearch_connection.0.username"},
    },
    "endpoints": {
    Description: "A list of endpoints the Terraform provider will point to. They must include the http(s) schema and port number.",
    Type: schema.TypeList,
    Optional: true,
    Sensitive: true,
    Elem: &schema.Schema{
    Type: schema.TypeString,
    },
    },
    },
    },
    so this way we will stay consistent in all the places, and then you can update code in
    if u := conn["username"]; u != nil {
    config.Username = u.(string)
    }
    if p := conn["password"]; p != nil {
    config.Password = p.(string)
    }
    if endpoints := conn["endpoints"]; endpoints != nil {
    var addrs []string
    for _, e := range endpoints.([]interface{}) {
    addrs = append(addrs, e.(string))
    }
    config.Addresses = addrs
    }
    to read the newly added option
  • after changes are maybe, the docs must be updates as well, you can run make gen command which will re-generate the docs and adds this options as well
  • and one more step is you will have you sign the Contributor Agreement for the account you're pushing your commits from, in this case it looks like the commit is coming from https://github.com./mikejones-ucb

Once this is done we can once more go over the PR and see if there are any missing things to be added.

Thanks again for your contribution!

Severity: diag.Error,
Summary: "Unable to read CA File",
Detail: err.Error(),
})
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
})
})
return nil, diags

It would be great if we immediately return the diags here to report the error sooner.

@utilitynerd
Copy link
Author

Thanks for the feedback!

I'm not opposed to the "insecure=true" method instead of validating against a custom cert. Although maybe it would be nice to eventually have both options available.

Since it sounds like the project would prefer the insecure route, why don't I take your suggestions and see if I can put together a separate PR implementing it.

@Kushmaro
Copy link
Contributor

@utilitynerd thank you so much. We've actually discussed internally after our discussion here, and decided to allow for both options.

So again, really a lot of appreciation for these 2 PRs

@utilitynerd
Copy link
Author

utilitynerd commented Jan 18, 2022

I believe I've addressed the issues above:

  • return early if loading the ca_file results in an error
  • added the ca_file parameter to the elasticsearch_connection block
  • generated updated documentation
  • signed the contributor agreement with my other username

Let me know if there is anything else

@olksdr
Copy link
Contributor

olksdr commented Jan 19, 2022

jenkins test this please

@olksdr
Copy link
Contributor

olksdr commented Jan 19, 2022

@utilitynerd please, merge main into your branch and resolve the conflicts which are coming from #36

@olksdr olksdr added the enhancement New feature or request label Jan 19, 2022
@olksdr
Copy link
Contributor

olksdr commented Jan 20, 2022

ok to test

Copy link
Contributor

@olksdr olksdr left a comment

Choose a reason for hiding this comment

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

lgtm

@olksdr olksdr merged commit a5d7657 into elastic:main Jan 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support self-signed / internal Certificate Authorities
4 participants