Implement logging API #374
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
openshift-ci-robot
added
do-not-merge/work-in-progress
Miciah
force-pushed
the
NE-285-implement-logging-API
branch
4 times, most recently
from
116d38e to
830d2c7
Compare
openshift-ci-robot
added
the
needs-rebase
ironcladlou
reviewed
Apr 13, 2020
ironcladlou
reviewed
Apr 13, 2020
ironcladlou
reviewed
Apr 13, 2020
ironcladlou
reviewed
Apr 13, 2020
ironcladlou
reviewed
Apr 13, 2020
Miciah
force-pushed
the
NE-285-implement-logging-API
branch
from
830d2c7 to
46c688e
Compare
* pkg/operator/controller/ingress/deployment_test.go (checkDeploymentHasEnvVar, checkDeploymentHasContainer): New helpers. (TestDesiredRouterDeployment): Use the new helpers.
Contributor
Author
|
Rebased. |
Miciah
force-pushed
the
NE-285-implement-logging-API
branch
from
46c688e to
6ab76aa
Compare
openshift-ci-robot
removed
the
needs-rebase
Miciah
force-pushed
the
NE-285-implement-logging-API
branch
from
6ab76aa to
c13edae
Compare
Contributor
Author
|
/retest |
3 similar comments
Contributor
Author
|
/retest |
Contributor
Author
|
/retest |
Contributor
Author
|
/retest |
Contributor
Author
|
/test e2e-aws |
Contributor
Author
|
/retest |
Bump to github.com/openshift/api@8cfd79130f9c03777ae5db16e2c55e1039f977aa to get the logging API. * go.mod: Bump. * hack/update-generated-crd.sh: * hack/verify-generated-crd.sh: Update path to the vendored ingresscontroller CRD file. * go.sum: * manifests/00-custom-resource-definition.yaml: * pkg/manifests/bindata.go: * vendor/github.com/openshift/api/*: * vendor/modules.txt: Regenerate.
Miciah
force-pushed
the
NE-285-implement-logging-API
branch
from
c13edae to
9edc970
Compare
openshift-ci-robot
removed
the
do-not-merge/work-in-progress
Miciah
force-pushed
the
NE-285-implement-logging-API
branch
from
9edc970 to
c7c60a7
Compare
ironcladlou
reviewed
Apr 16, 2020
| } | ||
| } | ||
|
|
||
| func checkRollingUpdateParams(t *testing.T, deployment *appsv1.Deployment, maxUnavailable intstr.IntOrString, maxSurge intstr.IntOrString) { |
Contributor
There was a problem hiding this comment.
Nice little refactor here, thanks — one more thing it made me think of (not required, just a sidebar discussion) is you can now express your expectations with a struct and make a table test out of the env assertions, like
{
env{name: WildcardRouteAdmissionPolicy, equals: "false"},
env{name: "ROUTER_USE_PROXY_PROTOCOL", equals: nil},
}
And feed them into your assertion function. Likewise with container checks. And then keep adding sugar until we reinvent Ginkgo 😁
Miciah
force-pushed
the
NE-285-implement-logging-API
branch
2 times, most recently
from
e365ca3 to
6704902
Compare
This commit resolves NE-285. https://issues.redhat.com/browse/NE-285 * pkg/operator/controller/ingress/deployment.go (RouterLogLevelEnvName) (RouterSyslogAddressEnvName, RouterSyslogFormatEnvName) (RouterSyslogFacilityEnvName): New constants. (desiredRouterDeployment): Delete logic for the unsupported-logging annotation. Using the new accessLoggingForIngressController function, check whether spec.logging.access is specified on the ingresscontroller, and configure logging accordingly. (accessLoggingForIngressController): New function. Return the access logging parameters for the given ingresscontroller if it enables access logging. * pkg/operator/controller/ingress/deployment_test.go (TestDesiredRouterDeployment): Verify that the unsupported-logging annotation overrides spec.logging.access. Verify that spec.logging.access has the desired effect. (TestInferTLSProfileSpecFromDeployment): Use "logs" as the name of the syslog sidecar container in order to be consistent with the logging API. * pkg/operator/controller/ingress/rsyslog_configmap.go (EnableLoggingAnnotation, haproxyLogLevels): Delete definitions. (ExtraLoggingEnabled): Delete function. (desiredRsyslogConfigMap): Use accessLoggingForIngressController to determine whether a configmap is needed. * test/e2e/operator_test.go (TestSyslogLogging): New test. Create a pod that runs an rsyslog instance and an ingresscontroller that logs to that rsyslog instance and verify that the rsyslog instance logs an HTTP request. * test/e2e/operator_test.go (TestContainerLogging): New test. Create an ingresscontroller that logs to a sidecar container and verify that it becomes available.
Miciah
force-pushed
the
NE-285-implement-logging-API
branch
from
6704902 to
1da14ee
Compare
Contributor
|
lgtm, will let @danehans do the honors after he gets a chance to look through it |
danehans
reviewed
Apr 17, 2020
| return deployment, nil | ||
| } | ||
|
|
||
| // accessLoggingForIngressController returns an AccessLogging value for the |
Contributor
There was a problem hiding this comment.
@Miciah accessLoggingForIngressController returns a pointer not a value, right?
Contributor
Author
There was a problem hiding this comment.
Right, it's a pointer. We can fix the comment in a follow-up.
Contributor
|
/lgtm |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danehans, Miciah The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add helpers for deployment unit tests
pkg/operator/controller/ingress/deployment_test.go(checkDeploymentHasEnvVar,checkDeploymentHasContainer): New helpers.(
TestDesiredRouterDeployment): Use the new helpers.Bump openshift/api for logging API
Bump to github.com/openshift/api@8cfd79130f9c03777ae5db16e2c55e1039f977aa to get the logging API.
go.mod: Bump.hack/update-generated-crd.sh:hack/verify-generated-crd.sh: Update path to the vendored ingresscontroller CRD file.go.sum:manifests/00-custom-resource-definition.yaml:pkg/manifests/bindata.go:vendor/github.com/openshift/api/*:vendor/modules.txt: Regenerate.Implement logging API
This commit resolves NE-285.
pkg/operator/controller/ingress/deployment.go(RouterLogLevelEnvName,RouterSyslogAddressEnvName,RouterSyslogFormatEnvName,RouterSyslogFacilityEnvName): New constants.(
desiredRouterDeployment): Check whetherspec.logging.accessis specified on the ingresscontroller, and configure logging accordingly.pkg/operator/controller/ingress/deployment_test.go(TestDesiredRouterDeployment): Verify that theunsupported-loggingannotation overridesspec.logging.access. Verify thatspec.logging.accesshas the desired effect.(
TestInferTLSProfileSpecFromDeployment): Use "logs" as the name of the syslog sidecar container in order to be consistent with the logging API.test/e2e/operator_test.go(TestSyslog): New test. Create a pod that runs an rsyslog instance and an ingresscontroller that logs to that rsyslog instance and verify that the rsyslog instance logs an HTTP request.