apache / airflow

Apache Airflow - A platform to programmatically author, schedule, and monitor workflows
https://airflow.apache.org/
Apache License 2.0
37.46k stars 14.36k forks source link

Global Container Security Context does not work for Git Sync container #35350

Open ahipp13 opened 1 year ago

ahipp13 commented 1 year ago

Official Helm Chart version

1.10.0

Apache Airflow version

2.6.3

Kubernetes Version

1.26.5

Helm Chart configuration

No response

Docker Image customizations

No response

What happened

Idk if this is the intended way this should work, but when I set the global containerSecurityContext like:

# Detailed default security context for airflow deployments
securityContexts:
  pod:
    runAsUser: 50000
    runAsGroup: 50000
    fsGroup: 50000
  containers:
    capabilities:
      drop:
        - KILL
        - MKNOD
        - SYS_CHROOT

This does not apply to the "git sync" container. In order for me to get it to apply to the git sync container, I also have to set these in the "dags.gitSync" section like:

     securityContexts:
      container:
        capabilities:
          drop:
            - KILL
            - MKNOD
            - SYS_CHROOT

What you think should happen instead

When you set a global container security context, it should also apply to the git sync container

How to reproduce

The cluster I deploy to requires some container security context settings, so for me to reproduce I just set the global container security context and try to deploy, and if it won't let me deploy I look in the logs and it will say the git sync container does not have the required container security context.

Anything else

I looked through the 1.11.0 helm chart release notes and did not see this was fixed. Also I do not know if this is the intended way this is supposed to work. Just wanted to bring it to light!

Are you willing to submit PR?

Code of Conduct

JKrehling commented 4 months ago

I dont think this is a bug but its becoming more common to require

    allowPrivilegeEscalation: false
    capabilities:
      drop:
        - ALL
    privileged: false
    runAsNonRoot: true
    readOnlyRootFilesystem: true

On every container if possible but git-sync is different than the others because its writes to /etc/passwd on startup so its probably for this reason they left it separate.
https://github.com/kubernetes/git-sync

Its from this other project.
Most charts are putting these by default now and I think I might take a run at it and make an MR for that. Not sure what the outcome will be though.

JKrehling commented 4 months ago

@shahar1 Do you know if there is currently an ongoing plan to make airflow compatible with readOnlyRootFilesystem? And would there be any objection to adding the restrictive securityContexts by default?

potiuk commented 4 months ago

@shahar1 Do you know if there is currently an ongoing plan to make airflow compatible with readOnlyRootFilesystem? And would there be any objection to adding the restrictive securityContexts by default?

Just to add here - I don't think there are "plans" or roadmap (other than big Airflow 3 Airflow Improvement Proposals we have) - for things like this, if anyone wants to get this done, they are most welcome to implement it, second-best is finding somoene and making them implement it. Other than that, there is no-one who "knows" if other people have plans like that - generally if somone submits a change to make airflow works with readOnlyRootFilesystem and it will pass the review, then it will be done. But we (maintainers) do not know if any of the almost 3000 contributors will submit it.

So the best way to speed it up if someone needs it is to .... contribute it.

potiuk commented 4 months ago

I marked it as "good first issue" and hopefully that might attract somoene who will implement it.

JKrehling commented 4 months ago

Fair enough. I only ask because last time I went to submit a similar MR for another project there was already work being done to implement it.

If its not done by the time I get this working on my system I'll try and put together an MR

potiuk commented 4 months ago

Fair enough. I only ask because last time I went to submit a similar MR for another project there was already work being done to implement it.

Generally speaking any of us can do search and find out (in PRs or issues) - because absolutely no-one can keep it all in a head - this is why we have GitHub discussions/issues here - all open and searchable.

And there is notthing wrong with asking (opening a discussion for example) in general - but tasking a specific maintainer to give answer is likely not helping in getting an answer - because by doing it you limit your chances to get answer from somone who knows - because they will think the tagged person will .

But that person might or might not know anything about it, and might or not have time or might be on vacations or just busy at work - and you implicitly task (and delegate it to ) that person in looking for an answer for you if they do not know. That's not too nice for those people who often contribute here as volunteers. No bad feelings - just want to explain how it works here.

In other words - I am just saying - if you want to check if something is being done, best thing is to do search - here and in our wiki (see community link) and second best is ask a generic question without tagging anyone specific and hope somoene will be attracted and answer you.

If its not done by the time I get this working on my system I'll try and put together an MR

Cool !