OCPCLOUD-2208: Add E2E test for CAPI controllers cluster-wide proxy support#473
OCPCLOUD-2208: Add E2E test for CAPI controllers cluster-wide proxy support#473lunarwhite wants to merge 2 commits intoopenshift:masterfrom
Conversation
… clusters Extract MAPI-specific proxy env var verification into reusable helpers that work for any deployment. Add 'IsClusterProxyEnabled' (inspired by o/origin) to detect clusters with pre-existing proxy config, allowing proxy tests to skip MITM proxy deployment on proxy-enabled CI jobs and avoid cleanup conflict failure.
Add test verifying that when a cluster-wide proxy is configured, all Cluster API provider deployments have HTTP_PROXY, HTTPS_PROXY, and NO_PROXY environment variables injected into their containers.
openshift-ci-robot
added
the
jira/valid-reference
|
@lunarwhite: This pull request references OCPCLOUD-2208 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughRefactored proxy configuration testing framework by removing deployment-specific constants from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
bot
added
the
needs-ok-to-test
|
Hi @lunarwhite. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
|
/ok-to-test |
openshift-ci
bot
added
ok-to-test
|
@lunarwhite: all tests passed! 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. |
mdbooth
left a comment
mdbooth
left a comment
There was a problem hiding this comment.
/lgtm
Some comments, but this looks good to me. I'd approve, but I don't feel qualified to review the important-looking label.
| "[sig-cluster-lifecycle] Cluster API When cluster-wide proxy is configured, Cluster API provider deployments should", | ||
| framework.LabelDisruptive, framework.LabelConnectedOnly, framework.LabelPeriodic, framework.LabelCAPI, |
There was a problem hiding this comment.
I don't know how to review this label, but I suspect it's important. @damdo ?
| deployments := &appsv1.DeploymentList{} | ||
| Eventually(client.List(ctx, deployments, | ||
| runtimeclient.InNamespace(framework.ClusterAPINamespace), | ||
| runtimeclient.HasLabels{"cluster.x-k8s.io/provider"}, |
There was a problem hiding this comment.
I don't think we should continue to rely on these labels. Maybe the installer revision label? That would unambiguously identify any CAPI operand.
| var err error | ||
|
|
||
| client, err = framework.LoadClient() | ||
| Expect(err).NotTo(HaveOccurred(), "Failed to load client") |
There was a problem hiding this comment.
Skip if the CAPIMachineManagement (or whatever) feature gate is not set?
| runtimeclient.InNamespace(framework.ClusterAPINamespace), | ||
| runtimeclient.HasLabels{"cluster.x-k8s.io/provider"}, | ||
| )).Should(Succeed(), "timed out listing Cluster API provider Deployments.") | ||
| Expect(deployments.Items).NotTo(BeEmpty(), "no Cluster API provider Deployments found") |
There was a problem hiding this comment.
For extra robustness I'd add a wait, probably in BeforeEach, that generation == observedRevisionGeneration and desiredRevision == currentRevision on the ClusterAPI object.
4 participants
Depends on openshift/cluster-capi-operator#517
Summary
openshift-e2e-aws-proxy)Validation
I didn't follow the MAPI ones to add a dedicate test for testing machine provisioning through a proxy, since it might require a bit refactor/framework work, which better to be a follow-up. Instead I ran the full set of CAPI tests on some pj-rehearsal clusters.
Clusters with proxy enabled by default
New CAPI proxy test passing
Existing MAPI tests still passing
Clusters without proxy enabled
New CAPI proxy test passing
Existing MAPI tests still passing