Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueComment |
|
Skipping CI for Draft Pull Request. |
|
/test e2e-gcp-operator |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: frobware The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/test e2e-gcp-operator |
1 similar comment
|
/test e2e-gcp-operator |
e2e-gcp-operator is blocked on a cluster-profile permission gapThis blocks GCP e2e. The suite builds a Network Connectivity Center hub as a test prerequisite, and the The run now reports the whole role up front instead of dying on the first denial. From build 2099835132385955840, project That is The askGrant Delete matters as much as create: without it a failed run would leak a hub into a shared project. That denial was invisible until now because no run got far enough to try. Nothing here looks like a regression on your side. The installer does not create a hub, so the provisioner role would never have needed this; no cluster this job has built has had one. Why every project
Granting in one project only leaves the job a coin flip, and on the evidence it would have to be the project that is already over-privileged. Pinning to SeparatelyNot part of the ask, but the next thing that will bite. In build 2098472557710675968, which did get its estate built in This PRStill WIP. The probe that produced the table is 4cbbe80; it runs on every GCP e2e run, so each future run gives a full capability reading for whichever project it lands in. |
`az -o tsv` has the same shape as `aws --output text`: tab separated, and a bare newline for an empty result, which the obvious `| grep .` filter reports as a failure that then has to be swallowed. So the Azure scripts want the same helper, and a cloud that does not source AWS's library should still get it. Four assertions cover the behaviour and a fifth pins the reason for the move, by calling it from a shell that has sourced common.sh and nothing else. (cherry picked from commit c4a5467)
hack/lib/ci.sh sourced hack/aws/lib.sh, and its ci_bootstrap exported AWS_SHARED_CREDENTIALS_FILE and installed the aws CLI unconditionally, so an Azure or GCP job could use none of it without either a branch inside that file or a second copy of it that drifts. What remains in lib/ci.sh knows about no cloud: the kubeconfig prow leaves in SHARED_DIR, the scratch directory, and its removal. The AWS half moves to hack/aws/ci.sh, which defines ci_bootstrap as those pieces plus its own two. The entry points source that instead, which is the only change to them. Naming the pieces is what makes them testable. ci_bootstrap as a whole cannot be called from a unit test, because its last step provisions a CLI and would fetch sixty megabytes, so the credential handling had no coverage at all. The eleven assertions added here cover both halves against temporary directories. Behaviour is unchanged, measured rather than assumed: all three entry points produce identical output and identical exit codes before and after, run against no cluster and an unknown profile. (cherry picked from commit 133a958)
COPY . . copies .git, which in a worktree is a file naming a directory outside the build context. Copied in, it points nowhere, so the first git call fails with "not a git repository" and the build stops: the Dockerfile was observed exiting 128 at the safe.directory line, and Containerfile.bgp-cloud-connector carries the same line. Nothing in either build needs the metadata. Both run `go build ... cmd/main.go`, a file list rather than a package path, and go stamps no VCS information for those: `go version -m bin/manager` reports the build as command-line-arguments from a full clone as readily as from a worktree. The safe.directory line was guarding a git call that produces nothing, so it goes with the metadata rather than being made to work. Verified by building both files from a worktree and the Dockerfile again from an ordinary clone. (cherry picked from commit 71d9d6c)
buildGCPPlatform passed no credential, so the Google libraries fell through their default chain to the metadata server. Measured on an IPI cluster: discovery failed with credentials: cannot fetch token: Get "http://169.254.169.254/computeMetadata/v1/instance/service-accounts/ default/token": dial tcp 169.254.169.254:80: connect: connection refused which is the same shape the Azure side had before it asked for a credential of its own, and for the same reason: no pod here can reach the link-local metadata address. The order is cloud-network-config-controller's, because a cluster can be in any of these states and that controller has met all three. A federated config where the cluster uses workload identity, a service account key where the cloud credential operator mints one, and whatever the process already holds when somebody runs the manager from a desk. Where none of them exist the request is raised and ErrCredentialsPending is returned, which Reconcile waits out rather than treating as a fault. universe_domain is set explicitly on every credential. Without it the client asks the metadata server which universe it is in, which fails exactly like the token fetch above. cloud-network-config-controller carries the same workaround and says why, which saved finding it the expensive way. The credential type is named rather than inferred, as the Azure side does it and as that controller does: which credential is in use should be readable here rather than deduced at run time. The permissions are the calls the operator makes and nothing else -- reading the Cloud Router, rewriting its peers, reading and rewriting the NCC spoke, and setting canIpForward -- with a test that fails if the request ever asks for one the operator does not use.
buildGCPPlatform passed none, so the clients fell back to the Google libraries' own chain and reached for the metadata server. Config now carries what ResolveCredentials returned and the two constructors take it, keeping the fallback for a manager run from a desk, where that chain is the right answer. The permissions are what a cluster said, not what the documentation implied. compute.instances.updateNetworkInterface is not enough for canIpForward or nested virtualisation: both are whole-instance updates and want compute.instances.update. Rewriting a Cloud Router's BGP peers is a change to the network's policy as far as IAM is concerned and wants compute.networks.updatePolicy on the cluster's own network, which no other operator in openshift/* asks for, because none of them touch router peers. Each of those was a 403 naming the permission it lacked. The request is now reconciled whenever the operator uses the minted secret, rather than only when there is no secret to find. Without that a permission added in code never reaches the cluster: the secret already exists, nothing rewrites the request, and the operator fails with a 403 for something it believes it asked for. That is a test as well, because it is invisible until the list changes. The bundle is regenerated alongside config/rbac. The two disagreeing is what broke e2e-azure-operator: make deploy builds the role from config/rbac and OLM installs from the ClusterServiceVersion, so a rule present in one and missing from the other works locally and fails in CI. Verified on a live GCP cluster with nothing pre-minted: the operator raised its own CredentialsRequest, the cloud credential operator minted the service account, discovery found the Cloud Router, six peers were created and all six BGP sessions reached Established.
The rule is the one hack/aws/lib.sh is built around, and the reason Azure needed the same file: a read that failed is not an answer. Swallowing gcloud's exit status with `2>/dev/null || true` turns "I could not ask" into "there is nothing there", which a create acts on by building a second estate and a delete acts on by reporting success over the first. stderr goes to a file rather than into the value. gcloud writes deprecation notices and property chatter to stderr on calls it answers with 0, so folding it in with 2>&1 leaves that text inside a router name, which then goes back to GCP as --router. Breaking that rule fails two of the assertions here and nothing else, which is what they are for. The existence checks are filtered lists rather than describes, and report a failed question as 2 rather than as absent, because a describe cannot tell "it is gone" from "I could not ask" and both exit non-zero. Names are derived from the infra id so two clusters in one project cannot collide.
Two bugs a stub cannot show you, both found by running the helpers against a live cluster rather than only against fakes. gcp_hub_exists reported a hub that was plainly there as absent. The two GCP APIs disagree about what a name filter matches: compute routers filter on the short name, so name=<name> works, while network-connectivity hubs filter on the full resource path and --format='value(name)' prints only the last segment. An equality filter therefore matches nothing and reads as absent, which a create would act on by building a second hub. The Cloud Router's interface addresses came back as 10.0.128.5/17;10.0.128.6/17: one semicolon-separated value, each carrying the mask it was allocated with. A BGP neighbour address is neither, and the generated profile needs one per line without the prefix. Both are pinned by assertions that fail when the fix is removed, and both were re-checked against the cluster afterwards: the hub reads present, an absent one still reports 1 rather than 0, and the addresses come back as 10.0.128.5 and 10.0.128.6 -- the same pair the runbook recorded by hand on 11 August.
Prerequisites only: the NCC hub, the Cloud Router with its redundant pair of interfaces, and a firewall rule for tcp:179. The operator creates the BGP peers, the router appliance spoke, and the canIpForward and nested virtualisation flags, and it orders them itself, so this stops where the operator starts. Creating the peers here is worse than redundant. GCP refuses two peers sharing a peer IP on one interface, so the operator's own peers are rejected and the configuration sits Degraded on "Invalid value for field 'resource.bgpPeers[].peerIpAddress'" -- measured against an estate a script had helpfully finished. It would also let a broken operator adopt a working estate and look identical to a healthy one, which is why the Azure script stops in the same place. Identity is decided in shell, not by gcloud. --filter is not an identity test and is not consistent about what it is: compute routers match the short name, network-connectivity hubs match the full resource path while the value projection prints the last segment, and firewall rules match by prefix. Two of those bit today in opposite directions. A hub that existed read as absent, which would have built a second one, and name=<infra>-bgp matched <infra>-bgp-worker-subnet so a rule that did not exist read as present, which would have left nothing opening 179. Both directions are expensive, so the lists are unfiltered on identity and the comparison is exact. The firewall is sourced from the Cloud Router's own interface addresses rather than from the subnet, so it is as narrow as the thing it exists for, and it comes last because those addresses do not exist until the router does. Verified against a live cluster: a dry run adopts the hub and router that are there, and reports the firewall absent with sources 10.0.128.5,10.0.128.6.
Removes only what the create script builds: the firewall rule, the Cloud Router and the NCC hub. The peers and the router appliance spoke are the operator's and go when the BGPCloudConfiguration is deleted. The order is forced rather than chosen. A hub refuses to go while a spoke is attached and the spoke belongs to the operator, so a teardown that removed the CRs afterwards would fail on the hub and leave it behind. Deleting the router releases the interfaces the firewall rule is sourced from, so the rule goes first while those addresses still read. A hub that still has spokes says so, names them, and points at hack/delete-e2e-crs.sh, because "resource in use" on its own sends you looking in the wrong place. Every step treats "already gone" as success, so a second run after a partial failure finishes the job rather than erroring on the half that worked. Measured against a live estate: 83s for the first run, then 14s and 15s reporting all three already gone, all exit 0. It also leaves alone what it did not create. The installer's Cloud NAT router and a firewall rule from an older script both survived, which is the point of comparing names exactly: under gcloud's prefix matching, name=<infra>-bgp matches <infra>-bgp-worker-subnet, and this would have deleted somebody else's rule.
Both only appear when the operator creates the router appliance spoke rather than adopting one somebody else made, which is what building the estate as prerequisites only made it do. networkconnectivity.operations.get. The two API families name the operation-polling permission separately: compute has zone and region operations and networkconnectivity has its own. Missing it is not a failure to create. The spoke is made, the poll is refused, the reconcile errors, and the next pass sees the spoke and reports complete -- so the configuration oscillated between Ready and Degraded while nothing worked. Granting it did not fix anything; it made the real failure visible. compute.networks.get. Creating the spoke fetches the network the instances are in, and without it the operation fails with code=7 on "failed to fetch resource .../global/networks/<name>". That was the failure the unpollable operation had been hiding. Verified on a live cluster against an estate built by hack/gcp/create-cloud-router.sh: the spoke went CREATING to ACTIVE, the configuration reached Ready, and all six BGP sessions Established.
Generated rather than checked in, because a profile has to name the Cloud Router of the cluster it runs against and that is minted a minute earlier. The checked-in profiles under test/e2e/manifests cannot. It carries spec.gcp and no spec.bgp.peerGroups. The CRD requires peer groups only under platform Manual and forbids them on every cloud, so naming them would be rejected; the operator discovers the Cloud Router's interfaces and publishes them in status.peerGroups, which is what a cloud suite asserts against. The ASNs are checked rather than assumed. The Cloud Router's is whatever it was created with and the cluster's is what this is about to write, and if they match the session is not eBGP -- a failure that looks like a stuck session rather than like a configuration error. The router node label is the repo's own, bgp_router=true, as hack/label-router-nodes.sh applies and as AWS and Azure both use. The older GCP scripts used networking.openshift.io/cudn-bgp-router=, which spec.routerNodeSelector would have to match; one convention is better than two, and this is the one the rest of the repo already has. The CUDN subnet clears the cluster network, the service network and the installer's VPC, which the runbook records as the constraint. Verified against a live cluster: both manifests pass server-side validation with their CEL rules, and applying the generated configuration reached Ready with one peer group keyed on the Cloud Router.
e2e-gcp-operator exists in openshift/release and its test step runs hack/ci-e2e-gcp.sh, which until now did not exist: the job fails with exit 127. This is the shell half. Three files, composed the way the AWS and Azure ones are. ci-e2e-gcp-run.sh creates and never removes, ci-e2e-gcp-teardown.sh removes and never creates, and ci-e2e-gcp.sh is the only one that says "always". The run half ends by running the suite, as ci-e2e-aws-run.sh:93 does and as the name says. GCP needs no image of its own, unlike Azure, and hack/gcp/ensure-cli.sh is why. az is a Python distribution with nothing to fetch, so e2e-azure-operator had to be given an image with it baked in and an openshift/release change to go with it. The Google Cloud CLI ships as a self-contained tarball carrying its own interpreter, exactly like the aws CLI's zip, so this fetches it when the one on PATH is too old and e2e-gcp-operator stays on `from: src`. Nothing is downloaded when a usable gcloud is already there, which is the desk case. The credentials come from CLUSTER_PROFILE_DIR/gce.json, which is the path the release repo's own ipi-conf-gcp steps read, and are applied through CLOUDSDK_AUTH_CREDENTIAL_FILE_OVERRIDE rather than gcloud auth activate-service-account: the override needs no writable config and mutates nothing outside the process. gcloud's config goes in the run's scratch directory, because a prow container runs as a random uid whose home it may not own. The sequencer uses job control rather than the setsid hack/ci-e2e-aws.sh still has. setsid forks when its caller is already a process group leader, and then wait returns immediately while the test runs on and kill -TERM -$! answers "no such process group". Verified against a live cluster: the teardown removed the CRs so the operator released its spoke, then the firewall, the Cloud Router and the hub, in the order the hub's own constraint forces, and an independent check found none of the three left.
The shared suite under test/e2e derives every expectation from spec.bgp.peerGroups, which the CRD requires under platform Manual and forbids under every cloud, so it cannot serve GCP. This reads status.peerGroups, as a cloud suite has to. Four specs in an Ordered container with setup in BeforeAll, so any one can be run alone with --focus. What it asserts is GCP's shape rather than a translation of Azure's. One peer group keyed on the Cloud Router, because a GCP subnet is regional and there is one router for the region, where AWS has one group per availability zone. Two neighbours, the router's redundant pair of interfaces. Two peers per router node, one per interface, against Azure's one per node. A router appliance spoke that must reach ACTIVE before anything can peer, which Azure has no analogue for, and whose count is derived from NCCMaxInstancesPerSpoke rather than assumed to be one. And canIpForward on each instance, which is GCP's equivalent of enableIPForwarding. Peer names come from gcpplatform.PeerName rather than a reimplementation, so the suite and the operator cannot drift on the one thing that marks a peer as ours. The neighbour ASN is checked against the Cloud Router's own and asserted to differ from the cluster's, because two that match are not eBGP and the symptom is a session that never comes up rather than a configuration error. It talks to GCP through the SDK rather than through the operator's own clients, for the reason the AWS suite talks to EC2: a suite that observes through the code under test cannot see a fault in it. Cleanup runs at the start of a run and deliberately nowhere else, and the diagnostics print on failure, both for the reasons the Azure suite carries: Ginkgo skips later specs once one fails so the wreckage is left to read, and in CI the sequencer tears the estate down whatever happens, so the log is the only record that survives.
The two helpers that perturb the estate -- removing a peer, turning canIpForward off -- are how a spec sets up the thing it wants to watch the operator repair. They are not the thing under test. Observed on the first live run: E2E-GCP-03 failed in 1.1 seconds with "Error 503: Internal error. Please try again", from Instances.Update, with the configuration Ready and all six conditions True either side of it. The diagnostics said so immediately, which is how it was clear the operator had done nothing wrong. Server-side refusals are retried, four attempts with a widening gap. A 4xx is passed straight back, because that means the request was wrong and a suite that retried its way past that would be hiding a real failure.
The spec failed twice with "Error 503: Internal error", four retries and fifty-three seconds apart, on an instance that was RUNNING and idle with no competing operation. I read the first as a transient cloud failure and added a retry, which was the wrong fix for the wrong diagnosis: the second run failed identically and no operation was ever created, so GCP had refused the request outright rather than failed to serve it. EnsureCanIPForward sends back the whole instance it read and names MostDisruptiveAllowedAction REFRESH. The helper here sent three fields and no disruption policy, which GCP answers with a bare 503 instead of a useful 400. It now perturbs the estate the same way the operator changes it, so the value is the only difference between them. The retry is kept, because a genuine 5xx should not fail a spec that is only setting up, but it is no longer load-bearing. With this, the suite passes in full against a live cluster: 4 of 4 in 836 seconds.
A GCP call that fails with invalid_grant reports "account not found" whether the service account was deleted underneath a good secret or the secret names an account that was never ours, and the diagnostics could not tell those apart: they said the request was provisioned and the secret had a service_account.json, neither of which identifies anybody. The dump now carries the account the secret actually holds beside the one the cloud credential operator says it minted, along with the request's generation and lastSyncTimestamp and the secret's resourceVersion. Equal accounts mean the identity we were given stopped existing; different ones mean we are holding something stale. The generation says whether the operator provoked a re-mint by rewriting its own request, and the resourceVersion says whether the secret moved while the run was going. Only the account part is printed, never the project: prow logs for openshift repositories are public and the project is redacted there.
The e2e-gcp-operator job died on a single PERMISSION_DENIED for networkconnectivity.hubs.create and could say nothing about the rest of the role. Learning a role one denial per run costs a cluster install per permission, and a green run proved only that nothing had failed yet, not what the account was actually allowed to do. So ask, for the whole list at once, before anything is read or created, and print the answer either way. gcp_permissions_held puts the estate's permissions to testIamPermissions and reports which came back; gcp_require_permissions prints every one as granted or DENIED and stops if any are missing, naming all of them rather than the first. testIamPermissions rather than a policy read: get-iam-policy needs resourcemanager.projects.getIamPolicy, which an account can itself be denied, and answers in roles that would then have to be expanded into permissions here. This needs no permission of its own and answers in the same vocabulary the denials use. It goes over REST because gcloud has no surface for it on a project -- checked against 565.0.0, where `gcloud projects test-iam-permissions` is not a command and `gcloud iam list-testable-permissions` answers what may be granted on the resource rather than what the caller holds. The answer is matched as whole quoted names. The closing quote is what stops compute.routers.get reading as held because compute.routers.getIamPolicy is, which is the trap gcp_name_exists already compares with grep -Fxq to avoid.
|
/test e2e-gcp-operator |
|
@frobware: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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-sigs/prow repository. I understand the commands that are listed here. |
4cbbe80 to
aecea0b
Compare
Work in progress. The shell half and the suite are complete and pass against a live cluster, but
e2e-gcp-operatorhas never run, and two of the three cherry-picks at the base of this branch also sit in #122, so this wants rebasing once that merges.e2e-gcp-operatorexists in openshift/release and itsteststep runshack/ci-e2e-gcp.sh, which until now did not exist: the job fails with exit 127. This is the GCP counterpart of #122, following the same shape, with the differences GCP actually has rather than a translation of Azure's.The operator could not reach GCP from inside a cluster
buildGCPPlatformpassed no credential, so the Google libraries fell through their default chain to the metadata server. Measured on an IPI cluster, discovery failed withdial tcp 169.254.169.254:80: connect: connection refused-- the same shape Azure had, for the same reason: no pod here can reach the link-local address.The resolution order is
cloud-network-config-controller's, because a cluster can be in any of these states and that controller has met all three: a federated config where the cluster uses workload identity, a service account key where the cloud credential operator mints one, and whatever the process already holds when somebody runs the manager from a desk.universe_domainis set explicitly, because without it the client asks the metadata server which universe it is in and fails exactly like the token fetch. That controller carries the same workaround and says why, which saved finding it the expensive way.The permissions are what a cluster said rather than what the documentation implied, and four of them only appeared by running it.
compute.instances.updaterather thanupdateNetworkInterface, becausecanIpForwardand nested virtualisation are whole-instance updates.compute.networks.updatePolicy, because rewriting a Cloud Router's BGP peers is a change to the network's policy as far as IAM is concerned; nothing else in openshift/* asks for it, because nothing else touches router peers. Andnetworkconnectivity.operations.getpluscompute.networks.get, which only surface when the operator creates the router appliance spoke rather than adopting one somebody else made.The
CredentialsRequestis reconciled whenever the operator uses the minted secret, not only when there is no secret to find. Without that a permission added in code never reaches the cluster: the secret already exists, nothing rewrites the request, and the operator fails with a 403 for something it believes it asked for.The estate stops where the operator starts
hack/gcp/create-cloud-router.shbuilds the NCC hub, the Cloud Router with its redundant pair of interfaces, and a firewall rule fortcp:179. The operator creates the BGP peers, the spoke, and the forwarding flags, and orders them itself.Creating the peers in the script is worse than redundant, which GCP demonstrated: it refuses two peers sharing a peer IP on one interface, so the operator's own peers were rejected and the configuration sat Degraded on
Invalid value for field 'resource.bgpPeers[].peerIpAddress'. It would also let a broken operator adopt a working estate and look identical to a healthy one.Identity is decided in shell rather than by gcloud, because
--filteris not an identity test and is not consistent about what it is. Compute routers match the short name; network-connectivity hubs match the full resource path while the value projection prints the last segment, soname=<name>finds nothing and a hub that exists reads as absent; firewall rules match by prefix, soname=<infra>-bgpmatched<infra>-bgp-worker-subnetand a rule that did not exist read as present. Both directions are expensive.No openshift/release change is needed
Unlike Azure, which needed an image with
azbaked in and a release pull request to go with it,hack/gcp/ensure-cli.shfetches the Google Cloud CLI when the one on PATH is too old.azis a Python distribution with nothing to fetch; the Cloud CLI ships as a self-contained tarball carrying its own interpreter, exactly like the aws CLI's zip. Soe2e-gcp-operatorstays onfrom: srcas it already is.The suite asserts GCP's shape
One peer group keyed on the Cloud Router, because a GCP subnet is regional and there is one router for the region, where AWS has one group per availability zone. Two neighbours, the router's redundant pair. Two peers per router node, one per interface, against Azure's one per node. A router appliance spoke that must reach
ACTIVEbefore anything can peer, which Azure has no analogue for, and whose count is derived fromNCCMaxInstancesPerSpokerather than assumed to be one. AndcanIpForwardper instance.It reads
status.peerGroups, never the spec, because the CRD forbidsspec.bgp.peerGroupson a cloud platform. Peer names come fromgcpplatform.PeerNamerather than a reimplementation, so the suite and the operator cannot drift on the one thing marking a peer as ours. Cleanup runs at the start of a run and deliberately nowhere else, and diagnostics print on failure, both carried over from #122: Ginkgo skips later specs once one fails so the wreckage stays readable, and in CI the sequencer tears the estate down whatever happens, so the log is the only record that survives.Test plan
The suite passes in full against a live IPI cluster in
us-east1:4 Passed | 0 Failedin 836 seconds, on an estate built entirely by these scripts from nothing.Every piece was verified against that cluster rather than only against fakes, and doing so found things the fakes could not: the two gcloud filter traps above, and a helper of mine that changed
canIpForwardwith a stripped-down instance and no disruption policy, which GCP answers with a bare 503 rather than a useful 400. It now changes the field the wayEnsureCanIPForwarddoes.The estate scripts converge.
create-cloud-router.shbuilds from nothing in 111s and adopts in 23s on a rerun.delete-cloud-router.shtook 83s, then 14s and 15s reporting everything already gone, all exit 0, and left the installer's Cloud NAT router and an older script's firewall rule alone.hack/lib-test.shgoes from 95 assertions to 113.What is not done
e2e-gcp-operatorhas not been triggered, so nothing here has run under prow. The three commits at the base --print_fieldsinto the common library, the CI bootstrap split, and the build-context fix -- are cherry-picked from #122 and should come out when that merges.