From 1dc684a30116eb1a829276aac2af4637f33a8137 Mon Sep 17 00:00:00 2001 From: Larry Hamel Date: Thu, 6 Jul 2017 15:33:25 -0700 Subject: [PATCH] gpconfig: add validation for readonly GUCs -- metadata about gucs, like GUC_DISALLOW_IN_FILE, is not available via sql. Work around that by parsing the C code that defines this metadata, and store relevant information in a file, to be read by gpconfig when called to change a GUC. -- the Makefile in gpMgmt/bin will create a local file during 'make install', used by gpconfig to validate that a given GUC can be changed Signed-off-by: C.J. Jameson Signed-off-by: Shoaib Lari Signed-off-by: Nadeem Ghani Signed-off-by: Marbin Tan --- gpMgmt/bin/Makefile | 2 + gpMgmt/bin/gpconfig | 36 ++++++++- .../gpconfig_modules/parse_guc_metadata.py | 73 +++++++++++++++++++ .../gppylib/test/unit/test_unit_gpconfig.py | 65 ++++++++++++++++- .../test/unit/test_unit_parse_guc_metadata.py | 32 ++++++++ 5 files changed, 203 insertions(+), 5 deletions(-) create mode 100644 gpMgmt/bin/gpconfig_modules/parse_guc_metadata.py create mode 100644 gpMgmt/bin/gppylib/test/unit/test_unit_parse_guc_metadata.py diff --git a/gpMgmt/bin/Makefile b/gpMgmt/bin/Makefile index 1384c43243..27a4b263c3 100644 --- a/gpMgmt/bin/Makefile +++ b/gpMgmt/bin/Makefile @@ -29,6 +29,7 @@ LIB_DIR=$(SRC)/lib PYLIB_DIR=$(SRC)/ext core: pygresql subprocess32 + python gpconfig_modules/parse_guc_metadata.py $(prefix) ifneq "$(wildcard $(CURDIR)/pythonSrc/ext/*.tar.gz)" "" all: core lockfile paramiko pycrypto stream psutil @@ -246,6 +247,7 @@ clean : @rm -rf $(STREAM_DIR)/stream lib/stream @rm -rf *.pyc lib/*.pyc @rm -f gpcheckcatc gpcrondumpc gpexpandc gptransferc + @rm -f gpconfig_modules/gucs_disallowed_in_file.txt .PHONY: disclean distclean: clean diff --git a/gpMgmt/bin/gpconfig b/gpMgmt/bin/gpconfig index 85988a6317..2fc36e4784 100755 --- a/gpMgmt/bin/gpconfig +++ b/gpMgmt/bin/gpconfig @@ -21,6 +21,7 @@ try: from gpconfig_modules.file_segment_guc import FileSegmentGuc from gpconfig_modules.guc_collection import GucCollection from gppylib.gpresgroup import GpResGroup + from gpconfig_modules.parse_guc_metadata import ParseGuc except ImportError as err: sys.exit('Cannot import modules. Please check that you have sourced ' 'greenplum_path.sh. Detail: ' + str(err)) @@ -29,6 +30,7 @@ EXECNAME = os.path.split(__file__)[-1] PROHIBITED_GUCS = set(["port", "listen_addresses"]) SAMEVALUE_GUCS = set(["gp_default_storage_options"]) +read_only_gucs = set() # populated at runtime LOGGER = get_default_logger() setup_tool_logging(EXECNAME, getLocalHostname(), getUserName()) @@ -75,6 +77,10 @@ def validate_mutual_options(options): if user is None or user == ' ': log_and_raise("USER environment variable must be set.") + gphome = os.getenv('GPHOME') + if not gphome: + log_and_raise("GPHOME environment variable must be set.") + if options.file and not options.show: log_and_raise("'--file' option must accompany '--show' option") if options.file and options.file_compare and options.show: @@ -273,7 +279,7 @@ def do_change(options): gp_array = GpArray.initFromCatalog(dbconn.DbURL(), utility=True) if not options.skipvalidation: - validate_options(options) + validate_change_options(options) except DatabaseError as ex: LOGGER.error(ex.__str__()) @@ -348,7 +354,33 @@ def do_change(options): LOGGER.info("completed successfully") -def validate_options(options): +def _is_guc_writeable(options): + """ +FYI, metadata about gucs, like GUC_DISALLOW_IN_FILE, is not available via +sql. We work around that by making use of a file already created during 'make install'. +(That file is created by parsing the C code that defines all GUCS, + storing all properties with GUC_DISALLOW_IN_FILE into a file.) + """ + + gphome = os.getenv('GPHOME') + gpconfig_modules_dir = os.path.join(gphome, ParseGuc.DESTINATION_DIR) + disallowed_guc_file = os.path.abspath(os.path.join(gpconfig_modules_dir, ParseGuc.DESTINATION_FILENAME)) + if os.path.exists(disallowed_guc_file): + with open(disallowed_guc_file) as f: + lines = f.readlines() + read_only_gucs.update([guc.strip() for guc in lines]) + else: + msg = "disallowed GUCs file missing: '%s'" % disallowed_guc_file + LOGGER.warning(msg) + return options.entry not in read_only_gucs + + +def validate_change_options(options): + if not _is_guc_writeable(options): + msg = "not a modifiable GUC: '%s'" % options.entry + LOGGER.fatal(msg) + raise Exception(msg) + conn = dbconn.connect(dbconn.DbURL(), True) guc = get_normal_guc(conn, options) if not guc: diff --git a/gpMgmt/bin/gpconfig_modules/parse_guc_metadata.py b/gpMgmt/bin/gpconfig_modules/parse_guc_metadata.py new file mode 100644 index 0000000000..f5b62c9b1e --- /dev/null +++ b/gpMgmt/bin/gpconfig_modules/parse_guc_metadata.py @@ -0,0 +1,73 @@ +import errno +import os +import re +import sys + +# this finds all instances of a property name that is both surrounded with quotes and is adjacent +# to the metadata "GUC_DISALLOW_IN_FILE", where adjacent means not separated by a closing } bracket, +# assuming all the separate lines have been combined together into one string +GUCS_DISALLOWED_IN_FILE_REGEX = r"\"(\w+)\"[^}]+GUC_DISALLOW_IN_FILE" + + +class ParseGuc: + """ + metadata about gucs, like GUC_DISALLOW_IN_FILE, is not available via +sql. Work around that by parsing the C code that defines this metadata, and store +relevant information in a file, to be read by gpconfig when called to +change a GUC. The Makefile in gpMgmt/bin will use this parser to + create a file during 'make install', used by gpconfig to validate that a given GUC can be changed + """ + + # hardcoded name for file looked up by gpconfig during its runtime + DESTINATION_FILENAME = "gucs_disallowed_in_file.txt" + DESTINATION_DIR = "share/greenplum" + + def __init__(self): + self.disallowed_in_file_pattern = re.compile(GUCS_DISALLOWED_IN_FILE_REGEX) + + @staticmethod + def get_source_c_file_paths(): + mypath = os.path.realpath(__file__) + misc_dir = os.path.join(os.path.dirname(mypath), "../../../src/backend/utils/misc") + return [os.path.join(misc_dir, "guc.c"), os.path.join(misc_dir, "guc_gp.c")] + + def parse_gucs(self, guc_path): + with open(guc_path) as f: + lines = f.readlines() + combined = " ".join([line.strip() for line in lines]) + return self.disallowed_in_file_pattern.finditer(combined) + + def write_gucs_to_file(self, filepath): + with open(filepath, 'w') as f: + paths = self.get_source_c_file_paths() + for guc_source in paths: + hits = self.parse_gucs(guc_source) + for match in hits: + title = match.groups() + f.write("%s\n" % title) + + def main(self): + if len(sys.argv) < 2: + print("usage: parse_guc_metadata ") + sys.exit(1) + prefix = sys.argv[1] + dir_path = os.path.join(prefix, self.DESTINATION_DIR) + _mkdir_p(dir_path, 0755) + output_file = os.path.join(dir_path, self.DESTINATION_FILENAME) + self.write_gucs_to_file(output_file) + + +def _mkdir_p(path, mode): + try: + os.makedirs(path, mode) + except OSError as exc: # Python >2.5 + if exc.errno == errno.EEXIST and os.path.isdir(path): + pass + else: + raise + +# -------------------------------------------------------------------------- +# Main +# -------------------------------------------------------------------------- +if __name__ == '__main__': + ParseGuc().main() diff --git a/gpMgmt/bin/gppylib/test/unit/test_unit_gpconfig.py b/gpMgmt/bin/gppylib/test/unit/test_unit_gpconfig.py index a7a77ae200..40dc6c1367 100644 --- a/gpMgmt/bin/gppylib/test/unit/test_unit_gpconfig.py +++ b/gpMgmt/bin/gppylib/test/unit/test_unit_gpconfig.py @@ -6,6 +6,8 @@ import sys import tempfile from StringIO import StringIO + +import errno from pygresql.pg import DatabaseError from gparray import GpDB, GpArray, Segment @@ -13,15 +15,18 @@ import shutil from mock import * from gp_unittest import * from gphostcache import GpHost +from gpconfig_modules.parse_guc_metadata import ParseGuc db_singleton_side_effect_list = [] def singleton_side_effect(unused1, unused2): # this function replaces dbconn.execSQLForSingleton(conn, sql), conditionally raising exception - if db_singleton_side_effect_list[0] == "DatabaseError": - raise DatabaseError("mock exception") - return db_singleton_side_effect_list[0] + if len(db_singleton_side_effect_list) > 0: + if db_singleton_side_effect_list[0] == "DatabaseError": + raise DatabaseError("mock exception") + return db_singleton_side_effect_list[0] + return None class GpConfig(GpTestCase): @@ -45,6 +50,7 @@ class GpConfig(GpTestCase): self.os_env = dict(USER="my_user") self.os_env["MASTER_DATA_DIRECTORY"] = self.temp_dir + self.os_env["GPHOME"] = self.temp_dir self.gparray = self._create_gparray_with_2_primary_2_mirrors() self.host_cache = Mock() @@ -81,6 +87,12 @@ class GpConfig(GpTestCase): ]) sys.argv = ["gpconfig"] # reset to relatively empty args list + shared_dir = os.path.join(self.temp_dir, ParseGuc.DESTINATION_DIR) + _mkdir_p(shared_dir, 0755) + self.guc_disallowed_readonly_file = os.path.abspath(os.path.join(shared_dir, ParseGuc.DESTINATION_FILENAME)) + with open(self.guc_disallowed_readonly_file, 'w') as f: + f.writelines("x\ny\n") + def tearDown(self): shutil.rmtree(self.temp_dir) super(GpConfig, self).tearDown() @@ -350,6 +362,43 @@ class GpConfig(GpTestCase): self.assertIn("[context: -1] [dbid: 2] [name: my_property_name] [value: foo | file: bar]", mock_stdout.getvalue()) + def test_setting_guc_when_guc_is_readonly_will_fail(self): + self.subject.read_only_gucs.add("is_superuser") + sys.argv = ["gpconfig", "-c", "is_superuser", "-v", "on"] + with self.assertRaisesRegexp(Exception, "not a modifiable GUC: 'is_superuser'"): + self.subject.do_main() + + def test_change_will_populate_read_only_gucs_set(self): + sys.argv = ["gpconfig", "--change", "foobar", "--value", "baz"] + try: + self.subject.do_main() + except Exception: + pass + self.assertEqual(len(self.subject.read_only_gucs), 2) + + def test_change_when_disallowed_gucs_file_is_missing_gives_warning(self): + os.remove(self.guc_disallowed_readonly_file) + db_singleton_side_effect_list.append("some happy result") + entry = 'my_property_name' + sys.argv = ["gpconfig", "-c", entry, "-v", "100", "--masteronly"] + # 'SELECT name, setting, unit, short_desc, context, vartype, min_val, max_val FROM pg_settings' + self.cursor.set_result_for_testing([['my_property_name', 'setting', 'unit', 'short_desc', + 'context', 'vartype', 'min_val', 'max_val']]) + self.subject.do_main() + + self.subject.LOGGER.info.assert_called_with("completed successfully") + target_warning = "disallowed GUCs file missing: '%s'" % self.guc_disallowed_readonly_file + self.subject.LOGGER.warning.assert_called_with(target_warning) + + def test_when_gphome_env_unset_raises(self): + self.os_env['GPHOME'] = None + sys.argv = ["gpconfig", "-c", 'my_property_name', "-v", "100", "--masteronly"] + + with self.assertRaisesRegexp(Exception, "GPHOME environment variable must be set"): + self.subject.do_main() + + + @staticmethod def _create_gparray_with_2_primary_2_mirrors(): master = GpDB.initFromString( @@ -365,5 +414,15 @@ class GpConfig(GpTestCase): return GpArray([master, primary0, primary1, mirror0, mirror1]) +def _mkdir_p(path, mode): + try: + os.makedirs(path, mode) + except OSError as exc: # Python >2.5 + if exc.errno == errno.EEXIST and os.path.isdir(path): + pass + else: + raise + + if __name__ == '__main__': run_tests() diff --git a/gpMgmt/bin/gppylib/test/unit/test_unit_parse_guc_metadata.py b/gpMgmt/bin/gppylib/test/unit/test_unit_parse_guc_metadata.py new file mode 100644 index 0000000000..31c6111d31 --- /dev/null +++ b/gpMgmt/bin/gppylib/test/unit/test_unit_parse_guc_metadata.py @@ -0,0 +1,32 @@ +import os + +import sys + +from gp_unittest import * +from gpconfig_modules.parse_guc_metadata import ParseGuc + + +class TestParseGucMetadata(GpTestCase): + def setUp(self): + self.subject = ParseGuc() + self.DEST_DIR = "/tmp/test_parseguc/bar" + + def test_main_writes_file(self): + sys.argv = ["parse_guc_metadata", self.DEST_DIR] + + self.subject.main() + + dest_file = os.path.join(self.DEST_DIR, "share/greenplum", self.subject.DESTINATION_FILENAME) + with open(dest_file, 'r') as f: + lines = f.readlines() + self.assertIn('is_superuser\n', lines) + self.assertIn('gp_session_id\n', lines) + + def test_main_when_no_prefix_will_fail(self): + sys.argv = ["parse_guc_metadata"] + + self.assertRaises(SystemExit, self.subject.main) + + +if __name__ == '__main__': + run_tests() -- GitLab