-
Notifications
You must be signed in to change notification settings - Fork 178
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
Enable $web and disable anonymous blob access on rpversions container #4074
Changes from all commits
276248d
f75f3c3
fb6db60
4d555a5
fe64a96
5d4dad0
784965e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1518,36 +1518,13 @@ func (g *generator) rpACRRBAC() []*arm.Resource { | |
|
||
func (g *generator) rpVersionStorageAccount() []*arm.Resource { | ||
return []*arm.Resource{ | ||
g.storageAccount("[parameters('rpVersionStorageAccountName')]", &mgmtstorage.AccountProperties{ | ||
AllowBlobPublicAccess: to.BoolPtr(true), | ||
}, map[string]*string{ | ||
tagKeyExemptPublicBlob: to.StringPtr(tagValueExemptPublicBlob), | ||
}), | ||
{ | ||
Resource: &mgmtstorage.BlobContainer{ | ||
Name: to.StringPtr("[concat(parameters('rpVersionStorageAccountName'), '/default/rpversion')]"), | ||
Type: to.StringPtr("Microsoft.Storage/storageAccounts/blobServices/containers"), | ||
ContainerProperties: &mgmtstorage.ContainerProperties{ | ||
PublicAccess: mgmtstorage.PublicAccessContainer, | ||
}, | ||
}, | ||
APIVersion: azureclient.APIVersion("Microsoft.Storage"), | ||
DependsOn: []string{ | ||
"[resourceId('Microsoft.Storage/storageAccounts', parameters('rpVersionStorageAccountName'))]", | ||
}, | ||
}, | ||
{ | ||
Resource: &mgmtstorage.BlobContainer{ | ||
Name: to.StringPtr("[concat(parameters('rpVersionStorageAccountName'), '/default/ocpversions')]"), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm fairly confident we're not using OCPVersions at all, but can you confirm? I'm pretty certain they're stale. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For context/history (if i'm remembering correctly), we used this back when we had a single version install target and didn't support multiple versions. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I checked our prod container and the newest OCP version it had was a 4.11.z version, so I hope we still aren't using it for anything There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's also worth noting that the absence of these blobcontainer resources doesn't result in existing containers being deleted, they're just left alone (verified in int). If we do want to delete these resources we'll likely need to add an explicit fixup step during deployment. |
||
Type: to.StringPtr("Microsoft.Storage/storageAccounts/blobServices/containers"), | ||
ContainerProperties: &mgmtstorage.ContainerProperties{ | ||
PublicAccess: mgmtstorage.PublicAccessContainer, | ||
}, | ||
}, | ||
APIVersion: azureclient.APIVersion("Microsoft.Storage"), | ||
DependsOn: []string{ | ||
"[resourceId('Microsoft.Storage/storageAccounts', parameters('rpVersionStorageAccountName'))]", | ||
g.storageAccount( | ||
"[parameters('rpVersionStorageAccountName')]", | ||
&mgmtstorage.AccountProperties{ | ||
AllowBlobPublicAccess: to.BoolPtr(false), | ||
MinimumTLSVersion: mgmtstorage.MinimumTLSVersionTLS12, | ||
}, | ||
}, | ||
map[string]*string{}, | ||
), | ||
} | ||
} |
This file was deleted.
This file was deleted.
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.
Nit: You mentioned in the walk though that if there was ever a reason to use a different version or expand our usage of this we may need to change this. Is there a code comment we should leave for future generations?
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 think the required code change should be fairly self-explanatory, and would also be dependent on whatever the reason for changing/extending this would be. I also don't think there's actually any compelling reason for us to need any kind other than
StorageV2
in the future, but unforeseen circumstances might require that.Regarding TLS, this property is just setting the "minimum" supported TLS version, the highest we can even set this to is 1.2 currently anyways (https://learn.microsoft.com/en-us/azure/storage/common/transport-layer-security-configure-minimum-version?tabs=azure-cli#configure-the-minimum-tls-version-for-a-storage-account).