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

policy: Create derivative policy for EgressDeny rules #23927

Merged
merged 2 commits into from Mar 7, 2023

Conversation

rockc2020
Copy link
Contributor

@rockc2020 rockc2020 commented Feb 22, 2023

This PR is to create derivative policy when egressDeny including toGroups rules.

With issue #23829, there is no derivative policy created when egressDeny including toGroups rules.

This fix includes following changes:

  • Add check for EgressDeny in method RequiresDerivative()
  • Add derivative policy creation for EgressDenyRule in method CreateDerivative()
  • Add unit tests for both methods

I also built an image with this change from my local and deployed in my EKS cluster. Then, I created a ccnp:

apiVersion: cilium.io/v2
kind: CiliumClusterwideNetworkPolicy
metadata:
  name: egress-global
spec:
  egressDeny:
    - toGroups:
        - aws:
            securityGroupsIds:
              - sg-07283569446d92723
  endpointSelector:
    matchExpressions:
      - key: app
        operator: In
        values:
          - network-multitool

Then, a derivative policy was created successfully:

apiVersion: cilium.io/v2
kind: CiliumClusterwideNetworkPolicy
metadata:
  labels:
    io.cilium.network.policy.kind: derivative
    io.cilium.network.policy.parent.uuid: 6b2c336b-1608-4611-b63d-21acd36fa75f
  managedFields:
    - apiVersion: cilium.io/v2
      fieldsType: FieldsV1
      fieldsV1:
        f:specs: {}
      manager: Go-http-client
      operation: Update
      time: '2023-02-22T02:07:18Z'
  name: egress-global-togroups-6b2c336b-1608-4611-b63d-21acd36fa75f
  ownerReferences:
    - apiVersion: cilium.io/v2
      kind: CiliumClusterwideNetworkPolicy
      name: egress-global
      uid: 6b2c336b-1608-4611-b63d-21acd36fa75f
  resourceVersion: '6154562'
  uid: 1e102dec-f882-4c08-817f-34b013c78b37
  selfLink: >-
    /apis/cilium.io/v2/ciliumclusterwidenetworkpolicies/egress-global-togroups-6b2c336b-1608-4611-b63d-21acd36fa75f
specs:
  - egressDeny:
      - toCIDRSet:
          - cidr: 10.103.25.222/32
          - cidr: 10.103.71.146/32
          - cidr: 10.103.217.142/32
    endpointSelector:
      matchExpressions:
        - key: any:app
          operator: In
          values:
            - network-multitool
    labels:
      - key: io.cilium.k8s.policy.derived-from
        source: k8s
        value: CiliumClusterwideNetworkPolicy
      - key: io.cilium.k8s.policy.name
        source: k8s
        value: egress-global
      - key: io.cilium.k8s.policy.uid
        source: k8s
        value: 6b2c336b-1608-4611-b63d-21acd36fa75f

Fixes: #23829

policy: Derivative policies (policies for cloud provider-specific identities) for egress deny rules were not being generated, this has now been fixed. 

This PR is to generate derivative policy when `egressDeny` including `toGroups` rules.

Fixes: cilium#23829

Signed-off-by: Rocky Chen <rocky.chen@outlook.com>
@rockc2020 rockc2020 requested a review from a team as a code owner February 22, 2023 02:51
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Feb 22, 2023
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Feb 22, 2023
@maintainer-s-little-helper
Copy link

Commit 72c34e85f2c66a6e269662eade4851c00b67acad does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Feb 22, 2023
Signed-off-by: Rocky Chen <rocky.chen@outlook.com>
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Feb 22, 2023
@nathanjsweet nathanjsweet added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Feb 22, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Feb 22, 2023
@nathanjsweet
Copy link
Member

Thanks for the PR @rockc2020, I'm off tomorrow, but I'll take a thorough look at this on Monday.

@nathanjsweet
Copy link
Member

/test

@sayboras sayboras changed the title Create derivative policy for EgressDeny rules policy: Create derivative policy for EgressDeny rules Feb 28, 2023
@rockc2020
Copy link
Contributor Author

hi @nathanjsweet as 1 check failed with my PR https://github.com/cilium/cilium/actions/runs/4238946330/jobs/7379640730. after I had a run on my local, it seems some errors of ERROR:NO_AUTHOR_SIGN_OFF that some commits missing Signed-off-by. I guess that might be because I merged master by clicking the update branch button before. so do I need to just revert that merging commit and using rebase again? or just wait for reviewing and then update?

@nathanjsweet
Copy link
Member

checkpatch is complaining about unrelated commits (checking them because the rebase is behind) to this PR.

@nathanjsweet nathanjsweet added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Mar 6, 2023
@YutaroHayakawa YutaroHayakawa merged commit bb408ed into cilium:master Mar 7, 2023
@rockc2020 rockc2020 deleted the engressDeny-toGroups branch March 7, 2023 04:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/community-contribution This was a contribution made by a community member. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NetworkPolicy: toGroups rule in egressDeny policy doesn't generate derivative policy
3 participants