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

feat: specify port binding for docker #592

Merged
merged 15 commits into from
Aug 22, 2024
Merged

feat: specify port binding for docker #592

merged 15 commits into from
Aug 22, 2024

Conversation

lvrach
Copy link
Member

@lvrach lvrach commented Aug 13, 2024

Description

Motivation

We have been facing overlapping issues with ipv4 ipv6 ports, causing tests to fail randomly. You can read more about the issue here.

This PR attempts to solve the problem differently by providing a cleaner solution.

Solution

We use port bindings to only bind to ipv4 host address.

Changes

Provide Host

control host for downstream libraries:

  • use src.GetBoundIP instead of hardcoding localhost
  • export URL or address (host:port) instead of just port

This gives us more control over ipv4 vs ipv6 if needed. It would allow us to use external docker hosting.
In most cases, it simplifies consumers of the API

defaults

PublishAllPorts = false Only publish ports being mentioned explicitly.

Dependants

rudder-server

rudderlabs/rudder-server#4998

Linear Ticket

https://linear.app/rudderstack/issue/PRI-29/rudder-go-kit-ipv4-port-binding

Ref PRI-29

Security

  • The code changed/added as part of this pull request won't create any security issues with how the software is being used.

@lvrach lvrach changed the title chore: wip: port binding for docker chore: specify docker binding Aug 14, 2024
@lvrach lvrach changed the title chore: specify docker binding feat: specify port binding for docker Aug 14, 2024
@@ -57,7 +58,8 @@ func Setup(pool *dockertest.Pool, d resource.Cleaner, opts ...func(*Config)) (*R
"POSTGRES_DB=" + postgresDefaultDB,
"POSTGRES_USER=" + postgresDefaultUser,
},
Cmd: cmd,
Cmd: cmd,
PortBindings: internal.IPv4PortBindings([]string{"5432"}),
}, func(hc *dc.HostConfig) {
Copy link
Member

Choose a reason for hiding this comment

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

Don't we need to set PublishAllPorts to false?

postgresContainer, err := pool.RunWithOptions(&dockertest.RunOptions{
		Repository: "postgres",
		Tag:        c.Tag,
		NetworkID:  c.NetworkID,
		Env: []string{
			"POSTGRES_PASSWORD=" + postgresDefaultPassword,
			"POSTGRES_DB=" + postgresDefaultDB,
			"POSTGRES_USER=" + postgresDefaultUser,
		},
		Cmd:          cmd,
		PortBindings: internal.IPv4PortBindings([]string{"5432"}),
	}, func(hc *dc.HostConfig) {
		hc.ShmSize = c.ShmSize
		hc.OOMKillDisable = c.OOMKillDisable
		hc.Memory = c.Memory
	},
		internal.DefaultHostConfig,
	)

Copy link
Member

Choose a reason for hiding this comment

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

Do we need to set PublishAllPorts to false in sshserver.go and transformer.go as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

We should set PublishAllPorts to false in all the resources and expose ports only via port binding.

Copy link
Contributor

@mihir20 mihir20 left a comment

Choose a reason for hiding this comment

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

LGTM

@lvrach lvrach merged commit ff8a8e2 into main Aug 22, 2024
10 checks passed
@lvrach lvrach deleted the chore.portbind-docker branch August 22, 2024 13:44
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.

4 participants