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

Fix auth for CLI #2265

Merged
merged 9 commits into from
Apr 13, 2015
Merged

Fix auth for CLI #2265

merged 9 commits into from
Apr 13, 2015

Conversation

corylanou
Copy link
Contributor

The auth for CLI was not setting credentials properly in the client package.

This PR does a couple things:

  1. Refactored the ParseConfig logic in the cmd/influxd package to be more consistent. This was a side affect of trying to trace down where configs were parsed and was just to difficult to leave the current code.
  2. Added a SetAuth to the client library to allow for resetting credentials.
  3. Changed the client library to use basic authentication via headers instead of the uri. Effectively moving from http://golang.org/pkg/net/url/#UserPassword to http://golang.org/pkg/net/http/#Request.SetBasicAuth

Fixes #2239

@corylanou
Copy link
Contributor Author

As a side note, there is no real way to write a test for this as both the cmd/influxd and cmd/influx are main packages, so I can't spin up a server in memory to test it. I'm open to other ideas if anyone has them.

@corylanou
Copy link
Contributor Author

@otoolep previously, we had just NewConfig which wasn't specific enough between what we wanted for true server defaults, vs what we wanted to have to run in Demo/Test mode.

However, when we know we want a "Test" config (AKA Demo Mode), then we call that specifically. This was already happening behind the scenes before I made this change, but now you see it where it happens, which I like a lot more.

For reference, here is what we add when you ask for a test config:

func NewTestConfig() (*Config, error) {
  c := NewConfig()

  // By default, store broker and data files in current users home directory
  u, err := user.Current()
  if err != nil {
    return nil, fmt.Errorf("failed to determine current user for storage")
  }

  c.Broker.Enabled = true
  c.Broker.Dir = filepath.Join(u.HomeDir, ".influxdb/broker")

  c.Data.Enabled = true
  c.Data.Dir = filepath.Join(u.HomeDir, ".influxdb/data")

  c.Admin.Enabled = true
  c.Admin.Port = 8083

  c.Monitoring.Enabled = false
  c.Snapshot.Enabled = true

  return c, nil
}

@otoolep
Copy link
Contributor

otoolep commented Apr 13, 2015

OK, thanks @corylanou, makes sense. This is a different change, but TestConfig isn't a great name IMHO, and is what threw me. It sounds like some sort of config used for our testing, not a default config that is used is no path is supplied. It's more like NewDefaultConfig.

@otoolep
Copy link
Contributor

otoolep commented Apr 13, 2015

Otherwise change looks fine.

@corylanou
Copy link
Contributor Author

Agree on the naming. it was between NewDemoConfig or NewTestConfig. Some day we may come up with a better name.

@toddboom
Copy link
Contributor

lgtm :shipit:

// Remove it to clean up past failed panics
// Defer it to clean up for successful tests
path := tempfile()
os.Remove(path)
Copy link
Contributor

Choose a reason for hiding this comment

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

tempfile() already removes the path, and cleaning up past runs is not possible, since the path changes each time.

@otoolep
Copy link
Contributor

otoolep commented Apr 13, 2015

Looks good, just some minor feedback. Unfortunately I don't think we can clean up directories if a test panics, so that part of the change might be optimistic.

toddboom added a commit that referenced this pull request Apr 13, 2015
@toddboom toddboom merged commit 6731ba4 into master Apr 13, 2015
@toddboom toddboom deleted the cli-auth-fix-2239 branch April 13, 2015 22:36
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

Successfully merging this pull request may close these issues.

Auth does not work in CLI
3 participants