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

fix: Exclude system namespaces correctly #779

Merged
merged 6 commits into from
Feb 8, 2024
Merged

fix: Exclude system namespaces correctly #779

merged 6 commits into from
Feb 8, 2024

Conversation

chrkl
Copy link
Contributor

@chrkl chrkl commented Feb 7, 2024

Description

Changes proposed in this pull request (what was done and why):

  • Exclude system namespaces in LogPipelines only if no other includes or excludes set

Changes refer to particular issues, PRs or documents:

Traceability

  • The PR is linked to a GitHub issue.
  • New features have a milestone set.
  • New features have defined acceptance criteria in a corresponding GitHub Issue, and all criteria are satisfied with this PR.
  • The corresponding GitHub issue has a respective area and kind label.
  • The follow-up issues (if any) are linked in the Related Issues section.
  • Adjusted the documentation if the change is user-facing.
  • The feature is unit-tested
  • The feature is e2e-tested

@chrkl chrkl requested a review from a team as a code owner February 7, 2024 13:30
@kyma-bot kyma-bot added cla: yes Indicates the PR's author has signed the CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 7, 2024
skhalash
skhalash requested changes Feb 7, 2024
View reviewed changes
Copy link
Collaborator

@skhalash skhalash left a comment

Choose a reason for hiding this comment

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

The unit test coverage is very thorough, but maybe let's add some basic E2E tests too (including the bug fix)?

internal/fluentbit/config/builder/input.go Outdated
@@ -28,16 +28,18 @@ func createInputSection(pipeline *telemetryv1alpha1.LogPipeline, includePath, ex
func createIncludePath(pipeline *telemetryv1alpha1.LogPipeline) string {
var toInclude []string
Copy link
Collaborator

Choose a reason for hiding this comment

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

It took me quite some time to understand the meaning of the variables toInclude and toExclude. I know it's old code, but since we are touching it, maybe let's change them to includePath/excludePath?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. Renamed it

@chrkl chrkl force-pushed the fix-system-namespaces branch from a3bbbdf to f908e57 Compare February 8, 2024 10:11
@chrkl
Copy link
Contributor Author

chrkl commented Feb 8, 2024

The unit test coverage is very thorough, but maybe let's add some basic E2E tests too (including the bug fix)?

Done

skhalash
skhalash requested changes Feb 8, 2024
View reviewed changes
test/e2e/logs_include_namespaces_test.go Outdated
var _ = Describe("Logs Include Namespaces", Label("logs"), Ordered, func() {
const (
mockNs = "log-include-namespaces-mocks"
systemNamespace = "kyma-system"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please use kitkyma.SystemNamespaceName in both new test suites. And maybe a constant for kube-system in kitkyma?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

test/e2e/logs_include_namespaces_test.go Outdated
WithSecretKeyRef(mockBackend.HostSecretRef()).
WithHTTPOutput().
WithIncludeNamespaces([]string{"kyma-system", mockNs}).
WithExcludeContainers([]string{logProducerName})
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are we excluding the log producer container? We do not assert it anywhere or am I missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, that was copy/pasted from another test by accident.

@chrkl chrkl force-pushed the fix-system-namespaces branch from 83b8c2f to c31dd0e Compare February 8, 2024 11:04
@kyma-bot kyma-bot added the lgtm Looks good to me! label Feb 8, 2024
@kyma-bot kyma-bot merged commit b0aa915 into kyma-project:main Feb 8, 2024
20 checks passed
@chrkl chrkl deleted the fix-system-namespaces branch February 8, 2024 12:16
@chrkl chrkl mentioned this pull request Feb 8, 2024
Closed
@a-thaler a-thaler added kind/bug Categorizes issue or PR as related to a bug. area/logs LogPipeline labels Feb 8, 2024
@a-thaler a-thaler added this to the 1.9.0 milestone Feb 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@skhalash skhalash skhalash approved these changes

Assignees

@skhalash skhalash

Labels
area/logs LogPipeline cla: yes Indicates the PR's author has signed the CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm Looks good to me! size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
1.9.0
Development

Successfully merging this pull request may close these issues.
4 participants
@chrkl @skhalash @a-thaler @kyma-bot