-
Notifications
You must be signed in to change notification settings - Fork 188
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
Add support for ApplicationGateway #3176
Conversation
v2/api/network/customizations/network_security_group_extension_test.go
Outdated
Show resolved
Hide resolved
│ ├── Owner: github.com/Azure/azure-service-operator/v2/api/resources/v1apiv20191001.ResourceGroup | ||
│ ├── Spec: Object (37 properties) | ||
│ │ ├── AuthenticationCertificates: Object (2 properties)[] | ||
│ │ │ ├── Data: *string |
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.
Is this just a public key, or does it contain private details?
It looks like probably just public data based on the documentation, in which case this is fine.
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.
This is the public key certificate that is uploaded to enable mtls on the application gateway, this is used by the app gateway to trust the client. It not contains sensitive data of the users.
}, | ||
} | ||
publicIPAddress := newPublicIp(tc, testcommon.AsOwner(rg)) | ||
tc.CreateResourceAndWait(publicIPAddress) |
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.
minor, you can combined all of these into a single create+wait
tc.CreateResourcesAndWait(publicIPAddress, vnet, subnet)
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.
fixed
v2/samples/network/v1api20220701/v1api20220701_applicationgateway.yaml
Outdated
Show resolved
Hide resolved
- name: app-gw-http-listner-1 | ||
frontendIPConfiguration: | ||
reference: | ||
armId: /subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/<rgName>/providers/Microsoft.Network/applicationGateways/<appgatewayname>/frontendIPConfigurations/<feIpConfigName> |
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.
You can replace <rgname>
with aso-sample-rg
sincethat's the sample rg which the sample is written in the context of.
Same with appgatewayname
, it can be aso-sample-application-gateway
, the name of this application gateway. Use the actual feIpCOnfigName
too.
Same for all the refs below as well.
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.
fixed
v2/samples/network/v1api20220701/v1api20220701_applicationgateway.yaml
Outdated
Show resolved
Hide resolved
armId: /subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/<rgName>/providers/Microsoft.Network/applicationGateways/<appgatewayname>/frontendIPConfigurations/<feIpConfigName> | ||
frontendPort: | ||
reference: | ||
armId: /subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/<rgName>/providers/Microsoft.Network/applicationGateways/<appgatewayname>/frontendPorts/<fePortName> |
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.
@super-harsh, is this going to pass sample validation? Do we deal with swapping subid into raw ARM refs?
v2/internal/controllers/crd_networking_applicationgateway_test.go
Outdated
Show resolved
Hide resolved
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.
Still a few cleanup items required (see comments by @matthchr) - including more reliance on Go type inference, which will clean up the test in controllers
some. I'll have a look at the test and work out what the failure means.
v2/azure-arm.yaml
Outdated
@@ -1561,6 +1561,51 @@ objectModelConfiguration: | |||
$exportAs: VirtualNetworksVirtualNetworkPeering | |||
$supportedFrom: v2.0.0-alpha.1 | |||
2022-07-01: | |||
ApplicationGateway: | |||
$export: true | |||
$supportedFrom: v2.3.0 |
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 don't think this will make it for v2.3.0.
$supportedFrom: v2.3.0 | |
$supportedFrom: v2.4.0 |
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.
fixed in updated commit
{ | ||
//KeyVaultSecretId: to.Ptr[string]("https://keyvaultname.vault.azure.net/secrets/secretname"), | ||
Name: to.Ptr[string](subResname), | ||
Data: to.Ptr[string](`MIIKEQIBAzCCCdcGCSqGSIb3DQEHAaCCCcgEggnEMIIJwDCCBHcGCSqGSIb3DQEH |
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.
If this is an embedded certificate, what's its expiry?
The test should either be self-contained (creating its own certificate, garanteeing that it's not stale), or at the very least there should be a comment here indicating when the certificate will expire so that a future dev knows what needs doing when the test fails.
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.
We're keen to get this PR merged, it's looking very close to finished.
There are just two items outstanding:
- The signature of
getFrontendPortsARMID()
needs to be modified to avoid the brittle nature of passing around arbitrary maps. - We can't commit secrets to the repo, so we need to tidy up the use of certificates.
I believe we have infrastructure available through the TestContext (tc
) to create the required secrets on the fly, so that nothing needs to be hard coded. If you find the capability you need is missing, let us know and we'll fill in the gap for you.
return subRes, subResARMID | ||
} | ||
|
||
func getFrontendPortsARMID(tc *testcommon.KubePerTestContext, rg *resources.ResourceGroup, params map[string]string) (string, error) { |
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.
As previously highlighted by @matthchr this method signature needs to be changed - passing around parameter in a map like this is brittle, creating a landmine for future maintainers where a simple change causes very non-obvious errors. We strongly prefer for problems to be identified by the compiler where possible.
There are two good alternatives.
Option 1: Replace the map with four parameters like this:
func getFrontendPortsARMID(
tc *testcommon.KubePerTestContext,
rg *resources.ResourceGroup,
type1 string,
value1 string,
type2 string,
value2 string,
) (string, error) {
calls would change to this form:
subResARMID, err := getFrontendPortsARMID(tc, rg, "applicationGateways", appGatewayName, "backendAddressPools", subResname)
Option 2: Replace the map with a varidic parameter:
func getFrontendPortsARMID(
tc *testcommon.KubePerTestContext,
rg *resources.ResourceGroup,
params ...string,
) (string, error) {
This would allow any number of parameters, so you might want to include a check to panic if someone passes an odd number.
Calling would be the same:
subResARMID, err := getFrontendPortsARMID(tc, rg, "applicationGateways", appGatewayName, "backendAddressPools", subResname)
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.
fixed with option 1. @theunrepentantgeek please review
v2/internal/tests/appgwcert.pfx
Outdated
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.
We don't want to have secrets committed to the repo, for all the usual policy reasons. I expect this would be promptly flagged as a security risk regardless of whether it's used elsewhere.
Remove this file.
v2/internal/tests/appgwcert64.pfx
Outdated
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.
Remove this file for the same reasons as appgwcert.pfx
.
v2/samples/network/v1api20220701/v1api20220701_applicationgateway.yaml
Outdated
Show resolved
Hide resolved
v2/samples/network/v1api20220701/v1api20220701_applicationgateway.yaml
Outdated
Show resolved
Hide resolved
If you're busy @vimorra and don't have the time to finish updating based on our feedback, we can pick this up and get it over the line. No promises that it makes it into 2.4.0 but we'll try. |
yes I will do my best, fixed the comment 1 about the signature of getFrontendPortsARMID. I will try to fix the certificate issue by adding the isSecret |
not clear which option you are suggesting if create the certificate in an automated way or mark the attribute as "isSecret" ? in any case con you provide an example @theunrepentantgeek |
I went back and had another look, and it's actually both of the issues you've identified. The issue I was referring to above is about hard coded certificates:
Checking certificates into repos is a security issue even if they only contain public keys. They also expire, which will result in the test failing in the future. The second issue is the This will have two benefits.
We believe there's infrastructure available on the |
@matthchr, @super-harsh and @theunrepentantgeek I need your support to understand how to generate an on the fly ssl certificate.
|
@vimorra - I'm looking into generating the SSL certificate; hoping to put together a sample for ASO users that will be relevant to your needs too. I'll update back here as that progresses. |
Use HTTP protocol for Application Gateway in controller test to avoid certs and fix sample
Hi @theunrepentantgeek and @matthchr with the support of @super-harsh the PR should be ready to be merged |
@vimorra An easy way to deal with these conflicts is to merge |
…e-service-operator into feature-app-gateway
…into feature-app-gateway
Codecov Report
@@ Coverage Diff @@
## main #3176 +/- ##
==========================================
- Coverage 54.32% 54.21% -0.12%
==========================================
Files 1566 1570 +4
Lines 655434 663722 +8288
==========================================
+ Hits 356073 359844 +3771
- Misses 241588 245516 +3928
- Partials 57773 58362 +589
|
/ok-to-test sha=d1ebad0 |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
Closes #2990
What this PR does / why we need it:
Add support for Application gateway service
Special notes for your reviewer:
Still working on the sample
How does this PR make you feel:
If applicable: