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

[AIRFLOW-5641] support running git sync container as root #6312

Merged
merged 1 commit into from
Oct 15, 2019

Conversation

houqp
Copy link
Member

@houqp houqp commented Oct 12, 2019

Make sure you have checked all steps below.

Jira

Description

Fixed a regression that prevents user from using root as user for git sync container.

Also changed _get_security_context_val so that it behaves as documented.

Tests

  • My PR adds the following unit tests OR does not need testing for this extremely good reason:

See changed files in tests folder

Commits

  • My commits all reference Jira issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":
    1. Subject is separated from body by a blank line
    2. Subject is limited to 50 characters (not including Jira issue reference)
    3. Subject does not end with a period
    4. Subject uses the imperative mood ("add", not "adding")
    5. Body wraps at 72 characters
    6. Body explains "what" and "why", not "how"

Documentation

  • In case of new functionality, my PR adds documentation that describes how to use it.
    • All the public functions and the classes in the PR contain docstrings that explain what it does
    • If you implement backwards incompatible changes, please leave a note in the Updating.md so we can assign it to a appropriate release

@houqp houqp force-pushed the git_sync_user branch 2 times, most recently from 0479cab to 8bd02cd Compare October 12, 2019 03:54
@codecov-io
Copy link

Codecov Report

Merging #6312 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6312      +/-   ##
==========================================
+ Coverage   80.34%   80.35%   +<.01%     
==========================================
  Files         616      616              
  Lines       35733    35734       +1     
==========================================
+ Hits        28711    28713       +2     
+ Misses       7022     7021       -1
Impacted Files Coverage Δ
airflow/kubernetes/worker_configuration.py 96.4% <ø> (ø) ⬆️
airflow/executors/kubernetes_executor.py 58.99% <100%> (+0.09%) ⬆️
airflow/utils/dag_processing.py 56.55% <0%> (-0.35%) ⬇️
airflow/models/taskinstance.py 93.77% <0%> (+0.5%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c0d98a7...8bd02cd. Read the comment docs.

@mik-laj mik-laj added the k8s label Oct 13, 2019
Copy link
Member

@ashb ashb left a comment

Choose a reason for hiding this comment

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

Test that if the git_sync_run_as_user is not specified at all that we still set the default of 65533 please

@houqp
Copy link
Member Author

houqp commented Oct 14, 2019

@ashb I tried testing that, Airflow exited on error because that config key exists in default config, so it is required. ('kubernetes', 'git_sync_run_as_user'): '' is the closest thing to that test.

@houqp
Copy link
Member Author

houqp commented Oct 15, 2019

Turns out I was setting gi_sync_run_as_user to None, which was causing the crash. I have added a test case to avoid setting this config key entirely, which results in the default 65533 uid.

@houqp houqp requested a review from ashb October 15, 2019 03:43
@ashb ashb merged commit 133085e into apache:master Oct 15, 2019
@ashb
Copy link
Member

ashb commented Oct 15, 2019

This is only a problem against master right? (The code looks quite different on the release branch)

@ashb
Copy link
Member

ashb commented Jun 17, 2020

Marking this for inclusion in 1.10.11 -- but that depends on @dimberman's backporting work.

dimberman pushed a commit that referenced this pull request Jun 25, 2020
potiuk pushed a commit that referenced this pull request Jun 29, 2020
kaxil pushed a commit that referenced this pull request Jul 1, 2020
cfei18 pushed a commit to cfei18/incubator-airflow that referenced this pull request Mar 5, 2021
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.

4 participants