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

Simplify Disallow hostPorts in CEL expressions #1108

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

lavishpal
Copy link

Related Issue(s)

fixes #1093

Description

This PR simplify the CEL expression by removing the repeated terms.

Checklist

  • [] I have read the policy contribution guidelines.
  • [] I have added test manifests and resources covering both positive and negative tests that prove this policy works as intended.
  • [] I have added the artifacthub-pkg.yml file and have verified it is complete and correct.

@Chandan-DK
Copy link
Contributor

Chandan-DK commented Aug 3, 2024

Hi @lavishpal - Thanks for the contribution!

We can create a variable allContainers to hold all the containers.

      validate:
        cel:
          variables:
            - name: allContainers
              expression: >-
                object.spec.containers + 
                object.spec.?initContainers.orValue([]) + 
                object.spec.?ephemeralContainers.orValue([])

We can then use this variable and simplify expression to:

         expressions:
            - expression: >- 
                variables.allContainers.all(container, 
                container.?ports.orValue([]).all(port, port.?hostPort.orValue(0) == 0))

@lavishpal
Copy link
Author

Yes we can use variable, but we can also directly use optional .

@Chandan-DK
Copy link
Contributor

Yes, using the optional syntax directly in the expression is a valid approach and works well. However, it's easier to read the expression when we can generalize a large expression like this into a variable.

object.spec.containers + object.spec.?initContainers.orValue([]) + object.spec.?ephemeralContainers.orValue([])

The newer CEL policies in the library make use of the allContainers variable, and it would help keep it consistent with that.

@lavishpal
Copy link
Author

cc : @JimBugwadia

Copy link
Contributor

@chipzoller chipzoller left a comment

Choose a reason for hiding this comment

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

Multiple tests failing.

@lavishpal lavishpal force-pushed the simplify-cel-host-ports branch 10 times, most recently from 237e5a2 to 3b925f4 Compare August 6, 2024 12:13
@lavishpal lavishpal requested a review from chipzoller August 6, 2024 12:24
Copy link
Contributor

@chipzoller chipzoller left a comment

Choose a reason for hiding this comment

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

Tests are failing.

@lavishpal lavishpal force-pushed the simplify-cel-host-ports branch from 3b925f4 to ab6d073 Compare August 9, 2024 14:54
@lavishpal
Copy link
Author

Multiple tests failing.

Can you guide me how to fix these test.

@chipzoller
Copy link
Contributor

Once CI completes, go look at the failing tests and see the logged messages.

@lavishpal lavishpal force-pushed the simplify-cel-host-ports branch from ab6d073 to 6382933 Compare August 13, 2024 11:19
@lavishpal lavishpal force-pushed the simplify-cel-host-ports branch from 5045ac9 to 5688761 Compare November 13, 2024 22:13
@realshuting
Copy link
Member

All disallow-host-ports tests failed, something's broken.

@lavishpal
Copy link
Author

Yeah i figured out that there is indentation mistake in YAML.

@realshuting
Copy link
Member

@lavishpal - are you fixing it?

@lavishpal
Copy link
Author

@lavishpal - are you fixing it?

Yeah i will fix it till tomorrow.

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.

disallow-host-ports: simplify CEL expressions
4 participants