State definitions in SLS files #3460

Closed
madduck opened this Issue Jan 26, 2013 · 21 comments

Comments

Projects
None yet
5 participants
@madduck
Contributor

madduck commented Jan 26, 2013

Would you consider providing a way by which I can define new states in SLS files, based on other states?

Let me elaborate, using the following example. Let's say you have to create three (hypothetical) files on the target system. The files are identical except for two "parameters", which differ between those files (e.g. "red"/1, "blue"/2 and "green"/3).

One way to achieve this is to create three file.managed stanzas using the same template, but providing different context.

If you do this, you'll quickly note the duplication, and you'll want to do something about it. You could define a sequence of parameters and loop over that sequence e.g. with mako, effectively instantiating the three file.managed stanzas with different parameters, e.g.

% for i in [('red',1),('blue',2),('green',3)]
/my/file/for/${i[0]}:
  file.managed:
    user: root
    template: myfile.j2
    context:
      - colour: ${i[0]}
      - number: ${i[1]}
% endfor

Alternatively, you'd wish to be able to define a macro, e.g.

define:
  mymacro:
    parameters:
      - colour
      - number
    body:
      /my/file/for/%colour%:
        file.managed:
          …
          context:
            …

The parameters could even be made available as context by default.

And now the above could be handled as follows:

mymacro:
  - colour: red
  - number: 1

mymacro:
  - colour: blue
  - number: 2

mymacro:
  - colour: green
  - number: 3

which would expand internally to three file.managed stanzas.

This is surely possible with templating, but the functionality is useful
enough to be integrated into Salt proper.

Maybe there is a better syntax for this. Does it have to be YAML?

@thatch45

This comment has been minimized.

Show comment
Hide comment
@thatch45

thatch45 Jan 26, 2013

Member

I agree that this needs to be added in, I have been chewing on it for a while how to best get it in. So far I have not settled on the syntax. Granted you can do this in jinja/mako via macros, but this would be better to be handled by the compiler in the data structure.
It is also noteworthy that this is the only construct people are asking for on the data structure layer. It is also noteworthy that your suggested data structure won't work, remember that this is a dict on the top so the mymacro statements won't work.
Gimme a minute to brainstorm this, it would be killer if you could help me kill some bugs so I have more time for this guy :)

Member

thatch45 commented Jan 26, 2013

I agree that this needs to be added in, I have been chewing on it for a while how to best get it in. So far I have not settled on the syntax. Granted you can do this in jinja/mako via macros, but this would be better to be handled by the compiler in the data structure.
It is also noteworthy that this is the only construct people are asking for on the data structure layer. It is also noteworthy that your suggested data structure won't work, remember that this is a dict on the top so the mymacro statements won't work.
Gimme a minute to brainstorm this, it would be killer if you could help me kill some bugs so I have more time for this guy :)

@thatch45

This comment has been minimized.

Show comment
Hide comment
@thatch45

thatch45 Jan 26, 2013

Member

Idea 1: This will allow us to make defines without changing the top level data structures at all, I really like this approach because it can just become a wrapper for the "use" system we already have. Similar to how the require_in stuff works as a wrapper to the extend system

mymacro:
  macro.define:
    - state: file.managed
    - user: root
    - group: root

filemacro:
  macro.use:
    - macro: mymacro
    - name: /etc/foo/bar
Member

thatch45 commented Jan 26, 2013

Idea 1: This will allow us to make defines without changing the top level data structures at all, I really like this approach because it can just become a wrapper for the "use" system we already have. Similar to how the require_in stuff works as a wrapper to the extend system

mymacro:
  macro.define:
    - state: file.managed
    - user: root
    - group: root

filemacro:
  macro.use:
    - macro: mymacro
    - name: /etc/foo/bar
@thatch45

This comment has been minimized.

Show comment
Hide comment
@thatch45

thatch45 Jan 26, 2013

Member

Idea 2: This does not have a wrapper, it just makes it so that an id that starts with underscores does not get executed, then you can use the "use" system directly.

__mymacro:
  file.managed:
    - user: root
    - group: root

/etc/cheese/spam:
  file.managed:
    - use: __mymacro

@herlo, we have talked about this a few times, any ideas?

Member

thatch45 commented Jan 26, 2013

Idea 2: This does not have a wrapper, it just makes it so that an id that starts with underscores does not get executed, then you can use the "use" system directly.

__mymacro:
  file.managed:
    - user: root
    - group: root

/etc/cheese/spam:
  file.managed:
    - use: __mymacro

@herlo, we have talked about this a few times, any ideas?

@kjkuan

This comment has been minimized.

Show comment
Hide comment
@kjkuan

kjkuan Jan 28, 2013

Contributor

@madduck, Just FYI, if not using YAML is an option for you, then this can also be solved elegantly with the recently added pydsl renderer:

#!pydsl
def mymacro(colour, number):
    s = state('/my/file/for/'+colour)
    return s.file.managed(
        user='root',
        source='salt://myfile.j2',
        template='jinja',
        context=dict(
            colour=colour,
            number=number
        )
    )
mymacro('red', 1)
mymacro('blue', 2)
mymacro('green', 3)
Contributor

kjkuan commented Jan 28, 2013

@madduck, Just FYI, if not using YAML is an option for you, then this can also be solved elegantly with the recently added pydsl renderer:

#!pydsl
def mymacro(colour, number):
    s = state('/my/file/for/'+colour)
    return s.file.managed(
        user='root',
        source='salt://myfile.j2',
        template='jinja',
        context=dict(
            colour=colour,
            number=number
        )
    )
mymacro('red', 1)
mymacro('blue', 2)
mymacro('green', 3)
@madduck

This comment has been minimized.

Show comment
Hide comment
@madduck

madduck Jan 28, 2013

Contributor

@madduck, Just FYI, if not using YAML is an option for you, then
this can also be solved elegantly with the recently added pydsl
renderer:

Thanks for the pointer to pydsl (and stateconf). This might just be
the road to take anyway. While I agree that another DSL is not
always ideal, I find the standard SLS YAML to be a little contrived
and brittle… For instance, often I don't really know when to prefix
a line with '- and when not. Surely, experience would teach, but
then later when I am not working with Salt on a daily basis, I might
not remember anymore.

Contributor

madduck commented Jan 28, 2013

@madduck, Just FYI, if not using YAML is an option for you, then
this can also be solved elegantly with the recently added pydsl
renderer:

Thanks for the pointer to pydsl (and stateconf). This might just be
the road to take anyway. While I agree that another DSL is not
always ideal, I find the standard SLS YAML to be a little contrived
and brittle… For instance, often I don't really know when to prefix
a line with '- and when not. Surely, experience would teach, but
then later when I am not working with Salt on a daily basis, I might
not remember anymore.

@madduck

This comment has been minimized.

Show comment
Hide comment
@madduck

madduck Jan 28, 2013

Contributor

also sprach Jack Kuan notifications@github.com [2013.01.28.1349 +1300]:

def mymacro(colour, number):
s = state('/my/file/for/'+colour)
return s.file.managed(
user='root',
source='salt://myfile.j2',
template='jinja',
context=dict(
colour=colour,
number=number
)
)

I wish I didn't have to iterate through all function arguments
I want to make available to the template in such a repetitive way.
I could use **kwargs, but then I would not get any argument
checking/enforcing when the function is called.

Other than that, pydsl is just love on first sight.

Contributor

madduck commented Jan 28, 2013

also sprach Jack Kuan notifications@github.com [2013.01.28.1349 +1300]:

def mymacro(colour, number):
s = state('/my/file/for/'+colour)
return s.file.managed(
user='root',
source='salt://myfile.j2',
template='jinja',
context=dict(
colour=colour,
number=number
)
)

I wish I didn't have to iterate through all function arguments
I want to make available to the template in such a repetitive way.
I could use **kwargs, but then I would not get any argument
checking/enforcing when the function is called.

Other than that, pydsl is just love on first sight.

@madduck

This comment has been minimized.

Show comment
Hide comment
@madduck

madduck Jan 28, 2013

Contributor

@tshatch: I like your first approach (using macro.define as
a special state), it does seem to blend in seamlessly, even though
it does add a level of indirection/indentation, but users of SLS
should be used to that (cf. e.g. extends)

Your second idea also sounds plausible, also I personally think that
leading underscores are an indication of using
implementation-specific stuff outside of the official interface that
you shouldn't actually be using. I am also not familiar with the
'use' system to judge how seamless this is.

Contributor

madduck commented Jan 28, 2013

@tshatch: I like your first approach (using macro.define as
a special state), it does seem to blend in seamlessly, even though
it does add a level of indirection/indentation, but users of SLS
should be used to that (cf. e.g. extends)

Your second idea also sounds plausible, also I personally think that
leading underscores are an indication of using
implementation-specific stuff outside of the official interface that
you shouldn't actually be using. I am also not familiar with the
'use' system to judge how seamless this is.

@madduck

This comment has been minimized.

Show comment
Hide comment
@madduck

madduck Jan 28, 2013

Contributor

@kjkuan: should I be filing issues against pydsl here at github
against saltstack proper?

For instance, I would like to make pydsl hide the stateconf.set
state for every run, as I think this isn't information that needs to
be exposed to the admin:

State: - stateconf
Name:      .1eac0104-524e-46b8-851d-c0e506eb7b4a
Function:  set
    Result:    True
    Comment:   
    Changes:   

(I also don't really know why I need this if I don't use stateconf…)

Also, I'd like to ask for a way to import functions from other state
definitions.

For instance, my sudo state provides a sudo_permission macro (to
create a file in /etc/sudoers.d much like you suggest in your last
comment). Now I'd like to import that in my salt-minion state
definition (to be able to let
%staff-group-members execute salt-call state.highstate without
a password…).

Unfortunately, include does not make the Python objects from the
included object available to the includee.

Contributor

madduck commented Jan 28, 2013

@kjkuan: should I be filing issues against pydsl here at github
against saltstack proper?

For instance, I would like to make pydsl hide the stateconf.set
state for every run, as I think this isn't information that needs to
be exposed to the admin:

State: - stateconf
Name:      .1eac0104-524e-46b8-851d-c0e506eb7b4a
Function:  set
    Result:    True
    Comment:   
    Changes:   

(I also don't really know why I need this if I don't use stateconf…)

Also, I'd like to ask for a way to import functions from other state
definitions.

For instance, my sudo state provides a sudo_permission macro (to
create a file in /etc/sudoers.d much like you suggest in your last
comment). Now I'd like to import that in my salt-minion state
definition (to be able to let
%staff-group-members execute salt-call state.highstate without
a password…).

Unfortunately, include does not make the Python objects from the
included object available to the includee.

@kjkuan

This comment has been minimized.

Show comment
Hide comment
@kjkuan

kjkuan Jan 28, 2013

Contributor

@madduck: I'm not sure what @thatch45 's take on filing pydsl issues here, I've no problem with it. If you do, you can assign the issue to me so I'll know and will work on it.

Currently, a stateconf.set state is generated for each .sls file when using pydsl to guard against the case that some object in the .sls python module is referenced by some objects in the rendered high state data structure.
A case of this is illustrated in the doc here, and when that happens, the added stateconf.set state keeps a reference to the module object, which will be passed to the low state data structure and retained there for the duration of the low state executions. This prevents the module from being GC'ed and also avoid the module from being kept in the memory longer than necessary. It's a hack, but it's the easiest thing that I could think of that works the way I want. Perhaps, @thatch45 would know of a better way in salt for this :)

As for importing python modules in to a pydsl .sls module, yah, I think adding a new function to pydsl for importing other pydsl .sls modules(i.e., with pydsl functions like include, extend, state, ... available), as well as, regular python modules from salt's file server is necessary.

Contributor

kjkuan commented Jan 28, 2013

@madduck: I'm not sure what @thatch45 's take on filing pydsl issues here, I've no problem with it. If you do, you can assign the issue to me so I'll know and will work on it.

Currently, a stateconf.set state is generated for each .sls file when using pydsl to guard against the case that some object in the .sls python module is referenced by some objects in the rendered high state data structure.
A case of this is illustrated in the doc here, and when that happens, the added stateconf.set state keeps a reference to the module object, which will be passed to the low state data structure and retained there for the duration of the low state executions. This prevents the module from being GC'ed and also avoid the module from being kept in the memory longer than necessary. It's a hack, but it's the easiest thing that I could think of that works the way I want. Perhaps, @thatch45 would know of a better way in salt for this :)

As for importing python modules in to a pydsl .sls module, yah, I think adding a new function to pydsl for importing other pydsl .sls modules(i.e., with pydsl functions like include, extend, state, ... available), as well as, regular python modules from salt's file server is necessary.

@madduck

This comment has been minimized.

Show comment
Hide comment
@madduck

madduck Jan 28, 2013

Contributor

@kjkuan: I have continued the discussion about importing functions at #3483 and will respond there.

I will consider filing a wishlist report about disablking stateconf.set for when that functionality is simply not required.

Contributor

madduck commented Jan 28, 2013

@kjkuan: I have continued the discussion about importing functions at #3483 and will respond there.

I will consider filing a wishlist report about disablking stateconf.set for when that functionality is simply not required.

@thatch45

This comment has been minimized.

Show comment
Hide comment
@thatch45

thatch45 Jan 28, 2013

Member

@madduck, thanks for the feedback, while I really like that we have a dsl, I do not think that people should need to lean on it. I think that only a small fraction of deployments should need to use the dsl. I will keep brainstorming this

Member

thatch45 commented Jan 28, 2013

@madduck, thanks for the feedback, while I really like that we have a dsl, I do not think that people should need to lean on it. I think that only a small fraction of deployments should need to use the dsl. I will keep brainstorming this

@madduck

This comment has been minimized.

Show comment
Hide comment
@madduck

madduck Jan 28, 2013

Contributor

@tshatch, I agree that 99% of all use-cases should be covered by
YAML configuration files. I also think that code reuse and
abstraction should be part of core functionality. I am interested to
see in what you come up with.

it's a shame that the renderers run on the minions. This means that
any change you make needs to first appear in a published version and
get backported before it lands on my minions. I am running the
master from source now, but that's too much trouble for the minions.

Contributor

madduck commented Jan 28, 2013

@tshatch, I agree that 99% of all use-cases should be covered by
YAML configuration files. I also think that code reuse and
abstraction should be part of core functionality. I am interested to
see in what you come up with.

it's a shame that the renderers run on the minions. This means that
any change you make needs to first appear in a published version and
get backported before it lands on my minions. I am running the
master from source now, but that's too much trouble for the minions.

@thatch45

This comment has been minimized.

Show comment
Hide comment
@thatch45

thatch45 Jan 28, 2013

Member

You can use the _renderers directory to sent out updated renderers to the minions if that is where the fix is, but yes, for core updates you need to update the minions.

I did start building an interface that would render sls files on the master though, called masterstate, I just never finished it because I did not see any demand at the time, I take it you see value here?

Member

thatch45 commented Jan 28, 2013

You can use the _renderers directory to sent out updated renderers to the minions if that is where the fix is, but yes, for core updates you need to update the minions.

I did start building an interface that would render sls files on the master though, called masterstate, I just never finished it because I did not see any demand at the time, I take it you see value here?

@madduck

This comment has been minimized.

Show comment
Hide comment
@madduck

madduck Jan 29, 2013

Contributor

@tshatch, about your idea 2 above (use system): this requires the callee (the user) to know what function to call (file.managed in your example). If the implementer changes the way the functionality is handled (e.g. switches to file.append), then all users need to update their code.

Idea 1 (macros) sufficienty hides this implementation detail.

Contributor

madduck commented Jan 29, 2013

@tshatch, about your idea 2 above (use system): this requires the callee (the user) to know what function to call (file.managed in your example). If the implementer changes the way the functionality is handled (e.g. switches to file.append), then all users need to update their code.

Idea 1 (macros) sufficienty hides this implementation detail.

@madduck

This comment has been minimized.

Show comment
Hide comment
@madduck

madduck Jan 29, 2013

Contributor

Let me provide a real-world example for your idea 1, hoping that you can glean from it the interface I am thinking of:

# in firewalled/init.sls:

  %openport:
    macro.declare:
      - parameters:
        - port
        - proto
        - iface

# in firewalled/ferm.sls:

  include('firewalled')

  &openport:
    macro.define:
      - implements: %openport
      - state: file.managed
      - user: root
      - mode: 440
      - name_template: /etc/ferm/rules.d/50-salt-openport-${name}.ferm
      - source: salt://ferm/openport.template.j2
      - parameters:
        - port
        - proto
        - iface

# in ssh/server.sls:

  include('firewalled')

  openport-22:
    macro.use:
      - macro: %openport
      - name: ssh
      - port: 22
      - proto: tcp
      - iface: all

The macro declaration specifies an interface which the macro definition must
implement. The user (ssh/server.sls) uses the declaration, not having to
care at all about what defines the macro. Salt just needs any one definition
that adheres to the interface, and if someone were to use e.g.
firewalled.ufw instead of firewalled.ferm, Salt would use that definition
without anything else having to change.

The interface defines mandatory (and optional) parameters. The definition then
makes the values of those parameters available to the templating agent, and
maybe even the state function.

Additional parameters passed to the macro could become optional parameters,
i.e. made available to the templating engine as well, but not required. Or
maybe optional parameters can be specified differently, e.g. with a '+'
prefix, a '?' postfix, or under 'optional_parameters'.

Care needs to be taken to handle the situation where a parameter is passed
that could override one passed to the state function in the definition. If the
parameter is exported, this could be desired behaviour…

The name parameter ought to be interpolated as shown in the example above.

As you can see, I used a bit of a naming convention. I wouldn't want macro.*
functions to show up in highstate output, and I think it'll be clearer if they
are somehow marked.

If this is deemed a good idea, then maybe it's possible to further facilitate
the syntax:

# declarations don't include a function name, just parameters
%openport:
  - parameters:
    - port
    - proto
    - iface

# definitions may pretend to be like states, except they don't yet get
# instantiated
&openport:
  implements: %openport
  file.managed:
    - user: root
    - mode: 440
    - name: /etc/ferm/rules.d/50-salt-openport-${name}.ferm
    - source: salt://ferm/openport.template.j2
    - parameters:
      - port
      - proto
      - iface

openport-22:
  %openport:
    - macro: &openport
    - name: ssh
    - port: 22
    - proto: tcp
    - iface: all
Contributor

madduck commented Jan 29, 2013

Let me provide a real-world example for your idea 1, hoping that you can glean from it the interface I am thinking of:

# in firewalled/init.sls:

  %openport:
    macro.declare:
      - parameters:
        - port
        - proto
        - iface

# in firewalled/ferm.sls:

  include('firewalled')

  &openport:
    macro.define:
      - implements: %openport
      - state: file.managed
      - user: root
      - mode: 440
      - name_template: /etc/ferm/rules.d/50-salt-openport-${name}.ferm
      - source: salt://ferm/openport.template.j2
      - parameters:
        - port
        - proto
        - iface

# in ssh/server.sls:

  include('firewalled')

  openport-22:
    macro.use:
      - macro: %openport
      - name: ssh
      - port: 22
      - proto: tcp
      - iface: all

The macro declaration specifies an interface which the macro definition must
implement. The user (ssh/server.sls) uses the declaration, not having to
care at all about what defines the macro. Salt just needs any one definition
that adheres to the interface, and if someone were to use e.g.
firewalled.ufw instead of firewalled.ferm, Salt would use that definition
without anything else having to change.

The interface defines mandatory (and optional) parameters. The definition then
makes the values of those parameters available to the templating agent, and
maybe even the state function.

Additional parameters passed to the macro could become optional parameters,
i.e. made available to the templating engine as well, but not required. Or
maybe optional parameters can be specified differently, e.g. with a '+'
prefix, a '?' postfix, or under 'optional_parameters'.

Care needs to be taken to handle the situation where a parameter is passed
that could override one passed to the state function in the definition. If the
parameter is exported, this could be desired behaviour…

The name parameter ought to be interpolated as shown in the example above.

As you can see, I used a bit of a naming convention. I wouldn't want macro.*
functions to show up in highstate output, and I think it'll be clearer if they
are somehow marked.

If this is deemed a good idea, then maybe it's possible to further facilitate
the syntax:

# declarations don't include a function name, just parameters
%openport:
  - parameters:
    - port
    - proto
    - iface

# definitions may pretend to be like states, except they don't yet get
# instantiated
&openport:
  implements: %openport
  file.managed:
    - user: root
    - mode: 440
    - name: /etc/ferm/rules.d/50-salt-openport-${name}.ferm
    - source: salt://ferm/openport.template.j2
    - parameters:
      - port
      - proto
      - iface

openport-22:
  %openport:
    - macro: &openport
    - name: ssh
    - port: 22
    - proto: tcp
    - iface: all
@thatch45

This comment has been minimized.

Show comment
Hide comment
@thatch45

thatch45 Jan 31, 2013

Member

This proposal is not the direction I want to go in, and I don't see how blurring the line between YAML and Jinja is really going to help here. So I am going to defer this for the discussion on a future release

Member

thatch45 commented Jan 31, 2013

This proposal is not the direction I want to go in, and I don't see how blurring the line between YAML and Jinja is really going to help here. So I am going to defer this for the discussion on a future release

@madduck

This comment has been minimized.

Show comment
Hide comment
@madduck

madduck Jan 31, 2013

Contributor

Can you please elaborate on which line is being blurred?

Contributor

madduck commented Jan 31, 2013

Can you please elaborate on which line is being blurred?

@thatch45

This comment has been minimized.

Show comment
Hide comment
@thatch45

thatch45 Jan 31, 2013

Member

I will later, this has become too much of a time suck for me right now

Member

thatch45 commented Jan 31, 2013

I will later, this has become too much of a time suck for me right now

@madduck

This comment has been minimized.

Show comment
Hide comment
@madduck

madduck Jun 10, 2013

Contributor

Any news on this front?

Contributor

madduck commented Jun 10, 2013

Any news on this front?

@rallytime rallytime added the Feature label Oct 1, 2014

@thatch45

This comment has been minimized.

Show comment
Hide comment
@thatch45

thatch45 May 9, 2017

Member

Agreed, I will close this out as solved

Member

thatch45 commented May 9, 2017

Agreed, I will close this out as solved

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment