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

Add hostNetwork config to Tenant spec #1942

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

mctoohey
Copy link
Contributor

Allows hostNetwork: true to be configured on the tenant pods (https://kubernetes.io/docs/concepts/services-networking/network-policies/#networkpolicy-and-hostnetwork-pods).

This can be desirable if you want to bypass the Kubernetes network layer and access MinIO more directly.

@jiuker
Copy link
Contributor

jiuker commented Jan 22, 2024

Please add minio-port and console-port.

When using host networking. It may be desirable to adjust the MinIO
and console ports to avoid port conflicts on the host. The services
still expose MinIO and the console on the same ports as before.
@mctoohey
Copy link
Contributor Author

@jiuker I have pushed a commit that makes the minio and console container ports configurable. This helps avoid port conflicts on the host when using host networking. The services will still expose MinIO and the console on the default ports, same as they did before.

Copy link
Contributor

@jiuker jiuker left a comment

Choose a reason for hiding this comment

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

Please set with default value and check if they are the same。

Sets the defaults for these ports and also a check in the Helm
chart to make sure they cannot be set to the same value.
@mctoohey mctoohey requested a review from jiuker January 28, 2024 23:10
Copy link
Contributor

@jiuker jiuker left a comment

Choose a reason for hiding this comment

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

Please use code to set default and check both.

@cniackz cniackz self-requested a review April 20, 2024 03:19
@cniackz cniackz self-assigned this Apr 20, 2024
@cniackz
Copy link
Contributor

cniackz commented Apr 20, 2024

@mctoohey please help us to resolve conflicts

Copy link
Contributor

@ramondeklein ramondeklein left a comment

Choose a reason for hiding this comment

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

LGTM, but having an integration-test here would also be good. We're supporting some edge-case scenarios that we don't actually provide an integration test for. Adding integration tests would help keeping this stable or we should mark some items as "community" provided features that has different kind of stability compared to our standard deployment. We also lack support in the console for these edge-cases.

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.

None yet

4 participants