Skip to content

Network: default egress policy Allow for Isolated networks on fresh installations - #13684

Open
andrijapanicsb wants to merge 1 commit into
apache:mainfrom
andrijapanicsb:isolated-network-egress-default-allow
Open

Network: default egress policy Allow for Isolated networks on fresh installations#13684
andrijapanicsb wants to merge 1 commit into
apache:mainfrom
andrijapanicsb:isolated-network-egress-default-allow

Conversation

@andrijapanicsb

@andrijapanicsb andrijapanicsb commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Problem

On a fresh CloudStack installation, the built-in DefaultIsolatedNetworkOfferingWithSourceNatService offering — the Availability.Required offering that auto-creates a user's first isolated network at VM deployment — is persisted with egress_default_policy = false (Deny). The very first thing almost every new user hits is "my VMs have no internet", and the offering's egress policy is immutable after creation (updateNetworkOffering does not expose egressdefaultpolicy), so it cannot be fixed without DB surgery or recreating offerings and networks.

This Deny default is a historical artifact, inconsistent with the rest of the codebase:

  • The API already defaults to Allow. NetworkOfferingBaseCmd.getEgressDefaultPolicy() returns true when the egressdefaultpolicy parameter is omitted, so offerings created via API/cloudmonkey default to Allow. Only the seeded built-in offering and the UI form said Deny.
  • The UI contradicted the API. The Add Network Offering form preselected Deny and sent an explicit egressdefaultpolicy=false, overriding the API default for every offering created through the UI.
  • The Kubernetes service requires Allow. KubernetesClusterManagerImpl rejects non-VPC offerings with egress Deny and creates its own default offering with egressdefaultpolicy = true; the shipped default isolated offering cannot be used for CKS today.
  • VPC tiers have no baked-in Deny — allow-all (default_allow ACL) is a first-class choice.

What this PR changes

Scope is deliberately fresh installations and new offerings only:

  1. ConfigurationServerImpl.createDefaultNetworkOfferings(): both built-in Isolated offerings are now seeded with egress_default_policy = true (Allow). For DefaultIsolatedNetworkOffering (no SourceNat, no Firewall service) the flag is not enforced anywhere; it is set for consistency so API responses don't advertise a misleading Deny policy.
  2. AddNetworkOffering.vue: the Egress default policy radio now preselects Allow, aligning the UI with the documented API default. The parameter is still sent explicitly when Deny is selected; admins retain the choice per offering.
  3. NetworkOfferingVO: adds the missing setEgressDefaultPolicy(boolean) setter.
  4. Tests: ConfigurationServerImplTest verifies the seeded isolated offerings carry Allow (and that the shared offering seeding is unchanged); a new CreateNetworkOfferingCmdTest locks in the API behavior — Allow when the parameter is omitted, explicit Allow/Deny honored.

No global setting is introduced. Per-offering configurability already exists via the egressdefaultpolicy parameter, and a global setting would have had confusing dual semantics: consumed once at first-boot seeding (before an operator can realistically set it, since the configuration row does not exist yet at that point) versus consumed live by every later createNetworkOffering call — the two could silently disagree. It would also have applied to Shared/L2/VPC offering creation despite its Isolated-scoped name.

Backward compatibility — existing installations provably untouched

  • createDefaultNetworkOfferings() runs only on first boot, guarded by the init configuration flag in persistDefaultValues(). It never executes on an upgraded installation.
  • NetworkOfferingDaoImpl.persistDefaultNetworkOffering() is find-or-create by unique name; it never updates an existing row.
  • No upgrade SQL is shipped, deliberately. Egress enforcement is evaluated live from the offering row (offering.isEgressDefaultPolicy() in NetworkOrchestrator/CommandSetupHelper) on every VR rule programming, so flipping existing rows would silently change the egress behavior of every existing isolated network — including networks created after the upgrade from the pre-existing built-in offering. On upgraded clouds, the built-in offering (and all networks using it, old and new) therefore keeps Deny. Giving upgraded clouds an Allow default for new networks without touching existing ones requires a new/versioned built-in offering and is left as an explicit follow-up.
  • No schema change, no API change, no VR/systemvm change. The egress-Allow path (System rule → FW_EGRESS_RULES default ACCEPT, user rules inverted to DROP) is already exercised today by any admin-created Allow offering.

How to test

  1. Fresh install (or simulator): listNetworkOfferings name=DefaultIsolatedNetworkOfferingWithSourceNatServiceegressdefaultpolicy: true.
  2. Deploy a VM without a network → isolated network auto-created → on the VR, iptables -S FW_EGRESS_RULES shows default ACCEPT; guest has outbound connectivity with no egress rules configured.
  3. Add an egress rule → confirms inverted (DROP) rule semantics still apply.
  4. Upgrade an existing DB → select name, egress_default_policy from network_offerings where name like 'DefaultIsolated%'; unchanged (0 for pre-existing rows).
  5. UI → Add Network Offering with Firewall service checked → Egress default policy preselects Allow; selecting Deny still creates a Deny offering.
  6. Unit tests: ConfigurationServerImplTest#testCreateDefaultNetworkOfferingsSeedsIsolatedOfferingsWithEgressAllow, CreateNetworkOfferingCmdTest.

Alternatives considered

  • Global setting (network.isolated.default.egress.policy.allow, earlier revision of this PR): dropped for the lifecycle/naming reasons above.
  • Upgrade-time SQL flipping existing offerings: rejected — changes egress behavior of existing networks in the field.
  • A new/versioned Allow built-in offering for upgraded clouds (with the legacy Deny offering hidden from new network creation): viable follow-up PR, kept out of this one to keep the scope reviewable.

@codecov

codecov Bot commented Jul 24, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 80.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 19.69%. Comparing base (4f11707) to head (0bd4dec).

Files with missing lines Patch % Lines
...in/java/com/cloud/offerings/NetworkOfferingVO.java 66.66% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main   #13684      +/-   ##
============================================
+ Coverage     19.65%   19.69%   +0.03%     
- Complexity    19792    19801       +9     
============================================
  Files          6368     6368              
  Lines        574881   574886       +5     
  Branches      70351    70351              
============================================
+ Hits         112970   113200     +230     
+ Misses       449639   449414     -225     
  Partials      12272    12272              
Flag Coverage Δ
uitests 3.41% <ø> (ø)
unittests 20.96% <80.00%> (+0.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@andrijapanicsb

Copy link
Copy Markdown
Contributor Author

@blueorangutan package kvm

@blueorangutan

Copy link
Copy Markdown

@andrijapanicsb a [SL] Jenkins job has been kicked to build packages. It will be bundled with kvm SystemVM template(s). I'll keep you posted as I make progress.

@andrijapanicsb
andrijapanicsb requested a review from Pearl1594 July 24, 2026 04:27
@blueorangutan

Copy link
Copy Markdown

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 18655

@andrijapanicsb andrijapanicsb added this to the 4.24.0 milestone Aug 5, 2026
…nstallations

The built-in Isolated network offerings were seeded with
egress_default_policy = false (Deny), while:

- the createNetworkOffering API already defaults egressdefaultpolicy to true
  (Allow) when the parameter is omitted (NetworkOfferingBaseCmd),
- the UI's Add Network Offering form contradicted that API default by
  preselecting Deny and sending an explicit egressdefaultpolicy=false,
- the Kubernetes service rejects isolated offerings with egress Deny and
  creates its own default offering with egress Allow,
- VPC tiers have no such baked-in Deny: allow-all (default_allow ACL) is a
  first-class choice.

Seed both built-in Isolated offerings with egress Allow on fresh
installations, and align the UI form default with the existing API default.
For DefaultIsolatedNetworkOffering (no Firewall service) the flag is inert
and set only so API responses do not advertise a misleading Deny policy.
No global setting is introduced: per-offering configurability already exists
via the egressdefaultpolicy parameter, and a setting consumed once at
first-boot seeding but live for later createNetworkOffering calls would have
inconsistent lifecycle semantics.

Backward compatibility: createDefaultNetworkOfferings() only runs on first
boot (guarded by the 'init' configuration flag) and
persistDefaultNetworkOffering() is find-or-create by unique name - it never
updates an existing row. No upgrade SQL is shipped, deliberately: egress
enforcement reads the offering row live on every VR rule programming, so
flipping existing rows would change the behavior of existing networks.
Upgraded clouds keep Deny on the pre-existing built-in offering; an
Allow-by-default offering for new networks on upgraded clouds is left as a
follow-up (new/versioned offering).

Unit tests cover the fresh-install seeding values and the API default for
omitted/explicit egressdefaultpolicy.
@andrijapanicsb
andrijapanicsb force-pushed the isolated-network-egress-default-allow branch from b768d9a to 0bd4dec Compare August 5, 2026 02:53
@andrijapanicsb andrijapanicsb changed the title network: default egress policy 'allow' for Isolated networks (configurable) Network: default egress policy Allow for Isolated networks on fresh installations Aug 5, 2026
@andrijapanicsb

Copy link
Copy Markdown
Contributor Author

Reworked per review: dropped the global setting — it had inconsistent lifecycle semantics (read once at first-boot seeding, but live for every later createNetworkOffering call) and an Isolated-scoped name with API-wide effect.

What the PR does now: both built-in Isolated offerings are seeded with egress Allow on fresh installations only, and the UI form default is aligned with the existing API default (the UI was sending an explicit deny, overriding it). Added unit tests for the seeding values and the API default.

Upgrades remain untouched by construction — the seeding only runs on first boot and never updates an existing row, and no upgrade SQL is shipped on purpose. An Allow-by-default offering for new networks on upgraded clouds is a possible follow-up.

@andrijapanicsb

Copy link
Copy Markdown
Contributor Author

@blueorangutan package kvm

@weizhouapache

Copy link
Copy Markdown
Member

@andrijapanicsb
UI change is good.
for other changes, to be frank, I think this will cause problems to some users

@wido

wido commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

@andrijapanicsb UI change is good. for other changes, to be frank, I think this will cause problems to some users

What are the real problems for users? Change in behavior? Can't this be solved by a proper note in the changelog?

@wido wido left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

CLGTM and I think this is a sane change to be made.

Does need to be communicated properly

@weizhouapache

Copy link
Copy Markdown
Member

What are the real problems for users? Change in behavior? Can't this be solved by a proper note in the changelog?

I am not sure how many users pay close attention to the changelog notes.

For regular users, this change means they will need to adapt their existing workflow or usage habits.

For enterprise users, many of them create networks through automation tools, so they would need to update their code. If they manage multiple CloudStack deployments running different versions, supporting both the old and new behaviors adds extra complexity.

Unless there is a strong reason for the change or the impact is truly negligible, I would generally prefer to keep backward compatibility.

@andrijapanicsb

Copy link
Copy Markdown
Contributor Author

I would put this very boldly in the Release notes as the first item.

@weizhouapache from what I know - if default behaviour is Deny, and automation/human is then adding ANY SINGLE RULE, then that deny is NO MORE active and they can allow this or that, but they have to have a DENY at the end of the list of rules as a security thing - that being said - I'm sure 99% of their code will simply work out of the box, except cases where someone deployes VMs in Isolated network and does NOT ever touch the Egress rules - which is the 1% IMO of users (all VMs need to exit to Internet at least to some ports/or some networks). Can you confirm this with what you know about Isolated netowrk - I mean the behavior of when a single rule is added the default DENY doesn't apply any more?

I did think about that @weizhouapache for a bit, I'm the first one who doesn't want to break backward compatibility- but if it's a silent switch, which needs to be turned ON - effectively this PR will never really introduce changes/improvements in peoples environments.

I would just make sure we put it as the very first "what's changed" line in Release Notes, and in bold - with NOTE to users to consider if their automation will need prior things.

@andrijapanicsb

Copy link
Copy Markdown
Contributor Author

@blueorangutan test

@blueorangutan

Copy link
Copy Markdown

@andrijapanicsb a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests

@weizhouapache

Copy link
Copy Markdown
Member

I would put this very boldly in the Release notes as the first item.

@weizhouapache from what I know - if default behaviour is Deny, and automation/human is then adding ANY SINGLE RULE, then that deny is NO MORE active and they can allow this or that, but they have to have a DENY at the end of the list of rules as a security thing - that being said - I'm sure 99% of their code will simply work out of the box, except cases where someone deployes VMs in Isolated network and does NOT ever touch the Egress rules - which is the 1% IMO of users (all VMs need to exit to Internet at least to some ports/or some networks). Can you confirm this with what you know about Isolated netowrk - I mean the behavior of when a single rule is added the default DENY doesn't apply any more?

I did think about that @weizhouapache for a bit, I'm the first one who doesn't want to break backward compatibility- but if it's a silent switch, which needs to be turned ON - effectively this PR will never really introduce changes/improvements in peoples environments.

I would just make sure we put it as the very first "what's changed" line in Release Notes, and in bold - with NOTE to users to consider if their automation will need prior things.

@andrijapanicsb
If CloudStack were a new project, I would completely agree with your changes. However, CloudStack has been around for more than 15 years.

While the current default behavior may not be ideal, changing it now could introduce more disadvantages.

For users who simply allow VMs to access the entire Internet, the migration is relatively straightforward: retrieve the current default egress policy and add an egress rule that allows all traffic if needed.

However, for users who only allow VMs to access specific CIDRs or port ranges, the migration becomes more complicated. They would need to create a new network offering and migrate to it.

One possible approach is to introduce a new default network offering with the egress policy set to Allow. This would give new deployments the desired default behavior while preserving backward compatibility for existing users and deployments.

@andrijapanicsb

andrijapanicsb commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

@weizhouapache thing that there is a misunderstanding here: from what I know (worth testing....) the default DENY rule applies only until someone adds a very first egress rule (any) - after that - if they want to allow just some outgoing/egress traffic and notall, there is no more "default/failback" DENY rule exising, it has to be explicitly added.
So - any automoation that ever adds ANY egress rule to a Isolated network - will continue to work (assuming the default ALLOW also stops being there after a very first egress rule is added).

Does that makes sense? (needs testing/confirmation)
So automation will not be broken, that is what I am saying, and certainly., we don't touch existsing networks.
pls advise if I got it wrong 🙏

EDIT: this PR is only about non-VPC isolated networks, doesn't touch VPCs

@weizhouapache

Copy link
Copy Markdown
Member

@weizhouapache thing that there is a misunderstanding here: from what I know (worth testing....) the default DENY rule applies only until someone adds a very first egress rule (any) - after that - if they want to allow just some outgoing/egress traffic and notall, there is no more "default/failback" DENY rule exising, it has to be explicitly added.

afaik, if egress policy is DENY, the REJECT rule is always the last rule in the iptables chain, the other rules in the chain are ACCEPT-ed.
Similarly, if egress policy is ALLOW, the ACCEPT rule is the last while other rules are REJECT-ed.
I will test tomorrow.

@weizhouapache

Copy link
Copy Markdown
Member

@weizhouapache thing that there is a misunderstanding here: from what I know (worth testing....) the default DENY rule applies only until someone adds a very first egress rule (any) - after that - if they want to allow just some outgoing/egress traffic and notall, there is no more "default/failback" DENY rule exising, it has to be explicitly added.

afaik, if egress policy is DENY, the REJECT rule is always the last rule in the iptables chain, the other rules in the chain are ACCEPT-ed. Similarly, if egress policy is ALLOW, the ACCEPT rule is the last while other rules are REJECT-ed. I will test tomorrow.

confirmed.

(use the default network offering with egress policy=false)

  • no rules
root@r-628-VM:~# iptables-save |grep FW_
-A FORWARD -i eth0 -o eth2 -j FW_OUTBOUND
-A FW_EGRESS_RULES -j DROP
-A FW_OUTBOUND -m state --state RELATED,ESTABLISHED -j ACCEPT
-A FW_OUTBOUND -j FW_EGRESS_RULES
  • affer adding a rule to allow port 22 and 1.2.3.4/32
root@r-628-VM:~# iptables-save |grep FW_
-A FORWARD -i eth0 -o eth2 -j FW_OUTBOUND
-A FW_EGRESS_RULES -p tcp -m set --match-set destCidrIpset-971 dst -m tcp --dport 22 -j ACCEPT
-A FW_EGRESS_RULES -j DROP
-A FW_OUTBOUND -m state --state RELATED,ESTABLISHED -j ACCEPT
-A FW_OUTBOUND -j FW_EGRESS_RULES

@blueorangutan

Copy link
Copy Markdown

[SF] Trillian test result (tid-16720)
Environment: kvm-ol8 (x2), zone: Advanced Networking with Mgmt server ol8
Total time taken: 58931 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr13684-t16720-kvm-ol8.zip
Smoke tests completed. 156 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test Result Time (s) Test File

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants