operator/ingress: Add Forwarded header policy #688
Conversation
Miciah
force-pushed
the
operator-slash-ingress-add-Forwarded-header-policy
branch
from
f2a9697 to
06870a2
Compare
| // IngressControllerHTTPHeaderPolicy is a policy for setting HTTP headers. | ||
| // | ||
| // +kubebuilder:validation:Enum=Append;Replace;IfNone;Never | ||
| type IngressControllerHTTPHeaderPolicy string |
There was a problem hiding this comment.
Deviating from the enhancement proposal, I changed the allowed values here from lower-case (append;replace;if-none;never) to the above value for consistency with other API values.
Miciah
force-pushed
the
operator-slash-ingress-add-Forwarded-header-policy
branch
2 times, most recently
from
7ad0c18 to
c902cce
Compare
operator/v1/types_ingress.go
Outdated
| // If this field is empty, the default values are used. | ||
| // | ||
| // +optional | ||
| Forwarded *IngressControllerHTTPForwardedHeaderPolicy `json:"forwarded,omitempty"` |
There was a problem hiding this comment.
pointers to strings are a no-no in our configuration. Empty has a defined meaning and valid, and should mean the same as nil, so nil shouldn't exist.
There was a problem hiding this comment.
IngressControllerHTTPForwardedHeaderPolicy (the type for spec.httpHeaders.forwarded) is not a string type. Are you referring to the fact that IngressControllerHTTPHeaderPolicy (the type for spec.httpHeaders.forwarded.policy) is a string type? I think we can make it required but allow empty to avoid breaking upgrades.
There was a problem hiding this comment.
I changed Forwarded to a non-pointer value. I assume the +optional still belongs. Everything look all right?
Miciah
force-pushed
the
operator-slash-ingress-add-Forwarded-header-policy
branch
2 times, most recently
from
da3e607 to
ecb90f3
Compare
operator/v1/types_ingress.go
Outdated
| // forwarded describes how the Forwarded HTTP header and related headers | ||
| // are handled. | ||
| // | ||
| // +optional |
There was a problem hiding this comment.
optional fields need to describe what happens on empty. Empty is a straight pass through or pass through while handling auth headers or what?
operator/v1/types_ingress.go
Outdated
|
|
||
| // IngressControllerHTTPHeaders specifies how the IngressController handles | ||
| // certain HTTP headers. | ||
| type IngressControllerHTTPHeaders struct { |
There was a problem hiding this comment.
What else would theoretically go into this struct as compared to what would theoretically get added to IngressControllerHTTPForwardedHeaderPolicy. Are these truly distinct structs? Maybe the next two most likely fields for each struct would help me see it.
There was a problem hiding this comment.
The next field we'd add is uniqueId, for injecting a custom, unique header into every request for tracing: 43ad0aa#diff-85d84174432a6d2df99d107129e012f8R601-R611
Beyond that, we have an RFE to allow setting arbitrary headers, such as Strict-Transport-Security and X-XSS-Protection, with static values. To implement that, I expect we'll add a field with a slice type for specifying arbitrary name-value pairs.
There was a problem hiding this comment.
miciah also came up with "Maybe an OmitNonStandard (the X-Forwarded-*" for the ForwardedHeaderPolicy.
@Miciah if nothing gets added to this in the next year, we should remember that in the future.
|
the doc is good, the validation wellformed. I just don't quite understand the need for two layers. I'd like to understand before approving. |
Miciah
force-pushed
the
operator-slash-ingress-add-Forwarded-header-policy
branch
from
ecb90f3 to
d69157b
Compare
|
/lgtm hold is just in case @Miciah has second thoughts about depth. If you like the depth, remove the hold at your discretion. |
openshift-ci-robot
added
the
do-not-merge/hold
openshift-ci-robot
added
lgtm
Miciah
force-pushed
the
operator-slash-ingress-add-Forwarded-header-policy
branch
from
d69157b to
79edba8
Compare
This commit implements NE-317. https://issues.redhat.com/browse/NE-317 * operator/v1/types_ingress.go (IngressControllerSpec): Add HTTPHeaders field with the new IngressControllerHTTPHeaders type. (IngressControllerHTTPHeaderPolicy): New type. (AppendHTTPHeaderPolicy, ReplaceHTTPHeaderPolicy, IfNoneHTTPHeaderPolicy) (NeverHTTPHeaderPolicy): New constants. (IngressControllerHTTPHeaders): New type. Describe policy for handling HTTP headers. For now, the only field is ForwardedHeaderPolicy, with the new IngressControllerHTTPHeaderPolicy type, for specifying how the HTTP Forwarded header and related headers should be handled. * operator/v1/0000_50_ingress-operator_00-ingresscontroller.crd.yaml: * operator/v1/zz_generated.deepcopy.go: * operator/v1/zz_generated.swagger_doc_generated.go: Regenerate.
Miciah
force-pushed
the
operator-slash-ingress-add-Forwarded-header-policy
branch
from
79edba8 to
03d62b9
Compare
|
/hold cancel |
openshift-ci-robot
removed
the
do-not-merge/hold
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k, Miciah 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 |
This commit implements NE-317.
operator/v1/types_ingress.go(IngressControllerSpec): AddHTTPHeadersfield with the newIngressControllerHTTPHeaderstype.(
IngressControllerHTTPHeaderPolicy): New type.(
AppendHTTPHeaderPolicy,ReplaceHTTPHeaderPolicy,IfNoneHTTPHeaderPolicy,NeverHTTPHeaderPolicy): New constants.(
IngressControllerHTTPHeaders): New type. Describe policy for handling HTTP headers. For now, the only field isForwardedHeaderPolicy, with the newIngressControllerHTTPHeaderPolicytype, for specifying how the HTTPForwardedheader and related headers should be handled.operator/v1/0000_50_ingress-operator_00-ingresscontroller.crd.yaml:operator/v1/zz_generated.deepcopy.go:operator/v1/zz_generated.swagger_doc_generated.go: Regenerate.Corresponding enhancement proposal: openshift/enhancements#371