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

Fix chart network policies definion and compliance #12009

Closed

Conversation

FloChehab
Copy link
Contributor

@FloChehab FloChehab commented Oct 31, 2020

Hello @mik-laj,

While I was adding support for an external redis to the chart I had issues with your latest changes regarding schema validation.

Here are the fixes.

Followup to #12003:

  • Some network policies & ingress are not valid
    against the jsonschema (empty values mostly)
  • Some network policies conditionnal definitions
    were incorrect

^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.

@boring-cyborg boring-cyborg bot added the area:helm-chart Airflow Helm Chart label Oct 31, 2020
{{ toYaml .Values.webserver.extraNetworkPolicies | indent 4 }}
ingress:
- from:
{{ toYaml .Values.webserver.extraNetworkPolicies | indent 6 }}
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand how to use option extraNetworkPolicies in NetworkPolicy. I tried a similar policy in helm / charts and couldn't find anything similar. It seems to me that we should think a little more about what these policies should do.
https://github.com/helm/charts/search?p=2&q=NetworkPolicy

Copy link
Contributor Author

@FloChehab FloChehab Nov 1, 2020

Choose a reason for hiding this comment

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

I am not that familiar with networkPolicies so no real opinion on this.

Also:

Copy link
Member

Choose a reason for hiding this comment

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

@thesuperzapper Do you have any experience with this? Can you help us please?

Copy link
Contributor

Choose a reason for hiding this comment

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

@mik-laj I don't know what the creator of this specific PR was trying to achieve, but from context, its just a way for users to specify things under from using a list value, for example:

webserver:
  extraNetworkPolicies:
    - ipBlock:
        cidr: 172.17.0.0/16
        except:
          - 172.17.1.0/24
    - namespaceSelector:
        matchLabels:
          project: myproject
    - podSelector:
        matchLabels:
          role: frontend

You can read about NetworkPolicy here: https://kubernetes.io/docs/concepts/services-networking/network-policies/

But this is just another case where there needs to be significant documentation updates for this chart before it's ready for normal users.

@kaxil
Copy link
Member

kaxil commented Nov 4, 2020

Can you please rebased your PR on latest Master since we have applied Black and PyUpgrade on Master.

It will help if your squash your commits into single commit first so that there are less conflicts.

Followup to apache#12003:
* Some network policies & ingress are not valid
  against the jsonschema (empty values mostly)
* Some network policies conditionnal definitions
  were incorrect
@FloChehab
Copy link
Contributor Author

Can you please rebased your PR on latest Master since we have applied Black and PyUpgrade on Master.

It will help if your squash your commits into single commit first so that there are less conflicts.

Done.

@stale
Copy link

stale bot commented Dec 26, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Dec 26, 2020
@FloChehab
Copy link
Contributor Author

Merged in master through #12010

@FloChehab FloChehab closed this Jan 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:helm-chart Airflow Helm Chart stale Stale PRs per the .github/workflows/stale.yml policy file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants