Add ProxyStatus to config/v1/types_proxy#335
Add ProxyStatus to config/v1/types_proxy#335openshift-merge-robot merged 1 commit intoopenshift:masterfrom
Conversation
openshift-ci-robot
added
the
size/S
|
/lgtm @patrickdillon https://github.com/openshift/api/blob/master/Makefile would might need to run some generation scripts.
|
| // ProxyStatus shows current known state of the cluster proxy | ||
| type ProxyStatus struct { | ||
| // noProxy is the list of domains for which the proxy should not be used. | ||
| NoProxy string `json:"noProxy,omitempty"` |
There was a problem hiding this comment.
Shouldn't a list of domains be []string?
There was a problem hiding this comment.
Also, does the presence of whatever.example.com in this field mean that some.subdomain.whatever.example.com should not be proxied either? Or are subdomains supposed to be listed explicitly?
There was a problem hiding this comment.
Shouldn't a list of domains be []string?
I set this to be the same type as ProxySpec.NoProxy. I believe a comma-separated string is the convention after looking over some 3.11 & AWS docs.
Also, does the presence of whatever.example.com in this field mean that some.subdomain.whatever.example.com should not be proxied either?
Yes, I believe that is the case. Looking at this 3.11 doc:
example.com would match example.com, example.com:80, and www.example.com.
There was a problem hiding this comment.
Those 3.11 docs are complicated. What's the policy for things like that? Do we want docs here? If so, should they be full/authoritative or sparse? If empty or sparse, should we link to some authoritative external doc?
There was a problem hiding this comment.
I think extensive docs are probably not best situated here.. but I do think we should expand the current field comments to include information like this is comma separated list, it can include DNS domains, networks, IPs etc...?
There was a problem hiding this comment.
Why only NoProxy? Shouldn't status be set for HTTPProxy and HTTPSProxy as well, as several components have their own proxy configuration (builds, image registry, etc.)
There was a problem hiding this comment.
several components have their own proxy configuration (builds, image registry, etc.)
this package has cluster level configuration and I'm not sure how ^^ is related to the change. All operators are going to have to setup proxy for their operands based on this object.
Why only NoProxy? Shouldn't status be set for HTTPProxy and HTTPSProxy as well
NoProxy is the spec section is user-provided, while NoProxy in the status section is observed value from the spec of this object + network object + other default values that might include platform specific values too.
Therefore, NoProxy makes sense as part of status. There's no such need for other spec fields therefore specifically skipped from status.
And afaik there isn't a requirement for all fields to be part of the status l, right?
There was a problem hiding this comment.
@abhinavdahiya imo it makes for a harder API to navigate. I've always interpreted spec = intent, status = what the cluster has implemented/understood.
By way of example, if the openshift controller manager needs to watch the global Proxy config to update the default build proxy, I would expect it to read off status, not spec and status depending on which field you're referring to.
There was a problem hiding this comment.
@abhinavdahiya imo it makes for a harder API to navigate. I've always interpreted
spec= intent,status= what the cluster has implemented/understood.
If we follow this anology, openshift controller should use the HTTP(S)_PROXY from the spec because the controller is fulfilling the intent of the user. and use the NO_PROXY from the status because to fulfil the intent in the spec, it needs to consume the value setup/provided by another component that setup the NO_PROXY from the spec.
But putting all fields in status too is something not too imp to me, and I am okay if we want to go that route.
There was a problem hiding this comment.
I have added http- & httpsProxy to the status in order to match the spec.
patrickdillon
force-pushed
the
add-proxy-status
branch
from
43090fb to
0771978
Compare
patrickdillon
force-pushed
the
add-proxy-status
branch
from
0771978 to
d1eb631
Compare
@abhinavdahiya yes that seemed to do the trick. can you refresh the |
|
/lgtm |
|
/lgtm cancel See #335 (comment) |
patrickdillon
changed the title
patrickdillon
force-pushed
the
add-proxy-status
branch
from
d1eb631 to
680ebe2
Compare
|
/assign @soltysh |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: abhinavdahiya, adambkaplan, patrickdillon, soltysh 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 |
openshift-ci-robot
added
the
approved
|
@patrickdillon are you working on the client-go API bump? Operators will need new listers and informers to watch the global proxy object. |
|
@adambkaplan planning on looking into it today. will reach out if I need help. |
|
@patrickdillon @danehans do we still need this? Seems like we're moving forward w/ just spec values and an admission controller? |
|
@bparees Is that the case? If so I will update the installer to put the complete no_proxy values in the spec. |
|
@patrickdillon confirm it w/ @danehans but that's my current understanding of the plan. |
9 participants
xref: https://jira.coreos.com/browse/CORS-1074
Adding a ProxyStatus object which contained a NoProxy field would represent the user-specified NO_PROXY settings from the installer install-config plus any platform-specific values that need to be added in addition to the user-specified values.
CC @abhinavdahiya