Skip to content

operator/ingress: Add logging API #616

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

Jump to bottom
Merged
openshift-merge-robot merged 1 commit into from
Apr 16, 2020
Merged

operator/ingress: Add logging API #616

openshift-merge-robot merged 1 commit into from
Apr 16, 2020

Conversation

@Miciah
Copy link
Contributor

@Miciah Miciah commented Apr 2, 2020

This commit implements NE-284.

  • operator/v1/types_ingress.go (IngressControllerSpec): Add Logging field with the new IngressControllerLogging type.
    (LoggingDestinationType): New type.
    (ContainerLoggingDestinationType, SyslogLoggingDestinationType): New constants to identify types of destinations for log messages.
    (ContainerLoggingSidecarContainerName): New constant.
    (SyslogLoggingDestinationParameters): New type. Describe a syslog endpoint and facility.
    (ContainerLoggingDestinationParameters): New type. Empty for now.
    (LoggingDestination): New type. Describe a destination for log messages using LoggingDestinationType, SyslogLoggingDestinationParameters, and ContainerLoggingDestinationParameters.
    (AccessLogging): New type. Describe how client requests are logged, using LoggingDestination.
    (IngressControllerLogging): New type. Describe what should be logged where, using AccessLogging. For now, other types of logging cannot be specified.
  • operator/v1/0000_50_ingress-operator_00-custom-resource-definition.yaml:
  • operator/v1/zz_generated.deepcopy.go:
  • operator/v1/zz_generated.swagger_doc_generated.go: Regenerate.

@Miciah Miciah mentioned this pull request Apr 2, 2020
Merged
@Miciah Miciah force-pushed the NE-284-operator-slash-ingress-add-logging-API branch from 12a2b6a to 776efe2 Compare April 2, 2020 17:29
ironcladlou
ironcladlou reviewed Apr 6, 2020
View reviewed changes
@Miciah Miciah force-pushed the NE-284-operator-slash-ingress-add-logging-API branch 2 times, most recently from f57c68d to 307c0bf Compare April 14, 2020 04:18
@Miciah
Copy link
Contributor Author

Miciah commented Apr 14, 2020 •
edited
Loading

Latest push replaces the spec.logging.access.destination.syslog.endpoint field with spec.logging.access.destination.syslog.address and spec.logging.access.destination.syslog.port fields with appropriate validation: The former field's value is required to be a string in "ipv4" or "ipv6" format, and the latter field's value is required to be an integer in the range of 1 to 63356 65535 (inclusive). @ironcladlou, is this a reasonable approach?

I added a patch file to do the required validation because I couldn't figure out how to do it using kubebuilder markers. In order to get crd-schema-gen.mk to apply the patch file, I had to rename to 0000_50_ingress-operator_00-custom-resource-definition.yaml to 0000_50_ingress-operator_00-ingresscontroller.crd.yaml.

An alternative would be to use a regexp, which would be very messy to match IPv4 and IPv6 addresses.

@Miciah
operator/ingress: Add logging API
1fc310b 
This commit implements NE-284.

https://issues.redhat.com/browse/NE-284

* operator/v1/types_ingress.go (IngressControllerSpec): Add Logging field
with the new IngressControllerLogging type.
(LoggingDestinationType): New type.
(ContainerLoggingDestinationType, SyslogLoggingDestinationType): New
constants to identify types of destinations for log messages.
(ContainerLoggingSidecarContainerName): New constant.
(SyslogLoggingDestinationParameters): New type.  Describe a syslog endpoint
and facility.
(ContainerLoggingDestinationParameters): New type.  Empty for now.
(LoggingDestination): New type.  Describe a destination for log messages
using LoggingDestinationType, SyslogLoggingDestinationParameters, and
ContainerLoggingDestinationParameters.
(AccessLogging): New type.  Describe how client requests are logged, using
LoggingDestination.
(IngressControllerLogging): New type.  Describe what should be logged
where, using AccessLogging.  For now, other types of logging cannot be
specified.
* operator/v1/0000_50_ingress-operator_00-custom-resource-definition.yaml:
Rename...
* operator/v1/0000_50_ingress-operator_00-ingresscontroller.crd.yaml:
...to this.  The file name must end in .crd.yaml so it can be patched.
* operator/v1/0000_50_ingress-operator_00-ingresscontroller.crd.yaml-merge-patch:
New file.  Patch the validation of the
spec.logging.access.destination.syslog.address field to require its value
to be either ipv4 or ipv6
* operator/v1/zz_generated.deepcopy.go:
* operator/v1/zz_generated.swagger_doc_generated.go: Regenerate.
@Miciah Miciah force-pushed the NE-284-operator-slash-ingress-add-logging-API branch from 307c0bf to 1fc310b Compare April 14, 2020 04:28
ironcladlou
ironcladlou reviewed Apr 14, 2020
View reviewed changes
operator/v1/types_ingress.go
//
// +kubebuilder:validation:Required
// +required
Address string `json:"address"`
Copy link
Contributor

@ironcladlou ironcladlou Apr 14, 2020

Choose a reason for hiding this comment

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

@sttts @deads2k any guidance on the best way to represent and validate endpoints (addr/port, ipv4/ipv6)? Are there any meaningful static validations we can apply in openapi?

Copy link
Contributor

@sttts sttts Apr 14, 2020

Choose a reason for hiding this comment

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

https://github.com/kubernetes/apiextensions-apiserver/blob/master/pkg/apis/apiextensions/v1/types_jsonschema.go#L53

There is an URL. Otherwise, we split host and port usually, don't we?

Copy link
Contributor Author

@Miciah Miciah Apr 14, 2020

Choose a reason for hiding this comment

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

By "split host and port", do you mean using separate fields? I changed to using separate fields to enable us to perform stricter validation. Our goal is to perform as much validation as possible using OpenAPI.

@ironcladlou
Copy link
Contributor

ironcladlou commented Apr 15, 2020

/approve

Any other feedback? @openshift/sig-network-edge

@ironcladlou
Copy link
Contributor

ironcladlou commented Apr 16, 2020

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 16, 2020
@ironcladlou
Copy link
Contributor

ironcladlou commented Apr 16, 2020

/approve

@ironcladlou
Copy link
Contributor

ironcladlou commented Apr 16, 2020

Whoops, I don't have the power :)

@knobunc
Copy link
Contributor

knobunc commented Apr 16, 2020

/approve

@openshift-ci-robot
Copy link

openshift-ci-robot commented Apr 16, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ironcladlou, knobunc, 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:

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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 16, 2020
@Miciah
Copy link
Contributor Author

Miciah commented Apr 16, 2020

All jobs are failing with the following:

2020/04/16 16:20:11 Tagging https://api.ci.openshift.org/openshift/release:golang-1.13 into pipeline:root
2020/04/16 16:55:01 waiting for importing pipeline:root ...
...
2020/04/16 16:55:01 waiting for importing pipeline:root ...
2020/04/16 16:55:11 could not resolve tag root in imagestream pipeline: timed out waiting for the condition

/retest

@Miciah
Copy link
Contributor Author

Miciah commented Apr 16, 2020

/retest

1 similar comment
@Miciah
Copy link
Contributor Author

Miciah commented Apr 16, 2020

/retest

@openshift-merge-robot openshift-merge-robot merged commit 8cfd791 into openshift:master Apr 16, 2020
@Miciah Miciah mentioned this pull request Apr 24, 2020
Merged
wking added a commit to wking/cluster-ingress-operator that referenced this pull request Dec 22, 2021
@wking
manifests: Drop single-node-developer
3155096 
There's an enhancement proposal for this profile [1], and the Code
Ready Containers folks took a run at using it in [2] before backing
off in [3].  I don't have any problems with having a specific CRC
profile, but until we end up going that way, save ourselves the mental
overhead of trying to guess whether it will want each of our manifest
resources by dropping the annotation across the board.

Effectively reverts abd8a20 (Annotate manifests for
single-node-developer cluster profile, 2020-11-27, openshift#498).

Generated with:

  $ sed -i '/single-node-developer/d' manifests/*.yaml
  $ git checkout HEAD -- manifests/00-custom-resource-definition.yaml

where I'm leaving the CRD alone to avoid [4]:

  hack/verify-generated-crd.sh
  --- vendor/github.com/openshift/api/operator/v1/0000_50_ingress-operator_00-ingresscontroller.crd.yaml	2021-12-22 07:10:24.000000000 +0000
  +++ manifests/00-custom-resource-definition.yaml	2021-12-22 07:10:25.000000000 +0000
  @@ -5,7 +5,6 @@ metadata:
       api-approved.openshift.io: openshift/api#616
       include.release.openshift.io/ibm-cloud-managed: "true"
       include.release.openshift.io/self-managed-high-availability: "true"
  -    include.release.openshift.io/single-node-developer: "true"
     name: ingresscontrollers.operator.openshift.io
   spec:
     group: operator.openshift.io
  invalid CRD: vendor/github.com/openshift/api/operator/v1/0000_50_ingress-operator_00-ingresscontroller.crd.yaml => manifests/00-custom-resource-definition.yaml

[1]: https://github.com/openshift/enhancements/blob/2911c46bf7d2f22eb1ab81739b4f9c2603fd0c07/enhancements/single-node/developer-cluster-profile.md
[2]: crc-org/snc#338
[3]: crc-org/snc#373 (comment)
[4]: https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/openshift_cluster-ingress-operator/692/pull-ci-openshift-cluster-ingress-operator-master-verify/1473551168843026432#1:build-log.txt%3A14
wking added a commit to wking/cluster-ingress-operator that referenced this pull request Dec 22, 2021
@wking
manifests: Drop single-node-developer
0dd22b3 
There's an enhancement proposal for this profile [1], and the Code
Ready Containers folks took a run at using it in [2] before backing
off in [3].  I don't have any problems with having a specific CRC
profile, but until we end up going that way, save ourselves the mental
overhead of trying to guess whether it will want each of our manifest
resources by dropping the annotation across the board.

Effectively reverts abd8a20 (Annotate manifests for
single-node-developer cluster profile, 2020-11-27, openshift#498).

Generated with:

  $ sed -i '/single-node-developer/d' manifests/*.yaml
  $ git checkout HEAD -- manifests/00-custom-resource-definition*

where I'm leaving the CRDs alone to avoid things like [4]:

  hack/verify-generated-crd.sh
  --- vendor/github.com/openshift/api/operator/v1/0000_50_ingress-operator_00-ingresscontroller.crd.yaml	2021-12-22 07:10:24.000000000 +0000
  +++ manifests/00-custom-resource-definition.yaml	2021-12-22 07:10:25.000000000 +0000
  @@ -5,7 +5,6 @@ metadata:
       api-approved.openshift.io: openshift/api#616
       include.release.openshift.io/ibm-cloud-managed: "true"
       include.release.openshift.io/self-managed-high-availability: "true"
  -    include.release.openshift.io/single-node-developer: "true"
     name: ingresscontrollers.operator.openshift.io
   spec:
     group: operator.openshift.io
  invalid CRD: vendor/github.com/openshift/api/operator/v1/0000_50_ingress-operator_00-ingresscontroller.crd.yaml => manifests/00-custom-resource-definition.yaml

[1]: https://github.com/openshift/enhancements/blob/2911c46bf7d2f22eb1ab81739b4f9c2603fd0c07/enhancements/single-node/developer-cluster-profile.md
[2]: crc-org/snc#338
[3]: crc-org/snc#373 (comment)
[4]: https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/openshift_cluster-ingress-operator/692/pull-ci-openshift-cluster-ingress-operator-master-verify/1473551168843026432#1:build-log.txt%3A14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@abhinavdahiya abhinavdahiya Awaiting requested review from abhinavdahiya

@adambkaplan adambkaplan Awaiting requested review from adambkaplan

2 more reviewers

@ironcladlou ironcladlou ironcladlou left review comments

@sttts sttts sttts left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@ironcladlou ironcladlou

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@Miciah @ironcladlou @knobunc @openshift-ci-robot @sttts @openshift-merge-robot