Skip to content

Removal of allowlist from priviledged tests #1928

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

Merged
merged 1 commit into from
Jul 25, 2024

Conversation

horecoli
Copy link
Contributor

@horecoli horecoli commented Mar 13, 2024

Description

This change updates priviledged test to consider
only containers from CNF under test. As a result of this change, the 'allowlist_helm_chart_container_names' parameter in the cnf-testsuite.yml configuration file is no longer needed.
Therefore this patch also removes this parameter from the configuration, as well as from all samples and examples.

Issues:

Refs: #1906

How has this been tested:

  • Covered by existing integration testing
  • Added integration testing to cover
  • Verified all A/C passes
    • develop
    • master
    • tag/other branch
  • Test environment
    • Shared Packet K8s cluster
    • New Packet K8s cluster
    • Kind cluster
  • Have not tested

Types of changes:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update

Checklist:

Documentation

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • No updates required.

Code Review

  • Does the test handle fatal exceptions, ie. rescue block

Issue

  • Tasks in issue are checked off

@HashNuke
Copy link
Collaborator

Thank you for the updates. Waiting for the build to run for the main branch.

I will re-run this build once the main branch is green - https://github.com/cnti-testcatalog/testsuite/actions/runs/8345433746

@taylor
Copy link
Member

taylor commented Mar 19, 2024

Needs further discussion and updates

@horecoli
Copy link
Contributor Author

horecoli commented Mar 28, 2024

@taylor, maybe you are right to have such exceptions in some cases. But the question is whether the allowlisting has to be done per-test or if there should be a common list of exceptions. Because as it is right now, we are giving the user the ability to make this test pass at any time. However, on the other side, there has to be motivation for the user to follow this best practice and refrain from making fraudulent attempts to pass the test.
Do we have any sample examples where privileged containers are necessary and need to be whitelisted?

@taylor taylor changed the title Removal of whitelisting from priviledged tests Removal of allowlist from priviledged tests Apr 2, 2024
Copy link
Member

@taylor taylor left a comment

Choose a reason for hiding this comment

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

  1. Remove updates in this PR which remove the feature allowlist -> we are keeping the feature
  2. manually test that each example CNFs pass the privilege mode test -> they are not checked by specs

@horecoli horecoli force-pushed the privileged_test_1906 branch from 252d720 to 5905b3f Compare April 8, 2024 07:21
@martin-mat
Copy link
Collaborator

lgtm

@horecoli horecoli changed the title Removal of allowlist from priviledged tests WIP: Removal of allowlist from priviledged tests Apr 11, 2024
@horecoli
Copy link
Contributor Author

This change is more or less done, but I am still verifying if all tests are running as expected.

@horecoli horecoli changed the title WIP: Removal of allowlist from priviledged tests Removal of allowlist from priviledged tests Apr 12, 2024
@taylor
Copy link
Member

taylor commented May 7, 2024

@horecoli rebase and this should be good to merge.

cc: @agentpoyo @HashNuke

Copy link
Collaborator

@martin-mat martin-mat left a comment

Choose a reason for hiding this comment

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

lgtm

@kosstennbl
Copy link
Collaborator

Some sample CNFs still have empty arrays:

  • sample-nonroot
  • sample_coredns_default_namespace

Copy link
Member

@Smitholi67 Smitholi67 left a comment

Choose a reason for hiding this comment

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

lgtm

This change updates privileged test to consider
only containers from CNF under test. As a result
of this change, the 'allowlist_helm_chart_container_names'
parameter in the cnf-testsuite.yml configuration file
of lot of examples and samples is no needed. Therefore
this patch also removes this parameter from cnf-testsuite.yml
in examples and samples where it is no need anymore.

Closes-Bug: lfn-cnti#1906
Signed-off-by: horecoli <[email protected]>
@horecoli horecoli force-pushed the privileged_test_1906 branch from 57ae261 to 9111444 Compare July 23, 2024 07:09
@horecoli
Copy link
Contributor Author

horecoli commented Jul 23, 2024

Some sample CNFs still have empty arrays:

  • sample-nonroot
  • sample_coredns_default_namespace

Those two samples were added after I created this PR, so at that time, they were not there. And now I just rebased and forgot to do some grep to check if there is something new. So I fixed it in the latest change. Thank you :)

Copy link
Collaborator

@kosstennbl kosstennbl left a comment

Choose a reason for hiding this comment

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

Needs CI rerun, otherwise - LGTM

@martin-mat martin-mat merged commit d5027ff into lfn-cnti:main Jul 25, 2024
87 checks passed
@horecoli horecoli deleted the privileged_test_1906 branch July 25, 2024 08:39
barmull pushed a commit to barmull/testsuite that referenced this pull request Jul 25, 2024
This change updates privileged test to consider
only containers from CNF under test. As a result
of this change, the 'allowlist_helm_chart_container_names'
parameter in the cnf-testsuite.yml configuration file
of lot of examples and samples is no needed. Therefore
this patch also removes this parameter from cnf-testsuite.yml
in examples and samples where it is no need anymore.

Closes-Bug: lfn-cnti#1906

Signed-off-by: horecoli <[email protected]>
barmull pushed a commit to barmull/testsuite that referenced this pull request Jul 25, 2024
This change updates privileged test to consider
only containers from CNF under test. As a result
of this change, the 'allowlist_helm_chart_container_names'
parameter in the cnf-testsuite.yml configuration file
of lot of examples and samples is no needed. Therefore
this patch also removes this parameter from cnf-testsuite.yml
in examples and samples where it is no need anymore.

Closes-Bug: lfn-cnti#1906

Signed-off-by: horecoli <[email protected]>
LuciaSirova pushed a commit that referenced this pull request Mar 27, 2025
This change updates privileged test to consider
only containers from CNF under test. As a result
of this change, the 'allowlist_helm_chart_container_names'
parameter in the cnf-testsuite.yml configuration file
of lot of examples and samples is no needed. Therefore
this patch also removes this parameter from cnf-testsuite.yml
in examples and samples where it is no need anymore.

Closes-Bug: #1906

Signed-off-by: horecoli <[email protected]>
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.

6 participants