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

[3.6] Verify docker access via non grouped resources #16465

Conversation

enj
Copy link
Contributor

@enj enj commented Sep 21, 2017

In the transition between 3.5 and 3.6, we should target the non grouped resources when performing SARs to confirm image access via docker. This is because a 3.5 master will only allow a SAR against the non grouped resources. Once a 3.5 master has been updated to 3.6, it will still only allow a SAR against the non grouped resources. After cluster role reconciliation is complete and the policy cache is up to date, then the upgraded masters will honor SARs against the group resources. Waiting for this to occur is not a viable solution since it breaks cluster functionality during a high availability upgrade.

Signed-off-by: Monis Khan mkhan@redhat.com

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1493213
Fixes #16380

/assign @simo5 @tiran @legionus @mfojtik

@openshift/sig-security

In the transition between 3.5 and 3.6, we should target the non
grouped resources when performing SARs to confirm image access via
docker.  This is because a 3.5 master will only allow a SAR against
the non grouped resources.  Once a 3.5 master has been updated to
3.6, it will still only allow a SAR against the non grouped
resources.  After cluster role reconciliation is complete and the
policy cache is up to date, then the upgraded masters will honor
SARs against the group resources.  Waiting for this to occur is not
a viable solution since it breaks cluster functionality during a
high availability upgrade.

Signed-off-by: Monis Khan <mkhan@redhat.com>

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1493213
@openshift-ci-robot openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Sep 21, 2017
@enj enj changed the title Verify docker access via non grouped resources [3.6] Verify docker access via non grouped resources Sep 21, 2017
@enj
Copy link
Contributor Author

enj commented Sep 21, 2017

This error message from #16380 (comment) seems relevant:

OpenShift access denied: User "system:serviceaccount:docker-test:default" cannot get imagestreams/layers.image.openshift.io in project "openshift"

@rezie you mentioned that you reconciled after the upgrade. Can you provide specifics on how you did this and the YAML for the system:image-puller cluster role? That plus an answer to #16380 (comment) should give us a better idea of if this change will fix the issue. I am doubtful that the policy cache was out of date, but it can lag behind if the cluster is busy.

@rezie
Copy link

rezie commented Sep 21, 2017

Sure - the commands I ran were taken directly from the manual upgrade guide.

oadm policy reconcile-cluster-roles \
    --additive-only=true \
    --confirm
oadm policy reconcile-cluster-role-bindings \
    --exclude-groups=system:authenticated \
    --exclude-groups=system:authenticated:oauth \
    --exclude-groups=system:unauthenticated \
    --exclude-users=system:anonymous \
    --additive-only=true \
    --confirm
oadm policy reconcile-cluster-role-bindings \
    system:build-strategy-jenkinspipeline \
    --confirm \
    -o name

The current system:image-puller clusterrole is:

apiVersion: v1
kind: ClusterRole
metadata:
  annotations:
    openshift.io/description: Grants the right to pull images from within a project.
  creationTimestamp: null
  name: system:image-puller
rules:
- apiGroups:
  - image.openshift.io
  - ""
  attributeRestrictions: null
  resources:
  - imagestreams/layers
  verbs:
  - get

@enj
Copy link
Contributor Author

enj commented Sep 21, 2017

@rezie can you also post the YAML for:

oc get role shared-resource-viewer -n openshift -o yaml

@rezie
Copy link

rezie commented Sep 21, 2017

Yup:

apiVersion: v1
kind: Role
metadata:
  creationTimestamp: 2017-07-18T16:26:31Z
  name: shared-resource-viewer
  namespace: openshift
  resourceVersion: "95"
  selfLink: /oapi/v1/namespaces/openshift/roles/shared-resource-viewer
  uid: db8fc76e-6bd5-11e7-aa6f-0220918b26fc
rules:
- apiGroups:
  - ""
  attributeRestrictions: null
  resources:
  - templates
  verbs:
  - get
  - list
  - watch
- apiGroups:
  - ""
  attributeRestrictions: null
  resources:
  - imagestreamimages
  - imagestreams
  - imagestreamtags
  verbs:
  - get
  - list
  - watch
- apiGroups:
  - ""
  attributeRestrictions: null
  resources:
  - imagestreams/layers
  verbs:
  - get

@simo5
Copy link
Contributor

simo5 commented Sep 21, 2017

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 21, 2017
@openshift-merge-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: enj, simo5
We suggest the following additional approver: legionus

Assign the PR to them by writing /assign @legionus in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@liggitt
Copy link
Contributor

liggitt commented Sep 21, 2017

do we deploy 3.6 registries before completing the 3.6 API upgrade and reconciliation?

@enj
Copy link
Contributor Author

enj commented Sep 21, 2017

@rezie an oc replace -f with:

apiVersion: v1
kind: Role
metadata:
  name: shared-resource-viewer
  namespace: openshift
rules:
- apiGroups:
  - ""
  - template.openshift.io
  attributeRestrictions: null
  resources:
  - templates
  verbs:
  - get
  - list
  - watch
- apiGroups:
  - ""
  - image.openshift.io
  attributeRestrictions: null
  resources:
  - imagestreamimages
  - imagestreams
  - imagestreamtags
  verbs:
  - get
  - list
  - watch
- apiGroups:
  - ""
  - image.openshift.io
  attributeRestrictions: null
  resources:
  - imagestreams/layers
  verbs:
  - get

should "reconcile" this role for you. We will look into a more permanent fix. The diff between this role and the one your cluster has: https://www.diffchecker.com/6qAPbKUQ

@rezie
Copy link

rezie commented Sep 21, 2017

@enj Thanks! I'll let you know if we still run into this problem after using that configuration.

@enj
Copy link
Contributor Author

enj commented Sep 21, 2017

/hold

I think we can avoid this by adding some post upgrade tasks to ansible from 3.5 to 3.6 along with some docs. 3.7 already handles RBAC role reconciliation via a post start hook.

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 21, 2017
@enj
Copy link
Contributor Author

enj commented Oct 11, 2017

Closing in favor of openshift/openshift-ansible#5617

@enj enj closed this Oct 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants