Add ingress-logging-api enhancement

[Miciah]

No description provided.

[ironcladlou]

This is really nice and detailed, thanks

[ironcladlou]

Thinking of how we're dealing with validation and admission-condition with other fields — is there any opportunity here for us to do defaulting or validation during admission to make this as easy as possible to understand without requiring the user to do separate update/verify steps to understand whether they have a valid config or what their effective config is?

[Miciah]

Do you mean defaulting as far as setting empty fields explicitly to their default values during admission? That would be useful generally for the IngressController API, and it could be useful specifically for the facility and httpLogFormat fields. Is there anything we can do now, or is this just something to keep in mind when we add an admission controller?

As far as validation, I think the validation I have already added and am adding in response to your feedback will make it reasonably difficult to misuse the API within the scope of what it is possible for us to do.

[ironcladlou]

Do you mean defaulting as far as setting empty fields explicitly to their default values during admission?

Yes

Is there anything we can do now, or is this just something to keep in mind when we add an admission controller?

Maybe let's figure that out — otherwise are we going to have to add another status field?

[Miciah]

I feel like status is unnecessary because there is little room for varying interpretation—if you specify spec.logging.access, then access logging is enabled, the log format is used verbatim, the syslog facility is strictly validated, and the endpoint is used verbatim. The only thing I can think of is that the operator could probe the syslog endpoint and report a status condition; would that be useful?

[ironcladlou]

I agree with your status assessment.

Re: probing, that could be useful, but probably out of scope for the initial feature. What do you think?

[danehans]

From cluster logging EP:

The following output types are planned for initial support:

Elasticsearch (v6.x) with/without TLS.
Fluent forward with/without TLS.
Syslog UDP, TCP, TLS.
Kafka

[danehans]

Trying to better understand why we are not using the cluster logging API.

[danehans]

@Miciah IMO configuring logging in one place for all platform components is preferred. From a quick review of the cluster logging EP, it attempts to provide a cluster-wide log fwd'ing solution for apps, infra and audit:

Selectively forward Application, Infrastructure and Audit inputs.

If this is the case, then why not use the cluster logging API? If the EP does not meet our requirements, then why not work with that team to fix their API? For example, both EP's provide an interface to configure standard Syslog parameters.

[ironcladlou]

Thinking about future IPv6 requirements, I'm still slightly paranoid about this — is there some standard validation that will account for different address families? Also, is there any need to represent the transport? Is UDP implied? etc.

[danehans]

is there some standard validation that will account for different address families

@ironcladlou what about something like this:

// Validate given Endpoint IP address.
func validateEndpoint(ep string) error {
        ip, port, err := net.SplitHostPort(ep)
        // handle error. Optionally validate port number.
        if ip := net.ParseIP(ip); ip != nil {
                return fmt.Errorf("Endpoint must be a valid IP address")
        }
        if ip.IsLoopback() {
                return fmt.Errorf("Endpoint can't be loopback address")
        }
        if ip.IsMulticast() {
                return fmt.Errorf("Endpoint can't be a multicast address")
        }
        if ip.IsLinkLocalUnicast() {
                return fmt.Errorf("Endpoint can't be a link-local unicast address")
        }
        if ip.IsUnspecified() {
                return fmt.Errorf("Endpoint can't be an all zeros address")
        }
        return nil
}

[Miciah]

@ironcladlou,

Thinking about future IPv6 requirements, I'm still slightly paranoid about this — is there some standard validation that will account for different address families? Also, is there any need to represent the transport? Is UDP implied? etc.

UDP is implied (HAProxy supports UDP and not TCP); I'll document that explicitly.

Does anything else validate addresses in openshift/api as you are describing? Do we need to do stricter validation than other components do? I could write a regular expression, but it would be gnarly.

@danehans, that validation looks good, but is adding validation that would require an admission controller a blocker for this enhancement?

[ironcladlou]

I don't know re: precedent, I just wanted to make sure we weren't painting ourselves into any corner with regards to endpoint addressing.

Although this now raises another question related to status. What happens if the endpoint is unreachable for any reason (misconfiguration, outage, etc)? Is there some minimum status reporting we should cover here for that case (e.g. logging degraded condition)?

[Miciah]

I added a section on validation here: https://github.com/openshift/enhancements/blob/f18621ac562e17bca6377c68e786e757f7a3bdf3/enhancements/ingress/logging-api.md#validation

I propose we validate that the endpoint can be parsed as a host name and IP address or port and possibly probe the endpoint when admitting the ingress controller. Does that seem reasonable?

[Miciah]

Latest push adds a section on validation: https://github.com/openshift/enhancements/blob/f18621ac562e17bca6377c68e786e757f7a3bdf3/enhancements/ingress/logging-api.md#validation

[Miciah]

I have submitted openshift/api#616 to add the API described in this enhancement.

[ironcladlou]

I still have concerns about treating this as opaque. What should happen if the implementation can't deal with the provided format?

And in our specific case (openshift/router), I suspect passing garbage all the way through environment could crash haproxy. So maybe the ingresscontroller would go degraded. Would that be the expectation to communicate (that invalid formats can cause the ingresscontroller to be unavailable/degraded)?

[ironcladlou]

Is this something that we can follow up on? Not sure if it should block the EP

[ironcladlou]

This lgtm, does anybody else have remaining feedback?

[ironcladlou]

/lgtm

[ironcladlou]

/approve

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ironcladlou, Miciah

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [ironcladlou]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment