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

docs: fix expansion yaml example #2551

Merged
merged 3 commits into from
Feb 8, 2023

Conversation

sozercan
Copy link
Member

@sozercan sozercan commented Feb 1, 2023

Signed-off-by: Sertac Ozercan sozercan@gmail.com

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, using fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when the PR gets merged):
Fixes #

Special notes for your reviewer:

Signed-off-by: Sertac Ozercan <sozercan@gmail.com>
@davis-haba
Copy link
Contributor

@sozercan Is there a binary or plugin you use to format the YAML like this?

My IDE (Intellij) seems to have different opinions 😞

Copy link
Contributor

@maxsmythe maxsmythe left a comment

Choose a reason for hiding this comment

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

LGTM

@maxsmythe
Copy link
Contributor

+1 to davis's question, having a linter here could be good if we're trying to be opinionated

Signed-off-by: Sertac Ozercan <sozercan@gmail.com>
@sozercan
Copy link
Member Author

sozercan commented Feb 1, 2023

@davis-haba I just used vscode's yaml formatter.
That's a good idea for markdown linter but I am not aware of any linters that do linting/formatting inside code blocks (yaml)

Looks like there is a syntax issue here too, since spec.source and spec.match.origin are not valid.

match:
scope: Namespaced
origin: "Generated"
source: All
Copy link
Member Author

Choose a reason for hiding this comment

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

@davis-haba i couldn't get this to work with Generated, any ideas?

Copy link
Contributor

@davis-haba davis-haba Feb 2, 2023

Choose a reason for hiding this comment

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

How were you testing it, and what were you expecting to see?

I fed this manifest into gator expand, using source: "Generated", and it produced the expected Pod. Changing the Assign's source to source: "Original" caused the Assign to not be applied, as expected.

w.r.t. the mutator, if you are just looking to test Assign.match.source, then you could try directly creating a Pod (or deployment) and seeing if the side-car gets attached. I just tested this on-cluster and it worked for me (i.e. source: Generated did not add the side-car; but source: Original|All added it)

Copy link
Member Author

@sozercan sozercan Feb 2, 2023

Choose a reason for hiding this comment

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

I tested on a kind v1.25 cluster. I was expecting to see pod to get injected sidecar and args to be mutated.
Here's a quick recording: https://asciinema.org/a/qEoR30Hl210qL0Tw70ZiOaBwz (this is with same snippet from PR but with source: Generated)

Tested with gator expand too, works there using source: Generated.

Copy link
Contributor

@davis-haba davis-haba Feb 3, 2023

Choose a reason for hiding this comment

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

Since the mutators have source: Generated they will not apply to the real pod that gets created on the cluster. The mutators will only apply to the ephemeral expanded pod, that is used for validating the deployment.

The assumption is the user is using mutators to simulate a controller adding the sidecar, so that their deployments can be properly validated. The controller they are simulating should add the sidecar to the real pod, not the mutators.

Copy link
Member Author

@sozercan sozercan Feb 6, 2023

Choose a reason for hiding this comment

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

That makes sense. I think what's confusing is we say "When expanded, the above configs will produce the following Pod:" which is not true with source: Generated since the produced Pod does not have these configs.

I'll update with source: All (changes as is in this PR) if that sounds good to you? @davis-haba

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, I agree that could be confusing. I think changing the example to source: All makes sense.

Thanks!

kinds: ["Pod"]
versions: ["v1"]
match:
source: All
Copy link
Member Author

Choose a reason for hiding this comment

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

same here for modifyset

@sozercan sozercan changed the title docs: fix expansion yaml indentation docs: fix expansion yaml example Feb 1, 2023
@sozercan sozercan merged commit f3824f4 into open-policy-agent:master Feb 8, 2023
@sozercan sozercan deleted the docs-fix-expansion-yaml branch February 8, 2023 00:15
davis-haba pushed a commit to davis-haba/gatekeeper that referenced this pull request Mar 7, 2023
davis-haba pushed a commit to davis-haba/gatekeeper that referenced this pull request Mar 8, 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.

None yet

3 participants