WIP: Adds proxy admission controller plugin #23384
Conversation
openshift-ci-robot
added
the
do-not-merge/work-in-progress
openshift-ci-robot
added
the
size/L
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: danehans 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 |
thought we all but concluded it could not be due to the need to dynamically update noproxy w/ the host node ips. |
danehans
force-pushed
the
proxy_admission
branch
from
d6d13cc to
d9b8a9c
Compare
| "net/url" | ||
|
|
||
| "k8s.io/apimachinery/pkg/util/sets" | ||
|
|
| ) | ||
|
|
||
|
|
||
| // Register registers a plugin |
| return obj, nil | ||
| } | ||
|
|
||
| type proxyV1 struct {} |
There was a problem hiding this comment.
move up and add godoc describing what this admission plugin does.
| allErrs := field.ErrorList{} | ||
|
|
||
| if len(spec.HTTPProxy) == 0 && len(spec.HTTPSProxy) == 0 { | ||
| allErrs = append(allErrs, field.Invalid(field.NewPath("spec.HTTPProxy"), spec.HTTPProxy, "must be set")) |
There was a problem hiding this comment.
why? These are optional. We also want to start with an empty object.
| } | ||
| } | ||
| if len(spec.HTTPSProxy) != 0 { | ||
| if err := validateURI(spec.HTTPSProxy); err != nil { |
There was a problem hiding this comment.
does this have to be an https URI? similarly above
| allErrs := field.ErrorList{} | ||
|
|
||
| if len(spec.HTTPProxy) == 0 && len(spec.HTTPSProxy) == 0 { | ||
| allErrs = append(allErrs, field.Invalid(field.NewPath("spec.HTTPProxy"), spec.HTTPProxy, "must be set")) |
There was a problem hiding this comment.
lower-case httpProxy. Same below
| allErrs = append(allErrs, field.Invalid(field.NewPath("HTTPSProxy"), spec.HTTPSProxy, err.Error())) | ||
| } | ||
| } | ||
| if len(spec.ReadinessEndpoints) != 0 { |
| } | ||
| } | ||
| } | ||
| if len(spec.NoProxy) != 0 { |
| if len(spec.ReadinessEndpoints) != 0 { | ||
| for _, endpoint := range spec.ReadinessEndpoints { | ||
| if err := validateURI(endpoint); err != nil { | ||
| allErrs = append(allErrs, field.Invalid(field.NewPath("ReadinessEndpoints"), spec.ReadinessEndpoints, err.Error())) |
There was a problem hiding this comment.
lower-case ReadinessEndpoints
| } | ||
| if len(spec.NoProxy) != 0 { | ||
| for _, v := range strings.Split(spec.NoProxy, ",") { | ||
| v = strings.TrimSpace(v) |
There was a problem hiding this comment.
why TrimSpace? Are spaces allowed? Why should they?
| return allErrs | ||
| } | ||
|
|
||
| allErrs = append(allErrs, validateProxySpec(obj.Spec)...) |
There was a problem hiding this comment.
will we get an operator which writes the status? If yes, we have to enable the status subresource in the CRD. If no, we have to check that status is empty here.
There was a problem hiding this comment.
yes status will be written by a controller.
| return nil | ||
| } | ||
|
|
||
| // validateReadinessEndpointWithRetries tries to validate the proxy readinessendpoint in a finite loop, |
|
|
||
| // runHTTPReadinessProbe issues an http GET request to endpoint and returns an error | ||
| // if a 2XX or 3XX http status code is not returned. | ||
| func runHTTPReadinessProbe(endpoint string) error { |
There was a problem hiding this comment.
can we split the file into validate.go, admission.go and readiness.go or something like that?
| if err != nil { | ||
| return err | ||
| } | ||
| timeout := time.Duration(5) * time.Second |
| return err | ||
| } | ||
| timeout := time.Duration(5) * time.Second | ||
| resp, err := http.Get(url.String()) |
There was a problem hiding this comment.
this will break for https most probably, until we have a CA.
| // TODO: Create client with configv1 scheme registered. | ||
| // Retrieve the cluster infrastructure config. | ||
| infraConfig := &configv1.Infrastructure{} | ||
| err = kubeClient.Get(context.TODO(), types.NamespacedName{Name: "cluster"}, infraConfig) |
There was a problem hiding this comment.
we have a typed client in openshift/client-go
| if err != nil { | ||
| return "", err | ||
| } | ||
| apiServerURL, err := url.Parse(getAPIServerURL(infraConfig.Status.APIServerURL)) |
|
PR based off #22102 |
|
|
||
| // validateReadinessEndpointWithRetries tries to validate the proxy readinessendpoint in a finite loop, | ||
| // it returns the last result if it never succeeds. | ||
| func validateReadinessEndpointWithRetries(endpoint string, retries int) field.ErrorList { |
There was a problem hiding this comment.
we shouldn't be testing this here... it's being done in the controller when it goes to copy spec -> status now.
|
@danehans not sure how much, if any, of this is still needed? we should be able to do some amount of basic spec validation via oapi schemas. If we want to do explicit syntax checking for things like urls, then i guess you probably still need this, but worst case the controller can catch those things when copying spec -> status and indicate an error. |
|
@danehans: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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/test-infra repository. I understand the commands that are listed here. |
|
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
openshift-ci-robot
added
the
lifecycle/stale
|
Due to cluster-wide egress proxy architecture changes, this PR is no longer needed. |
5 participants
TODOs
TrustedCAfield being used. See Add Trusted CA to Proxy API type api#377. If so, is CA validation required?ReadinessEndpointsand possiblyTrustedCA.@sttts @derekwaynecarr @deads2k @bparees @eparis @mrunalp PTAL