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

Set defaults for templated roles. #916

Merged
merged 1 commit into from
Apr 12, 2017
Merged

Conversation

russjones
Copy link
Contributor

Purpose

As covered in #912, at the moment if you create a role from a template, you have to explicitly define the values for all fields. This PR changes this behavior by filling in the default value for missing or empty fields.

Implementation

  • When building a role, call CheckAndSetDefaults to make sure the role is filled out.
  • In CheckAndSetDefaults require that all fields have default values if unset or empty.

Related Issues

Fixes #912

if r.Metadata.Name == "" {
return trace.BadParameter("missing parameter Name")
}
if r.Metadata.Namespace == "" {
Copy link
Contributor

@klizhentas klizhentas Apr 10, 2017

Choose a reason for hiding this comment

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

one thing that worries me a bit - it will be helpful to distinguish between "not specified" and left empty, e.g your pull request does not allow me to prohibit access to all nodes at all, it will allways override with wildcard.

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 changed r.Spec.Namespaces, r.Spec.NodeLabels, and r.Spec.Resources to only set defaults when nil.

I can't really do that here for Namespace without changing the underlying type for r.Metadata.Namespace since I can't distinguish not being set and being set to "" without making it a pointer.

What situation would we want to allow an empty namespace to restrict access to nodes? In your above example you can remove the User if you don't want them to have access any nodes?

Copy link
Contributor

Choose a reason for hiding this comment

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

you can make it a pointer, why not

@russjones russjones force-pushed the rjones/role-template-defaults branch from 47271d4 to e3a9641 Compare April 11, 2017 00:06
@russjones russjones force-pushed the rjones/role-template-defaults branch from e3a9641 to f85bb0d Compare April 12, 2017 00:03
@russjones russjones merged commit d28fe6c into master Apr 12, 2017
@russjones russjones deleted the rjones/role-template-defaults branch April 12, 2017 00:25
hatched pushed a commit that referenced this pull request Feb 1, 2023
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