Skip to content

Conversation

tklauser
Copy link
Member

@tklauser tklauser commented Sep 24, 2024

Previously it was possible to enable L7 protocol visibility using the policy.cilium.io/proxy-visibility Pod annotation. That method has various bugs, is no longer supported and has been deprecated since Cilium 1.15, see #28449.

See commits for details.

@tklauser tklauser added kind/cleanup This includes no functional changes. 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 Sep 24, 2024
@github-actions github-actions bot added the sig/policy Impacts whether traffic is allowed or denied based on user-defined policies. label Sep 24, 2024
@tklauser
Copy link
Member Author

/test

@tklauser tklauser force-pushed the pr/tklauser/rm-proxy-visibility-anno branch from cb24ec4 to c6611cf Compare September 24, 2024 09:41
@tklauser
Copy link
Member Author

/ci-ginkgo

@tklauser tklauser force-pushed the pr/tklauser/rm-proxy-visibility-anno branch from c6611cf to 3fb4b85 Compare September 24, 2024 09:50
@tklauser
Copy link
Member Author

/ci-ginkgo

@tklauser tklauser force-pushed the pr/tklauser/rm-proxy-visibility-anno branch from 3fb4b85 to cc7205a Compare September 24, 2024 11:04
@tklauser
Copy link
Member Author

/ci-ginkgo

@tklauser
Copy link
Member Author

/ci-integration

@tklauser
Copy link
Member Author

/test

@tklauser
Copy link
Member Author

tklauser commented Sep 24, 2024

/ci-integration

Hit #34092: https://github.com/cilium/cilium/actions/runs/11012829080/job/30579864094

@tklauser tklauser force-pushed the pr/tklauser/rm-proxy-visibility-anno branch from cc7205a to 4f31e62 Compare September 24, 2024 12:20
@tklauser
Copy link
Member Author

/test

@tklauser tklauser marked this pull request as ready for review September 24, 2024 12:22
@tklauser tklauser requested review from a team as code owners September 24, 2024 12:22
@tklauser tklauser requested a review from rolinh September 24, 2024 12:22
Copy link
Contributor

@derailed derailed left a comment

Choose a reason for hiding this comment

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

@tklauser Nice clean up!

@tklauser tklauser enabled auto-merge September 25, 2024 12:49
Copy link
Member

@rolinh rolinh left a comment

Choose a reason for hiding this comment

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

Nice cleanup!

Copy link
Contributor

@squeed squeed left a comment

Choose a reason for hiding this comment

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

Looks good. Would like to have @jrajahalme's review before merging.

There's a minor docs change needed, otherwise this is good.

L7 visibility using Pod annotations is deprecated and the feature will
be removed in a successive commit. Switch K8sAgentHubbleTest still using
it to use L7 visibility policy instead.

Signed-off-by: Tobias Klauser <tobias@cilium.io>
Previously it was possible to enable L7 protocol visibility using the
policy.cilium.io/proxy-visibility annotation. That method has various
known bugs, is no longer supported and has been deprecated since Cilium
1.15, see #28449.

Signed-off-by: Tobias Klauser <tobias@cilium.io>
…alue

The error return value is always nil, remove it.

Signed-off-by: Tobias Klauser <tobias@cilium.io>
@tklauser tklauser force-pushed the pr/tklauser/rm-proxy-visibility-anno branch from 4f31e62 to 5d88f2c Compare September 30, 2024 20:41
@tklauser
Copy link
Member Author

/test

@tklauser
Copy link
Member Author

@jrajahalme @learnitall could you please take a look?

Copy link
Member

@jrajahalme jrajahalme left a comment

Choose a reason for hiding this comment

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

Looking great, thanks for the clean-up!

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.

Could we backport at least the test changes up to v1.15 branch?

@tklauser
Copy link
Member Author

tklauser commented Oct 1, 2024

Could we backport at least the test changes up to v1.15 branch?

Sure, will open separate backports for these.

@tklauser tklauser added this pull request to the merge queue Oct 1, 2024
@tklauser tklauser removed the request for review from learnitall October 1, 2024 12:28
Merged via the queue into main with commit a01a39e Oct 1, 2024
263 checks passed
@tklauser tklauser deleted the pr/tklauser/rm-proxy-visibility-anno branch October 1, 2024 12:35
@maintainer-s-little-helper maintainer-s-little-helper bot added ready-to-merge This PR has passed all tests and received consensus from code owners to merge. labels Oct 1, 2024
@tklauser
Copy link
Member Author

tklauser commented Oct 1, 2024

Could we backport at least the test changes up to v1.15 branch?

Sure, will open separate backports for these.

v1.16: #35151
v1.15: #35152

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/proxy Impacts proxy components, including DNS, Kafka, Envoy and/or XDS servers. kind/cleanup This includes no functional changes. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/minor This PR changes functionality that users may find relevant to operating Cilium. sig/policy Impacts whether traffic is allowed or denied based on user-defined policies.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants