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.

Sign up for GitHub

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

Jump to bottom

enable users to allow or deny the security rules #3878

Merged
merged 1 commit into from
Aug 31, 2023
Merged

enable users to allow or deny the security rules #3878

merged 1 commit into from
Aug 31, 2023

Conversation

nawazkh
Copy link
Member

@nawazkh nawazkh commented Aug 23, 2023 •
edited
Loading

What type of PR is this?
/kind feature

What this PR does / why we need it:

  • This PR will let users specify action on the securityRules.
    • Users can specify "Allow" or "Deny" values to the security rule.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #3877

Special notes for your reviewer:

  • cherry-pick candidate

TODOs:

  • squashed commits
  • includes documentation
  • adds unit tests

Release note:

Users can add `Deny` security rules. Security Rules are defaulted to "Allow" if unspecified.

@k8s-ci-robot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 23, 2023
@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Aug 23, 2023
@nawazkh nawazkh force-pushed the allow_or_deny_security_rules branch 2 times, most recently from e66c620 to 897338e Compare August 23, 2023 23:21
@codecov
Copy link

codecov bot commented Aug 23, 2023 •
edited
Loading

Codecov Report

Patch coverage: 66.66% and project coverage change: +0.13% 🎉

Comparison is base (e9b10cd) 55.55% compared to head (cc669c8) 55.69%.
Report is 21 commits behind head on main.

❗ Current head cc669c8 differs from pull request most recent head 5b370e5. Consider uploading reports for the commit 5b370e5 to get more accurate results

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #3878      +/-   ##
==========================================
+ Coverage   55.55%   55.69%   +0.13%     
==========================================
  Files         190      190              
  Lines       19532    19527       -5     
==========================================
+ Hits        10851    10875      +24     
+ Misses       8071     8039      -32     
- Partials      610      613       +3
Files Changed Coverage Δ
api/v1beta1/types.go 60.71% <ø> (ø)
azure/converters/rules.go 0.00% <0.00%> (ø)
azure/scope/cluster.go 56.59% <100.00%> (+0.11%) ⬆️

... and 15 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 23, 2023
@nawazkh nawazkh marked this pull request as ready for review August 23, 2023 23:52
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 23, 2023
@k8s-ci-robot k8s-ci-robot requested a review from mboersma August 23, 2023 23:52
@nawazkh
Copy link
Member Author

nawazkh commented Aug 23, 2023

I will squash the commits once reviewed :)

@nawazkh
Copy link
Member Author

nawazkh commented Aug 23, 2023

/cc @CecileRobertMichon @nojnhuh

@nawazkh nawazkh force-pushed the allow_or_deny_security_rules branch from db82bc8 to f07e0fd Compare August 24, 2023 00:18
@mboersma mboersma added this to the v1.11 milestone Aug 24, 2023
willie-yao
willie-yao reviewed Aug 24, 2023
View reviewed changes
api/v1beta1/types.go Outdated Show resolved Hide resolved
azure/converters/rules.go Show resolved Hide resolved
@nawazkh nawazkh force-pushed the allow_or_deny_security_rules branch 2 times, most recently from cc669c8 to e3696bd Compare August 30, 2023 22:43
@nawazkh nawazkh changed the title add action field to the securityRule struct enable users to allow or deny the security rules Aug 30, 2023
@nawazkh nawazkh requested a review from willie-yao August 30, 2023 22:46
@nawazkh
Copy link
Member Author

nawazkh commented Aug 30, 2023

/test pull-cluster-api-provider-azure-windows-containerd-upstream-with-ci-artifacts

willie-yao
willie-yao approved these changes Aug 30, 2023
View reviewed changes
Copy link
Contributor

@willie-yao willie-yao left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 30, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: d413f8a49d60f0d9a16b03b09b9ea68d2c4c81b9

CecileRobertMichon
CecileRobertMichon reviewed Aug 31, 2023
View reviewed changes
Copy link
Contributor

@CecileRobertMichon CecileRobertMichon left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CecileRobertMichon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 31, 2023
@nawazkh
Copy link
Member Author

nawazkh commented Aug 31, 2023 •
edited
Loading

/hold
Need to update the CRDs and release note section that we are defaulting SecurityRule.Action=allow if unspecified.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 31, 2023
@nawazkh
Users can add "Allow" or "Deny" action to the security rules
5b370e5 
- Users can add "Allow" or "Deny" action to the security rules of the Azure Network Security Group (NSG) that is associated with the Azure Cluster.
@nawazkh nawazkh force-pushed the allow_or_deny_security_rules branch from e3696bd to 5b370e5 Compare August 31, 2023 18:27
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 31, 2023
@nawazkh
Copy link
Member Author

nawazkh commented Aug 31, 2023

Sorry about the last minute hold. I wanted to update the CRDs and release note section that we are defaulting SecurityRule.Action=allow if unspecified.
/unhold

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 31, 2023
@CecileRobertMichon
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 31, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: d3f0542eeb2befa10947f169d1747b74c3937dc8

@nawazkh
Copy link
Member Author

nawazkh commented Aug 31, 2023

pull-cluster-api-provider-azure-test — Pod got deleted unexpectedly 

🤔

@CecileRobertMichon
Copy link
Contributor

/retest

likely a prow flake

@k8s-ci-robot k8s-ci-robot merged commit bca7c89 into kubernetes-sigs:main Aug 31, 2023
@nawazkh nawazkh deleted the allow_or_deny_security_rules branch August 31, 2023 20:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marosset marosset Awaiting requested review from marosset

@sonasingh46 sonasingh46 Awaiting requested review from sonasingh46

@mboersma mboersma Awaiting requested review from mboersma

@nojnhuh nojnhuh Awaiting requested review from nojnhuh

@CecileRobertMichon CecileRobertMichon Awaiting requested review from CecileRobertMichon

@willie-yao willie-yao Awaiting requested review from willie-yao

Assignees

@CecileRobertMichon CecileRobertMichon

@willie-yao willie-yao

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
CAPZ Planning
Archived in project
Milestone
v1.11
Development

Successfully merging this pull request may close these issues.

SecurityRules should allow user to specify allow/deny rule
5 participants
@nawazkh @k8s-ci-robot @CecileRobertMichon @willie-yao @mboersma