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

Issue/718 default sg rule #748

Merged
merged 26 commits into from
Nov 8, 2024
Merged

Issue/718 default sg rule #748

merged 26 commits into from
Nov 8, 2024

Conversation

fraugabel
Copy link
Contributor

added alternative test, because the current test only works for the latest versions of the network and compute services, though older versions are not depricated yet

Copy link
Contributor

@mbuechse mbuechse left a comment

Choose a reason for hiding this comment

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

Error handling should be improved (don't just swallow exceptions by printing something to stdout), assert statements should not be used in production, and I suppose the code can be made less redundant and more concise.

@mbuechse
Copy link
Contributor

mbuechse commented Oct 25, 2024

BTW, I noticed that the test script is incredibly noisy when --debug is used. Okay, probably fine. However, logging is not initialized, so the channel is not included in the log line. This should be changed -- please include the following line:

    logging.basicConfig(format='%(levelname)s: %(message)s', level=logging.INFO)

And if --debug is supplied, the following must be done:

    logging.getLogger().setLevel(logging.DEBUG)

Last not least, the output of the script is not usable in any way. What we need is the following:

  • for every violation of a requirement: output a message on the ERROR channel
  • for every violation of a recommendation: output a message on the WARNING channel
  • the return code should be non-zero precisely when an ERROR had been emitted

Every other output should be on the debug or info channel.

Copy link
Contributor

@mbuechse mbuechse left a comment

Choose a reason for hiding this comment

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

I massaged it a bit. LGTM -- please check whether it looks good to you, too.

Signed-off-by: Katharina Trentau <[email protected]>
Signed-off-by: Katharina Trentau <[email protected]>
Signed-off-by: Katharina Trentau <[email protected]>
Signed-off-by: Katharina Trentau <[email protected]>
Signed-off-by: Katharina Trentau <[email protected]>
Signed-off-by: Katharina Trentau <[email protected]>
Signed-off-by: Katharina Trentau <[email protected]>
Signed-off-by: Katharina Trentau <[email protected]>
Signed-off-by: Katharina Trentau <[email protected]>
Signed-off-by: Katharina Trentau <[email protected]>
Signed-off-by: Katharina Trentau <[email protected]>
Signed-off-by: Katharina Trentau <[email protected]>
Signed-off-by: Katharina Trentau <[email protected]>
Signed-off-by: Katharina Trentau <[email protected]>
Signed-off-by: Katharina Trentau <[email protected]>
Signed-off-by: Katharina Trentau <[email protected]>
Signed-off-by: Katharina Trentau <[email protected]>
fraugabel and others added 6 commits November 8, 2024 08:40
Signed-off-by: Katharina Trentau <[email protected]>
Signed-off-by: Katharina Trentau <[email protected]>
Signed-off-by: Katharina Trentau <[email protected]>
Signed-off-by: Matthias Büchse <[email protected]>
Signed-off-by: Matthias Büchse <[email protected]>
This reverts commit 1092abe.

Signed-off-by: Matthias Büchse <[email protected]>
@mbuechse mbuechse merged commit 4ca42f9 into main Nov 8, 2024
8 checks passed
@mbuechse mbuechse deleted the issue/718_default_sg_rule branch November 8, 2024 08:50
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.

make compliance test for scs-0115-v1: Default Rules for Security Groups downward compatible
2 participants