Skip to content
This repository was archived by the owner on Mar 10, 2023. It is now read-only.

Fix network policy #667

Merged
merged 2 commits into from
Sep 17, 2020
Merged

Fix network policy #667

merged 2 commits into from
Sep 17, 2020

Conversation

wilsonianb
Copy link
Contributor

@wilsonianb wilsonianb commented Sep 10, 2020

Description

Fixes #669 by giving audit-event access to gateway since echo function is the default slack_url.
This would need to be reflected in ofc-bootstrap's stack.yml (as should 2c836f3).

Re-adds support for legacy ingress nginx network policy label (1713d32).

How Has This Been Tested?

Tested on k3s with calico

k3sup install --ip $IP --user $USER --k3s-extra-args "--flannel-backend=none --cluster-cidr=192.168.0.0/16 --no-deploy traefik"
kubectl apply -k github.com/wilsonianb/calico-k3s

Followed ofc-bootstrap user guide (without oauth) with network policies enabled and openfaas_cloud_version set to 0.14.1

Used pre-release ofc-bootstrap v0.9.6 with role: openfaas-system label added to audit-event in templates/stack.yml.

GitHub events no longer time out with this PR change.

Ran helm chart tests with go test in $GO_PATH/src/openfaas-cloud/chart/test/ and ran diff on chart/test/tmp/openfaas-cloud/templates/network-policy/ns-openfaas-net-policy.yml and yaml/network-policy/ns-openfaas-net-policy.yml files.

How are existing users impacted? What migration steps/scripts do we need?

Should only help OpenFaaS Cloud cluster administrators who had network policies previously enabled and re-run ofc-bootstrap.

Checklist:

I have:

  • updated the documentation and/or roadmap (if required)
  • read the CONTRIBUTION guide
  • signed-off my commits with git commit -s
  • added unit tests

Copy link
Member

@alexellis alexellis left a comment

Choose a reason for hiding this comment

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

Couple of comments

- namespaceSelector: {}
podSelector:
matchLabels:
app: nginx-ingress
Copy link
Member

Choose a reason for hiding this comment

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

Why was this removed? Shouldn't it be ingress-nginx now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I relocated it to edge-router and gateway specific policies since afaict those are the only openfaas pods ingress nginx needs to access.

- namespaceSelector: {}
podSelector:
matchLabels:
app.kubernetes.io/name: ingress-nginx
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to keep both for legacy users? ingress-nginx and nginx-ingress?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

re-added the old label
95ac2e4

@alexellis
Copy link
Member

Please could you raise an issue with the issues you've found?

The Nginx Ingress label name change is here:
1713d32

As I mentioned on Slack, I think it will take me a day away from work to verify this PR, so whatever we can do to make that smoother would go a long way.

@wilsonianb wilsonianb marked this pull request as draft September 11, 2020 23:10
audit-event needs access to gateway with echo function as the
default slack_url

Fixes openfaas#669

Signed-off-by: Brandon Wilson <[email protected]>
Update helm chart network policy.

Signed-off-by: Brandon Wilson <[email protected]>
@wilsonianb wilsonianb marked this pull request as ready for review September 12, 2020 00:34
Copy link
Contributor

@Waterdrips Waterdrips left a comment

Choose a reason for hiding this comment

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

LGTM

@alexellis
Copy link
Member

@wilsonianb did you get an issue raised? If so please link it back here through adding a comment or editing its description - GitHub will work out the rest.

Copy link
Member

@alexellis alexellis left a comment

Choose a reason for hiding this comment

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

Approved

@alexellis alexellis merged commit 508951c into openfaas:master Sep 17, 2020
@alexellis
Copy link
Member

There was a much bigger PR I remembered, was that not needed? Can you link it here if it is still around?

@wilsonianb
Copy link
Contributor Author

here's the original changeset
master...wilsonianb:og-net-policy
This ended up being much smaller since:

  • I realized metrics already had the openfaas-system role (It just isn't reflected yet in ofc-bootstrap's stack.yml)
  • I opted to stick with the openfaas-system role model, instead of adding additional network policy rules (which would help constrain pods to only access exactly what they need, but would likely be harder to maintain)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GitHub events time out with network policy
3 participants