-
Notifications
You must be signed in to change notification settings - Fork 302
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
[CLOUD-2261] adding permisssion to list pods #447
[CLOUD-2261] adding permisssion to list pods #447
Conversation
eap/eap71-tx-recovery-s2i.json
Outdated
"metadata": { | ||
"name": "listing-pod-role" | ||
}, | ||
"rules": [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume we're adding this because it's more restrictive than the view
role?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, the point was not using the view role but allow only the least necessary minimum
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect we'll have the same problem of recreating the role on subsequent template invocations. It may be better to use view
to avoid this problem for the time being.
eap/eap71-tx-recovery-s2i.json
Outdated
"apiVersion": "v1", | ||
"kind": "ServiceAccount", | ||
"metadata": { | ||
"name": "listing-pod" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using a static name in the template will cause problems on subsequent invocations, because the service account already exists.
eap/eap71-tx-recovery-s2i.json
Outdated
{ | ||
"kind": "ServiceAccount", | ||
"name": "listing-pod", | ||
"namespace": "${NAMESPACE}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is namespace required here? If so, that might present a usability problem. I thought most references defaulted to the containing namespace if not specified.
eap/eap71-tx-recovery-s2i.json
Outdated
{ | ||
"displayName": "Project Namespace", | ||
"description": "Project namespace where the template pods will be installed to. The value is used to define service account and role to permit listing pods from OpenShift API.", | ||
"name": "NAMESPACE", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should try to remove this. Having to specify the namespace directly is a bit of a usability problem, since it should default to the namespace within which the resources are being created.
4a4beff
to
fa1eeca
Compare
@rcernich thanks for your points. I've changed the template a bit to address them exept of the issue with the namespace. Without defining the namespace in role ref part of the template the image does not work. As I understand it' known issue, see openshift/origin#11566 (comment) Do you think using |
fa1eeca
to
cd74754
Compare
@rcernich I see. I've changed the template to use the |
hi @rcernich , what do you think about the change-set now? And what is your expectation for jboss-openshift/cct_module#230 ? Thank you |
eap/eap71-tx-recovery-s2i.json
Outdated
"apiVersion": "v1", | ||
"kind": "ServiceAccount", | ||
"metadata": { | ||
"name": "${SERVICE_ACCOUNT_NAME}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would recommend removing the parameter and just have it use something like ${APPLICATION_NAME}-account
or ${APPLICATION_NAME}-sa
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, thanks. Here you go.
cd74754
to
b46bf52
Compare
b46bf52
to
474cebd
Compare
https://issues.jboss.org/browse/CLOUD-2261
Adding new system role with permission to query api to get list of pods. This is needed for case the recovery of transaction is done via apy querying as discussed at CLOUD-2261 and at document https://docs.google.com/document/d/1JdOTMP6pdexJ__KYlw9hgwR6DOCHTtYCQy7PqyMk8OU/edit
/cc @rcernich