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 init container to the gateway to wait for node readiness #3273

Merged
merged 1 commit into from
Nov 21, 2024

Conversation

tpantelis
Copy link
Contributor

@submariner-bot
Copy link
Contributor

🤖 Created branch: z_pr3273/tpantelis/gw_init_container
🚀 Full E2E won't run until the "ready-to-test" label is applied. I will add it automatically once the PR has 2 approvals, or you can add it manually.

@tpantelis tpantelis requested a review from mkolesnik as a code owner November 21, 2024 02:49
@tpantelis tpantelis force-pushed the gw_init_container branch 3 times, most recently from d17fff3 to 932eda6 Compare November 21, 2024 03:29
@tpantelis tpantelis force-pushed the gw_init_container branch 2 times, most recently from 16593ea to f46ddb6 Compare November 21, 2024 04:06
Copy link
Member

@skitt skitt left a comment

Choose a reason for hiding this comment

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

I won’t object to this to let the bug fix in quickly, but I think the security context should be reworked, and perhaps we could even have a specific “wait for node readiness” binary.

@@ -110,7 +110,6 @@ func newRouteAgentDaemonSet(cr *v1alpha1.Submariner, name string) *appsv1.Daemon
Name: name + "-init",
Image: getImagePath(cr, opnames.RouteAgentImage, names.RouteAgentComponent),
ImagePullPolicy: images.GetPullPolicy(cr.Spec.Version, cr.Spec.ImageOverrides[names.RouteAgentComponent]),
Command: []string{"submariner-route-agent.sh"},
Copy link
Member

Choose a reason for hiding this comment

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

This removal is harmless, but was it intended as part of this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's cleanup that was missed before but technically should be in another PR. I'll remove it.

Privileged: ptr.To(true),
RunAsNonRoot: ptr.To(false),
// We need to be able to update /var/lib/alternatives (for iptables)
ReadOnlyRootFilesystem: ptr.To(false),
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn’t be necessary in the init container.

Comment on lines +142 to +143
AllowPrivilegeEscalation: ptr.To(true),
Privileged: ptr.To(true),
Copy link
Member

Choose a reason for hiding this comment

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

Given how the gateway container image is set up currently, this is necessary in all cases (since the entrypoint script writes to /proc/sys), but ideally the init container shouldn’t need these.

@tpantelis
Copy link
Contributor Author

I won’t object to this to let the bug fix in quickly, but I think the security context should be reworked, and perhaps we could even have a specific “wait for node readiness” binary.

I agree - it's not ideal for the init container. As you surmised, I added it b/c the entry script failed here:

/usr/local/bin/submariner.sh: line 17: /proc/sys/net/ipv4/conf/all/send_redirects: Read-only file system

Perhaps all we need is ReadOnlyRootFilesystem: ptr.To(false).

Longer term, we could add a “wait for node readiness” binary to the image as you suggest and specify that for the init container command.

@submariner-bot submariner-bot added the ready-to-test When a PR is ready for full E2E testing label Nov 21, 2024
@tpantelis tpantelis enabled auto-merge (rebase) November 21, 2024 13:24
@tpantelis tpantelis merged commit bf47293 into submariner-io:devel Nov 21, 2024
41 checks passed
@submariner-bot
Copy link
Contributor

🤖 Closed branches: [z_pr3273/tpantelis/gw_init_container]

@tpantelis tpantelis added the backport This change requires a backport to eligible release branches label Nov 21, 2024
@tpantelis tpantelis deleted the gw_init_container branch December 10, 2024 15:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport This change requires a backport to eligible release branches backport-handled ready-to-test When a PR is ready for full E2E testing
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants