Allow to set Supplemental Groups or fsGroup for the registry via the command line by mfojtik · Pull Request #12951 · openshift/origin

mfojtik · 2017-02-14T13:48:31Z

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1420541

php-coder · 2017-02-14T13:59:40Z

pkg/cmd/admin/registry/registry.go

 	cmd.Flags().StringVar(&cfg.ServingCertPath, "tls-certificate", cfg.ServingCertPath, "An optional path to a PEM encoded certificate (which may contain the private key) for serving over TLS")
 	cmd.Flags().StringVar(&cfg.ServingKeyPath, "tls-key", cfg.ServingKeyPath, "An optional path to a PEM encoded private key for serving over TLS")
+	cmd.Flags().StringSliceVar(&cfg.SupplementalGroupRange, "supplemental-groups", cfg.SupplementalGroupRange, "Specify supplemental groups which is an array of ID's that grants group access to registry shared storage")
+	cmd.Flags().StringVar(&cfg.FSGroup, "fs-group", "", "Specify fsGroup which is an ID's that grants group access to registry block storage")


php-coder · 2017-02-14T14:01:29Z

pkg/cmd/admin/registry/registry.go

+		}
+	}
+	if len(opts.Config.SupplementalGroupRange) > 0 && len(opts.Config.FSGroup) > 0 {
+		return kcmdutil.UsageError(cmd, "fsGroup and supplemental groups cannot be specified both at the same time")


I don't see a reason of why they cannot be specified both. Would be interesting to know.

@php-coder I think you either want block storage or shared for registry volume, not both.

php-coder · 2017-02-14T14:08:59Z

pkg/cmd/admin/registry/registry.go

+				// Complete().
+				continue
+			}
+			result.SupplementalGroups = append(result.SupplementalGroups, groupID)


BTW to be consistent with the code below and to reduce lines of code this can be re-written to:

if groupID, err := strconv.ParseInt(val, 10, 64); err == nil { result.SupplementalGroups = append(result.SupplementalGroups, groupID) }

php-coder · 2017-02-14T14:17:33Z

pkg/cmd/admin/registry/registry.go

 	}

+	if len(opts.Config.FSGroup) > 0 {
+		if val, err := strconv.ParseInt(opts.Config.FSGroup, 10, 64); err != nil || val == 0 {


Looks like this allows the values less than zero. Perhaps uint32 is more suitable in this case.

negative values will be refused by PodSpec validation, actually having this to be set to 0 might be valid case (you want root group?).

php-coder · 2017-02-14T14:23:04Z

pkg/cmd/admin/registry/registry.go

 	cmd.Flags().StringVar(&cfg.Selector, "selector", cfg.Selector, "Selector used to filter nodes on deployment. Used to run registries on a specific set of nodes.")
 	cmd.Flags().StringVar(&cfg.ServingCertPath, "tls-certificate", cfg.ServingCertPath, "An optional path to a PEM encoded certificate (which may contain the private key) for serving over TLS")
 	cmd.Flags().StringVar(&cfg.ServingKeyPath, "tls-key", cfg.ServingKeyPath, "An optional path to a PEM encoded private key for serving over TLS")
+	cmd.Flags().StringSliceVar(&cfg.SupplementalGroupRange, "supplemental-groups", cfg.SupplementalGroupRange, "Specify supplemental groups which is an array of ID's that grants group access to registry shared storage")


Range in the name is a bit confusing. As far I can see, it's just a list and not range like 10-15 or 10/5.

The code for parsing fsGroup/supplementalGroups from the openshift.io/sa.scc.supplemental-groups annotation that actually support ranges could be found here:

origin/pkg/security/scc/matcher.go

Lines 334 to 393 in bd3e362

// getPreallocatedFSGroup gets the annotated value from the namespace.

func getPreallocatedFSGroup(ns *kapi.Namespace) ([]kapi.IDRange, error) {

groups, err := getSupplementalGroupsAnnotation(ns)

if err != nil {

return nil, err

}

glog.V(4).Infof("got preallocated value for groups: %s in namespace %s", groups, ns.Name)

blocks, err := parseSupplementalGroupAnnotation(groups)

if err != nil {

return nil, err

}

return []kapi.IDRange{

{

Min: int64(blocks[0].Start),

Max: int64(blocks[0].Start),

},

}, nil

}

// getPreallocatedSupplementalGroups gets the annotated value from the namespace.

func getPreallocatedSupplementalGroups(ns *kapi.Namespace) ([]kapi.IDRange, error) {

groups, err := getSupplementalGroupsAnnotation(ns)

if err != nil {

return nil, err

}

glog.V(4).Infof("got preallocated value for groups: %s in namespace %s", groups, ns.Name)

blocks, err := parseSupplementalGroupAnnotation(groups)

if err != nil {

return nil, err

}

idRanges := []kapi.IDRange{}

for _, block := range blocks {

rng := kapi.IDRange{

Min: int64(block.Start),

Max: int64(block.End),

}

idRanges = append(idRanges, rng)

}

return idRanges, nil

}

// parseSupplementalGroupAnnotation parses the group annotation into blocks.

func parseSupplementalGroupAnnotation(groups string) ([]uid.Block, error) {

blocks := []uid.Block{}

segments := strings.Split(groups, ",")

for _, segment := range segments {

block, err := uid.ParseBlock(segment)

if err != nil {

return nil, err

}

blocks = append(blocks, block)

}

if len(blocks) == 0 {

return nil, fmt.Errorf("no blocks parsed from annotation %s", groups)

}

return blocks, nil

}

origin/pkg/security/uid/uid.go

Lines 18 to 40 in bd3e362

func ParseBlock(in string) (Block, error) {

if strings.Contains(in, "/") {

var start, size uint32

n, err := fmt.Sscanf(in, "%d/%d", &start, &size)

if err != nil {

return Block{}, err

}

if n != 2 {

return Block{}, ErrBlockSlashBadFormat

}

return Block{Start: start, End: start + size - 1}, nil

}

var start, end uint32

n, err := fmt.Sscanf(in, "%d-%d", &start, &end)

if err != nil {

return Block{}, err

}

if n != 2 {

return Block{}, ErrBlockDashBadFormat

}

return Block{Start: start, End: end}, nil

}

yeah, the name is wrong, shold be just --supplemental-groups=[]

mfojtik · 2017-02-14T20:42:14Z

@php-coder comments addressed, PTAL.

php-coder · 2017-02-15T15:39:19Z

LGTM (and Travis says that you have to re-generate docs).

mfojtik · 2017-04-04T11:36:12Z

[merge]

openshift-bot · 2017-04-04T11:37:32Z

Evaluated for origin merge up to ef3fc41

openshift-bot · 2017-04-04T11:37:33Z

[Test]ing while waiting on the merge queue

openshift-bot · 2017-04-04T11:45:30Z

Evaluated for origin test up to ef3fc41

openshift-bot · 2017-04-04T12:59:43Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/549/) (Base Commit: f0670df)

openshift-bot · 2017-04-05T04:05:25Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/549/) (Base Commit: 83a6089) (Image: devenv-rhel7_6118)

mfojtik · 2017-04-24T08:52:44Z

(0) Supplemental Group and fsGroup Support With Registry Install

php-coder reviewed Feb 14, 2017

View reviewed changes

mfojtik force-pushed the registry-groups branch 2 times, most recently from 3d69ad6 to a917cb1 Compare February 14, 2017 20:41

mfojtik force-pushed the registry-groups branch from a917cb1 to 7ea4f72 Compare February 14, 2017 21:02

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 26, 2017

registry: add --fs-group and --supplementary-groups to oc adm registry

ef3fc41

mfojtik force-pushed the registry-groups branch from 7ea4f72 to ef3fc41 Compare April 4, 2017 11:23

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 4, 2017

openshift-bot merged commit 7cb8747 into openshift:master Apr 5, 2017

mfojtik deleted the registry-groups branch September 5, 2018 21:08

	// getPreallocatedFSGroup gets the annotated value from the namespace.
	func getPreallocatedFSGroup(ns *kapi.Namespace) ([]kapi.IDRange, error) {
	groups, err := getSupplementalGroupsAnnotation(ns)
	if err != nil {
	return nil, err
	}
	glog.V(4).Infof("got preallocated value for groups: %s in namespace %s", groups, ns.Name)

	blocks, err := parseSupplementalGroupAnnotation(groups)
	if err != nil {
	return nil, err
	}
	return []kapi.IDRange{
	{
	Min: int64(blocks[0].Start),
	Max: int64(blocks[0].Start),
	},
	}, nil
	}

	// getPreallocatedSupplementalGroups gets the annotated value from the namespace.
	func getPreallocatedSupplementalGroups(ns *kapi.Namespace) ([]kapi.IDRange, error) {
	groups, err := getSupplementalGroupsAnnotation(ns)
	if err != nil {
	return nil, err
	}
	glog.V(4).Infof("got preallocated value for groups: %s in namespace %s", groups, ns.Name)

	blocks, err := parseSupplementalGroupAnnotation(groups)
	if err != nil {
	return nil, err
	}

	idRanges := []kapi.IDRange{}
	for _, block := range blocks {
	rng := kapi.IDRange{
	Min: int64(block.Start),
	Max: int64(block.End),
	}
	idRanges = append(idRanges, rng)
	}
	return idRanges, nil
	}

	// parseSupplementalGroupAnnotation parses the group annotation into blocks.
	func parseSupplementalGroupAnnotation(groups string) ([]uid.Block, error) {
	blocks := []uid.Block{}
	segments := strings.Split(groups, ",")
	for _, segment := range segments {
	block, err := uid.ParseBlock(segment)
	if err != nil {
	return nil, err
	}
	blocks = append(blocks, block)
	}
	if len(blocks) == 0 {
	return nil, fmt.Errorf("no blocks parsed from annotation %s", groups)
	}
	return blocks, nil
	}

	func ParseBlock(in string) (Block, error) {
	if strings.Contains(in, "/") {
	var start, size uint32
	n, err := fmt.Sscanf(in, "%d/%d", &start, &size)
	if err != nil {
	return Block{}, err
	}
	if n != 2 {
	return Block{}, ErrBlockSlashBadFormat
	}
	return Block{Start: start, End: start + size - 1}, nil
	}

	var start, end uint32
	n, err := fmt.Sscanf(in, "%d-%d", &start, &end)
	if err != nil {
	return Block{}, err
	}
	if n != 2 {
	return Block{}, ErrBlockDashBadFormat
	}
	return Block{Start: start, End: end}, nil
	}

Conversation

mfojtik commented Feb 14, 2017

Uh oh!

php-coder Feb 14, 2017

Choose a reason for hiding this comment

Uh oh!

php-coder Feb 14, 2017

Choose a reason for hiding this comment

Uh oh!

mfojtik Feb 14, 2017

Choose a reason for hiding this comment

Uh oh!

php-coder Feb 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

php-coder Feb 14, 2017

Choose a reason for hiding this comment

Uh oh!

mfojtik Feb 14, 2017

Choose a reason for hiding this comment

Uh oh!

php-coder Feb 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mfojtik Feb 14, 2017

Choose a reason for hiding this comment

Uh oh!

mfojtik commented Feb 14, 2017

Uh oh!

php-coder commented Feb 15, 2017

Uh oh!

mfojtik commented Apr 4, 2017

Uh oh!

openshift-bot commented Apr 4, 2017

Uh oh!

openshift-bot commented Apr 4, 2017

Uh oh!

openshift-bot commented Apr 4, 2017

Uh oh!

openshift-bot commented Apr 4, 2017

Uh oh!

openshift-bot commented Apr 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mfojtik commented Apr 24, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

php-coder Feb 14, 2017 •

edited

Loading

php-coder Feb 14, 2017 •

edited

Loading

openshift-bot commented Apr 5, 2017 •

edited

Loading