-
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
Enable $web and disable anonymous blob access on rpversions container #4074
Conversation
5153021
to
e1adf8f
Compare
…rsions containers
Changes lgtm, haven't tested by running a local deploy. |
e1adf8f
to
fe64a96
Compare
Tested this deployment change in an int deployment: https://msazure.visualstudio.com/AzureRedHatOpenShift/_build/results?buildId=114252528&view=results. |
}, | ||
{ | ||
Resource: &mgmtstorage.BlobContainer{ | ||
Name: to.StringPtr("[concat(parameters('rpVersionStorageAccountName'), '/default/ocpversions')]"), |
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'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 comment
The 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 comment
The 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 comment
The 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.
I think we have some doc updates to go along with this. Will reach out on Slack. |
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.
My only other thought is regarding the minimum TLS version. Since we must support TLSv3, is there any reason the changes here won't work with v3 as well? I'm assuming not since all Azure services are supporting v3.
@@ -101,40 +101,16 @@ | |||
"sku": { | |||
"name": "Standard_LRS" | |||
}, | |||
"kind": "StorageV2", |
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).
Which issue this PR addresses:
Fixes ARO-14928
What this PR does / why we need it:
Updates our insertion of the deployed RP version into the rpversion storageaccount to use the static website hosting feature's
$web
container. This will allow us to disable anonymous blob access on the storage account generally, while still allowing access to content within the$web
container. We enable static web hosting on the storage account before writing the version file to the container, as it is not enabled by default, and depends on a few configuration parameters that were not originally set on these SAs.In order to do this, we also force an upgrade of the storage account to StorageV2 if necessary, as V2 is required for serving static content. We also update the minimum TLS version on the SA to 1.2. These changes are made to the applied ARM template.
We also replace our blobsClient used in this process with the newer azure-sdk-for-go's azblob client, allowing us to drop the older vendored blobclient entirely (this was its last usage in the RP codebase).
This will require corresponding changes to our consumption of this content, as the URL will change from a
.blob.core.windows.net
URL to.web.core.windows.net
.Test plan for issue:
StorageV2
$web
container$web
container can be accessed anonymously.The entire
saveversion
function could benefit from some dedicated unit tests, I will try to add these in a follow-up effort.Is there any documentation that needs to be updated for this PR?
No
How do you know this will function as expected in production?
Above testing