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

analyze: start parsing anps and banp from kube server or path #239

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Peac36
Copy link
Contributor

@Peac36 Peac36 commented Jul 11, 2024

Issue: #201

@huntergregory - As the requester of the issue can you review the PR?


Notes:

  • Change the GetNetworkPoliciesInNamespaces function to accept context as the first argument.
  • Introduce GetAdminNetworkPoliciesInNamespace and GetBaseAdminNetworkPoliciesInNamespace methods
  • Use concurrency to get NetworkPolicies, ANPs, and BANP.
  • In case BANP is fetched from multiple sources(kube server or path). The latest successful fetch would be used.

Questions:

I wasn't sure if I could add a flag for context timeout so I hardcode it for now. No problem to add if you don't see a problem with that.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 11, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Peac36
Once this PR has been reviewed and has the lgtm label, please assign huntergregory for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

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

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 11, 2024
@k8s-ci-robot
Copy link
Contributor

Hi @Peac36. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@k8s-ci-robot k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jul 11, 2024
Copy link

netlify bot commented Jul 11, 2024

Deploy Preview for kubernetes-sigs-network-policy-api ready!

Name Link
🔨 Latest commit 2adcf15
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-network-policy-api/deploys/66c57a726388d10008fc69e0
😎 Deploy Preview https://deploy-preview-239--kubernetes-sigs-network-policy-api.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Contributor

@huntergregory huntergregory left a comment

Choose a reason for hiding this comment

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

Thank you @Peac36 ! This is a huge help, solving a large missing piece for policy-assistant.

For getting policies from API Server, could we check if ANP/BANP resources are defined before trying to get them? If they do exist, I think it would be more natural to have a panic/fatal if a GET call fails.

Also I'm not sure we need to optimize with concurrent GET calls but wdyt? If we use concurrency, could we simplify that code with sync.WaitGroup or errgroup.Group?

Otherwise, had some thoughts about the k8s interface and miscellaneous nitpicks.

cmd/policy-assistant/anps/anp-list.yaml Outdated Show resolved Hide resolved
cmd/policy-assistant/pkg/cli/analyze.go Outdated Show resolved Hide resolved
cmd/policy-assistant/pkg/cli/analyze.go Outdated Show resolved Hide resolved
cmd/policy-assistant/pkg/kube/ikubernetes.go Outdated Show resolved Hide resolved
cmd/policy-assistant/pkg/cli/analyze.go Outdated Show resolved Hide resolved
cmd/policy-assistant/pkg/kube/ikubernetes.go Outdated Show resolved Hide resolved
cmd/policy-assistant/pkg/kube/read.go Outdated Show resolved Hide resolved
cmd/policy-assistant/pkg/kube/read.go Outdated Show resolved Hide resolved
cmd/policy-assistant/pkg/kube/read_tests.go Show resolved Hide resolved
cmd/policy-assistant/pkg/kube/read.go Outdated Show resolved Hide resolved
@Peac36
Copy link
Contributor Author

Peac36 commented Jul 21, 2024

For getting policies from API Server, could we check if ANP/BANP resources are defined before trying to get them?

Can you help me with this ? I can not find a way to achieve this. I guess I need to check if ANP and BANP are registered in api-server as CRD but couldn't find a clientset method that can achieve that.
The only way I found was by using a hardcoded URI to the API server.

Also I'm not sure we need to optimize with concurrent GET calls but wdyt?

As the user of the command, I should expect to see the information as fast as possible. Till now we executed only one request to the server which was fine, but now we add 2 more or even more. Imagine the kube api-server is slow to respond - it takes about 5 seconds to return a response. So if we run these three requests sequentially - it would take about 15 seconds in which the client will not see anything on the screen. On the other side with concurrency, we might get the result much faster which I'm sure would make the user much happier.

If we use concurrency, could we simplify that code with sync.WaitGroup or errgroup.Group?

Yeah, sure, I'll need more time to test it.

@huntergregory
Copy link
Contributor

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 21, 2024
@huntergregory
Copy link
Contributor

For getting policies from API Server, could we check if ANP/BANP resources are defined before trying to get them?

