envoyproxy / envoyproxy/envoy

fix_format doesn't uniquely identify absolute protobuf message names

Open
#17,216 1 comment 0 reactions 0 assignees View on GitHub
area/build bug help wanted
Dominant language
C++
Stars
28.9k
Forks
5.6k
Avg merge
1d 22h
Merged PRs (30d)
430

Description

We currently have the following structure inside of [`envoy.extensions.transport_sockets.tls.v3.CommonTlsContext`](https://github.com/envoyproxy/envoy/blob/7da9989039cc7fbbe08835e2c00f813aedb24e03/api/envoy/extensions/transport_sockets/tls/v3/tls.proto#L129):
- There is a [`CertificateProviderInstance`](https://github.com/envoyproxy/envoy/blob/7da9989039cc7fbbe08835e2c00f813aedb24e03/api/envoy/extensions/transport_sockets/tls/v3/tls.proto#L154) message.
- There is a field called [`tls_certificate_certificate_provider_instance`](https://github.com/envoyproxy/envoy/blob/7da9989039cc7fbbe08835e2c00f813aedb24e03/api/envoy/extensions/transport_sockets/tls/v3/tls.proto#L234) inside of `CommonTlsContext` that use this type.

In #17201, I am moving the `CertificateProviderInstance` out of `CommonTlsContext`. In order to not break API compatibility, I need to leave the existing `CertificateProviderInstance` message and the field that uses it intact but deprecated, copy the message to the new location, and add new fields that use the message in the new location.

So I created the new message and added the following new field in `CommonTlsContext`:

```
.envoy.extensions.transport_sockets.tls.v3.CertificateProviderPluginInstance tls_certificate_provider_instance = 15;
```

That uses the full path of the proto to differentiate between the new version of the `CertificateProviderPluginInstance` message that exists outside of the `CommonTlsContext` proto from the old version that exists inside of `CommonTlsContext`. However, fix_format changed it to use the latter instead of the former:

```
CertificateProviderInstance tls_certificate_provider_instance = 15;
```

It seems that fix_format can't tell that `CertificateProviderInstance` exists both inside and outside of `CommonTlsContext` and that I wanted to refer to the one that is outside.

For now, I've worked around this by simply using a different name for the new message (`CertificateProviderPluginInstance` instead of `CertificateProviderInstance`). But I think we should fix fix_format to not require that.

CC @htuch

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.