Skip to content

Add ProxyStatus to config/v1/types_proxy #335

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
openshift-merge-robot merged 1 commit into from
Jun 6, 2019
Merged

Add ProxyStatus to config/v1/types_proxy #335

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 24 additions & 1 deletion config/v1/types_proxy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,12 @@ type Proxy struct {
// Spec holds user-settable values for the proxy configuration
// +required
Spec ProxySpec `json:"spec"`
// status holds observed values from the cluster. They may not be overridden.
// +optional
Status ProxyStatus `json:"status"`
Comment thread
patrickdillon marked this conversation as resolved.
}

// ProxySpec contains cluster proxy creation configuration.
type ProxySpec struct {
// httpProxy is the URL of the proxy for HTTP requests. Empty means unset and will not result in an env var.
// +optional
Expand All @@ -26,7 +30,26 @@ type ProxySpec struct {
// +optional
HTTPSProxy string `json:"httpsProxy,omitempty"`

// noProxy is the list of domains for which the proxy should not be used. Empty means unset and will not result in an env var.
// noProxy is a comma-separated list of hostnames and/or CIDRs for which the proxy should not be used.
// Each name is matched as either a domain which contains the host name as a suffix, or the host name itself.
// For instance, example.com would match example.com, example.com:80, and www.example.com.
// Wildcard(*) characters are not accepted, except a single * character which matches all hosts
// and effectively disables the proxy. Empty means unset and will not result in an env var.
// +optional
NoProxy string `json:"noProxy,omitempty"`
}

// ProxyStatus shows current known state of the cluster proxy.
type ProxyStatus struct {
// httpProxy is the URL of the proxy for HTTP requests.
// +optional
HTTPProxy string `json:"httpProxy,omitempty"`

// httpsProxy is the URL of the proxy for HTTPS requests.
// +optional
HTTPSProxy string `json:"httpsProxy,omitempty"`

// noProxy is a comma-separated list of hostnames and/or CIDRs for which the proxy should not be used.
// +optional
NoProxy string `json:"noProxy,omitempty"`
Copy link
Copy Markdown
Member

@wking wking Jun 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't a list of domains be []string?

Copy link
Copy Markdown
Member

@wking wking Jun 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor Author

@patrickdillon patrickdillon Jun 4, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

@wking wking Jun 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor

@abhinavdahiya abhinavdahiya Jun 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...?

Copy link
Copy Markdown
Contributor

@adambkaplan adambkaplan Jun 4, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Copy Markdown
Contributor

@abhinavdahiya abhinavdahiya Jun 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adambkaplan

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?

Copy link
Copy Markdown
Contributor

@adambkaplan adambkaplan Jun 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Copy Markdown
Contributor

@abhinavdahiya abhinavdahiya Jun 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Copy Markdown
Contributor Author

@patrickdillon patrickdillon Jun 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added http- & httpsProxy to the status in order to match the spec.

}
Expand Down
17 changes: 17 additions & 0 deletions config/v1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

19 changes: 16 additions & 3 deletions config/v1/zz_generated.swagger_doc_generated.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.