WIP: [tink-server] add command line flags for tls certs #266

detiber · 2020-08-25T21:40:29Z

Description

Adds flags to tink-server for configuring tls certificates

Why is this needed

How Has This Been Tested?

I tested these changes with the existing docker-compose workflow along with a POC I've been working on using kind and cert-manager

How are existing users impacted? What migration steps/scripts do we need?

Existing users should not be impacted, but depending on discussion around the proposal, we may want to deprecate the existing certificate configuration method.

Checklist:

I have:

updated the documentation and/or roadmap (if required)
added unit or e2e tests
provided instructions on how to upgrade

thebsdbox · 2020-09-02T08:46:00Z

This looks a great first step in adding flags for pointing to file locations for certs, will this:

Come with any additional testing
Break any of the current deployments (setup.sh)

gianarb · 2020-09-02T08:59:02Z

This is a great candidate for tinkerbell/proposals#15 as soon as it is accepted :) And if we agree to keep only one behavior.

codecov · 2020-09-09T19:30:47Z

Codecov Report

Merging #266 (a5d4bd2) into master (013d202) will increase coverage by 0.49%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master     #266      +/-   ##
==========================================
+ Coverage   32.70%   33.20%   +0.49%     
==========================================
  Files          44       44              
  Lines        3137     3090      -47     
==========================================
  Hits         1026     1026              
+ Misses       2019     1972      -47     
  Partials       92       92

Impacted Files	Coverage Δ
grpc-server/grpc_server.go	`0.00% <0.00%> (ø)`
grpc-server/hardware.go	`1.57% <ø> (+0.01%)`	⬆️
http-server/http_server.go	`0.00% <0.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 60a74c7...a5d4bd2. Read the comment docs.

detiber · 2020-09-09T19:57:05Z

Not quite sure what is going on with the vagrant-setup job, GitHub Actions is complaining that it can't find the self-hosted runner and I don't have permissions that allow me to see the configuration to verify.

gianarb · 2020-09-10T11:43:16Z

Yeah the vagrant test is not supposed to run for this repo anymore but only for sandbox.
Not sure if we have to have it here as well. Related to #277

gianarb · 2020-09-10T12:02:19Z

Can you add a detailed "how to migrate" section as PR description that tells users how to migrate from a running tinkerbell (assume vagrant and setup.sh) to this new way?

Thanks a lot

detiber · 2020-10-06T18:29:52Z

cmd/tink-server/cmd/root.go

+// createViper creates a Viper object configured to read in configuration files
+// (from various paths with content type specific filename extensions) and loads
+// environment variables.
+func createViper(logger log.Logger) (*viper.Viper, error) {
+	v := viper.New()
+	v.AutomaticEnv()
+	v.SetConfigName("tink-server")
+	v.AddConfigPath("/etc/tinkerbell")
+	v.AddConfigPath(".")
+	v.SetEnvKeyReplacer(strings.NewReplacer("-", "_"))
+
+	// If a config file is found, read it in.
+	if err := v.ReadInConfig(); err != nil {
+		if _, ok := err.(viper.ConfigFileNotFoundError); !ok {
+			logger.With("configFile", v.ConfigFileUsed()).Error(err, "could not load config file")
+
+			return nil, err
+		}
+
+		logger.Info("no config file found")
+	} else {
+		logger.With("configFile", v.ConfigFileUsed()).Info("loaded config file")
+	}
+
+	return v, nil
+}
+
+func applyViper(v *viper.Viper, cmd *cobra.Command) error {
+	errors := []error{}
+
+	cmd.Flags().VisitAll(func(f *pflag.Flag) {
+		if !f.Changed && v.IsSet(f.Name) {
+			val := v.Get(f.Name)
+			if err := cmd.Flags().Set(f.Name, fmt.Sprintf("%v", val)); err != nil {
+				errors = append(errors, err)
+
+				return
+			}
+		}
+	})
+
+	if len(errors) > 0 {
+		errs := []string{}
+		for _, err := range errors {
+			errs = append(errs, err.Error())
+		}
+
+		return fmt.Errorf(strings.Join(errs, ", "))
+	}
+
+	return nil
+}


@displague I modeled this after the work you did for tink-worker, I'm thinking we can move this to a shared library that all cli tools import.

detiber · 2020-10-06T18:36:04Z

cmd/tink-server/main.go

-	"github.com/tinkerbell/tink/db"
-	rpcServer "github.com/tinkerbell/tink/grpc-server"
-	httpServer "github.com/tinkerbell/tink/http-server"
+	"github.com/tinkerbell/tink/cmd/tink-worker/cmd"


Oops, this should be tink-server, not tink-worker, will be fixing shortly.

tstromberg · 2021-06-22T15:41:35Z

@detiber - Any movement on this PR? It seems to be doing the right thing but is also getting a bit long in the tooth.

Signed-off-by: Jason DeTiberus <detiber@users.noreply.github.com>

detiber · 2021-06-23T17:55:47Z

@tstromberg thanks for the nudge, I've started to attempt to rebase it off of the current state of the repo. Can you provide any guidance on the best way to test changes now? It looks like the former vagrant test I was relying on is no longer present.

detiber · 2021-06-23T18:32:25Z

Previous lint failure was related to an existing ineffective assignment in db/mock/template.go, I've added a separate commit to address the issue. I suspect a newer version of golangci-lint being available in the github action is the reason that it just now cropped up.

detiber

Added some comments around some of the rebase related changes that I still need to sort out.

detiber · 2021-06-23T18:35:09Z

grpc-server/grpc_server.go

+	//cert []byte
+	//modT time.Time


As far as I could tell from a quick glance, these didn't actually appear to be used anywhere outside of methods that were exposed and seemingly unused in grpc-server/hardware.go

detiber · 2021-06-23T18:35:34Z

grpc-server/hardware.go

+// TODO: is this used anywhere?
+// // Cert returns the public cert that can be served to clients
+// func (s *server) Cert() []byte {
+// 	return s.cert
+// }

-// ModTime returns the modified-time of the grpc cert
-func (s *server) ModTime() time.Time {
-	return s.modT
-}
+// // ModTime returns the modified-time of the grpc cert
+// func (s *server) ModTime() time.Time {
+// 	return s.modT
+// }


As far as I could tell from a quick glance, these didn't actually appear to be used anywhere.

detiber · 2021-06-23T18:37:49Z

test-docker-compose.yml

+    command:
+      - --tls-cert=/certs/${FACILITY:-onprem}/bundle.pem
+      - --tls-key=/certs/${FACILITY:-onprem}/server-key.pem


This was previously needed for the purposes of the old vagrant test, I'm not sure if this compose file is actually used anywhere now.

detiber · 2021-06-23T18:38:53Z

deploy/docker-compose.yml

+    command:
+      - --tls-cert=/certs/${FACILITY:-onprem}/bundle.pem
+      - --tls-key=/certs/${FACILITY:-onprem}/server-key.pem


This was previously needed for the purposes of the old vagrant test, I'm not sure if this compose file is actually used anywhere now.

detiber · 2021-06-24T20:36:59Z

I've created a separate PR for the golangci-lint fixes that are unrelated to this PR here: #511

tstromberg · 2021-09-21T15:37:45Z

@detiber - would you like any help with getting this PR in?

jacobweinstock · 2021-10-05T15:37:57Z

Hey @detiber, is this still something on your radar? I'll echo Thomas, would you like any help getting this through?

jacobweinstock · 2021-10-19T15:23:14Z

Hey @detiber, please re-open this if/when needed.

detiber mentioned this pull request Aug 25, 2020

0008: Add proposal for common certificate handling tinkerbell/proposals#12

Merged

thebsdbox added the kind/feature Categorizes issue or PR as related to a new feature. label Aug 25, 2020

detiber force-pushed the tlsFlags branch 2 times, most recently from eb8c76b to 3128a23 Compare September 9, 2020 19:29

detiber force-pushed the tlsFlags branch 2 times, most recently from 5fc8a2d to e6d5bfc Compare September 9, 2020 19:38

detiber added the ci-check/vagrant-setup This label trigger a GitHub action that tests the Vagrant Setup guide https://tinkerbell.org/setup/ label Sep 9, 2020

displague mentioned this pull request Sep 9, 2020

tink worker command line args #287

Merged

3 tasks

gianarb added the breaking-change Denotes a PR that introduces potentially breaking changes that require user action. label Sep 10, 2020

detiber force-pushed the tlsFlags branch 2 times, most recently from 409c606 to 2ab3bbc Compare October 6, 2020 18:27

detiber commented Oct 6, 2020

View reviewed changes

detiber force-pushed the tlsFlags branch 2 times, most recently from aedd181 to a05327e Compare October 9, 2020 18:04

This was referenced Feb 3, 2021

Migrate tink-server to cobra and viper #431

Merged

Let's talk about TLS #432

Closed

[tink-server] add command line flags for tls certs

2a97c16

Signed-off-by: Jason DeTiberus <detiber@users.noreply.github.com>

detiber force-pushed the tlsFlags branch from a05327e to 2a97c16 Compare June 23, 2021 17:54

Fix ineffective assignment in db/mock/template.go

a7ce467

detiber commented Jun 23, 2021

View reviewed changes

detiber added 2 commits June 24, 2021 12:28

cleanup

fbc8180

Defer cert parsing and such to grpc credentials helpers

a5d4bd2

detiber mentioned this pull request Jun 24, 2021

Fix ineffective assignment in db/mock/template.go #511

Merged

detiber mentioned this pull request Jun 24, 2021

Update docker-compose.yml for tls flag changes in tink-server tinkerbell/playground#89

Closed

3 tasks

tstromberg marked this pull request as draft August 27, 2021 16:38

jacobweinstock closed this Oct 19, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: [tink-server] add command line flags for tls certs #266

WIP: [tink-server] add command line flags for tls certs #266

detiber commented Aug 25, 2020

thebsdbox commented Sep 2, 2020

gianarb commented Sep 2, 2020

codecov bot commented Sep 9, 2020 •

edited

detiber commented Sep 9, 2020

gianarb commented Sep 10, 2020

gianarb commented Sep 10, 2020

detiber Oct 6, 2020

detiber Oct 6, 2020

tstromberg commented Jun 22, 2021

detiber commented Jun 23, 2021

detiber commented Jun 23, 2021

detiber left a comment

detiber Jun 23, 2021

detiber Jun 23, 2021

detiber Jun 23, 2021

detiber Jun 23, 2021

detiber commented Jun 24, 2021

tstromberg commented Sep 21, 2021

jacobweinstock commented Oct 5, 2021

jacobweinstock commented Oct 19, 2021

WIP: [tink-server] add command line flags for tls certs #266

WIP: [tink-server] add command line flags for tls certs #266

Conversation

detiber commented Aug 25, 2020

Description

Why is this needed

How Has This Been Tested?

How are existing users impacted? What migration steps/scripts do we need?

Checklist:

thebsdbox commented Sep 2, 2020

gianarb commented Sep 2, 2020

codecov bot commented Sep 9, 2020 • edited

Codecov Report

detiber commented Sep 9, 2020

gianarb commented Sep 10, 2020

gianarb commented Sep 10, 2020

detiber Oct 6, 2020

Choose a reason for hiding this comment

detiber Oct 6, 2020

Choose a reason for hiding this comment

tstromberg commented Jun 22, 2021

detiber commented Jun 23, 2021

detiber commented Jun 23, 2021

detiber left a comment

Choose a reason for hiding this comment

detiber Jun 23, 2021

Choose a reason for hiding this comment

detiber Jun 23, 2021

Choose a reason for hiding this comment

detiber Jun 23, 2021

Choose a reason for hiding this comment

detiber Jun 23, 2021

Choose a reason for hiding this comment

detiber commented Jun 24, 2021

tstromberg commented Sep 21, 2021

jacobweinstock commented Oct 5, 2021

jacobweinstock commented Oct 19, 2021

codecov bot commented Sep 9, 2020 •

edited