Skip to content
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.

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

Update Ingress spec #317

Merged
merged 70 commits into from Aug 11, 2017

Conversation

Projects
None yet
2 participants
@tamalsaha
Copy link
Member

commented Jul 21, 2017

Fixes #46
Fixes #73
Fixes #128
Fixes #310
Fixes #138
Fixes #319
Fixes #271
Fixes #333
Fixes #346
Fixes #318

@tamalsaha tamalsaha requested a review from sadlil Jul 21, 2017

@tamalsaha tamalsaha force-pushed the api-rev branch from b9e8987 to c39504a Jul 21, 2017

@tamalsaha tamalsaha added this to the 3.2.0 milestone Jul 21, 2017

@tamalsaha tamalsaha changed the title Update Ingress spec. WIP: Update Ingress spec Jul 21, 2017

func (be *Backend) canonicalize() {
sort.Strings(be.BackendRules)
sort.Strings(be.RewriteRules)
sort.Strings(be.HeaderRules)

This comment has been minimized.

Copy link
@sadlil

sadlil Aug 8, 2017

Contributor

Are these rules going to be sort lexicographically? If yes. then it is wrong.
These rules have meaning in the given order.

This comment has been minimized.

Copy link
@tamalsaha

tamalsaha Aug 8, 2017

Author Member

No. Reverted this 3 lines.

@sadlil
Copy link
Contributor

left a comment

i have push compile fixes for test merge. those validation needs to be discussed.

for ri, rule := range r.Spec.Rules {
if rule.HTTP != nil {
addr := address{Host: rule.Host}
if port, err := checkRequiredPort(rule.HTTP.Port); err != nil {

This comment has been minimized.

Copy link
@sadlil

sadlil Aug 8, 2017

Contributor

what if i do not set any port value. this should set 80/443 by default for http, as it currently is. this should not through an error.

This comment has been minimized.

Copy link
@tamalsaha

tamalsaha Aug 8, 2017

Author Member

Need to relax this check to allow port not defined for HTTP case,

This comment has been minimized.

Copy link
@tamalsaha

tamalsaha Aug 8, 2017

Author Member

Fixed.

} else {
addr.Port = port
}
if ei, found := addrs[addr]; found {

This comment has been minimized.

Copy link
@sadlil

sadlil Aug 8, 2017

Contributor

i thik it is possible to reuse same host:port in two different rule in http mode.

This comment has been minimized.

Copy link
@tamalsaha

tamalsaha Aug 8, 2017

Author Member

That is intended. Our TemplateData parser does not handle this case. So, no reason to support it here. Is there any reason why someone will want to do so?

if len(rule.TCP.Backend.HeaderRule) > 0 {
return fmt.Errorf("spec.rule[%d].tcp.backend.headerRule must be empty for addr %s", ri, addr)
}
if len(rule.TCP.Backend.RewriteRule) > 0 {

This comment has been minimized.

Copy link
@sadlil

sadlil Aug 8, 2017

Contributor

what if we do not allow this field to be here for tcp, as:.

// IngressBackend describes all endpoints for a given service and port.
type IngressBackend struct {
	// Specifies the name of the referenced service.
	ServiceName string `json:"serviceName,omitempty"`

	// Specifies the port of the referenced service.
	ServicePort intstr.IntOrString `json:"servicePort,omitempty"`
}

type HTTPIngressBackend struct {
        IngressBackend `json:",inline"`

	HostNames []string `json:"hostNames,omitempty"`

	// Serialized HAProxy rules to apply on server backend including
	// request, response or header rewrite. acls also can be used.
	// https://cbonte.github.io/haproxy-dconv/1.7/configuration.html#1
	BackendRule []string `json:"backendRule,omitempty"`

	// Path rewrite rules with haproxy formatted regex.
	//
	// Deprecated: Use backendRule, will be removed.
	RewriteRule []string `json:"rewriteRule,omitempty"`

	// Header rules to modifies the header.
	//
	// Deprecated: Use backendRule, will be removed.
	HeaderRule []string `json:"headerRule,omitempty"`
}

This comment has been minimized.

Copy link
@tamalsaha

tamalsaha Aug 8, 2017

Author Member

We can do this. But user can still do whatever they want in YAML. I believe the marshaler just ignores unknown fields.

This comment has been minimized.

Copy link
@tamalsaha

tamalsaha Aug 8, 2017

Author Member

Added.

addrs[addr] = ri

if np, err := checkOptionalPort(rule.HTTP.NodePort); err != nil {
return fmt.Errorf("spec.rule[%d].http.nodePort %s is invalid. Reason: %s", ri, rule.HTTP.NodePort, err)

This comment has been minimized.

Copy link
@sadlil

sadlil Aug 8, 2017

Contributor

if we through here optional Port is not really optional. If i do not set this valu in yaml it must set system-default.

This comment has been minimized.

Copy link
@tamalsaha

tamalsaha Aug 8, 2017

Author Member

What do you mean by "If i do not set this value in yaml it must set system-default." ?

api/port.go Outdated
usesHTTPRule = true
if _, foundTLS := r.FindTLSSecret(rule.Host); foundTLS && !rule.HTTP.NoSSL {
if port := rule.HTTP.Port.IntValue(); port > 0 {
mappings[port] = Target{PodPort: port, NodePort: rule.TCP.NodePort.IntValue()}

This comment has been minimized.

Copy link
@sadlil

sadlil Aug 9, 2017

Contributor

rule.HTTP.NodePort.IntValue() isn't it?

This comment has been minimized.

Copy link
@tamalsaha

tamalsaha Aug 9, 2017

Author Member

yes

@tamalsaha

This comment has been minimized.

Copy link
Member Author

commented Aug 9, 2017

@sadlil , I have updated the validation logic (last commit).

@tamalsaha tamalsaha merged commit 0d9b502 into master Aug 11, 2017

@tamalsaha tamalsaha deleted the api-rev branch Aug 11, 2017

tamalsaha added a commit that referenced this pull request Dec 13, 2017

Update Ingress spec (#317)
Fixes #46
Fixes #73
Fixes #128
Fixes #310
Fixes #138
Fixes #319
Fixes #271
Fixes #333
Fixes #346
Fixes #318
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.