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

deprecate old-style v1 JSON filter config #7129

Merged
merged 9 commits into from
Jun 5, 2019
Merged

Conversation

mattklein123
Copy link
Member

@mattklein123 mattklein123 commented May 31, 2019

Now that TCP proxy has parity, we can completely remove v1
style config loading.

Risk Level: Low
Testing: New UT
Docs Changes: Added.
Release Notes: Added.

Now that TCP proxy has parity, we can completely remove v1
style config loading.

Signed-off-by: Matt Klein <mklein@lyft.com>
@mattklein123
Copy link
Member Author

@alyssawilk @derekargueta PTAL

@derekargueta
Copy link
Member

derekargueta commented Jun 3, 2019

No code changes? Might be useful to users to hook this into the deprecated feature counter or start-up logging (although hopefully it's unlikely that anyone is still using deprecated_v1)

@derekargueta
Copy link
Member

Also is this different than the v1 deprecation in the 1.8 deprecation notes (first bullet)?

@alyssawilk
Copy link
Contributor

@PiotrSikora for parity comments, since this relates to #5355

If we're going to do this we should flag the actual API fields as well so the scripts pick it up, no?

@mattklein123
Copy link
Member Author

If we're going to do this we should flag the actual API fields as well so the scripts pick it up, no?

Unfortunately there are no API fields here. This is related to this code:

if (filter_config->getBoolean("deprecated_v1", false)) {

and

if (filter_config->getBoolean("deprecated_v1", false)) {

I agree with @derekargueta that we need to engage the stat/warn logic here, but it will need special casing. I was being lazy. Let me look into how to handle this. @alyssawilk any quick thoughts on how to one off engage the logic here since we aren't dealing with proto fields?

@mattklein123
Copy link
Member Author

@PiotrSikora for parity comments, since this relates to #5355

It doesn't actually relate to bind to port. See ^.

@alyssawilk
Copy link
Contributor

Yeah, sorry, I was misreading the bit about the tcp proxy session and network filter config to think we were removing deprecated_v1 fields altogether

alyssawilk
alyssawilk previously approved these changes Jun 3, 2019
@mattklein123
Copy link
Member Author

@alyssawilk I still want to address @derekargueta comment about engaging the stat/warn/fail by default behavior. Do you think it would be possible to actually guard these if checks with some type of fake runtime field check? I can look into it later today.

Signed-off-by: Matt Klein <mklein@lyft.com>
@mattklein123
Copy link
Member Author

@derekargueta @alyssawilk updated PTAL

Signed-off-by: Matt Klein <mklein@lyft.com>
Signed-off-by: Matt Klein <mklein@lyft.com>
@mattklein123
Copy link
Member Author

/retest

@repokitteh-read-only
Copy link

🔨 rebuilding ci/circleci: compile_time_options (failed build)

🐱

Caused by: a #7129 (comment) was created by @mattklein123.

see: more, trace.

derekargueta
derekargueta previously approved these changes Jun 3, 2019
Signed-off-by: Matt Klein <mklein@lyft.com>
@mattklein123
Copy link
Member Author

@alyssawilk updated

Copy link
Contributor

@alyssawilk alyssawilk left a comment

Choose a reason for hiding this comment

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

It makes me wonder if deprecatedFeatureEnabled should take an error argument, but I guess that'd bring us the same problem we have with configs where some call sites might otherwise get too spammy.

Also I guess I should go fix that TODO on deprecation tooling for features now :-)

@mattklein123 mattklein123 merged commit 0f7983c into master Jun 5, 2019
@mattklein123 mattklein123 deleted the deprecate_v1_config branch June 5, 2019 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants