Skip to content

Remove comma separated path support#117

Closed
bdeferme wants to merge 1 commit into
voxpupuli:masterfrom
bdeferme:rm_interpolate
Closed

Remove comma separated path support#117
bdeferme wants to merge 1 commit into
voxpupuli:masterfrom
bdeferme:rm_interpolate

Conversation

@bdeferme

@bdeferme bdeferme commented Feb 5, 2026

Copy link
Copy Markdown

Pull Request (PR) description

This feature does not make sense. A node should only have one role anyway.

This Pull Request (PR) fixes the following issues

Can not use '/' as a path.

This feature does not make sense. A node should only have one role
anyway.

Also, the feature breaks using '/' as a path.
@yakatz

yakatz commented Feb 5, 2026

Copy link
Copy Markdown
Member

Can you explain more about the issue you are trying to solve?
Providing specific examples, or even better actual tests, will help with the review.

The feature you are proposing removing has uses besides the example of multiple roles (whether people should be doing that or not). Removing it could be disruptive.

@yakatz yakatz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Needs a better justification for why this change is necessary.

@yakatz yakatz added the backwards-incompatible This change will lead to a major version bump for the next release label Feb 5, 2026
@bdeferme

bdeferme commented Feb 6, 2026

Copy link
Copy Markdown
Author

The issue with the current interpolate function: it silently drops the root path (/). This happens because split('/') on / returns an empty array ([]), which is then skipped due to the unless segments.empty? condition.

For our use case, the root path (/) is a valid and meaningful entry. It allows us to use explicit lookups in hieradata, which brings several benefits:

  • Reduced traversal through the vault tree.
  • Improved readability of configurations.
  • More flexibility in how we structure and migrate secrets.

For example, with a path list like:

  • /
  • nodes/%{facts.networking.fqdn}
  • environments/%{trusted.extensions.pp_role}
  • common

We can resolve secrets like this in common.yaml:

profile::cool::access_key: "%{lookup('provider/s3/access-keys/fancy-prd.key')}"

This would map to the vault path /provider/s3/access-keys/fancy-prd.key.

We’re migrating all our secrets to explicit lookups, and this path list allows us to do it gradually, rather than in a big-bang migration. However, the current implementation makes this difficult.

#83 addresses this issue but introduces new problems, such as breaking the comma-separated path functionality.

To preserve both the root path and comma separation, we could add a simple check at the start:

if path == '/'
  allowed_paths += ['/']
  next
end

This ensures / is included in the output while keeping the rest of the logic intact.

@yakatz

yakatz commented Feb 6, 2026

Copy link
Copy Markdown
Member

Thank you for the detail. If you can open a PR with just a fix for the problem and not break existing features, that would be great.

@yakatz yakatz closed this Feb 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backwards-incompatible This change will lead to a major version bump for the next release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants