Expand proxy detection support to Dex and support proxy configuration #3636
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
pasanw
commented
Dec 6, 2024
Comment on lines
+314
to
+385
| // Determine the current deployment availability. | ||
| var currentAvailabilityTransition metav1.Time | ||
| var currentlyAvailable bool | ||
| dexDeployment := v1.Deployment{} | ||
| err = r.client.Get(ctx, client.ObjectKey{Name: render.DexObjectName, Namespace: render.DexNamespace}, &dexDeployment) | ||
| if err != nil && !errors.IsNotFound(err) { | ||
| r.status.SetDegraded(oprv1.ResourceReadError, "Failed to read the deployment status of Dex", err, reqLogger) | ||
| return reconcile.Result{}, nil | ||
| } else if err == nil { | ||
| for _, condition := range dexDeployment.Status.Conditions { | ||
| if condition.Type == v1.DeploymentAvailable { | ||
| currentAvailabilityTransition = condition.LastTransitionTime | ||
| if condition.Status == corev1.ConditionTrue { | ||
| currentlyAvailable = true | ||
| } | ||
| break | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Resolve the proxies used by each Dex pod. We only update the resolved proxies if the availability of the | ||
| // Dex deployment has changed since our last reconcile and the deployment is currently available. We restrict | ||
| // the resolution of pod proxies in this way to limit the number of pod queries we make. | ||
| if !currentAvailabilityTransition.Equal(&r.lastAvailabilityTransition) && currentlyAvailable { | ||
| // Query dex pods. | ||
| labelSelector := labels.SelectorFromSet(map[string]string{ | ||
| "app.kubernetes.io/name": render.DexObjectName, | ||
| }) | ||
| pods := corev1.PodList{} | ||
| err := r.client.List(ctx, &pods, &client.ListOptions{ | ||
| LabelSelector: labelSelector, | ||
| Namespace: render.DexNamespace, | ||
| }) | ||
| if err != nil { | ||
| r.status.SetDegraded(oprv1.ResourceReadError, "Failed to list the pods of the Dex deployment", err, reqLogger) | ||
| return reconcile.Result{}, nil | ||
| } | ||
|
|
||
| // Resolve the proxy config for each pod. Pods without a proxy will have a nil proxy config value. | ||
| var podProxies []*httpproxy.Config | ||
| for _, pod := range pods.Items { | ||
| for _, container := range pod.Spec.Containers { | ||
| if container.Name == render.DexObjectName { | ||
| var podProxyConfig *httpproxy.Config | ||
| var httpProxy, httpsProxy, noProxy string | ||
| for _, env := range container.Env { | ||
| switch env.Name { | ||
| case "http_proxy", "HTTP_PROXY": | ||
| httpProxy = env.Value | ||
| case "https_proxy", "HTTPS_PROXY": | ||
| httpsProxy = env.Value | ||
| case "no_proxy", "NO_PROXY": | ||
| noProxy = env.Value | ||
| } | ||
| } | ||
| if httpProxy != "" || httpsProxy != "" || noProxy != "" { | ||
| podProxyConfig = &httpproxy.Config{ | ||
| HTTPProxy: httpProxy, | ||
| HTTPSProxy: httpsProxy, | ||
| NoProxy: noProxy, | ||
| } | ||
| } | ||
|
|
||
| podProxies = append(podProxies, podProxyConfig) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| r.resolvedPodProxies = podProxies | ||
| } | ||
| r.lastAvailabilityTransition = currentAvailabilityTransition | ||
|
|
There was a problem hiding this comment.
For context, this logic is a mirror of what is found in the cluster connection controller
pasanw
changed the title
rene-dekker
reviewed
Dec 9, 2024
| @@ -18,6 +18,9 @@ import ( | |||
| "context" | |||
| "fmt" | |||
|
|
|||
| "golang.org/x/net/http/httpproxy" | |||
There was a problem hiding this comment.
nit: the imports in this file are a bit of a mess
rene-dekker
reviewed
Dec 9, 2024
| // HTTPSProxy defines the value of the HTTPS_PROXY environment variable that will be set on Tigera containers that connect to | ||
| // destinations outside the cluster. | ||
| // +optional | ||
| HTTPSProxy string `json:"httpsProxy,omitempty"` |
There was a problem hiding this comment.
Not to throw a wrench into things, but since these vars can take a separated list of items should we make them lists in our API?
httpsProxy:
- foo.com
- bar.org
and combine them? Or nah?
There was a problem hiding this comment.
httpProxy and httpsProxy should always be a single value, not a list AFAIK. noProxy can be a list of exempted destinations, but it's expected that this is a comma-separated list. If we ask for an array, it will likely be an inconvenience for the user IMO - they've likely been passing a comma-separated list elsewhere
There was a problem hiding this comment.
ah right, it was noProxy I was thinking about. I'm fine with the stance that it should just be the same as the env var.
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.
Description
This PR expands the support for proxy detection to Dex, and adds support for users to specify a proxy that the Operator will set on Guardian and Dex containers.
The proxy detection logic will always be used to render policy, in case the user has used both the proxy configuration and a mutating admission webhook to set the proxy environment variables.
For PR author
make gen-filesmake gen-versionsFor PR reviewers
A note for code reviewers - all pull requests must have the following:
kind/bugif this is a bugfix.kind/enhancementif this is a a new feature.enterpriseif this PR applies to Calico Enterprise only.