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.

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

proxy: Remove access-log option #10393

Merged
merged 1 commit into from Mar 4, 2020
Merged

proxy: Remove access-log option #10393

merged 1 commit into from Mar 4, 2020

Conversation

tgraf
Copy link
Member

@tgraf tgraf commented Feb 29, 2020 •
edited

L7 flow logging has been supported via Hubble for a while. The file logging is
no longer required.

Review comment:

  • The option access-log has been removed on purpose without marking it deprecated. Action must be taken and a warning in the log files will unlikely cause users to change the option. Ignoring the option would likely keep an old accesslog file around without ever updating it again and the situation could be left unnoticed for a long time.

This change is Reviewable

@tgraf tgraf added pending-review area/proxy Impacts proxy components, including DNS, Kafka, Envoy and/or XDS servers. release-note/minor This PR changes functionality that users may find relevant to operating Cilium. labels Feb 29, 2020
@tgraf tgraf requested a review from a team February 29, 2020 12:46
@tgraf tgraf requested review from a team as code owners February 29, 2020 12:46
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.8.0 Feb 29, 2020
@coveralls
Copy link

coveralls commented Feb 29, 2020 •
edited

Coverage Status

Coverage decreased (-0.01%) to 45.627% when pulling 0424309 on pr/tgraf/remove-proxy-accesslog into fe98c5b on master.

@tgraf
Copy link
Member Author

tgraf commented Feb 29, 2020 •
edited

test-me-please

EDIT:
Hit a flake testing managed etcd mode:

01:11:32.606  • Failure [756.606 seconds]
01:11:32.606  K8sDatapathConfig
01:11:32.606  /home/jenkins/workspace/Cilium-PR-Ginkgo-Tests-Validated/k8s-1.17-gopath/src/github.com/cilium/cilium/test/ginkgo-ext/scopes.go:395
01:11:32.606    ManagedEtcd
01:11:32.606    /home/jenkins/workspace/Cilium-PR-Ginkgo-Tests-Validated/k8s-1.17-gopath/src/github.com/cilium/cilium/test/ginkgo-ext/scopes.go:395
01:11:32.606      Check connectivity with managed etcd [It]
01:11:32.606      /home/jenkins/workspace/Cilium-PR-Ginkgo-Tests-Validated/k8s-1.17-gopath/src/github.com/cilium/cilium/test/ginkgo-ext/scopes.go:430
01:11:32.606  
01:11:32.606      cilium pre-flight checks failed
[2020-02-29T15:11:26.930Z]     Expected
[2020-02-29T15:11:26.930Z]         <*errors.errorString | 0xc0003e2f10>: {
[2020-02-29T15:11:26.930Z]             s: "CiliumPreFlightCheck error: Timeout reached: PreflightCheck failed: Last polled error: connectivity health is failing: Cluster connectivity is unhealthy on 'cilium-pqnm8': Exitcode: 255 \nStdout:\n \t \nStderr:\n \t Error: Cannot get status/probe: Put \"http://%2Fvar%2Frun%2Fcilium%2Fhealth.sock/v1beta/status/probe\": context deadline exceeded\n\t \n\t command terminated with exit code 255\n\t \n",
[2020-02-29T15:11:26.930Z]         }
[2020-02-29T15:11:26.930Z]     to be nil

@tgraf
Copy link
Member Author

tgraf commented Mar 1, 2020

test-me-please

aanm
aanm approved these changes Mar 2, 2020
View reviewed changes
Copy link
Member

@aanm aanm left a comment

Choose a reason for hiding this comment

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

LGTM besides the requested changes, doesn't require 2nd review

Documentation/install/upgrade.rst Outdated Show resolved Hide resolved
Documentation/install/upgrade.rst Outdated Show resolved Hide resolved
@tgraf tgraf force-pushed the pr/tgraf/remove-proxy-accesslog branch from bf1527e to a402dc0 Compare March 2, 2020 12:24
@tgraf
Copy link
Member Author

tgraf commented Mar 2, 2020

test-me-please

@tgraf tgraf changed the title proxy: Remove accesslog option proxy: Remove access-log option Mar 3, 2020
qmonnet
qmonnet requested changes Mar 3, 2020
View reviewed changes
Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

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

The option is also used in contrib/vagrant/start.sh and mentioned in Documentation/envoy/extensions.rst, could you please also update those files?

@tgraf
proxy: Remove access-log option
0424309 
L7 flow logging has been supported via Hubble for a while. The file logging is
no longer required.

Signed-off-by: Thomas Graf <thomas@cilium.io>
@tgraf tgraf force-pushed the pr/tgraf/remove-proxy-accesslog branch from a402dc0 to 0424309 Compare March 3, 2020 14:17
@tgraf tgraf requested a review from a team as a code owner March 3, 2020 14:17
@tgraf tgraf requested a review from qmonnet March 3, 2020 14:17
@tgraf
Copy link
Member Author

tgraf commented Mar 3, 2020

test-me-please

@tgraf tgraf merged commit 6d7f7e1 into master Mar 4, 2020
1.8.0 automation moved this from In progress to Merged Mar 4, 2020
@tgraf tgraf deleted the pr/tgraf/remove-proxy-accesslog branch March 4, 2020 09:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aanm aanm aanm approved these changes

@qmonnet qmonnet qmonnet approved these changes

@jrajahalme jrajahalme Awaiting requested review from jrajahalme

Assignees
No one assigned
Labels
area/proxy Impacts proxy components, including DNS, Kafka, Envoy and/or XDS servers. release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
No open projects
1.8.0
  
Merged
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@tgraf @coveralls @aanm @qmonnet