Added a check to "osdctl network verify-egress" for public subnets supplied by user with --subnet-id#508
Conversation
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
aliceh
changed the title
|
cancel /hold |
|
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
aliceh
changed the title
| } | ||
|
|
||
| // This function checks the gateway attached to the subnet and returns true if the subnet starts with igw- (for InternetGateway) and has a route to 0.0.0.0/0 | ||
| func (e *EgressVerification) isSubnetPublic(ctx context.Context, subnetID string) (bool, error) { |
There was a problem hiding this comment.
This code seems to be similar to https://github.com/openshift/osdctl/blob/master/cmd/cluster/cpd.go#L125
Can you pull out the common logic into a separate shared function?
There was a problem hiding this comment.
Can you show me an example where this has been done in osdctl or other repo?
There was a problem hiding this comment.
I linked the other function above and that function and this function have most of the same code. I would recommend creating a third function, something called findRouteTableForSubnet and placing all the shared code there, then calling it within isSubnetPublic and the other fucntion I linked
There was a problem hiding this comment.
Where should I put this shared function? A new file in /pkg/utils ? Just want to make sure I understand what the ask is.
There was a problem hiding this comment.
I am thinking /pkg/utils/utils.go
There was a problem hiding this comment.
The aws client used by verifier.go and cpd.go implement two separate, incompatible interfaces and a refactor is non-trivial. It would likely involve having the code in verifier.go create one of our AWS Client wrappers defined here instead of using the raw ec2.Client it does today.
@aliceh I'm happy to pair on this if you'd like, let me know!
There was a problem hiding this comment.
I spent some time digging into this issue this morning and speaking with @mjlshen in this slack thread- I think the right path forward here is to leave this for now and have a future effort to clean up all of the functions in this interface such that they don't hide the context parameter. Then our wrapper interface will be directly compatible with base AWS clients while still allowing all of the helper functions we provide still.
Curious to hear other thoughts, as I think a refactor here would be a bit large for this PR.
There was a problem hiding this comment.
All the better - I'm also perfectly happy deprecating the wrapper interface as a whole and directly using clients from the aws go sdk :)
|
Ran into this while on-call today. Any chance we can get this merged soon? @aliceh |
|
I'll look at it today. |
openshift-merge-robot
added
the
needs-rebase
openshift-merge-robot
removed
the
needs-rebase
| }, | ||
| }, | ||
| }, | ||
| describeRouteTablesResp: &ec2.DescribeRouteTablesOutput{ |
There was a problem hiding this comment.
I think a few other tests also need this describeRouteTablesResp property defined now so they can pass.
|
As an FYI, this is the CAD implementation: https://github.com/openshift/configuration-anomaly-detection/pull/276/files |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aliceh, clcollins The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
@aliceh: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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. |
7 participants
In this commit, I added a check when a subnet is supplied by a user with --sibnet-id. In the code, I check whether a subnet is public with isSubnetPublic function that checks if there is an internet gateway attached to the subnet and whether the RouteTable for the subnet has a default route to 0.0.0.0/0.