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

NetworkPolicy resources #778

Merged
merged 15 commits into from
Mar 17, 2022
Merged

Conversation

mathieu-benoit
Copy link
Contributor

@mathieu-benoit mathieu-benoit commented Mar 8, 2022

Fine granular NetworkPolicy resources for Online Boutique

Couple of thoughts and discussions about these new manifests:

  • Not in vanilla/default kubernetes-manifests folder, that's extra manifests for now.

Not part of this PR, but future considerations:

  • Use these NetworkPolicy resources CI process for each PR
  • Deploy in our own Production environment hosting https://onlineboutique.dev/ (not yet done, let's chat about that before)
  • Do the same approach for Bank of Anthos

@mathieu-benoit mathieu-benoit requested a review from a team as a code owner March 8, 2022 13:37
@github-actions
Copy link

github-actions bot commented Mar 8, 2022

🚲 PR staged at http://35.193.239.45

@mathieu-benoit mathieu-benoit marked this pull request as draft March 8, 2022 14:03
@github-actions
Copy link

github-actions bot commented Mar 8, 2022

🚲 PR staged at http://35.193.239.45

@github-actions
Copy link

github-actions bot commented Mar 8, 2022

🚲 PR staged at http://35.193.239.45

@github-actions
Copy link

github-actions bot commented Mar 8, 2022

🚲 PR staged at http://35.193.239.45

@NimJay
Copy link
Collaborator

NimJay commented Mar 14, 2022

Note from 1-on-1 with Mathieu:

@mathieu-benoit mathieu-benoit marked this pull request as ready for review March 14, 2022 23:37
@github-actions
Copy link

🚲 PR staged at http://35.193.239.45

Copy link
Collaborator

@NimJay NimJay left a comment

Choose a reason for hiding this comment

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

Thanks so much for adding these samples, @mathieu-benoit!

I've made a few comments.
I'm happy to apply these changes myself later today — if you'd like. :)

I've tested everything (in my own GKE cluster) except for step 5 of network-policies.md which I've proposed we shorten (i.e., just link to Exploring Anthos Service Mesh in the Cloud Console).

We'll talk further during our 1-on-1 today.

docs/manifests/network-policies/deny-all.yaml Outdated Show resolved Hide resolved
docs/network-policies.md Outdated Show resolved Hide resolved
docs/manifests/network-policies/shippingservice.yaml Outdated Show resolved Hide resolved
docs/network-policies.md Outdated Show resolved Hide resolved
docs/network-policies.md Outdated Show resolved Hide resolved
docs/network-policies.md Outdated Show resolved Hide resolved
docs/network-policies.md Outdated Show resolved Hide resolved
@github-actions
Copy link

🚲 PR staged at http://35.193.239.45

@mathieu-benoit
Copy link
Contributor Author

Thanks @NimJay for your comments, review and suggestions. I think I took into account all your points, please let me know if you see anything else. Thanks!

@github-actions
Copy link

🚲 PR staged at http://35.193.239.45

@github-actions
Copy link

🚲 PR staged at http://35.193.239.45

@github-actions
Copy link

🚲 PR staged at http://35.193.239.45

@mathieu-benoit
Copy link
Contributor Author

mathieu-benoit commented Mar 16, 2022

FYI: since we talked, I also took the initiative to prefix the files of these new resources by networkpolicy_. As a good practice by anticipating end-users having in same folder the kubernetes-manifests and these, and so, to avoid any conflict with file names. Also by anticipation of coming PRs with Sidecar, ServiceAccount, AuthorizationPolicy, etc. resources which will follow that same pattern. Hope that's ok with you and that makes sense?

@github-actions
Copy link

🚲 PR staged at http://35.193.239.45

1 similar comment
@github-actions
Copy link

🚲 PR staged at http://35.193.239.45

Copy link
Collaborator

@NimJay NimJay left a comment

Choose a reason for hiding this comment

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

Thanks for addressing all my concerns!
The new changes look good.

I like the idea of prefixing the files with something like "networkpolicy_", but I've gone ahead and used dashes to be consistent with file naming conventions of this repo and other repos.
I have also made some minors adjustments to the README.md file (see commits).

Everything looks good!
I'm so happy about this additional. Thanks again, @mathieu-benoit, for all your hard work here!
Approved.

@mathieu-benoit mathieu-benoit merged commit aff4091 into main Mar 17, 2022
@mathieu-benoit mathieu-benoit deleted the mathieu-benoit/network-policies branch March 17, 2022 19:17
@NimJay
Copy link
Collaborator

NimJay commented Mar 22, 2022

Hi @askmeegs,

This is the pull-request that we decided (during our meeting yesterday) to revert.
Reasons for reverting:

However, I'm realizing that the samples from this pull-request:

  1. are samples we were planning on applying to onlineboutique.dev.
  2. will not be included in a tutorial (unlike the samples from pull/781). So after we revert this pull-request, these samples will not be included in kubernetes-engine-samples or any other repo.

So before I revert, I want to know what you think about applying these manifests to onlineboutique.dev and moving them to /.github/release-cluster. For instance, do you think it's even worth adding this to onlineboutique.dev?

Note: I talked with @mathieu-benoit and we're both totally okay with deleting these samples forever, so please don't feel pressured to preserve these samples. :)
If you'd like, we can chat about it during our 1-on-1 tomorrow.
Thank you.

CC: @bourgeoisor

sitaramkm pushed a commit to sitaramkm/microservices-demo that referenced this pull request Mar 27, 2022
* Create network-policies.md

* Create deny-all.yaml

* netpol manifests

* refactoring with allow-egress-to-all

* docs

* more docs

* more granular egress netpol

* update docs

* Create redis.yaml

* docs/network-policies/*

* remove the network policy logging feature part

* we don't deploy yet these resources in our own cluster

* networkpolicy_*

* Use dashes for NetworkPolicy file names

Reason: consistency across repository.

* Fix minor issues in network-policies/README.md

Co-authored-by: Nim Jayawardena <nimjay@google.com>
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.

2 participants