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

The example of disallowed/allowed ingress resources in the unique ingress host example has incorrect hostnames #484

Closed
WilliamRockwellEvans opened this issue Feb 12, 2024 · 10 comments
Labels

Comments

@WilliamRockwellEvans
Copy link

Link: https://open-policy-agent.github.io/gatekeeper-library/website/validation/uniqueingresshost/
The constraint template example uses a simple host_1 == host_2 logic, but the allowed and disallowed examples don't share the same host name -- accordingly the disallowed resource isn't blocked.

example-allowed:
example-allowed-host.example.com
example-allowed-host1.example.com

example-disallowed:
example-host.example.com

example-disallowed2:
example-host2.example.com
example-host3.example.com

@apeabody
Copy link
Contributor

Hi Everyone!

There are three independent tests cases/samples, the resources aren't shared between them:

https://github.com/open-policy-agent/gatekeeper-library/blob/master/library/general/uniqueingresshost/suite.yaml

  - name: example-allowed
    object: samples/unique-ingress-host/example_allowed.yaml
    assertions:
    - violations: no
  - name: example-disallowed
    object: samples/unique-ingress-host/example_disallowed.yaml
    inventory:
    - samples/unique-ingress-host/example_inventory_disallowed.yaml
    assertions:
    - violations: yes
  - name: example-disallowed2
    object: samples/unique-ingress-host/example_disallowed2.yaml
    inventory:
    - samples/unique-ingress-host/example_inventory_disallowed2.yaml
    assertions:
    - violations: yes

example-allowed: (0 violations)

example-disallowed: (1 violation)

example-disallowed2: (1 violation)

@JaydipGabani - These tests appears to be working as intended to me, can you confirm?

@JaydipGabani
Copy link
Contributor

JaydipGabani commented Feb 16, 2024

@apeabody I think the issue refers to the information that is directly user-facing on the library website where the available allowed and disallowed examples appear to have not been violating the policy.

As in if user if trying out the policy,

  • user applies the policy
  • user applies allowed example
  • then user applies disallowed example expecting it to generate violation/get denied - the host does not match currently between allowed and disallowed - but the disallowed object gets created leaving the user thinking the policy is faulty

The tests are working as intended as inventory is in direct conflict with examples however inventory objects are not part of the policy documentation on the website. So on the website, naming examples allowed and disallowed makes it so that user might think these provided examples are supposed to be conflicting - that is the case for many policies that do not require sync as far as I can tell.

@apeabody
Copy link
Contributor

Thanks @JaydipGabani!

I think the issue refers to the information that is directly user-facing on the library website where the available allowed and disallowed examples appear to have not been violating the policy.

Got it, makes total sense. Would it perhaps be more sustainable to automate inclusion of the "missing inventory resources into the documentation examples? This gap could potentially apply to all templates/samples which use data.inventory, and it might not be feasible to manually "fix" all of them in this manner. Nor do we have any sort of automated testing to avoid drift in the future.

@JaydipGabani
Copy link
Contributor

@apeabody Agreed! I took a look and there are some policies that uses data,inventory. For instance - hpa policy requires additional nginx deployment to exists out of box.

Do you suggest merging the associated PR for this and fixing this for now and opening a new issue to fix a greater problem?
or do you want to close this issue without merging the PR and open a new issue to fix a greater problem?

@apeabody
Copy link
Contributor

Do you suggest merging the associated PR for this and fixing this for now and opening a new issue to fix a greater problem?
or do you want to close this issue without merging the PR and open a new issue to fix a greater problem?

No concerns with the current PR. :) More a point that the tests are working as intended, so the PR is a workaround for what is really a documentation gap. So we should have an open issue to fix that regardless.

Copy link

stale bot commented Apr 17, 2024

This issue/PR has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Apr 17, 2024
@JaydipGabani
Copy link
Contributor

still valid

@stale stale bot removed the stale label Apr 17, 2024
Copy link

stale bot commented Jun 16, 2024

This issue/PR has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jun 16, 2024
@JaydipGabani
Copy link
Contributor

Not stale

@stale stale bot removed the stale label Jun 17, 2024
Copy link

stale bot commented Aug 16, 2024

This issue/PR has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Aug 16, 2024
@stale stale bot closed this as completed Aug 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants