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

OSSM-2187 Allow usage of spec.proxy.networking.protocol.autoDetect #1049

Merged
merged 1 commit into from Nov 24, 2022

Conversation

dgn
Copy link
Contributor

@dgn dgn commented Oct 27, 2022

This removes the 'validation' that was effectively removing support for protocol sniffing and allows usage of the fields in 2.2 and 2.3 control planes. We previously removed this feature from OSSM 2.0 because of security concerns, however I cannot find any documentation of these concerns and upstream has been enabling it by default since Istio 1.6, so I consider it safe enough to allow usage. Note that we will still default to disabling it in order not to change behavior in any existing deployments.

We might want to discuss enabling it by default in 2.4

@dgn
Copy link
Contributor Author

dgn commented Nov 1, 2022

/retest

1 similar comment
@dgn
Copy link
Contributor Author

dgn commented Nov 7, 2022

/retest

@maistra-bot
Copy link
Contributor

@dgn: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
istio-operator_integ-kind-2.3 7a0fa24 link true /test integration-kind

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. I understand the commands that are listed here.

Copy link
Member

@jewertow jewertow left a comment

Choose a reason for hiding this comment

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

I tested your change on OCP and I can confirm that it worked. After setting protocol auto detection, istiod had updated env vars:

- name: PILOT_ENABLE_PROTOCOL_SNIFFING_FOR_OUTBOUND
  value: 'true'
- name: PILOT_ENABLE_PROTOCOL_SNIFFING_FOR_INBOUND
  value: 'true'

When I set autoDetect.timeout then istiod updated connected proxies.

There is only one thing that I would change.

if autoDetect.Timeout != "" {
        if err := setHelmStringValue(proxyValues, "protocolDetectionTimeout", autoDetect.Timeout); err != nil {
	        return err
	}
	if err := setHelmStringValue(meshConfigValues, "protocolDetectionTimeout", autoDetect.Timeout); err != nil {
		return err
	}
}

Setting proxyValues has no effect, because istiod respects only MeshConfig, so maybe we could remove it? It works anyway, so you can ignore this suggestion, but this PR is an opportunity to simplify this piece of code.

@dgn
Copy link
Contributor Author

dgn commented Nov 23, 2022

There is only one thing that I would change.

if autoDetect.Timeout != "" {
        if err := setHelmStringValue(proxyValues, "protocolDetectionTimeout", autoDetect.Timeout); err != nil {
	        return err
	}
	if err := setHelmStringValue(meshConfigValues, "protocolDetectionTimeout", autoDetect.Timeout); err != nil {
		return err
	}
}

Setting proxyValues has no effect, because istiod respects only MeshConfig, so maybe we could remove it? It works anyway, so you can ignore this suggestion, but this PR is an opportunity to simplify this piece of code.

Good catch Jacek! Thank you. I removed that code

This removes the 'validation' that was effectively removing support for protocol sniffing and allows usage of the fields in 2.2 and 2.3 control planes. We previously removed this feature from OSSM 2.0 because of security concerns, however I cannot find any documentation of these concerns and upstream has been enabling it by default since Istio 1.6, so I consider it safe enough to allow usage. Note that we will still default to disabling it in order not to change behavior in any existing deployments.
@dgn
Copy link
Contributor Author

dgn commented Nov 24, 2022

/retest

@maistra-bot maistra-bot merged commit 50f32fd into maistra:maistra-2.3 Nov 24, 2022
@dgn dgn deleted the OSSM-2187 branch November 24, 2022 09:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants