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

Add rbacApiVersion and use separate resources instead of a list #2609

Merged
merged 1 commit into from
Apr 17, 2019
Merged

Add rbacApiVersion and use separate resources instead of a list #2609

merged 1 commit into from
Apr 17, 2019

Conversation

duglin
Copy link
Contributor

@duglin duglin commented Apr 12, 2019

Closes #2602

See #2602 for more info

Signed-off-by: Doug Davis [email protected]

ping @kramvan1

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 12, 2019
kramvan1
kramvan1 suggested changes Apr 15, 2019
View reviewed changes
charts/catalog/values.yaml Outdated
@@ -9,6 +9,7 @@ useAggregator: true
## If true, create & use RBAC resources
##
rbacEnable: true
rbacApiVersion: v1
Copy link

Choose a reason for hiding this comment

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

should be rbac.authorization.k8s.io/v1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

kramvan1
kramvan1 approved these changes Apr 15, 2019
View reviewed changes
Copy link

@kramvan1 kramvan1 left a comment

Choose a reason for hiding this comment

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

lgtm

@jberkhahn
Copy link
Contributor

/test pull-service-catalog-xbuild

mszostok
mszostok reviewed Apr 15, 2019
View reviewed changes
charts/catalog/values.yaml
@@ -9,6 +9,7 @@ useAggregator: true
## If true, create & use RBAC resources
##
rbacEnable: true
rbacApiVersion: rbac.authorization.k8s.io/v1
Copy link
Contributor

@mszostok mszostok Apr 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

In rbac.yaml the apiVersion is set by {{template "rbacApiVersion" . }} because of that this variable will never be taken into account and it could be misleading, so what is the purpose of it?

And also how this is solving the problem described in #2602?

I've executed locally such command with helm client version v2.10.0
helm template --name=catalog --namespace=default ./catalog
and the apiVersion param was still empty as expected.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, I don't think we can do this here. I agree that we should start defaulting to the v1 version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I fixed this

Copy link
Contributor

Choose a reason for hiding this comment

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

You should also remove the rbacApiVersion definition from the _helpers.tpl file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done! PTAL

Copy link
Contributor

Choose a reason for hiding this comment

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

I tested that and it's working. LGTM
I was able to install the Service Catalog using this command
helm template ./charts/catalog --name core --namespace default | kubectl apply -f -

But one minor thing. I saw that the #2606 was merged. And those PRs are slightly overlapping. Here we using version from values.yaml file and in merged PR there was used the {{- if .Capabilities.APIVersions.Has "rbac.authorization.k8s.io/v1" -}}.

IMHO it could be unified. After merging this PR, for RBAC we will use the direct version from values.yaml and for APIService we will version based on Capabilities.

@MHBauer gives approve for #2606, so probably he has a broader view of the whole subject.

MHBauer
MHBauer reviewed Apr 15, 2019
View reviewed changes
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 think we're being silly with the anti-list brigading. luckily github diffs can ignore whitespace changes.

@duglin duglin force-pushed the issue2602 branch 2 times, most recently from abd91e3 to e2bc464 Compare April 17, 2019 02:40
@k8s-ci-robot k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 17, 2019
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 17, 2019
@duglin
Copy link
Contributor Author

duglin commented Apr 17, 2019

/test pull-service-catalog-integration

1 similar comment
@jberkhahn
Copy link
Contributor

/test pull-service-catalog-integration

@jberkhahn
Copy link
Contributor

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jberkhahn

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

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 17, 2019
@MHBauer
Copy link
Contributor

MHBauer commented Apr 17, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 17, 2019
@k8s-ci-robot k8s-ci-robot merged commit 5639887 into kubernetes-retired:master Apr 17, 2019
@jberkhahn jberkhahn mentioned this pull request Apr 18, 2019
Closed
viviyww pushed a commit to viviyww/service-catalog that referenced this pull request May 10, 2019
Add rbacApiVersion and use separate resources instead of a list (kube…
1952882 
…rnetes-retired#2609)

Closes kubernetes-retired#2602

See kubernetes-retired#2602 for more info

Signed-off-by: Doug Davis <[email protected]>
@mszostok mszostok mentioned this pull request May 19, 2019
Closed
6 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@MHBauer MHBauer MHBauer left review comments

@mszostok mszostok mszostok left review comments

@kramvan1 kramvan1 kramvan1 approved these changes

@jberkhahn jberkhahn Awaiting requested review from jberkhahn

@jboyd01 jboyd01 Awaiting requested review from jboyd01

Assignees
No one assigned
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm Indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

RBAS null apiVersion
6 participants
@duglin @jberkhahn @k8s-ci-robot @MHBauer @kramvan1 @mszostok