Skip to content
This repository has been archived by the owner on May 6, 2022. It is now read-only.

Adding rbac definition for v1 api endpoint. #1284

Merged
merged 8 commits into from
Oct 16, 2017

Conversation

n3wscott
Copy link
Contributor

For rbac v1 issue, I duplicated the v1beta1 definition of rbac endpoints.

I am not sure this is how the bug wanted this solved. Please let me know if this is off base. The other option is to wrap each definition with something like:

{{- if .Capabilities.APIVersions.Has "rbac.authorization.k8s.io/v1beta1" }}
- apiVersion: rbac.authorization.k8s.io/v1beta1
{{- else if .Capabilities.APIVersions.Has "rbac.authorization.k8s.io/v1" }}
- apiVersion: rbac.authorization.k8s.io/v1
{{- end }}

But usage of this does tends to wrap the file with {{- if .Values.rbac.enabled }} see traekif for an example.

@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 27, 2017
@n3wscott n3wscott closed this Sep 27, 2017
@n3wscott n3wscott reopened this Sep 27, 2017
@n3wscott n3wscott closed this Sep 27, 2017
@n3wscott n3wscott reopened this Sep 27, 2017
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Sep 27, 2017
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 27, 2017
@n3wscott n3wscott changed the title Duplicating rbac definition for v1 api endpoint. Adding rbac definition for v1 api endpoint. Sep 27, 2017
Copy link
Contributor

@MHBauer MHBauer left a comment

Choose a reason for hiding this comment

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

Seems cool to me. I'll try and remember to test it in the morning.

@@ -127,7 +135,7 @@ items:
resources: ["endpoints"]
resourceNames: ["service-catalog-controller-manager"]
verbs: ["get","update"]
- apiVersion: rbac.authorization.k8s.io/v1beta1
- apiVersion: rbac.authorization.k8s.io/v1
Copy link
Contributor

Choose a reason for hiding this comment

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

variable for this one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

@MHBauer
Copy link
Contributor

MHBauer commented Sep 28, 2017

Not sure if rbac isn't on on jenkins or the variable assignment trick isn't working.

Error: parse error in "catalog/templates/rbac.yaml": template: catalog/templates/rbac.yaml:1: undefined variable "$apiVersion"

@kibbles-n-bytes
Copy link
Contributor

@MHBauer It looks unrelated to RBAC on Jenkins. Jenkins is able to successfully create RBAC roles, it just won't enforce them 100% at the moment. This error is from Helm, so something must be off with the variable assignment.

@vaikas vaikas added the LGTM1 label Oct 2, 2017
@MHBauer
Copy link
Contributor

MHBauer commented Oct 2, 2017

I did not get any rbac objects created on a 1.8 cluster.

# helm install ../../charts/catalog     --name ${HELM_RELEASE_NAME} --namespace ${SVCCAT_NAMESPACE}     --set apiserver.auth.enabled=true         --set useAggregator=true         --set apiserver.tls.ca=$(base64 --wrap 0 ${SC_SERVING_CA})         --set apiserver.tls.cert=$(base64 --wrap 0 ${SC_SERVING_CERT})         --set apiserver.tls.key=$(base64 --wrap 0 ${SC_SERVING_KEY})
NAME:   catalog
LAST DEPLOYED: Mon Oct  2 12:57:32 2017
NAMESPACE: catalog
STATUS: DEPLOYED

RESOURCES:
==> v1beta1/APIService
NAME                            KIND
v1alpha1.servicecatalog.k8s.io  APIService.v1beta1.apiregistration.k8s.io

==> v1/ServiceAccount
NAME                                SECRETS  AGE
service-catalog-apiserver           1        0s
service-catalog-controller-manager  1        0s

==> v1/Secret
NAME                            TYPE    DATA  AGE
catalog-catalog-apiserver-cert  Opaque  2     0s

==> v1/Service
NAME                       CLUSTER-IP     EXTERNAL-IP  PORT(S)        AGE
catalog-catalog-apiserver  10.98.188.189  <nodes>      443:30443/TCP  0s

==> v1beta1/Deployment
NAME                                DESIRED  CURRENT  UP-TO-DATE  AVAILABLE  AGE
catalog-catalog-apiserver           1        1        1           0          0s
catalog-catalog-controller-manager  1        1        1           0          0s
# k api-versions
...
rbac.authorization.k8s.io/v1
rbac.authorization.k8s.io/v1beta1
...

