Skip to content
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

[DPE-4189] Fix TLS double-restart + update charm components - move to DP lib #244

Merged
merged 69 commits into from
May 6, 2024

Conversation

Mehdi-Bendriss
Copy link
Contributor

@Mehdi-Bendriss Mehdi-Bendriss commented Apr 19, 2024

Issue

  • The tls-certificate-operator and tls v1 interface is deprecated
  • we're using many old versions of python dependencies and charm libs
  • multiple parts of the TLS integ. tests are not asserted / awaited / correct, effectively not validating the behavior of TLS
  • the CI/CD pipelines don't use DP workflows
  • every single new issue of certificate (internal / external) triggers a restart - during the same flow

Solution

  • migrate to the DP workflows
  • removes dependencies to the deprecated tls-certificate-operator + v1 interface
  • introduces the self-signed-certificates operator + v3 interface
  • upgrades charm libs
  • upgrade python dependencies
  • upgrade dp action libs to the latest
  • fix tests:
    • CSR retrievals from secrets
    • CA cert retrieval from units
    • missing asserts and awaits on coroutines
  • fix additional (double) TLS restart

This PR updates all dependencies of the charm:

  • migration to dp workflows
  • CI / CD workflows
  • python dependencies
  • charm libs
  • TLS operator references

@Mehdi-Bendriss Mehdi-Bendriss changed the title [DPE-4189] Fix TLS tests - update charm components - move to DP lib [DPE-4189] update charm components - move to DP lib May 2, 2024
@Mehdi-Bendriss Mehdi-Bendriss changed the title [DPE-4189] update charm components - move to DP lib [DPE-4189] Fix TLS double-restart + update charm components - move to DP lib May 2, 2024
MiaAltieri
MiaAltieri previously approved these changes May 2, 2024
.gitignore Outdated Show resolved Hide resolved
pyproject.toml Outdated
Comment on lines 24 to 29
ops = "^2.12.0"
pydantic = "^1.10.7" # cos_agent lib
cryptography = "^42.0.5" # tls_certificates lib v3
jsonschema = "^4.22.0" # tls_certificates lib v3
cosl = "^0.0.11" # loki_push_api
bcrypt = "^4.0.1"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as #244 (comment) about avoiding adding extra constraint beyond the lib's constraint

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bcrypt was already removed in a previous commit, or am I mistaken?

Copy link
Contributor

@carlcsaposs-canonical carlcsaposs-canonical left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(nits are non-blocking; 1 non-nit comment)

- tls-integration
- metrics-integration
name: ${{ matrix.tox-environments }}
name: Integration test charm | 3.1.7
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: ^

issues: write # Needed to create GitHub issue comment
issues: write # Needed to create GitHub issue comment
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: ^

pyproject.toml Outdated
Comment on lines 24 to 29
ops = "^2.12.0"
pydantic = "^1.10.7" # cos_agent lib
cryptography = "^42.0.5" # tls_certificates lib v3
jsonschema = "^4.22.0" # tls_certificates lib v3
cosl = "^0.0.11" # loki_push_api
bcrypt = "^4.0.1"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^

- name: Run tests
run: tox run -e unit

build:
name: Build "${{ matrix.path }}" charm
uses: canonical/data-platform-workflows/.github/workflows/[email protected]
strategy:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: ^

@Mehdi-Bendriss
Copy link
Contributor Author

Thanks @carlcsaposs-canonical - I believe all the non-nit feedback has been addressed (?) I did not address the nits I did not agree with

@carlcsaposs-canonical
Copy link
Contributor

carlcsaposs-canonical commented May 6, 2024

I believe all the non-nit feedback has been addressed

are the additional version constraints on charm lib dependencies intentional? (#244 (comment))

@Mehdi-Bendriss
Copy link
Contributor Author

Yes.

@Mehdi-Bendriss Mehdi-Bendriss merged commit 015a5de into 6/edge May 6, 2024
19 of 23 checks passed
@Mehdi-Bendriss Mehdi-Bendriss deleted the update-deps branch May 6, 2024 11:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants