You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by "andrijapanicsb (via GitHub)" <gi...@apache.org> on 2026/10/03 17:37:39 UTC
[PR] Network: default egress policy Allow for Isolated networks on fresh installations (reopen of #13684) [cloudstack]
andrijapanicsb opened a new pull request, #14300:
URL: https://github.com/apache/cloudstack/pull/14300
### Description
This reopens #13684, which was merged as commit `e0f3006764d00f18a59945861b50bf55d01f64eb` and then reverted by #14116. The dev@ discussion on whether to change this default is still ongoing, so the change should not have landed on `main` yet. This PR restores the **identical** change for continued review (it is a revert of the revert — the diff is byte-for-byte the same as the merged #13684).
Reference:
- Original PR: #13684 (merged `e0f3006764`)
- Revert: #14116 (`8afd1148a2`)
### Background
Isolated guest networks created from the built-in default network offerings (`DefaultIsolatedNetworkOfferingWithSourceNatService` and `DefaultIsolatedNetworkOffering`) deny all egress by default, because they are seeded with a `NetworkOfferingVO` constructor that leaves `egressdefaultpolicy` at the Java primitive default (`false` = deny). Meanwhile `createNetworkOffering` without an explicit `egressdefaultpolicy` already defaults to **allow**, and VPC tiers (NetworkACL) allow egress out of the box. So the shipped defaults are inconsistent, and the default "simple Isolated network" is the surprising one — freshly deployed VMs cannot reach package mirrors, NTP, metadata, etc. until an allow-all egress rule is added.
### Changes
- `ConfigurationServerImpl` seeds the two built-in Isolated network offerings with egress default policy **Allow**, on **fresh installations only**.
- `AddNetworkOffering.vue` default flipped from `deny` to `allow`, so the UI no longer sends an explicit `deny` that overrides the `createNetworkOffering` API default.
- `NetworkOfferingVO.setEgressDefaultPolicy` setter.
- Unit tests: `ConfigurationServerImplTest` (seeding) and `CreateNetworkOfferingCmdTest` (API default).
### Backward compatibility
- **Existing deployments are untouched.** The default offerings are seeded once, at first initialization; upgrades do not re-seed them and `persistDefaultNetworkOffering` never updates existing rows. No data migration / upgrade SQL runs against existing `network_offerings` or networks.
- Only **fresh installations** get allow-by-default; the `egressdefaultpolicy` parameter and per-offering behaviour are unchanged.
- The `egress_default_policy` DB column default is intentionally not changed.
The one consideration is security posture on **new** clouds, which is exactly why there is an open dev@ discussion; this PR exists to keep the change reviewable while that concludes.
### Types of changes
- [ ] Breaking change (fix or feature that would cause existing functionality to change)
- [x] New feature (non-breaking change which adds functionality)
- [x] Enhancement (improves an existing feature and functionality)
- [ ] Cleanup (Code refactoring and cleanup, that may add test cases)
- [ ] build/CI
### How Has This Been Tested?
Change is identical to the previously merged #13684. `ConfigurationServerImplTest` and `CreateNetworkOfferingCmdTest` cover the seeded default and the API default respectively.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Network: default egress policy Allow for Isolated networks on fresh installations (reopen of #13684) [cloudstack]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #14300:
URL: https://github.com/apache/cloudstack/pull/14300#issuecomment-5971805569
## [Codecov](https://app.codecov.io/gh/apache/cloudstack/pull/14300?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 3.71%. Comparing base ([`1a48a87`](https://app.codecov.io/gh/apache/cloudstack/commit/1a48a87587c03472feefb081cfac71b2ebd0f407?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`a5a937a`](https://app.codecov.io/gh/apache/cloudstack/commit/a5a937a44e0496af287ceba03dc181122fb53d58?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
> :exclamation: There is a different number of reports uploaded between BASE (1a48a87) and HEAD (a5a937a). Click for more details.
>
> <details><summary>HEAD has 1 upload less than BASE</summary>
>
>| Flag | BASE (1a48a87) | HEAD (a5a937a) |
>|------|------|------|
>|unittests|1|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #14300 +/- ##
=============================================
- Coverage 19.91% 3.71% -16.21%
=============================================
Files 6373 487 -5886
Lines 577230 41992 -535238
Branches 70696 7942 -62754
=============================================
- Hits 114950 1558 -113392
+ Misses 449713 40208 -409505
+ Partials 12567 226 -12341
```
| [Flag](https://app.codecov.io/gh/apache/cloudstack/pull/14300/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [uitests](https://app.codecov.io/gh/apache/cloudstack/pull/14300/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `3.71% <ø> (ø)` | |
| [unittests](https://app.codecov.io/gh/apache/cloudstack/pull/14300/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/cloudstack/pull/14300?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Network: default egress policy Allow for Isolated networks on fresh installations (reopen of #13684) [cloudstack]
Posted by "andrijapanicsb (via GitHub)" <gi...@apache.org>.
andrijapanicsb commented on PR #14300:
URL: https://github.com/apache/cloudstack/pull/14300#issuecomment-5971943549
For reference, the dev@ discussion thread is here:
https://lists.apache.org/thread/t8kwx2y6yy38comy683v1pk688wxgrlk — **[DISCUSS] Default egress policy Allow for Isolated networks on fresh installations**
Let's keep this PR parked until that thread reaches consensus.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Network: default egress policy Allow for Isolated networks on fresh installations (reopen of #13684) [cloudstack]
Posted by "RosiKyu (via GitHub)" <gi...@apache.org>.
RosiKyu commented on PR #14300:
URL: https://github.com/apache/cloudstack/pull/14300#issuecomment-6295837906
Thank you for working on this @andrijapanicsb . Closing this in favor of https://github.com/apache/cloudstack/pull/14286.
Since there is no consensus in changing the default policy for egress rules, this PR will introduce a new offering for the desired policies without changing the default behavior
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Network: default egress policy Allow for Isolated networks on fresh installations (reopen of #13684) [cloudstack]
Posted by "andrijapanicsb (via GitHub)" <gi...@apache.org>.
andrijapanicsb commented on PR #14300:
URL: https://github.com/apache/cloudstack/pull/14300#issuecomment-5971758390
This reopens #13684, which was merged as `e0f3006764` and then reverted by #14116. The change here is identical to what was merged.
The default egress policy change is still under discussion on the dev@ mailing list, so this should **not** be merged until that thread reaches consensus — it shouldn't land on `main` ahead of the discussion. Reopening it here only so the code stays reviewable in the meantime.
cc @DaanHoogland for visibility.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Network: default egress policy Allow for Isolated networks on fresh installations (reopen of #13684) [cloudstack]
Posted by "andrijapanicsb (via GitHub)" <gi...@apache.org>.
andrijapanicsb commented on PR #14300:
URL: https://github.com/apache/cloudstack/pull/14300#issuecomment-5971883122
Thanks @DaanHoogland — ironically 😉 — for both merging *and* reverting this one, all without a heads-up. Jokes aside, no harm done and thanks for the quick revert. Maybe let's keep the merge button holstered until we're back from the beach next time 😄
On a serious note: I've reopened the identical change as #14300. It should stay parked until the dev@ [DISCUSS] thread concludes — please don't merge ahead of that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Network: default egress policy Allow for Isolated networks on fresh installations (reopen of #13684) [cloudstack]
Posted by "Damans227 (via GitHub)" <gi...@apache.org>.
Damans227 commented on code in PR #14300:
URL: https://github.com/apache/cloudstack/pull/14300#discussion_r4174923608
##########
ui/src/views/offering/AddNetworkOffering.vue:
##########
@@ -704,7 +704,7 @@ export default {
isolation: 'dedicated',
conservemode: true,
availability: 'optional',
- egressdefaultpolicy: 'deny',
+ egressdefaultpolicy: 'allow',
Review Comment:
this ui default also changes things for existing clouds, since admins making a new offering there now get allow unless they switch it. is that ok given the description says existing setups are untouched?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Network: default egress policy Allow for Isolated networks on fresh installations (reopen of #13684) [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on code in PR #14300:
URL: https://github.com/apache/cloudstack/pull/14300#discussion_r4181835915
##########
server/src/main/java/com/cloud/server/ConfigurationServerImpl.java:
##########
@@ -1067,6 +1067,12 @@ public void doInTransactionWithoutResult(TransactionStatus status) {
Network.GuestType.Isolated, true, false, false, false, true, false);
defaultIsolatedSourceNatEnabledNetworkOffering.setState(NetworkOffering.State.Enabled);
+ // Default egress policy is Allow on fresh installations, consistent with the
+ // createNetworkOffering API default (egressdefaultpolicy=true when not specified).
+ // Existing installations are not affected: this method only runs on first boot
+ // (guarded by the "init" configuration flag) and persistDefaultNetworkOffering()
+ // never updates an already existing offering.
Review Comment:
```suggestion
```
I don't think we need this comment in the code here. Maybe move it to docs or to a top of method comment? I think it would actually make sense to have a method per default offering create.
##########
server/src/main/java/com/cloud/server/ConfigurationServerImpl.java:
##########
@@ -1084,6 +1090,9 @@ public void doInTransactionWithoutResult(TransactionStatus status) {
false, true, null, null, true, Availability.Optional, null, Network.GuestType.Isolated, true, true, false, false, false, false);
defaultIsolatedEnabledNetworkOffering.setState(NetworkOffering.State.Enabled);
+ // This offering carries no Firewall service, so the flag is not enforced anywhere;
+ // it is set for consistency so API responses do not advertise a misleading Deny policy.
+ defaultIsolatedEnabledNetworkOffering.setEgressDefaultPolicy(true);
Review Comment:
```suggestion
defaultIsolatedEnabledNetworkOffering.setEgressDefaultPolicy(true);
```
no comment needed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Network: default egress policy Allow for Isolated networks on fresh installations (reopen of #13684) [cloudstack]
Posted by "RosiKyu (via GitHub)" <gi...@apache.org>.
RosiKyu commented on PR #14300:
URL: https://github.com/apache/cloudstack/pull/14300#issuecomment-6295882886
Thank you for working on this @andrijapanicsb . Closing this in favor of https://github.com/apache/cloudstack/pull/14286.
Since there is no consensus in changing the default policy for egress rules, this PR will introduce a new offering for the desired policies without changing the default behavior
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Network: default egress policy Allow for Isolated networks on fresh installations (reopen of #13684) [cloudstack]
Posted by "RosiKyu (via GitHub)" <gi...@apache.org>.
RosiKyu closed pull request #14300: Network: default egress policy Allow for Isolated networks on fresh installations (reopen of #13684)
URL: https://github.com/apache/cloudstack/pull/14300
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org