Do not know how to debug this.

@MHBauer
Copy link
Contributor

MHBauer commented Oct 2, 2017

I do not see the output I expect in Jenkins either.

Example with the chart as it currently exists:

# helm install ../../charts/catalog     --name ${HELM_RELEASE_NAME} --namespace ${SVCCAT_NAMESPACE}     --set apiserver.auth.enabled=true         --set useAggregator=true         --set apiserver.tls.ca=$(base64 --wrap 0 ${SC_SERVING_CA})         --set apiserver.tls.cert=$(base64 --wrap 0 ${SC_SERVING_CERT})         --set apiserver.tls.key=$(base64 --wrap 0 ${SC_SERVING_KEY})
NAME:   catalog
LAST DEPLOYED: Mon Oct  2 13:04:29 2017
NAMESPACE: catalog
STATUS: DEPLOYED

RESOURCES:
==> v1beta1/RoleBinding
NAME                                                   AGE
servicecatalog.k8s.io:apiserver-authentication-reader  0s
service-catalog-controller-manager                     0s

==> v1beta1/Deployment
NAME                                DESIRED  CURRENT  UP-TO-DATE  AVAILABLE  AGE
catalog-catalog-apiserver           1        0        0           0          0s
catalog-catalog-controller-manager  1        0        0           0          0s

==> v1beta1/ClusterRole
NAME                                      AGE
servicecatalog.k8s.io:apiserver           0s
servicecatalog.k8s.io:controller-manager  0s

==> v1beta1/APIService
NAME                            KIND
v1alpha1.servicecatalog.k8s.io  APIService.v1beta1.apiregistration.k8s.io

==> v1beta1/ClusterRoleBinding
NAME                                            AGE
servicecatalog.k8s.io:apiserver                 0s
servicecatalog.k8s.io:apiserver-auth-delegator  0s
servicecatalog.k8s.io:controller-manager        0s

==> v1beta1/Role
NAME                                                     AGE
servicecatalog.k8s.io:leader-locking-controller-manager  0s

==> v1/ServiceAccount
NAME                                SECRETS  AGE
service-catalog-apiserver           1        0s
service-catalog-controller-manager  1        0s

==> v1/Secret
NAME                            TYPE    DATA  AGE
catalog-catalog-apiserver-cert  Opaque  2     0s

==> v1/Service
NAME                       CLUSTER-IP      EXTERNAL-IP  PORT(S)        AGE
catalog-catalog-apiserver  10.110.113.151  <nodes>      443:30443/TCP  0s

Copy link
Contributor

@MHBauer MHBauer left a comment

Choose a reason for hiding this comment

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

I do not know what needs to change, but this does not work as it is.

@n3wscott
Copy link
Contributor Author

n3wscott commented Oct 2, 2017

I wonder if .Has is matching "rbac.authorization.k8s.io/v1" when APIVersions is "rbac.authorization.k8s.io/v1beta1" from .Capabilities.APIVersions.Has

This is why semver is awesome... Let me look into that direction.

@pmorie
Copy link
Contributor

pmorie commented Oct 4, 2017

I wonder if .Has is matching "rbac.authorization.k8s.io/v1" when APIVersions is "rbac.authorization.k8s.io/v1beta1" from .Capabilities.APIVersions.Has

This is why semver is awesome... Let me look into that direction.

If that's the case, it's definitely a helm bug.

@n3wscott
Copy link
Contributor Author

I am following the steps here: https://github.com/kubernetes-incubator/service-catalog/blob/master/docs/install-1.7.md

But get stuck at this step:

⦿ source ../../contrib/svc-cat-apiserver-aggregation-tls-setup.sh
Killed: 9
Killed: 9

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 13, 2017
@n3wscott
Copy link
Contributor Author

This should work now. The do not merge tag can be removed. Thanks!

@pmorie pmorie added the LGTM2 label Oct 16, 2017
@pmorie pmorie merged commit 268294e into kubernetes-retired:master Oct 16, 2017
@pmorie pmorie added this to the 0.1.0-rc2 milestone Oct 17, 2017
@n3wscott n3wscott deleted the rbac_v1 branch October 18, 2017 22:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. LGTM1 LGTM2 size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants