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

Fix #17: Make RBAC resources unique and optional #18

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

rendhalver
Copy link

Problem:
With the current Chart RBAC resource names are hard coded and not optional
so it's impossible to deploy more than one instance in a namespace

Solution:
This uses the helper f5-bigip-ctlr.fullname as the name of each resource to it's unique.
It also add variables to make the RBAc resources optional.
You can also set the name of the service account to use so you can use it across all deploys.

Testing (optional if not described in Solution section): Description of tests that were run to exercise the solutions (unit tests, system tests, etc)

Pete Brown added 3 commits September 12, 2018 08:23
Problem:
With the current Chart RBAC resource names are hard coded and not optional
so it's impossible to deploy more than one instance in a namespace

Solution:
This uses the helper f5-bigip-ctlr.fullname as the name of each resource to it's unique.
It also add variables to make the RBAc resources optional.
You can also set the name of the service account to use so you can use it across all deploys.

Testing (optional if not described in Solution section): Description of tests that were run to exercise the solutions (unit tests, system tests, etc)
Fix name of ClusterRole in ClusterRoleBinding
@rendhalver
Copy link
Author

Any progress on this?

@recursivelycurious
Copy link
Contributor

@rendhalver - Thanks for the contribution. Under review.

Copy link
Contributor

@recursivelycurious recursivelycurious left a comment

Choose a reason for hiding this comment

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

@rendhalver - initial review looks good. Will be running regression tests and will update you on outcome. Provided regression / system tests are green, happy to add a commit to modify the version per the earlier comment. If anything is needed from you, will let you know.

src/incubator/f5-bigip-ctlr/Chart.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

@recursivelycurious recursivelycurious left a comment

Choose a reason for hiding this comment

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

@f5tsomanchi - As we discussed on site and per the comment left in this review I would advise keeping the naming convention already in place, unless we can make a strong case to change them based on the way charts work in production when multiple versions might be deployed.

The RBAC aspects look fine, however, this MR had not been merged yet due to difficulty around a testing environment to run the system tests. They may need to be modified to run the newer charts.

labels:
app: {{ template "f5-bigip-ctlr.name" . }}
chart: {{ .Chart.Name }}-{{ .Chart.Version | replace "+" "_" }}
chart: {{ template "f5-bigip-ctlr.chart" . }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Charts in helm are expected to be tied to a version. This naming convention is deliberate and should be retained. Operators may be expected to use multiple versions of a chart during rolling deployments, testing etc., and having the name explicitly reference the version should make tracking, debugging, and human error elimination easier.

@f5tsomanchi The names of resources should be validated in a test environment

Copy link

Choose a reason for hiding this comment

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

@recursivelycurious - as said in the above comments,

The RBAC aspects look fine, however, this MR had not been merged yet due to difficulty around a testing environment to run the system tests. They may need to be modified to run the newer charts.

  • Can you please elaborate the modifications needed to run newer charts ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

If you look in the helpers template "f5-bigip-ctlr.chart" . is functionally equivalent to .Chart.Name }}-{{ .Chart.Version | replace "+" "_"

@ghost
Copy link

ghost commented May 10, 2019

@rendhalver : Please update the patch with requested changes

@rendhalver
Copy link
Author

@rendhalver : Please update the patch with requested changes

I answered the question.
No one responded.

@recursivelycurious
Copy link
Contributor

@trinaths -- please take another look at this MR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants