Skip to content

Users org - #26

Open
kbhonagiri16 wants to merge 1 commit into
funrun11:auth_proxyfrom
kbhonagiri16:users_org
Open

Users org#26
kbhonagiri16 wants to merge 1 commit into
funrun11:auth_proxyfrom
kbhonagiri16:users_org

Conversation

@kbhonagiri16

@kbhonagiri16 kbhonagiri16 commented Aug 3, 2017

Copy link
Copy Markdown
Contributor

Added openstack token and
create user if not created
create organization(s) if not created
give roles in organization according to data in context

@kbhonagiri16
kbhonagiri16 force-pushed the users_org branch 4 times, most recently from 8100a43 to 0042b4e Compare August 3, 2017 21:06
@funrun11
funrun11 self-requested a review August 3, 2017 21:10
@kbhonagiri16
kbhonagiri16 force-pushed the users_org branch 7 times, most recently from da05fd7 to e2f7d44 Compare August 4, 2017 19:55
Comment thread pkg/auth_proxy/openstack.go Outdated
"github.com/gophercloud/gophercloud/openstack/identity/v3/tokens"
"github.com/kbhonagiri16/visualization-client"
"github.com/shuaiming/mung/middlewares"
"github.com/spf13/viper"

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I see later you are using it for reading information from config file.
Please read all your need on auth_proxy/main.go level and pass here as init arguments.

Comment thread pkg/auth_proxy/openstack.go Outdated
username := viper.GetString("openstack.username")
password := viper.GetString("openstack.password")
domain := viper.GetString("openstack.domain_name")
project := viper.GetString("openstack.project_name")

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

As I said above. Just pass all these parameters as some options model

Comment thread pkg/auth_proxy/openstack.go Outdated
)

var openstackToken = ""
var expiresAt int64

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

  1. It shouldn't be in module level, as far as it is connected to specific instance of token
  2. It will lead to concurrency issues just several go routings update the value

Please,

  1. Create structure for handling this specific problem (getting openstack token) with all methods assigned to it.
  2. Check https://golang.org/pkg/sync/ package and use it to make this code safe to be executed concurrently

Comment thread pkg/auth_proxy/openstack.go Outdated

sess.Values[SessionUsername] = username

vclient, clientErr := client.NewVisualizationClient(visualizationEndpoint, http.Client{}, openstackToken)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Client should be created once during NewOpenStackAuthHandler

Comment thread pkg/auth_proxy/openstack.go Outdated
return
}
// Get the user by its name if exists
userGet, err := vclient.GetUserName(username)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Let us move all this code below to separate method returning only GafanaUpdateCommand, error

Comment thread pkg/auth_proxy/openstack.go Outdated
http.Redirect(rw, r, "/auth/openstack", http.StatusForbidden)
}
// Create User
_, err = vclient.CreateUser(user)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

We shouldn't create any entities in visualization-api or Grafana here.
https://github.com/zaletniy/visualization-api/blob/auth_proxy/pkg/auth_proxy/visualization.go is the place for this.

The responsibility of OpenStackAuthHandler

  • check user's credentials in openstack - login to keystone
  • if passed. Create GafanaUpdateCommand and save in session

Comment thread pkg/auth_proxy/openstack.go Outdated

log.Logger.Debugf("%s is setting as '%s'", SessionUsername, username)
// Check if credentials are correct
ok, err := oh.authenticate(username, password, grafanaEndpoint)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

It is authentication to OpenStack Keystone, not Grafana

@kbhonagiri16
kbhonagiri16 force-pushed the users_org branch 2 times, most recently from ca12277 to 72e2da9 Compare August 9, 2017 20:27
@kbhonagiri16
kbhonagiri16 changed the base branch from master to auth_proxy August 10, 2017 15:08
@kbhonagiri16

Copy link
Copy Markdown
Contributor Author

@zaletniy done with suggested changes but have few CI issues will update them soon.

@kbhonagiri16
kbhonagiri16 force-pushed the users_org branch 6 times, most recently from fcecfec to ed732d9 Compare April 5, 2018 19:46
create user if not created,
give roles in organization according to data in context

Change-Id: 45485b1
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.

2 participants