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

Fix k8s nat manager logic and add --Xnat-kube-service-namespace flag #6088

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

Conversation

alex123012
Copy link

PR description

Now the Kubernetes nat manager requires cluster-wide permissions (list service resources in all namespaces). Also, it has a bug: when multiple besu networks are configured in the same cluster with the same service names in different namespaces, the nat manager may set an inappropriate IP address for the besu node due to enumerating services from all namespaces and checking only their names.

I suggest adding a new flag: --Xnat-kube-service-namespace to specify a concrete besu node service namespace and not use a list of services, but a get request for a specific service.

Fixed Issue(s)

fixes #5002

Signed-off-by: MAKHONIN Aleksey M <Aleksey.MAKHONIN@raiffeisen.ru>
@github-actions
Copy link

  • I thought about documentation and added the doc-change-required label to this PR if updates are required.
  • I thought about the changelog and included a changelog update if required.
  • If my PR includes database changes (e.g. KeyValueSegmentIdentifier) I have thought about compatibility and performed forwards and backwards compatibility tests

Copy link
Contributor

@macfarla macfarla left a comment

Choose a reason for hiding this comment

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

minor suggestions on help messages

alex123012 and others added 4 commits November 8, 2023 10:22
Co-authored-by: Sally MacFarlane <macfarla.github@gmail.com>
Signed-off-by: Makhonin Alexey <60808275+alex123012@users.noreply.github.com>
Co-authored-by: Sally MacFarlane <macfarla.github@gmail.com>
Signed-off-by: Makhonin Alexey <60808275+alex123012@users.noreply.github.com>
Co-authored-by: Sally MacFarlane <macfarla.github@gmail.com>
Signed-off-by: Makhonin Alexey <60808275+alex123012@users.noreply.github.com>
Co-authored-by: Sally MacFarlane <macfarla.github@gmail.com>
Signed-off-by: Makhonin Alexey <60808275+alex123012@users.noreply.github.com>
@macfarla
Copy link
Contributor

macfarla commented Nov 9, 2023

@alex123012 there's a couple of unit tests failing. you should be able to reproduce locally

Task :besu:test

BesuCommandTest > natManagerServiceNameCannotBeUsedWithNatDockerMethod FAILED
java.lang.AssertionError at BesuCommandTest.java:1971

BesuCommandTest > natManagerServiceNameCannotBeUsedWithNatNoneMethod FAILED
java.lang.AssertionError at BesuCommandTest.java:1981

Signed-off-by: MAKHONIN Aleksey M <Aleksey.MAKHONIN@raiffeisen.ru>
@macfarla
Copy link
Contributor

code looks ok - @alex123012 how would I verify that it's working? new to kubernetes

@alex123012
Copy link
Author

alex123012 commented Nov 16, 2023

So, you could try to use kind/minikube with https://github.com/Consensys/quorum-kubernetes repo with changed image. I've tested it with ibft2.

Something like a plan:

  • Install minikube and kubectl.
  • Build the besu image and load it to the minikube cluster (for example, with cache or other option in docs).
  • Change images of besu in k8s manifests to image you've build (for example, here and you'll need to change it in all other files in this dir like in the example).
  • Namespace in code is setted to besu by default, so you can omit the new flag, by you can explicitly set it here and in all other files in this dir.
  • Go through quorum-kubernetes docs (minikube cluster is created in it).
  • If all is good - check besu logs and find entries about K8S manager - it should have no errors

Note: for more restricted rules, to check, that all is ok - delete "list" verb from Role manifest and in other files in this dir do the same.

If you encounter any problems, you can write to me in telegram or email (makhonin.a.ru@gmail.com)

@alex123012
Copy link
Author

@macfarla Wrote a few notes^

@alex123012
Copy link
Author

@macfarla Hi!
Any updates here?:)

Copy link
Contributor

@garyschulte garyschulte left a comment

Choose a reason for hiding this comment

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

👍 , but we should make namespace optional in order to be backward compatible with prior behavior.

v1Service -> v1Service.getMetadata().getName().contains(besuServiceNameFilter))
.findFirst()
.orElseThrow(() -> new NatInitializationException("Service not found"));
api.readNamespacedService(besuServiceName, besuServiceNamespace, null);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think service namespace should be an Optional parameter to the manager. Not having to explicitly specify the namespace is a useful feature IMO, and can prevent breaking existing clustered implementations.

The presence or absence of the namespace can gate the behavior of finding first vs explicitly specify the target namespace

Copy link
Contributor

Choose a reason for hiding this comment

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

@alex123012 any word on this?

@macfarla
Copy link
Contributor

@cdivitotawela any input on this PR?

@cdivitotawela
Copy link
Contributor

@cdivitotawela any input on this PR?

Let me check whether I can run this on minikube and verify,

@lancemarks
Copy link

lancemarks commented Jul 17, 2024

Testing successful with Kubernetes in AWS with v24.5.2 of Besu. There was one issue I ran into, I had to name the port 'discovery' or it would throw an error. I would have liked to been able to name it 'discovery-tcp' or 'p2p-tcp'.

@matthew1001
Copy link
Contributor

Just wondering after talking to some others about K8S behaviour. Does the namespace need manually specifying via a config option, or can the K8S namespace of the Besu pod just be queried and used in discovering the service endpoint? A K8S service can't serve a pod in a different namespace anyway, so it's not clear if there's a reason to add the new --Xnat-kube-service-namespace instead of auto-discovering the pod's namespace.

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.

Besu always needs cluster level permissions for Kubernetes NAT manager on GKE
6 participants