Mirror keystore & secretstore #3411

justinsb · 2017-09-17T22:39:47Z

This allows us to have our API objects in kops-server, but our
configuration on S3 or GCS.

justinsb · 2017-09-17T22:40:07Z

Builds on #3409

k8s-github-robot · 2017-09-19T08:05:13Z

@justinsb PR needs rebase

k8s-github-robot · 2017-09-23T18:40:27Z

@justinsb PR needs rebase

This allows us to have our API objects in kops-server, but our configuration on S3 or GCS.

chrislovecnm

Lots of questions. How are we keeping these in sync? What is the basic premise you are using?

chrislovecnm · 2017-09-30T22:58:04Z

cmd/kops/create_cluster.go

@@ -225,6 +228,9 @@ func NewCmdCreateCluster(f *util.Factory, out io.Writer) *cobra.Command {
 	cmd.Flags().StringVar(&options.Target, "target", options.Target, "Target - direct, terraform, cloudformation")
 	cmd.Flags().StringVar(&options.Models, "model", options.Models, "Models to apply (separate multiple models with commas)")

+	// Configuration / state location
+	cmd.Flags().StringVar(&options.ConfigBase, "config-base", options.ConfigBase, "The location where the cluster state files should be stored")


How does this differ from the state store? I am concerned a n00b would be confused.

From Slack

@justinsb [5:02 PM]
So if you set a different config-base, you can have the objects that the cluster reads be different from the state store

Please fix this

I think I'm actually going to feature-flag it, and probably tie to it kops-server initially.

(I also reworked the description a little, but it's a tricky concept)

chrislovecnm · 2017-09-30T23:00:21Z

pkg/client/simple/vfsclientset/clientset.go

+	return fi.NewVFSCAStore(basedir), nil
+}
+
+func DeleteAllClusterState(basePath vfs.Path) error {


I will need to test this in a subfolder state store without list perms. I will open an issue for me.

This function was moved from pkg/apis/kops/registry

chrislovecnm · 2017-09-30T23:03:13Z

pkg/client/simple/vfsclientset/clientset.go

+			continue
+		}
+
+		return fmt.Errorf("refusing to delete: unknown file found: %s", path)


return fmt.Errorf("refusing to delete: unknown file found: %s", path)

how about

return fmt.Errorf("unknown file found %q, not deleting: %q", path, basePath)

This is the existing code, the point is that we do a kops delete cluster and we refuse to delete anything, because we can't prove it safe. That's what the first message is trying to imply (and I think the proposed alternative does not capture)

the current message makes sense 👍

Disregard my comment

chrislovecnm · 2017-09-30T23:04:46Z

pkg/client/simple/api/clientset.go

+// FederationsFor implements the FederationsFor method of Clientset for a kubernetes-API state store
+func (c *RESTClientset) FederationsFor(federation *kops.Federation) kopsinternalversion.FederationInterface {
+	// Unsure if this should be namespaced or not - probably, so that we can RBAC it...
+	panic("Federations are currently not supported by the server API")


panic ouch. We do not do that in many places. You want the server to restart?

It's a panic because there's no error code here, and because we don't expect people to be using Federations. I'd say panic is appropriate - our code should be crash-safe.

chrislovecnm · 2017-09-30T23:07:11Z

pkg/client/simple/vfsclientset/instancegroup.go

+	defaultReadVersion := v1alpha1.SchemeGroupVersion.WithKind(kind)
+	r.defaultReadVersion = &defaultReadVersion
+	r.validate = func(o runtime.Object) error {
+		return validation.ValidateInstanceGroup(o.(*kops.InstanceGroup))


Should we test if o.(*kops.InstanceGroup) is a *kops.InstanceGroup?

No, it would be a bug, and panic-ing is appropriate here

chrislovecnm · 2017-09-30T23:10:56Z

upup/pkg/fi/fitasks/mirrorkeystore.go

+	return fi.DefaultDeltaRunMethod(e, c)
+}
+
+func (s *MirrorKeystore) CheckChanges(a, e, changes *MirrorKeystore) error {


Should we code comments?

I can put the normal CheckChanges implements Task::CheckChanges.

chrislovecnm · 2017-09-30T23:11:29Z

upup/pkg/fi/fitasks/mirrorsecrets.go

+}
+
+func (e *MirrorSecrets) Find(c *fi.Context) (*MirrorSecrets, error) {
+	// TODO:


Will expand

chrislovecnm · 2017-09-30T23:12:19Z

upup/pkg/fi/secrets.go

@@ -36,8 +36,11 @@ type SecretStore interface {
 	// ListSecrets lists the ids of all known secrets
 	ListSecrets() ([]string, error)

-	// VFSPath returns the path where the SecretStore is stored
-	VFSPath() vfs.Path
+	//// VFSPath returns the path where the SecretStore is stored


remove commented code please

chrislovecnm · 2017-09-30T23:12:49Z

upup/pkg/fi/secrets/clientset_secretstore.go

+
+		primary := fi.FindPrimary(keyset)
+		if primary == nil {
+			glog.Warningf("skipping secret with no primary data: %s", keyset.Name)


How would we have no primary data?

It would be unexpected

Then go boom please ... fmt.Error or panic

chrislovecnm · 2017-09-30T23:13:36Z

upup/pkg/fi/secrets/vfs_secretstore.go

+
+	return vfs.CopyTree(c.basedir, basedir)
+	//
+	//files, err := c.basedir.ReadDir()


Can we kill commented code please?

chrislovecnm · 2017-09-30T23:15:29Z

If you could take a look at the err's that are not returned with fmt please. We have ALOT in this code.

chrislovecnm

Comments about fmt

chrislovecnm · 2017-09-30T23:25:19Z

pkg/client/simple/vfsclientset/clientset.go

+	for _, path := range paths {
+		relativePath, err := vfs.RelativePath(basePath, path)
+		if err != nil {
+			return err


we may want to fmt.Errorf here as the user may not know what vfs is actually doing here.

chrislovecnm · 2017-09-30T23:26:06Z

upup/pkg/fi/clientset_castore.go

+
+		primary := FindPrimary(keyset)
+		if primary == nil {
+			glog.Warningf("skipping keyset with no primary data: %s", keyset.Name)


Should we fail here? What is the use case?

Good call - just being tolerant, but we don't expect this. If we see it happening for some reason (it shouldn't), then we can downgrade to a warning.

chrislovecnm · 2017-09-30T23:30:12Z

util/pkg/vfs/vfssync.go

+	for _, srcFile := range srcFiles {
+		relativePath, err := RelativePath(src, srcFile)
+		if err != nil {
+			return err


I cannot find RelativePath in your source tree. github search is not being kind. You call if it would be helpful to fmt.Errorf more in here.

justinsb · 2017-09-30T23:36:49Z

How are we keeping these in sync?

The MirrorKeystore and MirrorSecrets tasks copy them, to keep these in sync.

justinsb · 2017-09-30T23:40:25Z

RelativePath is here:

kops/util/pkg/vfs/vfs.go

Lines 69 to 82 in 8585f00

    
           func RelativePath(base Path, child Path) (string, error) { 
        
           	basePath := base.Path() 
        
           	childPath := child.Path() 
        
           	if !strings.HasSuffix(basePath, "/") { 
        
           		basePath += "/" 
        
           	} 
        
           	if !strings.HasPrefix(childPath, basePath) { 
        
           		return "", fmt.Errorf("Path %q is not a child of %q", child, base) 
        
           	} 
        
           	relativePath := childPath[len(basePath):] 
        
           	return relativePath, nil 
        
           }

My rules of thumb for when to fmt.Errof:

If we're calling our function, we don't need fmt.Errorf - we should return good errors
If we're calling a library function we probably should fmt.Errorf - we shouldn't assume anyone else returns good errors
If we have extra context, we can fmt.Errorf (even on our code)
Because of the lack of stack-traces on errors, sometimes we do have to leave breadcrumbs with an otherwise unnecessary fmt.Errorf. Probably more when we're calling into functions that are complicated, than a simpler function like RelativePath.

chrislovecnm · 2017-09-30T23:42:56Z

I am copying what you wrote and putting in in an issue, we probably should have that in the developer guide. I do a bit more cause we don't have stack exceptions. Or log the error.

justinsb · 2017-10-01T03:02:59Z

Put in fixes as suggested - thanks!

chrislovecnm · 2017-10-01T03:12:04Z

/lgtm

k8s-github-robot · 2017-10-01T03:14:00Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chrislovecnm

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~OWNERS~~ [chrislovecnm]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-10-01T05:32:13Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2017-10-01T06:08:33Z

Automatic merge from submit-queue.

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 17, 2017

justinsb force-pushed the mirror_stores branch 3 times, most recently from be3755e to 0636e65 Compare September 18, 2017 03:46

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 19, 2017

justinsb force-pushed the mirror_stores branch from 0636e65 to d83dbf0 Compare September 23, 2017 03:41

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 23, 2017

justinsb force-pushed the mirror_stores branch from d83dbf0 to 37c3162 Compare September 23, 2017 18:02

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 23, 2017

justinsb force-pushed the mirror_stores branch 2 times, most recently from bc8b5e8 to 555bdd0 Compare September 24, 2017 01:15

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 24, 2017

justinsb force-pushed the mirror_stores branch from 555bdd0 to 9bea210 Compare September 24, 2017 03:04

justinsb added 2 commits September 24, 2017 00:09

Mirror keystore & secretstore

559d885

This allows us to have our API objects in kops-server, but our configuration on S3 or GCS.

Fix tests: create helper function for PopulateClusterSpec

e08dfc6

justinsb force-pushed the mirror_stores branch from 9bea210 to e08dfc6 Compare September 24, 2017 04:09

chrislovecnm suggested changes Sep 30, 2017

View reviewed changes

chrislovecnm reviewed Sep 30, 2017

View reviewed changes

Mirroring - Fixes from code review

a4a58d2

justinsb force-pushed the mirror_stores branch from fac6e76 to a4a58d2 Compare October 1, 2017 02:05

chrislovecnm approved these changes Oct 1, 2017

View reviewed changes

k8s-ci-robot assigned chrislovecnm Oct 1, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 1, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 1, 2017

k8s-github-robot merged commit 0905e71 into kubernetes:master Oct 1, 2017

Mirror keystore & secretstore #3411

Mirror keystore & secretstore #3411

Conversation

justinsb commented Sep 17, 2017

justinsb commented Sep 17, 2017

k8s-github-robot commented Sep 19, 2017

k8s-github-robot commented Sep 23, 2017

chrislovecnm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinsb Sep 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrislovecnm commented Sep 30, 2017

chrislovecnm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinsb commented Sep 30, 2017 • edited Loading

justinsb commented Sep 30, 2017 • edited Loading

chrislovecnm commented Sep 30, 2017

justinsb commented Oct 1, 2017

chrislovecnm commented Oct 1, 2017

k8s-github-robot commented Oct 1, 2017

k8s-github-robot commented Oct 1, 2017

k8s-github-robot commented Oct 1, 2017

justinsb Sep 30, 2017 •

edited

Loading

justinsb commented Sep 30, 2017 •

edited

Loading

justinsb commented Sep 30, 2017 •

edited

Loading