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

TLS support for http and RPC #1853

Merged
merged 7 commits into from
Oct 25, 2016
Merged

TLS support for http and RPC #1853

merged 7 commits into from
Oct 25, 2016

Conversation

diptanu
Copy link
Contributor

@diptanu diptanu commented Oct 24, 2016

A subsequent PR is going to introduce support for TLS in the Nomad CLI and API client lib.

@diptanu diptanu force-pushed the f-rpc-http-tls branch 2 times, most recently from f834ea5 to 348382c Compare October 25, 2016 17:35
@@ -164,12 +165,22 @@ var (

// NewClient is used to create a new client from the given configuration
func NewClient(cfg *config.Config, consulSyncer *consul.Syncer, logger *log.Logger) (*Client, error) {
//Create the tls wrapper
Copy link
Contributor

Choose a reason for hiding this comment

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

Space after //

@@ -132,6 +133,32 @@ type Config struct {
// PublishAllocationMetrics determines whether nomad is going to publish
// allocation metrics to remote Telemetry sinks
PublishAllocationMetrics bool

// HttpTLS enables TLS for the HTTP endpoints on the clients.
HttpTLS bool
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you just embed the TLSConfig struct


// CAFile is a path to a certificate authority file. This is used with VerifyIncoming
// or VerifyOutgoing to verify the TLS connection.
CAFile string `mapstructure:"ca_file"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing CAPath

@@ -191,6 +192,29 @@ type Config struct {
// This period is meant to be long enough for a leader election to take
// place, and a small jitter is applied to avoid a thundering herd.
RPCHoldTimeout time.Duration

// Enable TLS for incoming RPC calls from Nomad clients
RpcTLS bool
Copy link
Contributor

Choose a reason for hiding this comment

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

Same thing here

@@ -0,0 +1,237 @@
package tlsutil
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be in helper pkg?

@@ -0,0 +1,17 @@
-----BEGIN CERTIFICATE-----
Copy link
Contributor

Choose a reason for hiding this comment

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

These should be in a subdirectory under the tlsutil, not at the top level

@dadgar dadgar mentioned this pull request Oct 25, 2016
@diptanu diptanu merged commit 70ec22f into master Oct 25, 2016
@diptanu diptanu deleted the f-rpc-http-tls branch October 25, 2016 23:14
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants