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.

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

bugfix(accesslog): allow different provider between modes #39696

Conversation

j2gg0s
Copy link
Contributor

@j2gg0s j2gg0s commented Jun 29, 2022

Please provide a description of this PR:

Take workload's mode into consideration, when compute inScopeProviders.
So, we can allow user config different access log providers for different mode.

This maybe what we expected.

Please check any characteristics that apply to this pull request.

  • [] Does not have any user-facing changes. This may include CLI changes, API changes, behavior changes, performance improvements, etc.

@j2gg0s j2gg0s requested a review from a team as a code owner June 29, 2022 16:16
@istio-testing istio-testing added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jun 29, 2022
@istio-testing
Copy link
Collaborator

Hi @j2gg0s. Thanks for your PR.

I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@zirain
Copy link
Member

zirain commented Jun 30, 2022

this seems related to #39521

@j2gg0s
Copy link
Contributor Author

j2gg0s commented Jun 30, 2022

this seems related to #39521

Yes, it is related but not same. This PR don't change current logic and just fix obvious unexpected behavior.

@zirain
Copy link
Member

zirain commented Jun 30, 2022

this seems related to #39521

Yes, it is related but not same. This PR don't change current logic and just fix obvious unexpected behavior.

I think the root case is multi accesslogging not working, let's hold this first, WDYK?

@j2gg0s
Copy link
Contributor Author

j2gg0s commented Jun 30, 2022

Yes, two PR is conflict

But if we want to change Telemetry's AccessLogging behavior, Maybe it's better to define what we expected before coding.

one Telemetry has many AccessLoging,one HCM also has many access_log.
one AccessLogging has many providers, how we map this to envoy's HCM?

@istio-testing
Copy link
Collaborator

@j2gg0s: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@istio-testing istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Jul 20, 2022
@istio-policy-bot istio-policy-bot added the lifecycle/stale Indicates a PR or issue hasn't been manipulated by an Istio team member for a while label Jul 30, 2022
@istio-policy-bot
Copy link

🚧 This issue or pull request has been closed due to not having had activity from an Istio team member since 2022-06-30. If you feel this issue or pull request deserves attention, please reopen the issue. Please see this wiki page for more information. Thank you for your contributions.

Created by the issue and PR lifecycle manager.

@istio-policy-bot istio-policy-bot added the lifecycle/automatically-closed Indicates a PR or issue that has been closed automatically. label Aug 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lifecycle/automatically-closed Indicates a PR or issue that has been closed automatically. lifecycle/stale Indicates a PR or issue hasn't been manipulated by an Istio team member for a while needs-ok-to-test needs-rebase Indicates a PR needs to be rebased before being merged size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants