Daemon bridge clean #4806
Daemon bridge clean #4806
Conversation
JoseVillalta
force-pushed
the
daemon-bridge-clean
branch
from
53d094c to
8e1f88d
Compare
JoseVillalta
force-pushed
the
daemon-bridge-clean
branch
from
8e1f88d to
77421fb
Compare
| Index int | ||
|
|
||
| // NetworkMode represents the network mode for this namespace. | ||
| // Supported values: awsvpc (default), daemon-bridge (managed-instances only). |
There was a problem hiding this comment.
Is there someplace we enforce this? like can a caller call with any mode possible or can a caller not using EMI call with daemon-bridge mode?
|
|
||
| // WithNetworkMode sets the NetworkMode field | ||
| func (ns *NetworkNamespace) WithNetworkMode(mode types.NetworkMode) *NetworkNamespace { | ||
| ns.NetworkMode = mode |
There was a problem hiding this comment.
Should we add a validation here that the caller should only send the supported modes?
| NetworkInterfaces: networkInterfaces, | ||
| KnownState: status.NetworkNone, | ||
| DesiredState: status.NetworkReadyPull, | ||
| NetworkMode: types.NetworkModeAwsvpc, |
There was a problem hiding this comment.
should we add a comment here that we are defaulting to awsvpc if the caller does not explicity set the network mode.
| err = nb.startAWSVPC(ctx, taskID, netNS) | ||
| case types.NetworkModeHost: | ||
| err = nb.platformAPI.HandleHostMode() | ||
| case "daemon-bridge": |
There was a problem hiding this comment.
Can we remove this hard-coded config and move this to the enum.
| err = nb.stopAWSVPC(ctx, netNS) | ||
| case types.NetworkModeHost: | ||
| err = nb.platformAPI.HandleHostMode() | ||
| case "daemon-bridge": |
| case types.NetworkModeHost: | ||
| err = nb.platformAPI.HandleHostMode() | ||
| case "daemon-bridge": | ||
| // Adding extra logging to help with debug TODO remove later. |
There was a problem hiding this comment.
we can remove this comment.
| err = nb.platformAPI.HandleHostMode() | ||
| case "daemon-bridge": | ||
| // Adding extra logging to help with debug TODO remove later. | ||
| logger.Info("Stopping Daemon network namespace setup", logFields) |
There was a problem hiding this comment.
Do we need this log in production?
| BridgeInterfaceName = "fargate-bridge" | ||
| ManagedInstanceBridgeName = "mi-bridge" |
There was a problem hiding this comment.
- This change will impact all EMI tasks, not just the daemon tasks. Should we split this into a separate PR?
- should we call it emi-bridge ?
| } | ||
|
|
||
| // createBridgePluginConfig constructs the configuration object for bridge plugin | ||
| func createDaemonBridgePluginConfig(netNSPath string) ecscni.PluginConfig { |
There was a problem hiding this comment.
This should be a managed instance bridge, not a daemon bridge .. correct?
|
|
||
| // buildDefaultNetworkNamespace return default network namespace of host ENI for host mode. | ||
| func (m *managedLinux) buildDefaultNetworkNamespace(taskID string) ([]*tasknetworkconfig.NetworkNamespace, error) { | ||
| func (m *managedLinux) buildDefaultNetworkNamespaceConfig(taskID string) ([]*tasknetworkconfig.NetworkNamespace, error) { |
There was a problem hiding this comment.
I think we should be explicit and call it HostModeNetworkNamespaceConfig .. default can be awsvpc so that can be confusing to readers.
| if err != nil { | ||
| return nil, errors.Wrap(err, "failed to create network namespace with host eni") | ||
| } | ||
| case "daemon-bridge": |
| hostENI.SubnetGatewayIpv4Address = aws.String(ipv4SubNet) | ||
| } | ||
|
|
||
| netNSName := "host-daemon" |
| netInt.DesiredStatus = status.NetworkReadyPull | ||
| netInt.KnownStatus = status.NetworkNone | ||
| daemonNamespace, err := tasknetworkconfig.NewNetworkNamespace(netNSName, netNSPath, | ||
| 0, nil, netInt) |
There was a problem hiding this comment.
should we add a comment, why 0.
| } | ||
|
|
||
| // StopDaemonNetNS stops and cleans up a daemon network namespace. | ||
| func (m *managedLinux) StopDaemonNetNS(ctx context.Context, netNS *tasknetworkconfig.NetworkNamespace) error { |
There was a problem hiding this comment.
Lets rethink what we want to do in stop daemons.
aviral92
left a comment
aviral92
left a comment
There was a problem hiding this comment.
Left some comments on rev1.
4 participants
Summary
This pull request implements support for daemon-bridge network mode in the ECS agent, enabling tasks to run in isolated network namespaces with bridge connectivity to the host for managed daemon workloads.
Implementation details
daemon-bridgenetwork mode support inBuildTaskNetworkConfigurationbuildHostDaemonNamespaceConfigto create daemon network namespaces using host ENI configurationConfigureDaemonNetNSandStopDaemonNetNSmethods to the platform API for daemon network namespace lifecycle managementNetworkNamespacemodel withNetworkModefield andWithNetworkMode()methodcreateDaemonBridgePluginConfigfor CNI bridge configuration specific to daemon tasksTesting
buildHostDaemonNamespaceConfigcovering IPv4 and IPv6 scenariosNetworkNamespacemodel changes includingWithNetworkMode()methodTestCreateDaemonBridgeConfig)NetworkModevaluesNew tests cover the changes: yes
Description for the changelog
Add support for daemon-bridge network mode for managed daemon workloads
Additional Information
Does this PR include breaking model changes? If so, Have you added transformation functions?
No, this PR maintains backward compatibility. The
NetworkModefield inNetworkNamespacehas a default value ofawsvpcto ensure existing functionality remains unchanged.Does this PR include the addition of new environment variables in the README?
No, this PR does not add any new environment variables.
Licensing
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.