Add first draft of prereqs docs

[daxelrod-rh]

This will probably need some iteration before we are ready to merge it. I'll add a review with items to discuss.

Document just AWS prerequisites for now.

[coderabbitai]

Important

• 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Pro Plus

Run ID: 863ffac6-3aa9-43e2-b8f9-2d6c180b315b

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[daxelrod-rh]

Beyond the individual items I've flagged below, we also need to figure out:

1. Is this the correct format for this documentation in an operator?
2. Should we remove the corresponding parts of the README.md to keep one source of truth for things like the IAM role and policies?

[daxelrod-rh]

I believe this is necessary to use CredentialsRequest. What docs should I link here?

[mffiedler]

We can start with the CCO doc explaining the different CCO credentials modes: https://docs.redhat.com/en/documentation/openshift_container_platform/4.22/html/authentication_and_authorization/managing-cloud-provider-credentials#about-cloud-credential-operator

[daxelrod-rh]

Should probably change to match the name of the operator

[daxelrod-rh]

Is this still the correct namespace and service account?

[mffiedler]

That is currently the namespace, but we need to change the default. I'll open a JIRA.

[daxelrod-rh]

This was copied verbatim from the README. However, the original prototype further scoped down this action to only instances tagged as owned by the cluster. Should we do that here?

https://github.com/rh-mobb/rosa-bgp/blob/4abb8eac2a0d2e35169827e66fa3cdff232ea94d/eni-srcdst-iam.tf#L28

[daxelrod-rh]

We need to decide whether we support VPC Sharing. I included this statement to indicate that we do not, because I don't think anyone has tested this with a shared VPC yet.

[daxelrod-rh]

We had decided in a meeting to target 4.22 or newer. However, the operator will likely work on 4.21.13 and newer. Older than 4.21.13 has this OVN-K bug ovn-kubernetes/ovn-kubernetes#6063

[daxelrod-rh]

Needs a period at the end of the sentence.

[daxelrod-rh]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: daxelrod-rh

That's fun. I didn't intend to indicate approval with my review, in fact, the GitHub interface didn't let me select an approval status, presumably because I am the author.

[daxelrod-rh]

Should we also include ccoctl instructions?

[mffiedler]

I vote yes. We can call it an example.

[daxelrod-rh]

/approve cancel

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign omark-rh for approval. For more information see the Code Review Process.

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

Details

Needs approval from an approver in each of these files:

• OWNERS

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

[frobware]

Thanks for starting this -- having the prerequisites written down somewhere other than in people's heads is overdue, and the shape you've picked (cluster, IAM, VPC, route server) is the right one.

Everything below comes from the setup I use to stand up a cluster for testing the operator day to day, so take it as one configuration that demonstrably works rather than as the definitive one. Where it differs from what you've written, that's worth talking through rather than assuming either of us has it right.

What I run

I drive this with aws-create-route-servers. It sits in a scratch repo of mine today, but I'd expect this -- or something close to it -- to end up in this repo before long, since we'll need exactly this estate stood up for e2e testing in prow. For a given cluster it builds:

• one route server, with an Amazon-side ASN
• the association to the cluster's VPC
• route server endpoints -- two in the first private subnet of each AZ, so six for a standard 3-AZ cluster
• route server propagation onto every route table in the VPC

And it deliberately stops there. It never creates route server peers and never touches SourceDestCheck, because those are the operator's own work. That boundary is exactly what this document is trying to describe, so the script is a reasonable cross-check on it: everything the script builds is a prerequisite, and everything it refuses to build isn't.

It works. On the cluster I brought up this morning that gives me rs-..., ASN 65000, six endpoints across us-east-2a/b/c. I hand the operator this and nothing else:

spec:
  platform: AWS
  bgp:
    localASN: 65001
  routerNodeSelector:
    bgp_router: "true"
  aws:
    region: us-east-2
    routeServerIDs:
      - rs-0c08421914c50de2b

and it discovers the rest -- all six endpoint addresses, their AZs, and the remote ASN -- then produces three FRRConfigurations, one per zone:

cudn-bgp-1 zone=us-east-2a neighbours=10.0.31.87 10.0.17.145
cudn-bgp-2 zone=us-east-2b neighbours=10.0.39.105 10.0.48.133
cudn-bgp-3 zone=us-east-2c neighbours=10.0.89.147 10.0.83.24

with all five conditions True.

Where the doc and that estate differ

I can't see propagation mentioned anywhere. This is the one I'd fix first. It's also the one that caught me out, which is why the script does it on every route table and why its header says:

Propagation is the one that hides. Without it every peer reaches available, every BGP session establishes, FRR advertises the CUDN prefix, and the routes stay inside the route server: nothing in the VPC can reach a pod while every signal says healthy.

My reading is that following the doc as written would leave you with every condition True, every BGP session Established and no traffic actually flowing, which is a horrible thing to go looking for. I'd give it a step of its own in the Route Server section, straight after creating the endpoints.

I don't think the endpoint IPs are an input. The doc says to note the IP address of each endpoint "for when you configure the operator", but going by what I pass above, spec.aws takes only region and routeServerIDs and the addresses come from discovery. I believe the same is true of the Amazon-side ASN. What I'd have found more useful in its place is being told it must differ from localASN. (There's a subtler trap too: reusing an ASN another route server in the region already holds is fine while the VPCs stay apart, but if they're ever peered or joined by a Transit Gateway, routes get silently discarded.) And spec.aws.region is required but unmentioned.

I think the IAM section is missing the ServiceAccount annotation. It looks lifted from the README's "AWS authentication (IRSA)" section, which is sensible, but that section has a third step this one doesn't:

oc annotate serviceaccount openshift-cudn-bgp-routing-controller-manager \
  -n openshift-cudn-bgp-routing eks.amazonaws.com/role-arn=<role-arn>

As I understand it the role never gets assumed without that, so I don't think the instructions can work as they stand. There's a visible join in the rendered page too -- "Step 2 --" is still labelled, but step 1 became prose and step 3 disappeared.

Related: two copies of the same policies in one repo will drift, and this one has already drifted on arrival. Could we pick one home? Either move the IRSA content here and link to it from the README, or leave it there and link from here. I lean towards here, since this is where people will be pointed.

Subnet placement, where I do it differently. The doc says two endpoints per subnet, "in each subnet your worker nodes are in". I place one pair per AZ, in that AZ's private subnet. Same six for a 3-AZ cluster, but the current wording suggests one pair per subnet, and doesn't say private.

I couldn't find anything about labelling the worker nodes. The operator creates one route server peer per node matching spec.routerNodeSelector, so with the bgp_router: "true" selector the rest of the repo uses:

for n in $(oc get nodes -l node-role.kubernetes.io/worker -o name); do
  oc label "$n" bgp_router=true --overwrite
done

Until that's done there are no peers and no BGP sessions, however correct the rest of the estate is.

Two small ones

The permissions policy is spot on, for what it's worth -- it matches the operator's EC2 client interface exactly, all nine actions plus sts:GetCallerIdentity. No notes.

"Persist routes: Disable" is right but it's already the AWS default; I created a route server without passing that option and got PersistRoutesState: disabled back. Fine to keep if the console makes you choose, just not something anyone has to do.

And "desgined" -> "designed".

---

Linking the script from the doc would give readers an executable reference alongside the prose, and make it harder for the two to drift. That gets more valuable rather than less over time: the estate it stands up is precisely the estate this document asks a reader to build by hand, so when it moves into this repo and starts backing the prow e2e jobs, it becomes the thing that proves the document right or wrong.

Addendum: the other two clouds

Since the doc opens with "follow the set of prerequisites that corresponds to the cloud provider you are using", it's probably worth knowing there are equivalents of that script for GCP and Azure, trying to capture the same thing:

• gcp-create-cloud-router -- the NCC hub, the Cloud Router and its interfaces, and a firewall rule opening tcp:179
• azure-create-route-server -- the subnet, the public IP, and the Route Server itself

Both have a --prerequisites-only flag, and that flag is more or less the answer to "what are the prerequisites on this cloud". Without it they also build the half the operator reconciles for itself -- canIpForward, the spoke and the BGP peers on GCP, the peerings on Azure -- because they predate operator support for those clouds and were the only way to stand a datapath up. With it they stop at the estate the operator expects to find. Whoever writes those sections could do worse than start from what that flag skips. The AWS script has no such flag, incidentally, because it never grew the second half: the operator could already create peers by the time it was written.

One thing that won't mirror the AWS section: Azure allows exactly one Route Server per virtual network, and it presents a redundant pair of addresses for the whole vnet rather than a per-zone endpoint. Every node peers with both whichever zone it's in, so the operator emits one peer group instead of one per AZ. The GCP shape is closer to Azure's than to AWS's here.

Worth saying that operator support for both is still in flight (#63 and #64), so I wouldn't write either section as settled yet -- but the prerequisite halves are the stable part, and they're unlikely to move much.