Can you help me with this ? I can not find a way to achieve this. I guess I need to check if ANP and BANP are registered in api-server as CRD but couldn't find a clientset method that can achieve that. The only way I found was by using a hardcoded URI to the API server.

I’m not sure off the top of my head, but perhaps there’s an equivalent to kubectl get resources, or get crds? Otherwise, maybe there’s a certain error type returned from kubectl get adminnetworkpolicies when anp doesn’t exist.

@Peac36
Copy link
Contributor Author

Peac36 commented Aug 11, 2024

I’m not sure off the top of my head, but perhaps there’s an equivalent to kubectl get resources, or get crds? Otherwise, maybe there’s a certain error type returned from kubectl get adminnetworkpolicies when anp doesn’t exist.

Ok, I found it. DiscoveryClient can get us all registered resources.

I'm ready with the PR, but want to test it more. Do you know where I can find the latest CRD?. The ones in the project seem to be outdated and using one of the example policies yields following error:

panic: invalid admin peer: must have exactly one of NamespaceSelector, SameLabels, or NotSameLabels

@huntergregory
Copy link
Contributor

huntergregory commented Aug 12, 2024

Ok, I found it. DiscoveryClient can get us all registered resources.

I'm ready with the PR, but want to test it more. Do you know where I can find the latest CRD?. The ones in the project seem to be outdated and using one of the example policies yields following error:

panic: invalid admin peer: must have exactly one of NamespaceSelector, SameLabels, or NotSameLabels

Nice find! For creating (B)ANPs, can you try this if you haven't already:

  • Go to this repo's Releases and apply the latest install.yaml. As of today, that is: kubectl apply -f https://github.com/kubernetes-sigs/network-policy-api/releases/download/v0.1.1/install.yaml

The latest CRD is encoded in that install.yaml. I'm also adding some new example (B)ANPs in #245 under examples/.

EDIT: assuming you were looking at https://network-policy-api.sigs.k8s.io/getting-started/ ?
Curious which example policy you were using which gave the above panic? That panic is the correct behavior (or at least the error message is correct).

@Peac36
Copy link
Contributor Author

Peac36 commented Aug 14, 2024

Ok, I found it. DiscoveryClient can get us all registered resources.
I'm ready with the PR, but want to test it more. Do you know where I can find the latest CRD?. The ones in the project seem to be outdated and using one of the example policies yields following error:

panic: invalid admin peer: must have exactly one of NamespaceSelector, SameLabels, or NotSameLabels

Nice find! For creating (B)ANPs, can you try this if you haven't already:

  • Go to this repo's Releases and apply the latest install.yaml. As of today, that is: kubectl apply -f https://github.com/kubernetes-sigs/network-policy-api/releases/download/v0.1.1/install.yaml

The latest CRD is encoded in that install.yaml. I'm also adding some new example (B)ANPs in #245 under examples/.

EDIT: assuming you were looking at https://network-policy-api.sigs.k8s.io/getting-started/ ? Curious which example policy you were using which gave the above panic? That panic is the correct behavior (or at least the error message is correct).

For resource definitions : https://github.com/kubernetes-sigs/network-policy-api/tree/main/config/crd
Example from here: https://github.com/kubernetes-sigs/network-policy-api/tree/main/conformance/base


I'm ready with the PR, feel free to look through it.

Copy link
Contributor

@huntergregory huntergregory left a comment

Choose a reason for hiding this comment

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

Thanks @Peac36 for making those changes and thanks for adding all those nice example policies. I have some minor requests, then I think we can merge

cmd/policy-assistant/pkg/kube/ikubernetes.go Outdated Show resolved Hide resolved
cmd/policy-assistant/pkg/kube/ikubernetes.go Outdated Show resolved Hide resolved
cmd/policy-assistant/pkg/cli/analyze.go Outdated Show resolved Hide resolved
cmd/policy-assistant/pkg/cli/analyze.go Outdated Show resolved Hide resolved
cmd/policy-assistant/pkg/cli/analyze.go Outdated Show resolved Hide resolved
@Peac36
Copy link
Contributor Author

Peac36 commented Aug 21, 2024

@huntergregory - Thanks for the feedback. The issues have been addressed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants