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

compute rule update delta against the cloud in plugins #276

Merged
merged 1 commit into from
Jul 14, 2023

Conversation

shenmo3
Copy link
Contributor

@shenmo3 shenmo3 commented Jul 11, 2023

Description

Currently, the plugin directly invokes cloud APIs with the update rule set passed from the network policy. However, this approach becomes problematic if the cloud rule indexer is out of sync with the actual cloud state, due to user actions or other failures. To address this issue, this PR introduces a solution that fetches and compares the current cloud rules with the update sets in the plugin before invoking cloud APIs. It computes the final sets of rules that need to be updated based on this comparison.

Changes

  1. Implement normalization for AWS returned rules and introduced logic for deduplication and finding duplicates. Deduplication is used to compare the add rule set and avoid adding rules that already exist in the cloud. Finding duplicates is used to compare the remove rule set and avoid removing rules that do not exist in the cloud.
  2. Move rule conversion to a separate function instead of inside cloud API call function.
  3. Implement deduplication checks for Azure to prevent adding rules that already exist in the cloud. The logic for finding duplicates already existed.
  4. Move adding Azure default deny rule to separate func to avoid unnecessary comparisons in delta compute logic.

@shenmo3 shenmo3 self-assigned this Jul 11, 2023
Comment on lines 140 to 142

func convertIngressToIpPermission(rules []*cloudresource.CloudRule, cloudSGNameToObj map[string]*ec2.SecurityGroup) (
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: add a function description.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

}
description, err := utils.GenerateCloudDescription(obj.NpNamespacedName)
if err != nil {
return nil, fmt.Errorf("unable to generate rule description, err: %v", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

If any of the rules do not contain description, you fail the entire conversion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is generating description for Nephe rules, which shouldn't fail at all. In an error case, we do want to fail the entire conversion or operation so the cloud won't be in partial state.

FromPort: ipPermission.FromPort,
IpProtocol: ipPermission.IpProtocol,
ToPort: ipPermission.ToPort,
UserIdGroupPairs: []*ec2.UserIdGroupPair{{GroupId: group.GroupId, Description: group.Description}},
Copy link
Contributor

Choose a reason for hiding this comment

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

Why Description is stored only for user-groups? Isnt not needed for ipblocks

Copy link
Contributor Author

@shenmo3 shenmo3 Jul 12, 2023

Choose a reason for hiding this comment

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

For groups we are constructing new UserIdGroupPair with only groupId and Description to ignore other fields. For IpRange we are directly copying the object from previous slice into the new slice, which contains both cidr and description.


addIngressRules = dedupIpPermissions(addIngressRules, cloudSGObjToAddRules.IpPermissions)
addEgressRules = dedupIpPermissions(addEgressRules, cloudSGObjToAddRules.IpPermissionsEgress)
removeIngressRules = findDupIpPermissions(removeIngressRules, cloudSGObjToAddRules.IpPermissions)
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this for now..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

normalizedRule := normalizeAzureSecurityRule(rule)
idx, found := findSecurityRule(normalizedRule, removeRules)
// skip the rule if found in remove list.
idx, found := findSecurityRule(normalizedRule, removeAzureRules)
Copy link
Contributor

Choose a reason for hiding this comment

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

swap the arguments

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@reachjainrahul
Copy link
Contributor

/nephe-test-e2e-agentless

Copy link
Contributor

@reachjainrahul reachjainrahul left a comment

Choose a reason for hiding this comment

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

/LGTM

@reachjainrahul reachjainrahul merged commit c2e7b91 into main Jul 14, 2023
@reachjainrahul reachjainrahul deleted the aws-delta branch July 14, 2023 18:22
@reachjainrahul reachjainrahul added this to the Nephe v0.6.0 release milestone Jul 24, 2023
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.

3 participants