[GitHub] xnuinside commented on a change in pull request #4238: [AIRFLOW-987] pass kerberos cli args keytab and principal to kerberos…
xnuinside commented on a change in pull request #4238: [AIRFLOW-987] pass
kerberos cli args keytab and principal to kerberos…
URL: https://github.com/apache/incubator-airflow/pull/4238#discussion_r237049395
##
File path: airflow/security/kerberos.py
##
@@ -80,10 +82,8 @@ def perform_krb181_workaround():
ret = subprocess.call(cmdv, close_fds=True)
if ret != 0:
-principal = "%s/%s" % (
-configuration.conf.get('kerberos', 'principal'),
-socket.getfqdn()
-)
+principal = "%s/%s" % (configuration.conf.get('kerberos', 'principal'),
Review comment:
done
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] xnuinside commented on a change in pull request #4238: [AIRFLOW-987] pass kerberos cli args keytab and principal to kerberos…
xnuinside commented on a change in pull request #4238: [AIRFLOW-987] pass
kerberos cli args keytab and principal to kerberos…
URL: https://github.com/apache/incubator-airflow/pull/4238#discussion_r236970400
##
File path: tests/security/test_kerberos.py
##
@@ -32,13 +33,26 @@ def setUp(self):
if not configuration.conf.has_section("kerberos"):
configuration.conf.add_section("kerberos")
-
-configuration.conf.set("kerberos",
- "keytab",
+configuration.conf.set("kerberos", "keytab",
os.environ['KRB5_KTNAME'])
+keytab_from_cfg = configuration.conf.get("kerberos", "keytab")
+self.args = Namespace(keytab=keytab_from_cfg, principal=None, pid=None,
+ daemon=None, stdout=None, stderr=None,
log_file=None)
def test_renew_from_kt(self):
"""
We expect no result, but a successful run. No more TypeError
"""
-self.assertIsNone(renew_from_kt())
+self.assertIsNone(renew_from_kt(args=self.args))
+
+def test_args_from_cli(self):
+"""
+We expect no result, but a run with sys.exit(1) because keytab not
exist.
+"""
+configuration.conf.set("kerberos", "keytab", "")
+self.args.keytab = "test_keytab"
+
+with self.assertRaises(SystemExit) as se:
+renew_from_kt(args=self.args)
Review comment:
@ashb New in version 3.4. method added only in 3.4, at 2.7 will fail with
`with self.assertLogs(LoggingMixin().log) as log:
AttributeError: 'KerberosTest' object has no attribute 'assertLogs'`
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] xnuinside commented on a change in pull request #4238: [AIRFLOW-987] pass kerberos cli args keytab and principal to kerberos…
xnuinside commented on a change in pull request #4238: [AIRFLOW-987] pass kerberos cli args keytab and principal to kerberos… URL: https://github.com/apache/incubator-airflow/pull/4238#discussion_r236967908 ## File path: airflow/security/kerberos.py ## @@ -80,7 +82,7 @@ def perform_krb181_workaround(): ret = subprocess.call(cmdv, close_fds=True) if ret != 0: -principal = "%s/%s" % ( +principal = args.principal or "%s/%s" % ( Review comment: oh, I understand what you talking about, I think you right, yes. My mistake. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] xnuinside commented on a change in pull request #4238: [AIRFLOW-987] pass kerberos cli args keytab and principal to kerberos…
xnuinside commented on a change in pull request #4238: [AIRFLOW-987] pass kerberos cli args keytab and principal to kerberos… URL: https://github.com/apache/incubator-airflow/pull/4238#discussion_r236365655 ## File path: airflow/security/kerberos.py ## @@ -27,19 +27,21 @@ log = LoggingMixin().log -def renew_from_kt(): +def renew_from_kt(args=None): Review comment: okay, make sense This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] xnuinside commented on a change in pull request #4238: [AIRFLOW-987] pass kerberos cli args keytab and principal to kerberos…
xnuinside commented on a change in pull request #4238: [AIRFLOW-987] pass kerberos cli args keytab and principal to kerberos… URL: https://github.com/apache/incubator-airflow/pull/4238#discussion_r236365531 ## File path: airflow/security/kerberos.py ## @@ -80,7 +82,7 @@ def perform_krb181_workaround(): ret = subprocess.call(cmdv, close_fds=True) if ret != 0: -principal = "%s/%s" % ( +principal = args.principal or "%s/%s" % ( Review comment: We don't override config here. It is used only to prepare a command - https://github.com/apache/incubator-airflow/blob/3dc9e4021786e2c2416956f83a1f19ea1052ff3b/airflow/security/kerberos.py#L46 This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] xnuinside commented on a change in pull request #4238: [AIRFLOW-987] pass kerberos cli args keytab and principal to kerberos…
xnuinside commented on a change in pull request #4238: [AIRFLOW-987] pass
kerberos cli args keytab and principal to kerberos…
URL: https://github.com/apache/incubator-airflow/pull/4238#discussion_r236365027
##
File path: tests/security/test_kerberos.py
##
@@ -32,13 +33,26 @@ def setUp(self):
if not configuration.conf.has_section("kerberos"):
configuration.conf.add_section("kerberos")
-
-configuration.conf.set("kerberos",
- "keytab",
+configuration.conf.set("kerberos", "keytab",
os.environ['KRB5_KTNAME'])
+keytab_from_cfg = configuration.conf.get("kerberos", "keytab")
+self.args = Namespace(keytab=keytab_from_cfg, principal=None, pid=None,
+ daemon=None, stdout=None, stderr=None,
log_file=None)
def test_renew_from_kt(self):
"""
We expect no result, but a successful run. No more TypeError
"""
-self.assertIsNone(renew_from_kt())
+self.assertIsNone(renew_from_kt(args=self.args))
+
+def test_args_from_cli(self):
+"""
+We expect no result, but a run with sys.exit(1) because keytab not
exist.
+"""
+configuration.conf.set("kerberos", "keytab", "")
+self.args.keytab = "test_keytab"
+
+with self.assertRaises(SystemExit) as se:
+renew_from_kt(args=self.args)
Review comment:
it sys.exit(1) -
https://github.com/apache/incubator-airflow/blob/3dc9e4021786e2c2416956f83a1f19ea1052ff3b/airflow/security/kerberos.py#L62
about assertLogs - okay, I will add it
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] xnuinside commented on a change in pull request #4238: [AIRFLOW-987] pass kerberos cli args keytab and principal to kerberos…
xnuinside commented on a change in pull request #4238: [AIRFLOW-987] pass
kerberos cli args keytab and principal to kerberos…
URL: https://github.com/apache/incubator-airflow/pull/4238#discussion_r236364587
##
File path: airflow/security/kerberos.py
##
@@ -110,11 +112,11 @@ def detect_conf_var():
return b'X-CACHECONF:' in f.read()
-def run():
-if configuration.conf.get('kerberos', 'keytab') is None:
+def run(args):
+if not args.keytab:
Review comment:
no, it not needed, because we get configuration.conf.get('kerberos',
'keytab') as default value in arg -
https://github.com/apache/incubator-airflow/blob/master/airflow/bin/cli.py#L1685
, so if args.keytabe will be None it mean what config empty too
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] xnuinside commented on a change in pull request #4238: [AIRFLOW-987] pass kerberos cli args keytab and principal to kerberos…
xnuinside commented on a change in pull request #4238: [AIRFLOW-987] pass
kerberos cli args keytab and principal to kerberos…
URL: https://github.com/apache/incubator-airflow/pull/4238#discussion_r236334789
##
File path: airflow/security/kerberos.py
##
@@ -55,8 +57,8 @@ def renew_from_kt():
if subp.returncode != 0:
log.error("Couldn't reinit from keytab! `kinit' exited with
%s.\n%s\n%s" % (
subp.returncode,
-b"\n".join(subp.stdout.readlines()),
-b"\n".join(subp.stderr.readlines(
+"\n".join(subp.stdout.readlines()),
+"\n".join(subp.stderr.readlines(
Review comment:
Do we still need these lines?
https://github.com/apache/incubator-airflow/blob/master/airflow/bin/airflow#L26-L28
- I don't know :/ i fix only bug, what args from cli not passed to
kerberos.run, but they are defined in cli doc
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] xnuinside commented on a change in pull request #4238: [AIRFLOW-987] pass kerberos cli args keytab and principal to kerberos…
xnuinside commented on a change in pull request #4238: [AIRFLOW-987] pass
kerberos cli args keytab and principal to kerberos…
URL: https://github.com/apache/incubator-airflow/pull/4238#discussion_r236334348
##
File path: airflow/security/kerberos.py
##
@@ -55,8 +57,8 @@ def renew_from_kt():
if subp.returncode != 0:
log.error("Couldn't reinit from keytab! `kinit' exited with
%s.\n%s\n%s" % (
subp.returncode,
-b"\n".join(subp.stdout.readlines()),
-b"\n".join(subp.stderr.readlines(
+"\n".join(subp.stdout.readlines()),
+"\n".join(subp.stderr.readlines(
Review comment:
@ashb ,
also b"\n".join(subp.stdout.readlines()) caused error on 3.5, it was not
catched because wasn't covered with tests
error in 3.5 -
` b"\n".join(subp.stderr.readlines(
TypeError: sequence item 0: expected a bytes-like object, str found`
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
