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.

Sign up for GitHub

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

Jump to bottom

`gateways.properties.tls.minimumProtocolVersion` is required when it shouldn't be #5682

Closed

willdavsmith opened this issue Jun 8, 2023 · 0 comments · Fixed by #5683

Labels

bug triaged

Contributor

willdavsmith commented Jun 8, 2023 •

edited by azure-boards bot

Loading

Not specifying minimumProtocolVersion causes a failed deployment due to a nil pointer dereference.

resource gateway 'Applications.Core/gateways@2022-03-15-privatepreview' = {
  name: 'tls-gtwy-gtwy'
  properties: {
    application: app.id
    tls: {
      certificateFrom: certificate.id
    } 
    routes: [
      {
        path: '/'
        destination: frontendRoute.id
      }
    ]
  }
}

The text was updated successfully, but these errors were encountered:

willdavsmith added the bug label

willdavsmith mentioned this issue

Fixing Gateways TLS minimumProtocolVersion bug #5683

Merged

5 tasks

shalabhms added public-release triaged labels

willdavsmith closed this as completed in #5683

willdavsmith added a commit that referenced this issue


          Fixing Gateways TLS minimumProtocolVersion bug (#5683)

7875c92

# Description

* Fixing a bug where not specifying TLS minumumProtocolVersion would
cause deployments to fail. It should default to `1.2`

## Issue reference

<!--
We strive to have all PR being opened based on an issue, where the
problem or feature have been discussed prior to implementation.
-->

Fixes: #5682 

## Checklist

Please make sure you've completed the relevant tasks for this PR, out of
the following list:

* [ ] Code compiles correctly
* [ ] Adds necessary unit tests for change
* [ ] Adds necessary E2E tests for change
* [ ] Unit tests passing
* [ ] Extended the documentation / Created issue for it

## Auto-generated summary

<!--
GitHub Copilot for docs will auto-generate a summary of the PR
-->

<!--
copilot:all
-->
### <samp>🤖 Generated by Copilot at 0b8569f</samp>

### Summary
📄🧪🐛

<!--
1.  📄 for adding and modifying files in the `testdata` folder.
2. 🧪 for adding and updating tests and test fixtures in the
`gateway_conversion_test.go` file.
3. 🐛 for fixing a bug and removing unused code in the
`gateway_conversion.go` file.
-->
This pull request updates the gateway conversion and testing code to
handle the changes in the versioned API model and the data model for the
gateway resource with TLS termination. It also fixes a bug in the
conversion logic and adds test cases for the scenario when the minimum
protocol version is not specified. It also modifies the bicep template
for deploying a gateway resource with TLS termination to remove the
`minimumProtocolVersion` property.

> _`gateway` resource_
> _TLS conversion fixed_
> _autumn bug hunting_

### Walkthrough
* Remove unused import statement from `gateway_conversion.go`
([link](https://github.com/project-radius/radius/pull/5683/files?diff=unified&w=0#diff-1b1effc4610d0933423711d51cfe96a6835aa5cc54cc922a0f5f3acb1e858c3fL28))
* Add nil check and default value for TLS minimum protocol version
conversion in `gateway_conversion.go`
([link](https://github.com/project-radius/radius/pull/5683/files?diff=unified&w=0#diff-1b1effc4610d0933423711d51cfe96a6835aa5cc54cc922a0f5f3acb1e858c3fL154-R163))
* Update expected TLS minimum protocol version from 1.2 to 1.3 in
`gateway_conversion_test.go` and test fixture files
([link](https://github.com/project-radius/radius/pull/5683/files?diff=unified&w=0#diff-cea35ff8fa843c923eab085ec2345fa1109868714214ff009efd93c832dfaa14L166-R166),
[link](https://github.com/project-radius/radius/pull/5683/files?diff=unified&w=0#diff-bd441cfd96ffeb8421c84f3b346d283b7d43995f296cc986503b5fa751974ba2L31-R31),
[link](https://github.com/project-radius/radius/pull/5683/files?diff=unified&w=0#diff-e68be033427ad045d11d1beb15409cf3a2230eb3cf6c753592868bb609d6db5bL42-R42))
* Add new test functions and fixture files for TLS minimum protocol
version conversion when not specified in `gateway_conversion_test.go`
([link](https://github.com/project-radius/radius/pull/5683/files?diff=unified&w=0#diff-cea35ff8fa843c923eab085ec2345fa1109868714214ff009efd93c832dfaa14R195-R252),
[link](https://github.com/project-radius/radius/pull/5683/files?diff=unified&w=0#diff-ae6b4a2f16cf22d0cc0de932d17ebe284142eff9d31f30e999674cd8fb5be010L1-R33),
[link](https://github.com/project-radius/radius/pull/5683/files?diff=unified&w=0#diff-79809ffd8c8a9ebacab2dd81a72546337d6be933b59c58c5e30d9ce71f441eb6L1-R44))
* Remove `minimumProtocolVersion` property from bicep template for
gateway deployment in `corerp-resources-gateway-tlstermination.bicep`
([link](https://github.com/project-radius/radius/pull/5683/files?diff=unified&w=0#diff-eef570133c134137d3c5b556de0fe53a586ae1dd477e5d9cdd46e21948c618f6L29-R29))

---------

Co-authored-by: Young Bu Park <youngp@microsoft.com>

nithyatsu pushed a commit that referenced this issue


          Fixing Gateways TLS minimumProtocolVersion bug (#5683)

# Description

* Fixing a bug where not specifying TLS minumumProtocolVersion would
cause deployments to fail. It should default to `1.2`

## Issue reference

<!--
We strive to have all PR being opened based on an issue, where the
problem or feature have been discussed prior to implementation.
-->

Fixes: #5682 

## Checklist

Please make sure you've completed the relevant tasks for this PR, out of
the following list:

* [ ] Code compiles correctly
* [ ] Adds necessary unit tests for change
* [ ] Adds necessary E2E tests for change
* [ ] Unit tests passing
* [ ] Extended the documentation / Created issue for it

## Auto-generated summary

<!--
GitHub Copilot for docs will auto-generate a summary of the PR
-->

<!--
copilot:all
-->
### <samp>🤖 Generated by Copilot at 0b8569f</samp>

### Summary
📄🧪🐛

<!--
1.  📄 for adding and modifying files in the `testdata` folder.
2. 🧪 for adding and updating tests and test fixtures in the
`gateway_conversion_test.go` file.
3. 🐛 for fixing a bug and removing unused code in the
`gateway_conversion.go` file.
-->
This pull request updates the gateway conversion and testing code to
handle the changes in the versioned API model and the data model for the
gateway resource with TLS termination. It also fixes a bug in the
conversion logic and adds test cases for the scenario when the minimum
protocol version is not specified. It also modifies the bicep template
for deploying a gateway resource with TLS termination to remove the
`minimumProtocolVersion` property.

> _`gateway` resource_
> _TLS conversion fixed_
> _autumn bug hunting_

### Walkthrough
* Remove unused import statement from `gateway_conversion.go`
([link](https://github.com/project-radius/radius/pull/5683/files?diff=unified&w=0#diff-1b1effc4610d0933423711d51cfe96a6835aa5cc54cc922a0f5f3acb1e858c3fL28))
* Add nil check and default value for TLS minimum protocol version
conversion in `gateway_conversion.go`
([link](https://github.com/project-radius/radius/pull/5683/files?diff=unified&w=0#diff-1b1effc4610d0933423711d51cfe96a6835aa5cc54cc922a0f5f3acb1e858c3fL154-R163))
* Update expected TLS minimum protocol version from 1.2 to 1.3 in
`gateway_conversion_test.go` and test fixture files
([link](https://github.com/project-radius/radius/pull/5683/files?diff=unified&w=0#diff-cea35ff8fa843c923eab085ec2345fa1109868714214ff009efd93c832dfaa14L166-R166),
[link](https://github.com/project-radius/radius/pull/5683/files?diff=unified&w=0#diff-bd441cfd96ffeb8421c84f3b346d283b7d43995f296cc986503b5fa751974ba2L31-R31),
[link](https://github.com/project-radius/radius/pull/5683/files?diff=unified&w=0#diff-e68be033427ad045d11d1beb15409cf3a2230eb3cf6c753592868bb609d6db5bL42-R42))
* Add new test functions and fixture files for TLS minimum protocol
version conversion when not specified in `gateway_conversion_test.go`
([link](https://github.com/project-radius/radius/pull/5683/files?diff=unified&w=0#diff-cea35ff8fa843c923eab085ec2345fa1109868714214ff009efd93c832dfaa14R195-R252),
[link](https://github.com/project-radius/radius/pull/5683/files?diff=unified&w=0#diff-ae6b4a2f16cf22d0cc0de932d17ebe284142eff9d31f30e999674cd8fb5be010L1-R33),
[link](https://github.com/project-radius/radius/pull/5683/files?diff=unified&w=0#diff-79809ffd8c8a9ebacab2dd81a72546337d6be933b59c58c5e30d9ce71f441eb6L1-R44))
* Remove `minimumProtocolVersion` property from bicep template for
gateway deployment in `corerp-resources-gateway-tlstermination.bicep`
([link](https://github.com/project-radius/radius/pull/5683/files?diff=unified&w=0#diff-eef570133c134137d3c5b556de0fe53a586ae1dd477e5d9cdd46e21948c618f6L29-R29))

---------

Co-authored-by: Young Bu Park <youngp@microsoft.com>

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