OSD-26330: Refactor and add tests for `egress_lists` package #284

joshbranham · 2024-11-01T21:43:44Z

What does this PR do? / Related Issues / Jira

When working on a bugfix in #282, I wanted to consolidate duplicate code and clean up the control flow further for generating egress lists. This PR does just that, by first extracting the existing behavior into a common type, and adding tests for the various scenarios. A couple of notes:

I did not add validation for invalid PlatformType as was setup before, since the caller should be using the various exported vars for PlatformType and not manually constructing there own alias type with custom name values.
Opted for a small interface to abstract the GitHub API call to fetch contents
Moved the various Getters in egress_lists to function receivers on the Generator struct. These functions are not used anywhere in the openshift organization outside of this project.
Cleaned up some variables and comments

See below for output of running for both AWS and GCP:

AWS

> ./osd-network-verifier egress --profile devaccount --subnet-id subnet-0bab965a3722b750d --region us-west-2        
Using region: us-west-2
Using egress URL list from https://api.github.com/repos/openshift/osd-network-verifier/contents/pkg/data/egress_lists/aws-classic.yaml?ref=main at SHA c31b52add1381fa61e5c0446370c608e68feeb58
Created security group with ID: sg-03f26bb00f4a69df7
Created instance with ID: i-0e1d53172c1849c21
Deleting instance with ID: i-0e1d53172c1849c21
Deleting security group with ID: sg-03f26bb00f4a69df7
Summary:
All tests passed!
Success

GCP

Note: The GCP failure is expected as the egress list is still being validated

> GCP_PROJECT_ID=jbranham ./osd-network-verifier egress --platform gcp --vpc-name verifier-test-sg5b9-network --subnet-id verifier-test-sg5b9-worker-subnet        
Using Project ID jbranham
Using egress URL list from https://api.github.com/repos/openshift/osd-network-verifier/contents/pkg/data/egress_lists/gcp-classic.yaml?ref=main at SHA 92b7c45af595e5a23006d560222a57a83a0e61e3
Created instance with ID: verifier-1263
Applying labels
Successfully applied labels 
ComputeService Instance: verifier-1263 RUNNING
Gathering and parsing console log output...
Summary:
printing out failures:
 - egressURL error: https://events.amazonaws.com:443 (Could not resolve host: events.amazonaws.com)

printing out exceptions preventing the verifier from running the specific test:
printing out errors faced during the execution:
Failure!

Checklist

I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have made corresponding change to the default configuration files
I have added tests that prove my fix is effective or that my feature works
I have tested the functionality against gcp / aws, it doesn't cause any regression
I have added execution results to the PR's readme

Reviewer's Checklist

(This needs to be done after technical review) I've run the branch on my local, verified that the functionality is ok

How to test this PR locally / Special Instructions

You can perform a basic test locally by running the verifier and passing a custom list via the CLI

Logs

joshbranham · 2024-11-01T21:45:23Z

pkg/data/egress_lists/egress_lists.go

@@ -1,21 +1,14 @@
 package egress_lists

-// TRANSITIONAL IMPLEMENTATION (UNSTABLE API)


This ticket was closed a while ago, this comment is no longer valid.

joshbranham · 2024-11-01T21:47:13Z

pkg/verifier/aws/entry_point.go

+	// Generate both egress lists for the given PlatformType. Note: the result of this is ignored by the Legacy probe.
+	generatorVariables := map[string]string{"AWS_REGION": a.AwsClient.Region}
+	generator := egress_lists.NewGenerator(vei.PlatformType, generatorVariables, a.Logger)

-	egressListStr, tlsDisabledEgressListStr, err = egress_lists.EgressListToString(egressListYaml, map[string]string{"AWS_REGION": a.AwsClient.Region})
+	egressListStr, tlsDisabledEgressListStr, err := generator.GenerateEgressLists(vei.Ctx, vei.EgressListYaml)


