Beef up clusterwide proxy support #1999
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
openshift-ci
bot
added
the
approved
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #1999 +/- ##
=======================================
Coverage 57.71% 57.72%
=======================================
Files 186 186
Lines 25171 25219 +48
=======================================
+ Hits 14528 14557 +29
- Misses 9409 9425 +16
- Partials 1234 1237 +3
|
2uasimojo
force-pushed
the
HIVE-2198/clusterwide-proxy-2
branch
3 times, most recently
from
d526970
to
3ec5962
Compare
|
/hold pending verification |
openshift-ci
bot
added
the
do-not-merge/hold
2uasimojo
force-pushed
the
HIVE-2198/clusterwide-proxy-2
branch
2 times, most recently
from
5a7880f
to
e5c883c
Compare
|
/hold cancel Pending ack by @jianping-shu, but I think this might be ready to go now. |
openshift-ci
bot
removed
the
do-not-merge/hold
|
/retest |
1 similar comment
|
/retest |
This commit rounds out our support for clusterwide proxies by doing two things: - Previously, we relied on an outside agent (typically OLM) to set `HTTP_PROXY`, `HTTPS_PROXY`, and `NO_PROXY` environment variables on the hive-operator deployment We would pass these values down to the controllers and thence to workload pods such as provisioner and deprovisioner. This commit adds logic to discover those values from the cluster proxy object if not already set. - Install the cluster proxy's trusted CA bundle in all provision and deprovision pods. The ultimate source of this bundle is the ConfigMap referenced by the proxy's spec.trustedCA.name. We take advantage of the Cluster Network Operator's facility [1] to inject the merged CA bundle -- which includes the proxy's trusted CA bundle -- into a ConfigMap in the appropriate namespace and mount that ConfigMap on the pods. [1] https://docs.openshift.com/container-platform/4.12/networking/configuring-a-custom-pki.html#certificate-injection-using-operators_configuring-a-custom-pki HIVE-2198
2uasimojo
force-pushed
the
HIVE-2198/clusterwide-proxy-2
branch
from
e5c883c
to
253408e
Compare
2uasimojo
commented
Apr 20, 2023
| @@ -2239,7 +2239,7 @@ func TestClusterDeploymentReconcile(t *testing.T) { | |||
| func() runtime.Object { | |||
| cd := testClusterDeploymentWithDefaultConditions(testClusterDeploymentWithInitializedConditions(testClusterDeployment())) | |||
| cd.Status.InstallRestarts = 1 | |||
| cd.Spec.InstallAttemptsLimit = pointer.Int32Ptr(2) | |||
| cd.Spec.InstallAttemptsLimit = pointer.Int32(2) | |||
There was a problem hiding this comment.
ntr: latent, resolve linter complaints throughout
|
/retest |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 2uasimojo, abutcher 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:
Approvers can indicate their approval by writing |
|
@2uasimojo: 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. |
2uasimojo
added a commit
to 2uasimojo/hive
that referenced
this pull request
Apr 24, 2023
In HIVE-2198 / openshift#1999 / 253408e we started injecting a ConfigMap containing a CA bundle for better proxy support. Alas, we were only injecting it in the provisioning path. For hives that never had the earlier code, this probably would have been fine, as the ConfigMap would continue to exist until the cluster was deleted, whereupon it could properly be mounted on the deprovisioning pods. But for spokes that existed prior to upgrading to that code, and which were never re-reconciled before they were deleted (or, conceivably, if a user deleted the ConfigMap at some point in there) the ConfigMap would not exist, resulting in mount failures attempting to bring up the deprovisioning pods. This commit moves the ConfigMap injection earlier in the flow, ensuring it exists for both provisioning and deprovisioning paths. HIVE-2207
tsorya
pushed a commit
to tsorya/hive
that referenced
this pull request
Apr 27, 2023
In HIVE-2198 / openshift#1999 / 253408e we started injecting a ConfigMap containing a CA bundle for better proxy support. Alas, we were only injecting it in the provisioning path. For hives that never had the earlier code, this probably would have been fine, as the ConfigMap would continue to exist until the cluster was deleted, whereupon it could properly be mounted on the deprovisioning pods. But for spokes that existed prior to upgrading to that code, and which were never re-reconciled before they were deleted (or, conceivably, if a user deleted the ConfigMap at some point in there) the ConfigMap would not exist, resulting in mount failures attempting to bring up the deprovisioning pods. This commit moves the ConfigMap injection earlier in the flow, ensuring it exists for both provisioning and deprovisioning paths. HIVE-2207
2uasimojo
added a commit
to 2uasimojo/hive
that referenced
this pull request
May 4, 2023
HIVE-2198 / openshift#1999 / 253408e introduced a `Watch()` on `Proxy` so we can update environment variables and CA bundles when it changes. But `Proxy` is an OpenShift CRD, so that `Watch()` blows up the operator when running on vanilla k8s (and similar). This commit conditions that `Watch()` such that we only add it if we're running on OpenShift. HIVE-2210
2uasimojo
added a commit
to 2uasimojo/hive
that referenced
this pull request
May 5, 2023
In HIVE-2198 / openshift#1999 / 253408e we started injecting a ConfigMap containing a CA bundle for better proxy support. Alas, we were only injecting it in the provisioning path. For hives that never had the earlier code, this probably would have been fine, as the ConfigMap would continue to exist until the cluster was deleted, whereupon it could properly be mounted on the deprovisioning pods. But for spokes that existed prior to upgrading to that code, and which were never re-reconciled before they were deleted (or, conceivably, if a user deleted the ConfigMap at some point in there) the ConfigMap would not exist, resulting in mount failures attempting to bring up the deprovisioning pods. This commit moves the ConfigMap injection earlier in the flow, ensuring it exists for both provisioning and deprovisioning paths. HIVE-2207 (cherry picked from commit af76f94)
2uasimojo
added a commit
to 2uasimojo/hive
that referenced
this pull request
May 5, 2023
HIVE-2198 / openshift#1999 / 253408e introduced a `Watch()` on `Proxy` so we can update environment variables and CA bundles when it changes. But `Proxy` is an OpenShift CRD, so that `Watch()` blows up the operator when running on vanilla k8s (and similar). This commit conditions that `Watch()` such that we only add it if we're running on OpenShift. HIVE-2210 (cherry picked from commit f1f63cb)
2uasimojo
added a commit
to 2uasimojo/hive
that referenced
this pull request
May 11, 2023
In HIVE-2198 / openshift#1999 / 253408e we started injecting a ConfigMap containing a CA bundle for better proxy support. Alas, we were only injecting it in the provisioning path. For hives that never had the earlier code, this probably would have been fine, as the ConfigMap would continue to exist until the cluster was deleted, whereupon it could properly be mounted on the deprovisioning pods. But for spokes that existed prior to upgrading to that code, and which were never re-reconciled before they were deleted (or, conceivably, if a user deleted the ConfigMap at some point in there) the ConfigMap would not exist, resulting in mount failures attempting to bring up the deprovisioning pods. This commit moves the ConfigMap injection earlier in the flow, ensuring it exists for both provisioning and deprovisioning paths. HIVE-2207 (cherry picked from commit af76f94)
2uasimojo
added a commit
to 2uasimojo/hive
that referenced
this pull request
May 11, 2023
HIVE-2198 / openshift#1999 / 253408e introduced a `Watch()` on `Proxy` so we can update environment variables and CA bundles when it changes. But `Proxy` is an OpenShift CRD, so that `Watch()` blows up the operator when running on vanilla k8s (and similar). This commit conditions that `Watch()` such that we only add it if we're running on OpenShift. HIVE-2210 (cherry picked from commit f1f63cb)
2uasimojo
added a commit
to 2uasimojo/hive
that referenced
this pull request
Jun 15, 2023
It is valid to want to clean up a ClusterDeployment and its associated artifacts by deleting the whole namespace. Unfortunately, that results in the hive-trusted-cabundle ConfigMap (introduced by 253408e / openshift#1999) being deleted before the CD gets a chance to finish cleaning up. Then the clusterdeployment controller hot loops trying to recreate the ConfigMap, which fails because you can't create new things in a namespace that's terminating. With this commit, we add OwnerReferences from that ConfigMap to any ClusterDeployments in the same namespace. This should prevent the ConfigMap from being deleted until the owning CDs have finished cleaning up and been garbage collected. HIVE-2249
2uasimojo
added a commit
to 2uasimojo/hive
that referenced
this pull request
Jun 16, 2023
Deleting a namespace containing a ClusterDeployment and its associated artifacts results in an impossible situation: the deletion of the ClusterDeployment wants to trigger a deprovision, but we cannot create the necessary artifacts (e.g. ClusterDeprovision CR, deprovision Pod) in a namespace that is terminating. We have logic to skip deprovisioning and simply leak the cloud resources in this case. ...but we forgot to incorporate this logic for the hive-trusted-cabundle ConfigMap (introduced by 253408e / openshift#1999), and would hot-loop attempting and failing to create it, never reaching the aforementioned "give up" logic, resulting in wedging the namespace. This commit adds the logic, resulting in the same behavior as before. NOTE: It is still not supported / a good idea to delete a namespace containing extant ClusterDeployments, as this leaks cloud resources as described above. HIVE-2249
2uasimojo
added a commit
to 2uasimojo/hive
that referenced
this pull request
Jul 3, 2023
Deleting a namespace containing a ClusterDeployment and its associated artifacts results in an impossible situation: the deletion of the ClusterDeployment wants to trigger a deprovision, but we cannot create the necessary artifacts (e.g. ClusterDeprovision CR, deprovision Pod) in a namespace that is terminating. We have logic to skip deprovisioning and simply leak the cloud resources in this case. ...but we forgot to incorporate this logic for the hive-trusted-cabundle ConfigMap (introduced by 253408e / openshift#1999), and would hot-loop attempting and failing to create it, never reaching the aforementioned "give up" logic, resulting in wedging the namespace. This commit adds the logic, resulting in the same behavior as before. NOTE: It is still not supported / a good idea to delete a namespace containing extant ClusterDeployments, as this leaks cloud resources as described above. HIVE-2249
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR rounds out our support for clusterwide proxies by doing two
things:
HTTP_PROXY,HTTPS_PROXY, andNO_PROXYenvironment variables on thehive-operator deployment We would pass these values down to the
controllers and thence to workload pods such as provisioner and
deprovisioner. This commit adds logic to discover those values from the
cluster proxy object if not already set.
deprovision pods. The ultimate source of this bundle is the ConfigMap
referenced by the proxy's spec.trustedCA.name. We take advantage of the
Cluster Network Operator's facility [1] to inject the merged CA bundle --
which includes the proxy's trusted CA bundle -- into a ConfigMap in the
appropriate namespace and mount that ConfigMap on the pods.
[1] https://docs.openshift.com/container-platform/4.12/networking/configuring-a-custom-pki.html#certificate-injection-using-operators_configuring-a-custom-pki
HIVE-2198