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

Multiple Policy Files #231

Merged
merged 9 commits into from
Apr 5, 2024
Merged

Conversation

jacobsee
Copy link
Contributor

@jacobsee jacobsee commented Apr 4, 2024

Update policy app config & config loader to support multiple policy files.
Files are merged and the merged policy document is validated after all files are loaded.
Added a check to disallow multiple bindings of the same action to the same type.

@jacobsee jacobsee requested review from a team as code owners April 4, 2024 18:28
…iles. Files are merged and the merged policy document is validated after all files are loaded. Added a check to disallow multiple bindings of the same action to the same type.

Signed-off-by: Jacob See <[email protected]>
bailinhe
bailinhe previously approved these changes Apr 4, 2024
Copy link
Contributor

@bailinhe bailinhe left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

Comment on lines 147 to 153
mergedPolicy.ActionBindings = append(mergedPolicy.ActionBindings, filePolicy.ActionBindings...)

mergedPolicy.Actions = append(mergedPolicy.Actions, filePolicy.Actions...)

mergedPolicy.ResourceTypes = append(mergedPolicy.ResourceTypes, filePolicy.ResourceTypes...)

mergedPolicy.Unions = append(mergedPolicy.Unions, filePolicy.Unions...)
Copy link
Contributor

Choose a reason for hiding this comment

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

Something that worries me, is the policy document getting extended and this function getting forgotten about.

Possibly having a method on the policy document that accepts another document and merges them so the logic is kept within the struct methods and a little closer to home feels safer? But I'm not sure how realistic my concern is. But thought I'd mention it.

mikemrm
mikemrm previously approved these changes Apr 4, 2024
Copy link
Contributor

@mikemrm mikemrm left a comment

Choose a reason for hiding this comment

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

Looks good. I think there might be one potential change. But I'm not sure it a concern we might really have.

Copy link
Contributor

@jnschaeffer jnschaeffer left a comment

Choose a reason for hiding this comment

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

I will be bad cop here: Let's document the merge logic in a Markdown file in the docs directory.

… to a function on the PolicyDocument itself

Signed-off-by: Jacob See <[email protected]>
@jacobsee jacobsee dismissed stale reviews from mikemrm and bailinhe via 02231be April 4, 2024 20:35
jnschaeffer
jnschaeffer previously approved these changes Apr 4, 2024
…ntains one file called policy.yaml, accept a config map (and mount point for it) and don't set the file by environment variable at all. The list of policy files to be read by permissions-api should instead be set as a list in the configuration file, exactly the same as it is for local development.

Signed-off-by: Jacob See <[email protected]>
Copy link
Contributor

@mikemrm mikemrm left a comment

Choose a reason for hiding this comment

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

I believe the only thing missing now is the values.yaml doesn't have a reference which is helpful for someone trying to figure out the configuration the the helm values.

mikemrm
mikemrm previously approved these changes Apr 5, 2024
…irectory, from which all files are read and interpreted as policy documents. This change was made because it makes it considerably easier to deploy this app as a subchart of another chart, which deploys the actoal policy configmap to be mounted as a volume

Signed-off-by: Jacob See <[email protected]>
Comment on lines 175 to 179
for _, file := range files {
if !file.IsDir() {
filePaths = append(filePaths, directoryPath+"/"+file.Name())
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we only accept yaml files, we might want to exclude non .yml or .yaml extensions.

@jacobsee jacobsee merged commit 23c9d73 into infratographer:main Apr 5, 2024
4 checks passed
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.

4 participants