Skip to content

YAML: Convert ip_allow.config - #4168

Merged
SolidWallOfCode merged 1 commit into
apache:masterfrom
SolidWallOfCode:yaml-ipallow
Aug 5, 2019
Merged

SolidWallOfCode merged 1 commit into
apache:masterfrom
SolidWallOfCode:yaml-ipallow

Conversation

@SolidWallOfCode

@SolidWallOfCode SolidWallOfCode commented Aug 29, 2018

Copy link
Copy Markdown
Member

This is a first pass at converting ip_allow.config to YAML. The early commits will need to be pulled out in to separate PRs once this is in final form.

This allows configuration in the old format or YAML. It guesses the contents based on the first line. If it's "#" or "# ats" then it uses the old format, otherwise it uses YAML.

This still needs documentation updates.

This ended up require the break out of other PRs. The current outstanding one is #4206.
Merged: #4175, #4176, #4149

Comment thread proxy/IPAllow.cc
return w.print("Line {}", mark.line);
}

BufferWriter &

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This should probably be moved to bw_std_format.h in a separate PR as it's not specific to this PR.

Comment thread proxy/IPAllow.cc

} // namespace ts

namespace YAML

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Need to comment the code, but this is needed in order to pass a TextView to the array operator on a node.

Comment thread proxy/IPAllow.cc Outdated

file_buff = readIntoBuffer(config_file_path, "ip-allow", &file_size);
std::error_code ec;
ts::file::path fpath{config_file_path};

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Change config_file_path from ats_scoped_str to ts::file::path and skip this step?

zwoop
zwoop previously requested changes Aug 29, 2018
Comment thread lib/ts/ts_file.cc
@@ -0,0 +1,129 @@
/** @file

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why do we need this? If we truly need std::filesystem features, why not use std::filesystem? Don't we have all these types of functionality in libts already ?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

std::filesystem is still experimental and I've already found one major difference between the experimental version and the standard version.

I don't think we have any of these in libtsutil currently.

@SolidWallOfCode

Copy link
Copy Markdown
Member Author

Added documentation, updated the default configuration.

@SolidWallOfCode

Copy link
Copy Markdown
Member Author

I am unable to find anyway to link with std::experimental::filesystem for the FreeBSD builds. I've tried various approaches, nothing, c++, stdc++fs, c++fs, c++experimental, which were recommend in various places, but none were successful. If anyone else wants to try, tweak the "AC_SUBST(STD_FS_LIB, [stdc++fs])" line in configure.ac.

@zwoop

zwoop commented Aug 31, 2018

Copy link
Copy Markdown
Contributor

This was a great exercise, because it found a fundamental problem with FreeBSD 11.1. Ask and I believe that upgrading to 11.2 can fix this, and I’m working on that. For ATS v8.0and later, we will have to require fbsd 11.2 or later.

@SolidWallOfCode

Copy link
Copy Markdown
Member Author

This has been waiting on other PRs that spun out of it, I think it's down to just #4206, after which I'll rebase and do a full submission.

@SolidWallOfCode

Copy link
Copy Markdown
Member Author

Rebased and ready for review.

@SolidWallOfCode

Copy link
Copy Markdown
Member Author

Hmmm, this failed clang-format, but when I tried to update it, make clang-format made no changes. We'll see if it fails again.

@apache apache deleted a comment from randall Oct 11, 2018
@SolidWallOfCode
SolidWallOfCode force-pushed the yaml-ipallow branch 2 times, most recently from ec6a5fd to 519bd38 Compare October 13, 2018 16:01
@SolidWallOfCode
SolidWallOfCode force-pushed the yaml-ipallow branch 2 times, most recently from 394bdb5 to f259b67 Compare October 22, 2018 15:46
@zwoop
zwoop dismissed their stale review December 6, 2018 21:10

Fooey

@SolidWallOfCode
SolidWallOfCode force-pushed the yaml-ipallow branch 3 times, most recently from 4d63661 to 0debf67 Compare March 14, 2019 16:30
@zwoop

zwoop commented Apr 4, 2019

Copy link
Copy Markdown
Contributor

[approve ci]

Comment thread configs/ip_allow.schema.json Outdated
@SolidWallOfCode
SolidWallOfCode force-pushed the yaml-ipallow branch 2 times, most recently from 85fb362 to afabf1a Compare April 13, 2019 19:16
@randall

randall commented May 22, 2019

Copy link
Copy Markdown
Contributor

@SolidWallOfCode Can this be rebased?

zwoop
zwoop previously approved these changes Jul 16, 2019
Comment thread proxy/IPAllow.cc

namespace YAML
{
template <> struct convert<ts::TextView> {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does this need to be TextView ? Can it not be std::string_view?

@zwoop
zwoop dismissed their stale review July 16, 2019 16:16

Gah

@randall randall left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍

We'll need to move the contents under a top-level key (aka ip_allow:) in another PR.

@SolidWallOfCode

Copy link
Copy Markdown
Member Author

I thought it was already under a top level key, "ip_addr_acl". You mean a different key.

@SolidWallOfCode
SolidWallOfCode merged commit 9db871a into apache:master Aug 5, 2019
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.

4 participants