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.

Sign up for GitHub

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

Jump to bottom

Disallow AttributeRestrictions in PolicyRules #13466

Merged
merged 1 commit into from
Mar 21, 2017
Merged

Disallow AttributeRestrictions in PolicyRules #13466

merged 1 commit into from
Mar 21, 2017

Conversation

enj
Copy link
Contributor

@enj enj commented Mar 20, 2017

Addresses #13334 (comment)

cc @liggitt

[test]

Signed-off-by: Monis Khan [email protected]

@enj enj requested a review from deads2k March 20, 2017 16:06
@deads2k
Copy link
Contributor

deads2k commented Mar 20, 2017

@smarterclayton I want this to plug a rolling update hole before you tag a level.

deads2k
deads2k reviewed Mar 20, 2017
View reviewed changes
pkg/authorization/api/validation/validation.go
@@ -252,7 +252,13 @@ func ValidateRole(role *authorizationapi.Role, isNamespaced bool) field.ErrorLis
}

func validateRole(role *authorizationapi.Role, isNamespaced bool, fldPath *field.Path) field.ErrorList {
Copy link
Contributor

Choose a reason for hiding this comment

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

clusterrole too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Everything funnels to validateRole.

Copy link
Contributor

Choose a reason for hiding this comment

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

Everything funnels to validateRole.

So it does. update text and bug me for merge.

deads2k
deads2k reviewed Mar 20, 2017
View reviewed changes
pkg/authorization/api/helpers.go Outdated
@@ -366,6 +366,10 @@ func (r *PolicyRuleBuilder) RuleOrDie() PolicyRule {
}

func (r *PolicyRuleBuilder) Rule() (PolicyRule, error) {
if r.PolicyRule.AttributeRestrictions != nil {
return PolicyRule{}, fmt.Errorf("rule may not have attributeRestrictions as they are deprecated and ignored: %#v", r.PolicyRule)
Copy link
Contributor

Choose a reason for hiding this comment

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

because they are deprecated and ignored.

deads2k
deads2k reviewed Mar 20, 2017
View reviewed changes
pkg/authorization/api/validation/validation.go Outdated
allErrs := validation.ValidateObjectMeta(&role.ObjectMeta, isNamespaced, path.ValidatePathSegmentName, fldPath.Child("metadata"))
for i, rule := range role.Rules {
if rule.AttributeRestrictions != nil {
allErrs = append(allErrs, field.Invalid(fldPath.Child("rules").Index(i).Child("attributeRestrictions"), rule.AttributeRestrictions, "must be null as they are deprecated and ignored"))
Copy link
Contributor

Choose a reason for hiding this comment

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

because they are ignored.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, don't have to say why, just "must be null"

@enj enj mentioned this pull request Mar 20, 2017
Closed
@enj enj force-pushed the enj/f/disallow_attribute_restrictions branch from 73c2b36 to a29c8a7 Compare March 20, 2017 17:53
deads2k
deads2k reviewed Mar 20, 2017
View reviewed changes
test/extended/testdata/roles/policy-clusterroles.yaml
@@ -33,15 +33,6 @@ items:
- projects
verbs:
- list
- apiGroups: null
Copy link
Contributor

Choose a reason for hiding this comment

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

Add the replacement rule for this

deads2k
deads2k reviewed Mar 20, 2017
View reviewed changes
test/extended/testdata/roles/policy-roles.yaml
@@ -42,15 +42,6 @@ objects:
- projects
verbs:
- list
- apiGroups: null
Copy link
Contributor

Choose a reason for hiding this comment

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

Add the replacement rule for this

@enj enj force-pushed the enj/f/disallow_attribute_restrictions branch from a29c8a7 to aef39f1 Compare March 20, 2017 18:14
@enj enj force-pushed the enj/f/disallow_attribute_restrictions branch from aef39f1 to 847e3fc Compare March 20, 2017 18:33
@deads2k
Copy link
Contributor

deads2k commented Mar 20, 2017

lgtm [merge]

liggitt
liggitt reviewed Mar 20, 2017
View reviewed changes
pkg/authorization/api/validation/validation.go
allErrs = append(allErrs, field.Invalid(fldPath.Child("rules").Index(i).Child("attributeRestrictions"), rule.AttributeRestrictions, "must be null"))
}
}
return allErrs
Copy link
Contributor

Choose a reason for hiding this comment

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

this is also where I was expecting to see validation to prevent a single rule from having both resource and non-resource data. if it's not here, it means we allow it, and have to split when converting to kube roles

Copy link
Contributor

@liggitt liggitt Mar 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

we also don't require apigroups, resources, verbs, or nonresourceurls to contain items, which means our converted roles won't pass kube validation... we can either drop rules like that (since they weren't actually granting any permissions), fail the conversion, or fail when attempting to persist the converted role to kube

@deads2k
Copy link
Contributor

deads2k commented Mar 20, 2017

this is also where I was expecting to see validation to prevent a single rule from having both resource and non-resource data. if it's not here, it means we allow it, and have to split when converting to kube roles

Or we could provide an oc migrate tool in 1.6 and tighten validation in 1.7. There will just be some rules that don't sync controller-wise until you migrate. I don't see a big issue with that.

@deads2k
Copy link
Contributor

deads2k commented Mar 20, 2017

we also don't require apigroups, resources, verbs, or nonresourceurls to contain items, which means our converted roles won't pass kube validation... we can either drop rules like that (since they weren't actually granting any permissions), fail the conversion, or fail when attempting to persist the converted role to kube

Or convert and fail trying to persist it. It's ok if the converted resources don't all validate at first. If we provide oc migrate tools to fix them up and perhaps label failing resources for inspection by the 3.7 update, there's a path forward.

@mfojtik
Copy link
Contributor

mfojtik commented Mar 21, 2017

@enj seems like something cracked in the namespace finalizer

@enj
Copy link
Contributor Author

enj commented Mar 21, 2017 •
edited
Loading

Flake #13067 #11016 (and others - will open issues and tag) re[test]

This was referenced Mar 21, 2017
Closed
Closed
@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 847e3fc

@deads2k
Copy link
Contributor

deads2k commented Mar 21, 2017

re[merge]

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 847e3fc

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/368/) (Base Commit: 0343989)

@openshift-bot
Copy link
Contributor

openshift-bot commented Mar 21, 2017 •
edited
Loading

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/168/) (Base Commit: 55e0221) (Image: devenv-rhel7_6093)

@openshift-bot openshift-bot merged commit d08d28a into openshift:master Mar 21, 2017
@enj
Copy link
Contributor Author

enj commented Mar 21, 2017

@deads2k @liggitt Do I need to spawn any followup issues for this?

@jhadvig
Copy link
Member

jhadvig commented Mar 21, 2017 •
edited
Loading

This change is breaking the upgrade process in a job that installes and upgrades origin cluster:
https://ci.openshift.redhat.com/jenkins/job/test_branch_origin_extended_conformance_install_update/88/consoleFull#-14246886155849c75fe4b0052e114d805c

Not sure if its just a flake, but I could reproduce it seweral times in a row

@liggitt
Copy link
Contributor

liggitt commented Mar 21, 2017

Are we sure that reconciliation will never try to persist a role with this attribute set in a rule ? I'm concerned about tightening validation with no announced deprecation or migration in place

@enj enj mentioned this pull request Mar 22, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liggitt liggitt liggitt left review comments

@deads2k deads2k deads2k left review comments

Assignees

@deads2k deads2k

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@enj @deads2k @mfojtik @openshift-bot @jhadvig @liggitt