You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "zeotuan (via GitHub)" <gi...@apache.org> on 2023/09/15 08:05:16 UTC
[GitHub] [airflow] zeotuan opened a new pull request, #34386: Add use_krb5ccache option to SparkSubmitHook
zeotuan opened a new pull request, #34386:
URL: https://github.com/apache/airflow/pull/34386
#11246 - Add use_krb5ccache option to tell spark to ticket cache instead of relying on keytab for Kerberos login
https://spark.apache.org/docs/latest/security.html#using-a-ticket-cache
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add use_krb5ccache option to SparkSubmitHook [airflow]
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on code in PR #34386:
URL: https://github.com/apache/airflow/pull/34386#discussion_r1364524089
##########
airflow/security/kerberos.py:
##########
@@ -59,10 +63,7 @@ def renew_from_kt(principal: str | None, keytab: str, exit_on_fail: bool = True)
# minutes to give ourselves a large renewal buffer.
renewal_lifetime = f"{conf.getint('kerberos', 'reinit_frequency')}m"
- cmd_principal = principal or conf.get_mandatory_value("kerberos", "principal").replace(
- "_HOST", get_hostname()
- )
-
+ cmd_principal = get_kerberos_principle(principal)
Review Comment:
https://github.com/apache/airflow/pull/34936 is merged can you please rebase?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add use_krb5ccache option to SparkSubmitHook [airflow]
Posted by "Lee-W (via GitHub)" <gi...@apache.org>.
Lee-W commented on PR #34386:
URL: https://github.com/apache/airflow/pull/34386#issuecomment-1746086068
I believe the CI failure in this branch has been resolved by https://github.com/apache/airflow/pull/34701. Could you please rebase the branch and see whether it passes the CI? Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add use_krb5ccache option to SparkSubmitHook [airflow]
Posted by "zeotuan (via GitHub)" <gi...@apache.org>.
zeotuan commented on code in PR #34386:
URL: https://github.com/apache/airflow/pull/34386#discussion_r1366370513
##########
airflow/security/kerberos.py:
##########
@@ -59,10 +63,7 @@ def renew_from_kt(principal: str | None, keytab: str, exit_on_fail: bool = True)
# minutes to give ourselves a large renewal buffer.
renewal_lifetime = f"{conf.getint('kerberos', 'reinit_frequency')}m"
- cmd_principal = principal or conf.get_mandatory_value("kerberos", "principal").replace(
- "_HOST", get_hostname()
- )
-
+ cmd_principal = get_kerberos_principle(principal)
Review Comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add use_krb5ccache option to SparkSubmitHook [airflow]
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on PR #34386:
URL: https://github.com/apache/airflow/pull/34386#issuecomment-1776612174
@zeotuan do we need to expose this parameter in SparkSubmitOperator as well?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add use_krb5ccache option to SparkSubmitHook [airflow]
Posted by "zeotuan (via GitHub)" <gi...@apache.org>.
zeotuan commented on code in PR #34386:
URL: https://github.com/apache/airflow/pull/34386#discussion_r1348191633
##########
airflow/providers/apache/spark/hooks/spark_submit.py:
##########
@@ -111,6 +111,7 @@ def __init__(
executor_memory: str | None = None,
driver_memory: str | None = None,
keytab: str | None = None,
+ use_krb5ccache: bool = False,
Review Comment:
Sure
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add use_krb5ccache option to SparkSubmitHook [airflow]
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on code in PR #34386:
URL: https://github.com/apache/airflow/pull/34386#discussion_r1358077855
##########
airflow/security/kerberos.py:
##########
@@ -59,10 +63,7 @@ def renew_from_kt(principal: str | None, keytab: str, exit_on_fail: bool = True)
# minutes to give ourselves a large renewal buffer.
renewal_lifetime = f"{conf.getint('kerberos', 'reinit_frequency')}m"
- cmd_principal = principal or conf.get_mandatory_value("kerberos", "principal").replace(
- "_HOST", get_hostname()
- )
-
+ cmd_principal = get_kerberos_principle(principal)
Review Comment:
This PR changes code in both core and in providers and it has separated release cycle.
So if we can extract the `airflow/security/kerberos.py` to a separated PR we will get it merge then rebase this PR
This will make release manager life much easier
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add use_krb5ccache option to SparkSubmitHook [airflow]
Posted by "Lee-W (via GitHub)" <gi...@apache.org>.
Lee-W commented on code in PR #34386:
URL: https://github.com/apache/airflow/pull/34386#discussion_r1345154160
##########
airflow/providers/apache/spark/hooks/spark_submit.py:
##########
@@ -111,6 +111,7 @@ def __init__(
executor_memory: str | None = None,
driver_memory: str | None = None,
keytab: str | None = None,
+ use_krb5ccache: bool = False,
Review Comment:
Should we make it a keyword-only argument? Also, could we add it to the docstring?
```python
*,
use_krb5ccache: bool = False,
```
##########
tests/providers/apache/spark/hooks/test_spark_submit.py:
##########
@@ -201,6 +207,22 @@ def test_build_spark_submit_command(self):
"baz",
]
assert expected_build_cmd == cmd
+ mock_get_env.assert_called_with("KRB5CCNAME")
+
+ @patch("airflow.configuration.conf.get_mandatory_value")
+ def test_resolve_spark_submit_env_vars_use_krb5ccache_missing_principal(self, mock_get_madantory_value):
+ mock_principle = "airflow"
+ mock_get_madantory_value.return_value = mock_principle
+ hook = SparkSubmitHook(conn_id="spark_yarn_cluster", principal=None, use_krb5ccache=True)
+ mock_get_madantory_value.assert_called_with("kerberos", "principal")
+ assert hook._principal == mock_principle
+
+ def test_resolve_spark_submit_env_vars_use_krb5ccache_missing_KRB5CCNAME_env(self):
+ hook = SparkSubmitHook(
+ conn_id="spark_yarn_cluster", principal="user/spark@airflow.org", use_krb5ccache=True
+ )
+ with pytest.raises(AirflowException):
Review Comment:
```suggestion
with pytest.raises(AirflowException, match="KRB5CCNAME environment variable required to use ticket ccache is missing."):
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add use_krb5ccache option to SparkSubmitHook [airflow]
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal merged PR #34386:
URL: https://github.com/apache/airflow/pull/34386
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #34386: Add use_krb5ccache option to SparkSubmitHook
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #34386:
URL: https://github.com/apache/airflow/pull/34386#issuecomment-1720987261
You need to make it compatible with Airflow 2.4. Seems that there is no `airflow.security.kerberos` in 2.4 (see the error in CI) so you need to see what was there and make conditional code based on `airflow.__version__` and comment to remove it after the compatibility is not needed - i.e. when we go to 2.5+ or 2.6+ support - depending when it was aded (you can search for similar examples in other providers)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add use_krb5ccache option to SparkSubmitHook [airflow]
Posted by "zeotuan (via GitHub)" <gi...@apache.org>.
zeotuan commented on code in PR #34386:
URL: https://github.com/apache/airflow/pull/34386#discussion_r1359200998
##########
airflow/security/kerberos.py:
##########
@@ -59,10 +63,7 @@ def renew_from_kt(principal: str | None, keytab: str, exit_on_fail: bool = True)
# minutes to give ourselves a large renewal buffer.
renewal_lifetime = f"{conf.getint('kerberos', 'reinit_frequency')}m"
- cmd_principal = principal or conf.get_mandatory_value("kerberos", "principal").replace(
- "_HOST", get_hostname()
- )
-
+ cmd_principal = get_kerberos_principle(principal)
Review Comment:
Sure. I have added another PR here #34936
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add use_krb5ccache option to SparkSubmitHook [airflow]
Posted by "zeotuan (via GitHub)" <gi...@apache.org>.
zeotuan commented on PR #34386:
URL: https://github.com/apache/airflow/pull/34386#issuecomment-1778569490
Oh Yes we do. I will submit a PR for that
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org