[SDN-2481] Enhancement proposal to migrate Multiple External Gateways to use a CRD

[jordigilh]

This PR contains a proposal to migrate the current annotation based implementation of the multiple external gateways to use a custom resource paired with its own controller. The main motivation is to provide better control over the security aspect of the workloads that can be labeled as gateways by moving the configuration out of the pod and namespace metadata and onto a CR.

@trozet PTAL

[trozet]

I think we should drop the word securely here. To me this implies some type of ipsec/encryption of traffic. You can just say "redirecting traffic to one of potentially multiple destinations".

"telco customers require the ability to dynamically configure one or more egress next hops per namespace via a common and secure API"?

[jordigilh]

Understood. Will update.

[trozet]

what does OPA mean?

[jordigilh]

Open Policy Agent it's a policy engine that uses rego language to define the policies. The example I am a bit familiar is gatekeeper. Kyverno in turn uses k8s manifests to define the rules of admission. In essence both are admission webhooks. This link details a bit more about each approach.
https://devopslearning.medium.com/day-6-open-policy-agent-opa-vs-kyverno-7604b5ecddd9

My point here was to give a generic example that would fit the narrative without specifying an implementation, such as kyverno or gatekeeper.

[jordigilh]

I will update to mention an admission webhook instead. It is less biased towards any of the implementations mentioned earlier.

[trozet]

maybe you can just mention that annotations will be removed in 4.14 but left in 4.13 as a migration path for any existing users? The only officially supported and documented method of configuration will be the CRD in 4.13 and beyond.

[jordigilh]

Understood. I will update

[trozet]

external-gw-pod-ips tracks IPs of pods that are acting as external gateways. It does not store IPs in routing-external-gws.

[jordigilh]

Thanks for the clarification. I will update

[jordigilh]

As per @tssurya 's comment, this field is no longer relevant at this point, so I have not included it in the spec.

[tssurya]

@jordigilh : but this can be removed only if like I mentioned someone tests the OVN fix and removes these bits form OVNK if the OVN fix works. Until that is done, we must support this annotation.

[jordigilh]

@trozet is there a plan to test the fix before this enhancement is implemented? If not, we'll have to update the annotation in both cases.

[tssurya]

@jordigilh : my two cents is you can do this as pre-work for the epic? I bet it will only simplify your epic so it will be an advantage for you, we could create a bz/card for this work and you can own it, outside of that it is currently not in the team's set of priorities AFAIK, I'll let @trozet chime in.

[trozet]

we can drop this because the CRD is namespace scoped. So it is implied that this CRD would affect pods in the same namespace. Alternatively, we could make the CRD global and then keep this field. The advantage would be one one CR to serve multiple namespaces, but then that would mean less granular RBAC between namespace traffic. I'm kind of inclined to keep it namespace scoped, but open to other's opinions.

[jordigilh]

My understanding is that the current implementation uses this annotation to allow gw pods to accept traffic from multiple namespaces. I'm fine with namespace scoped and remove this field as you suggested, but I worry if doing so we would be not providing the same functionality that exists with annotations. Thoughts?

[trozet]

Right, the user would have to make several CRDs, one for each namespace they want to route traffic from. That is a bit different than today as they can declare in a single place they want traffic from all namespaces. That seems less secure though, which is why I was thinking namespaced scope would be better.

@fedepaol wdyt?

[trozet]

Talking more with @jordigilh I think there is another angle we need to consider. If the object is namespace scoped then a tenant in that namespace would be able to control how their traffic egresses. In the real world use case I would imagine cluster admins want to control the traffic of their tenants and do not want to give this kind of access out. So maybe that is a good reason to make it cluster scoped and require cluster admin RBAC.

[trozet]

So talking in the OVNK community call yesterday with, Nvidia showed us their downstream policy based routing CRD, which has some overlap with what we are trying to do. So rather than having 2 different CRDs for routing, we discussed unifying into a single CRD. However, we would only implement pieces that we cared about (multiple external gateways) and Nvidia would bring their corresponding implementation for other cluster routing upstream later.

Example of their CRD:

apiVersion: k8s.ovn.org/v1beta1
kind: AdminPolicyBasedRoute
metadata:
  name: honeypotting
spec:
  networkAttachmentName: default/ovn-primary
  policies:
  - from:
      podSelector:
        matchLabels:
          under_attack: “true”
    to:
      cidr: 0.0.0.0/0
    nexthop:
      ips:
      - 10.192.14.9

Nvidia is using this to match certain traffic to a particular destination, and set the next hop. They do this by Logical Route Policy in OVN on ovn_cluster_router, while we are using Logical Router Static Routes in the gateway router (GR). The reason for using logical route policy is they can match on both source and dest in a route (as well as potentially other criteria), while if they were to use only source or dest they could use regular static routes. However, policy routing takes precedence over static routing in OVN, so we need to be careful here that no policy should ever overlap if we mix policy and static routes.

Here is my first stab at something that we could use as unified CRD for both cases (assume cluster CIDR of 10.244.0.0/24, external network of 172.18.0.0/24):

apiVersion: k8s.ovn.org/v1beta1
kind: AdminPolicyBasedRoute
metadata:
  name: honeypotting
spec:
  policies:
  ##intra cluster example
-  trafficType: internal
   from:
      podSelector:
        matchLabels:
          under_attack: “true”
    to:
      cidr: 9.0.0.0/24
    nexthops:
      - ip: "10.244.0.5"
        bfdEnabled: false
      - ip: "10.244.0.6"
        bfdEnabled: false
      - host
    networkAttachmentName: default/ovn-primary

  ## gateway example
- trafficType: external
  from:
    namespaceSelector:
       matchLabels:
          multiple_gws: true
    nexthops:
      - ip: "172.18.0.2"
        bfdEnabled: true
      - ip: "10.244.0.3"
        bfdEnabled: false
      - podSelector:
          matchLabels:
            external-gateway: true
        bfdEnabled: true
        networkAttachmentName: sriov1

status:
  externalGatewayIPs: ["172.18.0.2", "172.18.0.3", "172.18.0.4", "172.18.0.5"]
  internalGatewayIPs: ["10.244.0.5", "10.244.0.6"]

From the above we get can define a few behaviors (a few of these ideas are very long term, but just thinking outside the box):

1. CRD is cluster scoped for security.
2. If traffic type is internal, the rules are applied to ovn_cluster_router. If external, then applied to GR.
3. If a policy consists of both from and to, then logical route policy will be used. If only source or dest are provided, then logical static routes will be used.
4. Policies should not overlap.
5. In the first example there are 3 nexthops provided. Two are static ip addresses inside the cluster subnet, while a third nexthop "host" will indicate to route packets inside the local host where the pod lives (aka mgmt port). This allows users to redirect traffic to the kernel in case they want to use the host ip routing to egress a different interface.
6. networkAttachmentName defines either the network of the selected pods (primary or secondary) to apply policy to or choose IP from a pod acting as an external gateway.
7. networkAttachmentName is not applicable for trafficType external on selected pods to apply policy to as secondary networks have no ingress/egress.

[fedepaol]

I am not sure the status above makes sense, or at least we need to provide a status per namespace.
Not sure also that the internal / external naming is appropriate here as (iiuc) even the internal one might affect egress traffic.

Re - the host bit. It'd be nice if we could specify the next hop on the host. That would basically address what we are trying to achieve with services.

[tssurya]

Two things here,

1. I feel the MEG CRD should be cluster-scoped, not namespace scoped - a namespace scope doesn't make sense here; the feature is more for admins not users, similar to egressIPs...

2. I am slightly skeptical about merging this CR with what Nvidia is doing for another feature? Reason being we can't really change/expand CRDs like that over releases right, they are API breaking changes? Its going to be hard to get all the fields right in such a way that we can only expand in the future and not break the type of fields..., maybe we keep this v1alpha1 and hoping this feature is dev preview until Nvidia also brings its things in. OR I am missing details and maybe we have a doc from Nvidia that explains its feature in depth so that we are covered? -> example is their feature also for egress traffic or is it for internal traffic? Mixing those things up into a single CRD could get confusing if these two features are doing opposite things on a high level.

3rd extra point which is food for thought - how would the controller know whether its an internal v/s external gw thing here depending on which we'd need to do things on GR v/s ovn_cluster_router? - we'd need that signal somehow from the CRD i guess..

[jordigilh]

Two things here,

1. I feel the MEG CRD should be cluster-scoped, not namespace scoped - a namespace scope doesn't make sense here; the feature is more for admins not users, similar to egressIPs...

Yes, the CRD is cluster scoped. My initial proposal had a mix of cluster and namespaced but that's no longer the case.

2. I am slightly skeptical about merging this CR with what Nvidia is doing for another feature? Reason being we can't really change/expand CRDs like that over releases right, they are API breaking changes? Its going to be hard to get all the fields right in such a way that we can only expand in the future and not break the type of fields..., maybe we keep this v1alpha1 and hoping this feature is dev preview until Nvidia also brings its things in. OR I am missing details and maybe we have a doc from Nvidia that explains its feature in depth so that we are covered? -> example is their feature also for egress traffic or is it for internal traffic? Mixing those things up into a single CRD could get confusing if these two features are doing opposite things on a high level.

What I'm proposing is to use 2 CRDs, one for internal and another one for external traffic. We can keep the CRD structures similar but focus on what matters to us.

3rd extra point which is food for thought - how would the controller know whether its an internal v/s external gw thing here depending on which we'd need to do things on GR v/s ovn_cluster_router? - we'd need that signal somehow from the CRD i guess..

See my comment above.

[trozet]

So today each exgw pod can configure this field to indicate to OVNK which IP to use from their multus network attachment. That means if we have 2 exgws, Pod A and Pod B, they could potentially have different routing-network values and still serve the same client namespace/pods. If we only allow for a singular value here, it means any pods matching the selector (lets assume Pod A and Pod B) would have to use the same network. Should we make this a plural slice of potential networks, and OVNK will just search each pod for one of the values? Or should we make it more fine grained with multiple selectors like:

spec:
  podGateways:
    - network: blue
      podSelector:
        - matchLabels:
          - external-gateway: true
     - network: red
       podSelector:
         - matchLabels:
           - external-gateway2: true

Similarly I think we allow bfd to be enabled on a per pod basis (but not on a per static IP defined in the namespace basis). So maybe we need to have granularity there as well and separate it from statically assigned IPs. Like:

kind: MultipleExternalGateway
apiVersion: k8s.ovn.org/v1
metadata:
  name: multiple-external-gateways-configuration
  namespace: default
spec:
  staticGateways:  ["9.0.0.1", "9.0.0.2"]
  staticGatewaysBFD: true
  podGateways:
    - network: blue
      bfdEnabled: true
      podSelector:
        - matchLabels:
          - external-gateway: true
     - network: red
       podSelector:
         - matchLabels:
           - external-gateway2: true
status:
  externalGatewayPodIPs: ["10.10.1.2","10.10.1.5"]

[jordigilh]

Makes sense about BFD being per pod and being able to have multiple selectors.

[jordigilh]

What is the purpose of staticGatewaysBFD in the manifest? If it's a deployment/pod specific feature, does it make sense to apply to the whole manifest?

[trozet]

so that is there to signal to use BFD or not with the gateways specified in the staticGateways. Today we only allow you to configure BFD on the namespace for the entirety of all static IPs defined in the namespace annotation (not on an individual basis). But for pods acting as external gateways, we allow you to annotate those individually. In hindsight this wasn't a very consistent design decision :) So, since we do not want to change the functional behavior I think we need to still allow for individual BFD selection for pod external gateways, but not for statically assigned IPs (previously done in the namespace annotation).

Does that make sense?

@fedepaol can you double check me here please.

[jordigilh]

yeah, checking the code it does what you mentioned. On a second thought, it might make more sense to allow defining the BFD per external IP rather than as a whole as it is currently done

staticExternalGateways:
  - ip: "192.168.1.1"
    bfdEnabled: true
  - ip: "10.1.1.1"

I don't mind keeping it for backwards compatibility if it's worth keeping it, but I'm happy to rework/drop stuff that doesn't make sense or is not used/deprecated.

[trozet]

yeah it would be nice to allow that, i was just trying to avoid any change in functionality so that there would not be extra work to do there. I don't think it is a huge effort though to change this behavior.

[jordigilh]

I'm proposing to define 2 CRDs, one for each type of traffic, but keeping the similar high level fields (from,nextHops). Here is an update of the example posted earlier in this thread:

apiVersion: k8s.ovn.org/v1beta1
kind: AdminPolicyBasedExternalRoute
metadata:
  name: honeypotting
spec:
  policies:
  ## gateway example
  - from:
      namespaceSelector:
        matchLabels:
            multiple_gws: true
    nexthops:
      - ip: "172.18.0.2"
        bfdEnabled: true
      - ip: "10.244.0.3"
        bfdEnabled: false
      - podSelector:
          matchLabels:
            external-gateway: true
        bfdEnabled: true
        namespaceSelector:
          matchLabels:
            gateway: true
        networkAttachmentName: sriov1

[trozet]

@tssurya @fedepaol can you please also review?

[fedepaol]

I am torn about having a one - in - all crd for this.
Here in one place we define the gateways (two types of them) and their relationship to the namespaces.

If we add a crd representing the gateways, it will be easier to name them properly, and have another crd that establishes the relationship between a namespace and the gateways.

Something like "PodExternalGateway" for the pod part, "StaticExternalGateway" for the static part and "NamespaceGateways" to set the rules.

This sounds better also from a responsibility point of view: the gateways are part of the infrastructure while the choosing which namespace goes through which set of gateways can be done by a separate entity.

[jordigilh]

I like the idea. It makes sense since the definition of the gateways don't necessarily need to be linked to the namespaces. Linking them in the same CRD makes them less flexible.

[jordigilh]

@trozet what do you think?

[trozet]

I can understand why having 2 different CRDs for statically defined gateways and pod gateways could make sense. I don't understand NamespaceGateways part, wouldn't that just be the namespace scope of the other 2 CRDs? Or are you talking about making a globally scoped NamespaceGateway CRD to select the other 2 CRDs and apply them to multiple namespaces?

If so, that seems like overkill doesn't it?

[fedepaol]

my idea was, the gateway definition and how to apply them to a given namespace belong to two different concerns. One is "infrastructural", where you define the gateways, and another one is to choose which namespace should go to which more administrative.
And if you let the crds be namespaced, then they are under the user's control and it's more or less the same as annotations (which I think is what you wrote in #1338 (comment))

[fedepaol]

Would it make sense to have a flag that defines the behaviour of using the pod's ip (as opposed to implicitly using it when the network is not defined) ?

[jordigilh]

Agreed, I need to think about it. In essence we're looking at two possibilities:

• retrieve the IPs from a network reference from the multus config annotation.
• retrieve the IPs from the node's network interface, assuming that the pod has hostNetwork is set to true.

[trozet]

can you give an example of what you are thinking?

[jordigilh]

It was more about providing two fields:

• One that determines if the source of the IPs was primary (host network) or secondary (multus managed).
• For the multus case, it provided the network name. In case of the primary network, this field would not be used.

[fedepaol]

I know this comes from the previous iteration, but should this be routedNamespaces or just namespaces an now the purpouse of this crd is clear and self contained?

[jordigilh]

I'll rename it to namespaces, but if the idea of splitting the CRD into 3 different ones works out we will not need this field anymore 😉

[trozet]

see this discussion: #1338 (comment)

[fedepaol]

it was possible to set the bfdEnabled on both the gw pod and on the namespace. If we put it here, we are removing a degree of configuration.
It would make sense to raise it to the root level of the object.

[jordigilh]

I will add the ability to define it at the spec level for now, but I'm more inclined to break this CRD into 3 different ones following your previous suggestion.

[jordigilh]

Updated the spec. I think it's getting close to what you're suggesting about 3 CRDs: there's one section for staticGateways, another one for podGateways and the namespaces.

[trozet]

see my comment here: #1338 (comment)

[fedepaol]

I think we had e2e tests too covering external gateways (if somebody did not kill them :P )

[jordigilh]

😄 well the idea would be to clone these tests to use the CRs instead. The setup and flows should still apply as long as we are keeping the same feature parity to the annotation implementation.

[trozet]

yeah we still have those

[fedepaol]

@tssurya @fedepaol can you please also review?

left a few comments, hope they are relevant :P

[tssurya]

nit: I don't see how this opening statement around ingress and proxies is related to this enhancement. We could just straight start with explaining "what is the MEG feature" briefly and that there is a need to allow for a common API to expose the feature in a more GA-ed fashion.

[tssurya]

Also the second part of the opening,
telco customers require the ability to dynamically configure one or more egress next hops per namespace via a common and secure API
let the stress be on the common and secure API not the dynamically configure part which is already possible today through annotations. IMHO, Goal of Summary should be that this is just an API KEP exposing an existing feature that has been in dev preview for long and will provide a controller that will do the current functionality as is, (its not adding/implementing a new feature ofc this need not be mentioned explicitly).

[jordigilh]

Ack

[jordigilh]

@tssurya updated, please take a look.

[tssurya]

This sentence is confusing to me. An admission webhook concept is being introduced in this KEP in the previous sentence.

But we aren't even remotely doing that right? Our solution is CRD and a controller to manage that. I'd think these are two different ways to solve the problem.

So why are we even mentioning a webhook model, are these solutions synonyms that the controller model somehow implements the webhook need? maybe I am missing something (sorry for the dumb question). But in the end I am left with why did we mention webhook when we are going for the second option...

[jordigilh]

The difference is that in the current implementation there is no mechanism to control the usage of the annotations in pods or namespaces. You need an admission webhook to provide a level of control over which pods and namespaces can have these annotations. Using a CRD you get the benefit that it's bound to have an RBAC associated to it. Hence, the control that was enforced by the admission webhook is not translated into a set of roles/rolebindings to a given set of users/groups.

Note that the admission webhook is not a functional part of the annotation based implementation, more of an enhancement to grant a certain level of security that needs to be deployed separately.

My understanding is that the main motivator for this enhancement is to improve the security aspect of this solution, as well as to improve the usability by leveraging on CRDs. I'm not familiar with the current usability issues of the existing annotation based approach, so please let me know if I'm missing anything.

[tssurya]

Couple this point with point3? I don't see the need for a new redundant point to talk about tests.

[jordigilh]

I see 2 cases here:

• Only CRs exist, which is our ideal scenario. That's point 3.
• CRs and annotations exist, which is an scenario that we'd rather avoid but still possible. For 4.13, we are defining the annotations as the preferred form,. This is not covered by the current test scenarios and requires to be included in some shape or form.

These are 2 distinct test objectives and I wanted to highlight the scope and the reason, rather than just cover them in a single requirement.

[tssurya]

Maybe this is an implementation detail, but how do we plan to do this upstream? We don't have a versioning system there. Assuming this feature is going to go upstream first and then downstream.

[jordigilh]

Good point, I don't know if there is some kind of configuration that we could use to enable/disable the CRs. I'm not aware but it would be the best approach. Any ideas?

[tssurya]

Can you add more details on where this CR will live? I am hoping this is something similar to egressIP/egressFW and not egressRouter ?

[jordigilh]

It's planned to be cluster scoped, so I hope it will not be bound to a namespace. Do you still want me to include a reference to cluster scoped? Or maybe something else?

[tssurya]

ack no I meant which repo will this live in, in OVNK or OCP/API , I'm guessing its OVNK.

[jordigilh]

Yes it's going to be in ovn-kubernetes, inside go-controller/pkg/crd with egressfirewall, egressip and egressqos

[tssurya]

Why is this a namespace-scoped CRD? I'd assume this whole feature is not for users/app owners, more for admins.

[tssurya]

on second thoughts to make the below CRD neater, we can make this namespace-scoped but then the scoped namespace should be the one on which the config is applied to, that might mean a lot of crds, one per namespace.

[jordigilh]

It's going to be cluster scoped at the end. Updating soon 😄

[tssurya]

Two things here,

1. I feel the MEG CRD should be cluster-scoped, not namespace scoped - a namespace scope doesn't make sense here; the feature is more for admins not users, similar to egressIPs...

2. I am slightly skeptical about merging this CR with what Nvidia is doing for another feature? Reason being we can't really change/expand CRDs like that over releases right, they are API breaking changes? Its going to be hard to get all the fields right in such a way that we can only expand in the future and not break the type of fields..., maybe we keep this v1alpha1 and hoping this feature is dev preview until Nvidia also brings its things in. OR I am missing details and maybe we have a doc from Nvidia that explains its feature in depth so that we are covered? -> example is their feature also for egress traffic or is it for internal traffic? Mixing those things up into a single CRD could get confusing if these two features are doing opposite things on a high level.

3rd extra point which is food for thought - how would the controller know whether its an internal v/s external gw thing here depending on which we'd need to do things on GR v/s ovn_cluster_router? - we'd need that signal somehow from the CRD i guess..

[tssurya]

Please clarify "linked" as being the set of namespaces for which this gateway configuration will be applied for.

[jordigilh]

Will update. I've rewritten this part, but I will keep your description where it applies

[tssurya]

We should have a staticGatewayConfig and dynamicGatewayConfig (for a lack of a better word and somehow podGateways isn't working in my head) that both use the bfdEnabled field and we should be solid on the kubebuilder definitions of at least one of this must be defined if not both.
it would be good to mention for all these fields which are the mandatory ones and which are optional etc.

[jordigilh]

Pending confirmation, but we might end up droping the 3 CRDs in favor or a single CRD. I will mention which fields are optional.

[tssurya]

on second thoughts to make the below CRD neater, we can make this namespace-scoped but then the scoped namespace should be the one on which the config is applied to, that might mean a lot of crds, one per namespace.

[tssurya]

I am not following the purpose of this new podSelector field and what is this translating to from the annotations world, help needed in understanding and sorry for being dumb.

[jordigilh]

Sure, no problem 😄 : The current implementation uses pod's annotations to identify the pods that perform the role of gateways. Moving to a CRD, these annotations are no longer expected to be found in the pod's manifest. Instead, we have to resort using filtering criteria to identify the pods that are meant to be gateways. This label selector is exactly that: defines the filtering criteria via label matching that identifies the running pods to be processed as gateways.

[tssurya]

ah thanks for explaining! makes sense...

[tssurya]

would be good to have the "why they are not required" ? :) specially for an API change. I'd think the current MEG is not GA right?

[tssurya]

Is this in 4.14 or 4.15 ? i think this contradicts what's mentioned above? let's be consistent everywhere.

[jordigilh]

Good catch. I will update to reflect that in 4.14 the annotations will no longer be available.

[tssurya]

I think I mentioned this in another comment, but it would be good to outline how users currently using the annotations are expected to migrate to CRD? is it going to be manual?

[jordigilh]

Yes, manual as far as I understand. I will mention that there is no migration tool or documentation planned.

[tssurya]

See questions inline (I also responded to some outstanding questions on the outdated set of comments.)

[tssurya]

++ thanks for adding this.

[tssurya]

pending discussion from #1338 (comment) ; based on that we might need to update the info on external-gw-pod-ips

[tssurya]

@jordigilh : my two cents is you can do this as pre-work for the epic? I bet it will only simplify your epic so it will be an advantage for you, we could create a bz/card for this work and you can own it, outside of that it is currently not in the team's set of priorities AFAIK, I'll let @trozet chime in.

[tssurya]

We are one week away from feature complete deadline, we might have to update this to 4.14 and other places accordingly...

[tssurya]

Are we going to call this AdminPolicyBasedExternalRoute ? I don't think its a good name to add "adminpolicybased" into the CRD's name since we are going to control its RBAC as for cluster-admins only right? IMHO, the name of the CRD should reflect the feature's high-level goal.
If nvidia's use cases for "internalroute" is not going to be part of this CRD (since name is "externalroute") and its another one, then ExternalGateway or EgressGateway is enough right? cc @trozet @fedepaol WDYBT?
Maybe even PolicyBasedRouting ?

PS: I am bad at naming around APIs so if other reviewers are ok with the existing name, its fine to ignore this comment.

[tssurya]

why do we need this skipHostSNAT ? Don't we have that info on the from field? all pods in that namespace won't have the SNAT configured right?

Does this feature actually support having a SNAT towards hostIP? -> I thought this is something that can be deciphered from the disableSNATMultipleGWs option. Wondering why we need this flag here on a per externalgw basis? cc @trozet ?

[tssurya]

Another thought that just came in was both skipHostSNAT and bfdEnabled, these seem to be set for the whole set of static exgw pods or dynamic exw pods.. should this be allowed on a per exgw basis? -

because for the namespace annotations its true we behave this way but for pod annotations, we actually provide a way to do this per exgw pod.

[trozet]

disableSNATMultipleGWs was a poor naming choice by me. Really all it should mean is "perPodSNAT". Then with this granularity a user can decide whether or not the the pod should be SNAT'ed on a per route basis. This is an enhancement compared to the previous behavior.

[tssurya]

nit: staticGateways ?

[tssurya]

nit: dynamicGateways ?

[tssurya]

what if the provided networkAttachmentName is not found in the podSelector provided? Will the behaviour be an error or will you fallback to the hostNetwork IP?

[jordigilh]

it will return an error.

[tssurya]

Is this optional field? Does that mean the value is going to be false by default?

[jordigilh]

Correct. It's optional and defaults to false.

[tssurya]

nit: probably good to add upstream docs for this feature in its goals (I know downstream docs are a must for an API feature).. this can help other engineers for knowledge sharing.

[trozet]

/approve

[tssurya]

/lgtm thanks @jordigilh

[tssurya]

Suggested change

	- Extend the existing unit tests for the annotation based logic to accommodate for the scenario where CR and annotations exist in 4.13.
	- Extend the existing unit tests for the annotation based logic to accommodate for the scenario where CR and annotations exist in 4.14.

[tssurya]

Suggested change

	- Removal of the annotation based logic to configure multiple external gateways. This should be carrier out at least no later than OpenShift 4.14 to guarantee a stable deprecation path to existing deployments that leverage on the annotations to configure the external gateways.
	- Removal of the annotation based logic to configure multiple external gateways. This should be carrier out at least no later than OpenShift 4.15 to guarantee a stable deprecation path to existing deployments that leverage on the annotations to configure the external gateways.

[tssurya]

TODO: As discussed if any changes come in the API based on implementation, we can come back to update the fields, so loosely lgtm-ing on this initial one.

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: trozet, tssurya

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• enhancements/network/OWNERS [trozet]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[tssurya]

/lgtm

[openshift-ci]

@jordigilh: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.