Conversation
There was a problem hiding this comment.
Pull request overview
This PR transitions the Gateway controller to enforce that all Gateway resources must exist in the default namespace, and removes deprecated code related to the old multi-namespace architecture. The changes complete a migration from a flexible namespace model to a simplified single-namespace model for Gateway resources.
Changes:
- Enforces default namespace requirement for Gateway resources through validation and controller filtering
- Removes deprecated TODO code for old GatewayAgent, RBAC resources, and DaemonSets in gateway-specific namespaces
- Removes deprecated Agentless and AgentRef fields from GatewayCtrlConfig
- Updates CI workflow to use a development branch for testing
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| pkg/ctrl/gateway_ctrl.go | Updates namespace validation to require default namespace and removes deprecated cleanup code for old multi-namespace resources |
| api/meta/config.go | Removes deprecated Agentless and AgentRef fields and adds documentation for Namespace field |
| api/gateway/v1alpha1/gateway_types.go | Adds default namespace setting in Default() method and validation to reject non-default namespaces |
| .github/workflows/ci.yaml | Updates fabricatorref to development branch for testing |
Comments suppressed due to low confidence (2)
pkg/ctrl/gateway_ctrl.go:276
- This PR removes cleanup code for old resources (GatewayAgent, Role, RoleBinding, ConfigMap, and DaemonSet) that were previously created in the gateway's namespace. Without a migration path or one-time cleanup job, these old resources will be orphaned in existing installations when upgrading. Consider adding cleanup logic or documenting manual cleanup steps needed before or after upgrading. The removed code includes: old GatewayAgent creation (lines 250-276), fabric namespace Role/RoleBinding (lines 299-355), and ConfigMap/DaemonSet deletion (lines 366-384).
// we intentionally manage gateway agent in the default namespace
{
gwAg := &gwintapi.GatewayAgent{ObjectMeta: kmetav1.ObjectMeta{Namespace: kmetav1.NamespaceDefault, Name: gw.Name}}
if _, err := ctrlutil.CreateOrUpdate(ctx, r.Client, gwAg, func() error {
// TODO consider blocking owner deletion, would require foregroundDeletion finalizer on the owner
gwAg.Spec.AgentVersion = ""
gwAg.Spec.Gateway = gw.Spec
gwAg.Spec.VPCs = vpcs
gwAg.Spec.Peerings = peerings
gwAg.Spec.Groups = gwGroups
gwAg.Spec.Communities = comms
gwAg.Spec.Config = gwintapi.GatewayAgentSpecConfig{
FabricBFD: r.cfg.FabricBFD,
}
return nil
}); err != nil {
return kctrl.Result{}, fmt.Errorf("creating or updating gateway agent: %w", err)
}
}
if err := r.deployGateway(ctx, gw); err != nil {
return kctrl.Result{}, fmt.Errorf("deploying gateway: %w", err)
}
return kctrl.Result{}, nil
pkg/ctrl/gateway_ctrl.go:333
- The ServiceAccount is created in r.cfg.Namespace (line 292) while the Role and RoleBinding are created in kmetav1.NamespaceDefault (lines 300, 325). This creates a cross-namespace RBAC configuration where the ServiceAccount in one namespace is granted permissions via a Role in the default namespace. While this is technically valid in Kubernetes, it may be confusing and could cause permission issues. Consider documenting why r.cfg.Namespace is used for the ServiceAccount instead of kmetav1.NamespaceDefault, or align all resources to use the same namespace for consistency.
sa := &corev1.ServiceAccount{ObjectMeta: kmetav1.ObjectMeta{
Namespace: r.cfg.Namespace,
Name: saName,
}}
if _, err := ctrlutil.CreateOrUpdate(ctx, r.Client, sa, func() error { return nil }); err != nil {
return fmt.Errorf("creating service account: %w", err)
}
role := &rbacv1.Role{ObjectMeta: kmetav1.ObjectMeta{
Namespace: kmetav1.NamespaceDefault,
Name: saName,
}}
if _, err := ctrlutil.CreateOrUpdate(ctx, r.Client, role, func() error {
role.Rules = []rbacv1.PolicyRule{
{
APIGroups: []string{gwintapi.GroupVersion.Group},
Resources: []string{"gatewayagents"},
ResourceNames: []string{gw.Name},
Verbs: []string{"get", "watch"},
},
{
APIGroups: []string{gwintapi.GroupVersion.Group},
Resources: []string{"gatewayagents/status"},
ResourceNames: []string{gw.Name},
Verbs: []string{"get", "update", "patch"},
},
}
return nil
}); err != nil {
return fmt.Errorf("creating role: %w", err)
}
roleBinding := &rbacv1.RoleBinding{ObjectMeta: kmetav1.ObjectMeta{
Namespace: kmetav1.NamespaceDefault,
Name: saName,
}}
if _, err := ctrlutil.CreateOrUpdate(ctx, r.Client, roleBinding, func() error {
roleBinding.Subjects = []rbacv1.Subject{
{
Kind: "ServiceAccount",
Name: sa.Name,
Namespace: sa.Namespace,
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Sergei Lukianov <me@slukjanov.name>
Signed-off-by: Sergei Lukianov <me@slukjanov.name>
Signed-off-by: Sergei Lukianov <me@slukjanov.name>
805ca4b to
672af24
Compare
|
🚀 Temp artifacts published: |
Signed-off-by: Sergei Lukianov <me@slukjanov.name>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
🚀 Temp artifacts published: |
Signed-off-by: Sergei Lukianov <me@slukjanov.name>
No description provided.