The extraction of the fetching logic into egress_lists essentially allows us to have this common call flow across AWS + GCP, and reads more cleanly in the scope of this already large function.

codecov-commenter · 2024-11-01T21:50:09Z

Codecov Report

Attention: Patch coverage is 33.33333% with 34 lines in your changes missing coverage. Please review.

Project coverage is 26.39%. Comparing base (2b1f351) to head (47250ab).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/data/egress_lists/egress_lists.go	58.62%	10 Missing and 2 partials ⚠️
pkg/verifier/gcp/gcp_verifier.go	0.00%	9 Missing ⚠️
pkg/verifier/aws/entry_point.go	0.00%	8 Missing ⚠️
pkg/verifier/gcp/entry_point.go	0.00%	4 Missing ⚠️
pkg/verifier/aws/aws_verifier.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #284      +/-   ##
==========================================
+ Coverage   25.81%   26.39%   +0.58%     
==========================================
  Files          26       26              
  Lines        1844     1970     +126     
==========================================
+ Hits          476      520      +44     
- Misses       1340     1419      +79     
- Partials       28       31       +3

Files with missing lines	Coverage Δ
pkg/verifier/aws/aws_verifier.go	`35.94% <0.00%> (ø)`
pkg/verifier/gcp/entry_point.go	`0.00% <0.00%> (ø)`
pkg/verifier/aws/entry_point.go	`0.00% <0.00%> (ø)`
pkg/verifier/gcp/gcp_verifier.go	`0.00% <0.00%> (ø)`
pkg/data/egress_lists/egress_lists.go	`59.45% <58.62%> (+59.45%)`	⬆️

... and 1 file with indirect coverage changes

openshift-ci-robot · 2024-11-01T22:01:18Z

@joshbranham: This pull request references OSD-26330 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.18.0" version, but no target version was set.

In response to this:

What does this PR do? / Related Issues / Jira

When working on a bugfix in #282, I wanted to consolidate duplicate code and clean up the control flow further for generating egress lists. This PR does just that, by first extracting the existing behavior into a common type, and adding tests for the various scenarios. A couple of notes:

I did not add validation for invalid PlatformType as was setup before, since the caller should be using the various exported vars for PlatformType and not manually constructing there own alias type with custom name values.

Opted for a small interface to abstract the GitHub API call to fetch contents

Moved the various Getters in egress_lists to function receivers on the Generator struct. These functions are not used anywhere in the openshift organization outside of this project.

Cleaned up some variables and comments

See below for output of running for both AWS and GCP:

AWS
> ./osd-network-verifier egress --profile devaccount --subnet-id subnet-0bab965a3722b750d --region us-west-2        
Using region: us-west-2
Using egress URL list from https://api.github.com/repos/openshift/osd-network-verifier/contents/pkg/data/egress_lists/aws-classic.yaml?ref=main at SHA c31b52add1381fa61e5c0446370c608e68feeb58
Created security group with ID: sg-03f26bb00f4a69df7
Created instance with ID: i-0e1d53172c1849c21
Deleting instance with ID: i-0e1d53172c1849c21
Deleting security group with ID: sg-03f26bb00f4a69df7
Summary:
All tests passed!
Success
GCP

Note: The GCP failure is expected as the egress list is still being validated
> GCP_PROJECT_ID=jbranham ./osd-network-verifier egress --platform gcp --vpc-name verifier-test-sg5b9-network --subnet-id verifier-test-sg5b9-worker-subnet        
Using Project ID jbranham
Using egress URL list from https://api.github.com/repos/openshift/osd-network-verifier/contents/pkg/data/egress_lists/gcp-classic.yaml?ref=main at SHA 92b7c45af595e5a23006d560222a57a83a0e61e3
Created instance with ID: verifier-1263
Applying labels
Successfully applied labels 
ComputeService Instance: verifier-1263 RUNNING
Gathering and parsing console log output...
Summary:
printing out failures:
- egressURL error: https://events.amazonaws.com:443 (Could not resolve host: events.amazonaws.com)

printing out exceptions preventing the verifier from running the specific test:
printing out errors faced during the execution:
Failure!
Checklist

I have commented my code, particularly in hard-to-understand areas

I have made corresponding changes to the documentation

I have made corresponding change to the default configuration files

I have added tests that prove my fix is effective or that my feature works

I have tested the functionality against gcp / aws, it doesn't cause any regression

I have added execution results to the PR's readme

Reviewer's Checklist

(This needs to be done after technical review) I've run the branch on my local, verified that the functionality is ok

How to test this PR locally / Special Instructions

You can perform a basic test locally by running the verifier and passing a custom list via the CLI

Logs

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

joshbranham · 2024-11-03T01:25:42Z

pkg/data/egress_lists/egress_lists.go

 	path := "/pkg/data/egress_lists/"
-	if !platformType.IsValid() {
-		fmt.Printf("Platform type %s is invalid", platformType)


As described in the PR body, I removed this check, which was only printing the invalid platform and then continuing execution.

Do we want to change this to short circuit the execution at this point if the platform type is invalid? Worth noting there may be earlier checks before it gets this far that would short circuit already

We provide the valid platform types as global variables, and the method ByName(name string) (string, error), so it feels unnecessary to also then later validate a Platform is valid since callers should be utilizing the variables or ByName... to construct the type. This is further evident by the names field of the struct being private so it wouldn't be possible to construct an invalid one anyways afaict.

it wouldn't be possible to construct an invalid one anyways afaict

There are several points in the codebase where we return an empty (and therefore invalid) Platform{}

it wouldn't be possible to construct an invalid one anyways afaict

There are several points in the codebase where we return an empty (and therefore invalid) Platform{}

Based on my searching, this is only happening under error cases inside other functions and in tests. If an upstream caller is not handling that error, and then using the Platform{} then that is a bug that should be fixed. Was there another example you had of this (outside of tests and errors)?

Correct, there are currently no other cases currently where Platform{} is used in network-verifier directly but it is possible to return an invalid type (with error) through platform.ByName() or platform.IsValid(). As long as upstream callers handle that error I'm fine conceding the point and don't think the check is necessary here. As far as I can tell the ValidateEgress func handles that beforehand.

Correct, there are currently no other cases currently where Platform{} is used in network-verifier directly but it is possible to return an invalid type (with error) through platform.ByName() or platform.IsValid(). As long as upstream callers handle that error I'm fine conceding the point and don't think the check is necessary here. As far as I can tell the ValidateEgress func handles that beforehand.

Totally, and yup we handle the error validation already here in ValidateEgress (for AWS as an example)

pkg/data/egress_lists/egress_lists.go

abyrne55 · 2024-11-05T16:45:18Z

Successfully tested locally. Good stuff @joshbranham

/approve

openshift-ci · 2024-11-05T16:45:31Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abyrne55

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [abyrne55]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

dakotalongRH · 2024-11-05T20:40:53Z

/lgtm

openshift-ci · 2024-11-05T20:48:41Z

@joshbranham: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

openshift-ci bot requested review from dakotalongRH and reedcort November 1, 2024 21:44

joshbranham commented Nov 1, 2024

View reviewed changes

joshbranham changed the title ~~Refactor and add tests for egress_list package~~ OSD-26330: Refactor and add tests for egress_list package Nov 1, 2024

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Nov 1, 2024

joshbranham changed the title ~~OSD-26330: Refactor and add tests for egress_list package~~ OSD-26330: Refactor and add tests for egress_lists package Nov 1, 2024

joshbranham force-pushed the refactor-egress-list-generation branch 2 times, most recently from 4d64a5b to 0fec20c Compare November 3, 2024 01:23

joshbranham commented Nov 3, 2024

View reviewed changes

abyrne55 reviewed Nov 4, 2024

View reviewed changes

pkg/data/egress_lists/egress_lists.go Outdated Show resolved Hide resolved

abyrne55 reviewed Nov 4, 2024

View reviewed changes

pkg/data/egress_lists/egress_lists.go Outdated Show resolved Hide resolved

Refactor and add tests for egress_lists package

47250ab

joshbranham force-pushed the refactor-egress-list-generation branch from 0fec20c to 47250ab Compare November 5, 2024 15:46

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 5, 2024

openshift-ci bot assigned dakotalongRH Nov 5, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 5, 2024

openshift-merge-bot bot merged commit c75b952 into openshift:main Nov 5, 2024
6 checks passed

joshbranham deleted the refactor-egress-list-generation branch November 5, 2024 20:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OSD-26330: Refactor and add tests for `egress_lists` package #284

OSD-26330: Refactor and add tests for `egress_lists` package #284

joshbranham commented Nov 1, 2024 •

edited

Loading

joshbranham Nov 1, 2024

joshbranham Nov 1, 2024 •

edited

Loading

codecov-commenter commented Nov 1, 2024 •

edited

Loading

openshift-ci-robot commented Nov 1, 2024 •

edited by openshift-ci bot

Loading

What does this PR do? / Related Issues / Jira

AWS

GCP

Checklist

Reviewer's Checklist

How to test this PR locally / Special Instructions

Logs

joshbranham Nov 3, 2024

dakotalongRH Nov 5, 2024

joshbranham Nov 5, 2024

abyrne55 Nov 5, 2024 •

edited

Loading

joshbranham Nov 5, 2024

dakotalongRH Nov 5, 2024

joshbranham Nov 5, 2024

abyrne55 commented Nov 5, 2024

openshift-ci bot commented Nov 5, 2024

dakotalongRH commented Nov 5, 2024

openshift-ci bot commented Nov 5, 2024

		@@ -1,21 +1,14 @@
		package egress_lists

		// TRANSITIONAL IMPLEMENTATION (UNSTABLE API)

OSD-26330: Refactor and add tests for egress_lists package #284

OSD-26330: Refactor and add tests for egress_lists package #284

Conversation

joshbranham commented Nov 1, 2024 • edited Loading

What does this PR do? / Related Issues / Jira

AWS

GCP

Checklist

Reviewer's Checklist

How to test this PR locally / Special Instructions

Logs

joshbranham Nov 1, 2024

Choose a reason for hiding this comment

joshbranham Nov 1, 2024 • edited Loading

Choose a reason for hiding this comment

codecov-commenter commented Nov 1, 2024 • edited Loading

Codecov Report

openshift-ci-robot commented Nov 1, 2024 • edited by openshift-ci bot Loading

What does this PR do? / Related Issues / Jira

AWS

GCP

Checklist

Reviewer's Checklist

How to test this PR locally / Special Instructions

Logs

joshbranham Nov 3, 2024

Choose a reason for hiding this comment

dakotalongRH Nov 5, 2024

Choose a reason for hiding this comment

joshbranham Nov 5, 2024

Choose a reason for hiding this comment

abyrne55 Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

joshbranham Nov 5, 2024

Choose a reason for hiding this comment

dakotalongRH Nov 5, 2024

Choose a reason for hiding this comment

joshbranham Nov 5, 2024

Choose a reason for hiding this comment

abyrne55 commented Nov 5, 2024

openshift-ci bot commented Nov 5, 2024

dakotalongRH commented Nov 5, 2024

openshift-ci bot commented Nov 5, 2024

OSD-26330: Refactor and add tests for `egress_lists` package #284

OSD-26330: Refactor and add tests for `egress_lists` package #284

joshbranham commented Nov 1, 2024 •

edited

Loading

joshbranham Nov 1, 2024 •

edited

Loading

codecov-commenter commented Nov 1, 2024 •

edited

Loading

openshift-ci-robot commented Nov 1, 2024 •

edited by openshift-ci bot

Loading

abyrne55 Nov 5, 2024 •

edited

Loading