redhat-partner-solutions / crucible

Apache License 2.0
34 stars 59 forks source link

Add support to pass dualstack VIPs for OCP >= 4.12 #261

Closed manurodriguez closed 1 year ago

manurodriguez commented 1 year ago

Feature description

Hi! I'm reading this note in the crucible docs: https://github.com/redhat-partner-solutions/crucible/blob/main/docs/inventory.md#dual-stack

Openshift currently only allows the ingress and API VIPs to be single stack so you must choose IPv4 or IPv6. 

And I assume that's based on an older Openshift version, since starting in OCP 4.12 we can specify both VIPs using apiVips and ingressVIPs:

$ openshift-install explain installconfig.platform.baremetal
KIND:     InstallConfig
VERSION:  v1

RESOURCE: <object>
  BareMetal is the configuration used when installing on bare metal.

FIELDS:
    apiVIP <string>
      Format: ip
      DeprecatedAPIVIP is the VIP to use for internal API communication Deprecated: Use APIVIPs

    apiVIPs <[]string>
      Format: ip
      APIVIPs contains the VIP(s) to use for internal API communication. In dual stack clusters it contains an IPv4 and IPv6 address, otherwise only one VIP
...
    ingressVIP <string>
      Format: ip
      DeprecatedIngressVIP is the VIP to use for ingress traffic Deprecated: Use IngressVIPs

    ingressVIPs <[]string>
      Format: ip
      IngressVIPs contains the VIP(s) to use for ingress traffic. In dual stack clusters it contains an IPv4 and IPv6 address, otherwise only one VIP
...

Currently Crucible has two variables to pass the VIPs values: api_vip and ingress_vip but each variable is designed to have only one value either an IPv4 or an IPv6 address, not both.

As a crucible user that deploys OCP 4.12 and above, I would like to be able to specify VIPs for both IPv4 and IPv6.

Required statements

manurodriguez commented 1 year ago

Now the challenge is how to do this without affecting much current users that rely on using these variables. I was thinking two different approaches:

  1. Adding two new variables, i.e.: api_vip6 and ingress_vip6 specifically for Ipv6, and make sure to include validations to confirm the old vars are IPv4 and the new vars are IPv6, however that will force folks that currently use api_vip and ingress_vip for IPv6 VIPs to change their inventories/deployments.
    
    api_vip: <ipv4-address>
    api_vip6: <ipv6-address>

ingress_vip: ingress_vip6:


2. Modify the current variables to make them be an array each and be able to provide more than one value and include validation to make sure there is at least one value, this also could potentially break current folks deployments if the variables are not an array, but maybe we can do something about it. i.e.: 

api_vip

Wdyt? any comments or other options are welcome : )

manurodriguez commented 1 year ago

I thought about a third option, that aligns with the current way we are passing other dualstack variables with extra_machine_networks, extra_service_networks and extra_cluster_networks.

That is: we could add two new vars extra_api_vip and extra_ingress_vip and we'll keep using the current vip variables in all plays without breaking current deployments, and folks should be able to include extra vips with the new variables, either IPv4 or IPv6. And we'll add a condition to make sure install-config.yaml.j2 only takes those variables into account for OCP >= 4.12. :thinking:

nocturnalastro commented 1 year ago

And I assume that's based on an older Openshift version, since starting in OCP 4.12 we can specify both VIPs using apiVips and ingressVIPs:

Your correct. We haven't modified crucible to allow this yet. Coincidently I was planning looking into the second half of this week.

Think 3 or modified 2 where we used api_vips with a default of api_vips: ['{api_vip}'] that way, any current inventories shouldn't break.

I think any version requirements should be handled inventory validation. Then the rest of the code should not care.

manurodriguez commented 1 year ago

@nocturnalastro thanks for the feedback!

I started a PR in #262 it's a combination of a default of api_vips: ['{{ api_vip }}'] plus the variables I mentioned extra_api_vip and extra_ingress_vip,

I started a test of OCP 4.12 with dualstack on baremetal and validated VIPs were included: https://www.distributed-ci.io/jobs/41e4489a-dd34-4848-b7fb-b67fddc15e36/jobStates?sort=date I'm also testing this change is not breaking working deployments without dualstack. So we'll see more DCI jobs in the PR.