-
Notifications
You must be signed in to change notification settings - Fork 82
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
fetch resource group, vNet, subnet value from Infra status #492
Conversation
/test |
Testrun: e2e-csgc5 +---------------------+---------------------+-----------+----------+ | NAME | STEP | PHASE | DURATION | +---------------------+---------------------+-----------+----------+ | infrastructure-test | infrastructure-test | Succeeded | 34m39s | | bastion-test | bastion-test | Failed | 6m9s | +---------------------+---------------------+-----------+----------+ |
/test |
Testrun: e2e-57z2d +---------------------+---------------------+-----------+----------+ | NAME | STEP | PHASE | DURATION | +---------------------+---------------------+-----------+----------+ | infrastructure-test | infrastructure-test | Succeeded | 40m57s | | bastion-test | bastion-test | Succeeded | 11m7s | +---------------------+---------------------+-----------+----------+ |
/test |
Testrun: e2e-pl4xf +---------------------+---------------------+-----------+----------+ | NAME | STEP | PHASE | DURATION | +---------------------+---------------------+-----------+----------+ | infrastructure-test | infrastructure-test | Succeeded | 28m24s | | bastion-test | bastion-test | Succeeded | 12m56s | +---------------------+---------------------+-----------+----------+ |
@tedteng You need rebase this pull request with latest master branch. Please check. |
/test |
Testrun: e2e-hc56x +---------------------+---------------------+-----------+----------+ | NAME | STEP | PHASE | DURATION | +---------------------+---------------------+-----------+----------+ | infrastructure-test | infrastructure-test | Succeeded | 29m48s | | bastion-test | bastion-test | Succeeded | 10m36s | +---------------------+---------------------+-----------+----------+ |
rebased as request, Please reveiw and process the PR when available @gardener/gardener-extension-provider-azure-maintainers Thanks |
@@ -62,7 +70,15 @@ func (a *actuator) Reconcile(ctx context.Context, bastion *extensionsv1alpha1.Ba | |||
return err | |||
} | |||
|
|||
nic, err := ensureNic(ctx, factory, opt, publicIP) | |||
if infrastructureStatus.Networks.Layout != "SingleSubnet" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it really not work when there are multiple subnets? I would think that the bastion instance would be able to reach the other nodes, even if multiple subnets are used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't test it, the main Azure bastion PR base on a single Subnet mode is completed before multiple subnets are achieved #331. but we have an internal ticket about Bastion Azure extension multi zones support
in SRE backlog
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @kon-angelo, there are Firewall rules created together with Bastion that limit connections to the certain subnet. Originally there was a plan to implement multi-AZ right after the main Bastion development but priorities were changed, unfortunately...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just in case I dismiss our internal ticket because it's not visible to anyone...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
after rethinking, I think this PR also covers multiple subnets supported by a bastion extension now. By default, Azure subnets communicate with each other under the same virtual network. https://docs.microsoft.com/en-us/azure/virtual-network/virtual-networks-udr-overview#:~:text=Azure%20automatically%20routes%20traffic,of%20a%20virtual%20network
I am assuming infrastructureStatus.Networks.Subnets
always keep subnets which used by the gardener only, by default bastion uses the first subnet to create a bastion
nic, err := ensureNic(ctx, factory, opt, infrastructureStatus.Networks.VNet.Name, infrastructureStatus.Networks.Subnets[0].Name, publicIP) |
will remove the SingleSubnet
mode check logic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done 042d16b @kon-angelo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tedteng I checked it and as it is just removing the SingleSubnet
check is not enough.
Here is a solution in a draft PR i opened based on this one. You can do some testing yourself too to see that it works E2E.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, It works. I will close the PR.
Please process the draft PR you opened
How to categorize this PR?
/area ops-productivity
/kind bug
/platform azure
What this PR does / why we need it:
fetch vNet, subnet value from Infra status, instead of joint name value
Which issue(s) this PR fixes:
Fixes #485
Special notes for your reviewer:
current solution logic depends on SingleSubnet
gardener-extension-provider-azure/pkg/controller/bastion/actuator_reconcile.go
Lines 82 to 84 in 2ac7abf
Release note: