add cross domain scoping #1118

auhlig · 2019-05-06T13:41:57Z

As described in #1117

povilasv

I like the idea and changes 🥇

Please add changelog entry.

Also I think we should add:

logging in info level that we are using new config.
logging in warn level when we are using deprecated config.

povilasv · 2019-05-07T06:36:54Z

pkg/objstore/swift/swift.go

@@ -12,6 +12,7 @@ import (
 	"time"

 	"github.com/improbable-eng/thanos/pkg/objstore"
+	yaml "gopkg.in/yaml.v2"


please remove the blank line after this one

ran goimports again

goimports doesn't solve the blank line issue, please remove the blank line after this

i meant removed blank line and did the goimports again

povilasv · 2019-05-07T06:38:29Z

pkg/objstore/swift/swift.go

+	// Support for cross domain scoping (user in different domain than project)
+	// If a userDomainName or userDomainID is given, a user is scoped to this domain
+	// and the tenant (aka project) is expected to be in the domain given by domainName or domainID.
+	if sc.UserDomainName != "" {


could we use switch here instead

switch { case sc.UserDomaiName != "": ... case sc.UserDomainID != "": }

right. done

povilasv · 2019-05-07T06:38:48Z

pkg/objstore/swift/swift.go

+		}
+	}
+	if authOpts.Scope != nil {
+		if sc.TenantName != "" {


could we use switch here.

povilasv · 2019-05-07T11:53:58Z

@auhlig you just commented, but didn't actually push the changes

auhlig · 2019-05-07T15:40:03Z

@povilasv github was down this morning -.-
i ran make docs and updated the code slightly: default to project_id|project_name. tenant_id and tenant_name is marked as deprecated but still respected and could be removed easily (if desired at all; even gophercloud still struggles to use the terminology consequently).
cross-domain authentication is only supported if one provides a project via id or name.

also extended the get_env part used when testing.

tested in our clusters using the configuration:

auth_url: ..
username: ..
user_domain_name: ...
password: ...
project_name: ..
project_domain_name: ...
region_name: ...
container_name: ...

are you ok with this?

povilasv · 2019-05-07T16:26:47Z

pkg/objstore/swift/swift.go

+		level.Warn(logger).Log("msg", "usage of tenant_id is deprecated. use project_id instead")
+		authOpts.TenantID = sc.TenantID
+	case sc.ProjectName != "" || sc.ProjectID != "":
+		level.Info(logger).Log("msg", "using project_id or project_name")


I think we should remove this, it isn't a good message and IMO doesn't add any value

right. removed

povilasv · 2019-05-07T16:27:14Z

pkg/objstore/swift/swift.go

+		}
+	}
+	if authOpts.Scope != nil {
+		level.Info(logger).Log("msg", "scoping to domain, project")


same here let's not log this message.

povilasv · 2019-05-07T16:28:48Z

pkg/objstore/swift/swift.go


 		// Allow Gophercloud to re-authenticate automatically.
 		AllowReauth: true,
 	}

+	// The term `tenant` is used in the deprecated OpenStack Identity v2.
+	// In Identity v3 the term `project` is used instead.
+	switch {


could you also add a couple of examples how or when to configure project id vs project name in the docs/storage.md file

updated docs. added link to official documentation with examples

auhlig · 2019-05-09T16:56:15Z

LGTY @povilasv?

povilasv · 2019-05-10T11:28:29Z

@auhlig Sort of yes. Did you look at the tests? Would be amazing if we could add some proper testing here, so that when we change code we don't break things.

bwplotka

One suggestion - what do you think about using yaml.UnmarshalStrict in client.go instead leaving old fields behind? It will simplify flow and documentation (:

After that is addressed LGTM (:

bwplotka · 2019-05-10T11:36:33Z

pkg/objstore/swift/swift.go

+	RegionName        string `yaml:"region_name"`
+	ContainerName     string `yaml:"container_name"`
+
+	// Deprecated: Please use `project_id` instead.


I would maybe kill this and use yaml.UmarshalStrict instead? (: We already documented the change so let's fail fast on umarshalling! (:

sounds good. done

bwplotka · 2019-05-10T11:36:52Z

pkg/objstore/swift/swift.go

+	// The term `tenant` is used in the deprecated OpenStack Identity v2.
+	// In Identity v3 the term `project` is used instead.
+	switch {
+	case sc.TenantName != "":


let's use yaml.UnmarshalStrict instead?

bwplotka · 2019-05-10T11:37:00Z

pkg/objstore/swift/swift.go

+		UserDomainName:    os.Getenv("OS_USER_DOMAIN_NAME"),
+		ProjectDomainID:   os.Getenv("OS_PROJET_DOMAIN_ID"),
+		ProjectDomainName: os.Getenv("OS_PROJECT_DOMAIN_NAME"),
+		TenantID:          os.Getenv("OS_TENANT_ID"),


auhlig · 2019-05-13T15:38:28Z

Made this a breaking change by dropping tenant_id, tenant_name.
Extracted logic to parseConfig, authOptsFromConfig for easier unit testing.
Added unit tests.

LGTY @povilasv @bwplotka

auhlig · 2019-05-13T15:49:32Z

Netlify doesn't seem to be in the mood to run the tests. CircleCI passed however.

auhlig · 2019-05-13T15:53:26Z

squashed into single commit and force pushed.
giving netlify another chance by closing/re-opening PR

Signed-off-by: Arno Uhlig <arno.uhlig@sap.com>

auhlig · 2019-05-17T12:38:58Z

Updated the changelog after some merge conflicts. Can this be merged @povilasv @bwplotka?

GiedriusS · 2019-05-18T16:22:27Z

@auhlig so essentially with this PR we are getting rid of the support of the V2 API? I'm a bit anxious about this since, for example, some companies might still be running old versions of that API (cough not gonna point any fingers). Would it be hard to add some kind of backward compatibility?

auhlig · 2019-05-18T19:02:57Z

@GiedriusS This PR does not drop Identity v2 support. It's still possible to authenticate using project_id or project_name instead of the deprecated (since ~2013) terms tenant_id, tenant_name.
We only fix the terminology ( tenant became project with Identity v3) and add support for for authentication if a user is in a different domain than the project.
In summary, the only breaking change is the terminology.

povilasv · 2019-05-29T12:31:46Z

Ok so this does include a breaking change
tenantID, tenantName -> projectID, projectName.

I think in order not to break users, just fill tenantID to projectID and tenantName to projectName.

We try to avoid breaking changes when possible and in this case it's definitely doable

bwplotka

LGTM, thanks!

I think we noted verbosely that those fields changed, and we will fail fast so this is good IMO.

bwplotka · 2019-05-30T13:44:06Z

@povilasv I think breaking here is better that having inconsistent configuration -> users will be immdiately notified by verbose error message.

bwplotka · 2019-05-30T13:45:05Z

Waiting for @povilasv final LGTM to merge (:

auhlig · 2019-06-06T13:19:18Z

Thank you guys 🎉

povilasv requested changes May 7, 2019

View reviewed changes

auhlig force-pushed the cross-domain branch from 8378856 to 5b219e9 Compare May 7, 2019 15:38

auhlig force-pushed the cross-domain branch from 5b219e9 to e2e3b13 Compare May 7, 2019 15:45

povilasv reviewed May 7, 2019

View reviewed changes

bwplotka approved these changes May 10, 2019

View reviewed changes

auhlig force-pushed the cross-domain branch 2 times, most recently from 6253d02 to a680c93 Compare May 13, 2019 15:52

auhlig closed this May 13, 2019

auhlig reopened this May 13, 2019

add cross domain scoping

b21a62f

Signed-off-by: Arno Uhlig <arno.uhlig@sap.com>

auhlig force-pushed the cross-domain branch from a680c93 to b21a62f Compare May 17, 2019 12:37

bwplotka approved these changes May 30, 2019

View reviewed changes

povilasv approved these changes May 30, 2019

View reviewed changes

bwplotka merged commit 69782ff into thanos-io:master May 30, 2019

GiedriusS mentioned this pull request Jun 6, 2019

OpenStack cross domain authentication #1117

Closed

add cross domain scoping #1118

add cross domain scoping #1118

Conversation

auhlig commented May 6, 2019

povilasv left a comment • 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

povilasv commented May 7, 2019

auhlig commented May 7, 2019 • 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

auhlig commented May 9, 2019

povilasv commented May 10, 2019

bwplotka 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

auhlig commented May 13, 2019

auhlig commented May 13, 2019

auhlig commented May 13, 2019 • edited Loading

auhlig commented May 17, 2019

GiedriusS commented May 18, 2019

auhlig commented May 18, 2019 • edited Loading

povilasv commented May 29, 2019

bwplotka left a comment • edited Loading

Choose a reason for hiding this comment

bwplotka commented May 30, 2019

bwplotka commented May 30, 2019

auhlig commented Jun 6, 2019

povilasv left a comment •

edited

Loading

auhlig commented May 7, 2019 •

edited

Loading

auhlig commented May 13, 2019 •

edited

Loading

auhlig commented May 18, 2019 •

edited

Loading

bwplotka left a comment •

edited

Loading