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-3928] - feat: secrets integration #189

Merged
merged 78 commits into from
May 2, 2024
Merged

[DPE-3928] - feat: secrets integration #189

merged 78 commits into from
May 2, 2024

Conversation

marcoppenheimer
Copy link
Contributor

@marcoppenheimer marcoppenheimer commented Apr 11, 2024

Features

feat: zookeeper, peer and client data secrets supported

  • Includes abstracted unified unified secrets + peer data
  • More resilient relation interface with ZooKeeper to support zookeeper interface, and upcoming zookeeper_client interface, optionally using secrets

chore: bump data_interfaces to v34

  • Introduces a whole bunch of new stuff, predominantly unified secrets + peer data

Refactoring

chore: support tls + tls-ca and ca + ca-cert

  • Secret labels need to be >2 characters. For backwards compatibility, we now:
    • Look for ca-cert and ca in peer-data, cleaning up old ones when we get granted a new CA
    • Look for bool(tls-ca) from ZooKeeper, defaulting to original tls=enabled

refactor: align with ZooKeeper charm patterns

  • provider.update_connection_info() --> charm.update_client_data() with new state.clients + models.KafkaClient
  • literals.Substrate --> core.models.SUBSTRATES + literals.SUBSTRATE
  • state.broker --> state.unit_broker, mostly for clarity that this is 'the unit'

test: replace model_full_name with ops_test

  • Maybe it's easier to use, not sure, but removes amount of non-test code in tests

test: updating int-test helpers

  • Most pertinent change here is how we get Kafka + ZK data that both optionally use secrets, or Kafka provider data. Solved by getting every relation-data key and merging them with every secret key

test: sync unit-tests with K8s

  • I think they got a bit out of date, have updated and synced, especially the upgrade tests

Bug Fixes

fix: explicitly emit config_changed events where necessary

  • Before, we were using self._on_config_changed(event) whenever we wanted to do things
  • When config_changed deferred, it re-ran the whole event, which caused a whole lot of restarting issues and non-idempotent event handling and hard-to-grok event ordering
  • For example, we ran certificates-available, which called config_changed, which deferred. In certificates-available, we have a check that says 'If I already have a cert, restart'. So after we deferred the event, we re-run it, have a cert, and restart, which we may not have wanted
  • Explicitly emitting a config-changed event helps avoid issues like that, as only the 'changed' event gets deferred

chore: avoid ruff warnings

  • As ruff is still in active dev, need to update pyproject.toml to avoid warnings

test: add async sleeps to int-tests

  • It maybe a bug in the secrets implementation, but secrets seem to be really slow to update, or to trigger secret-changed events where before we had 'instant' relation-changed events
  • This has caused a bunch of instability in the int-tests, so adding fast-forward: asyncio.sleep has helped somewhat. Will keep looking at this

@marcoppenheimer marcoppenheimer marked this pull request as draft April 11, 2024 01:41
src/charm.py Outdated Show resolved Hide resolved
tests/integration/helpers.py Fixed Show fixed Hide fixed
tests/integration/helpers.py Fixed Show fixed Hide fixed
src/core/models.py Outdated Show resolved Hide resolved
@marcoppenheimer marcoppenheimer changed the title WIP [DPE-3928] - feat: secrets integration Apr 25, 2024
@marcoppenheimer marcoppenheimer marked this pull request as ready for review April 25, 2024 18:11
Copy link
Contributor

@Batalex Batalex left a comment

Choose a reason for hiding this comment

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

Mostly non-blocking nitpicks and questions. I have not reviewed the tests yet, I'll get back to them later

pyproject.toml Show resolved Hide resolved
src/core/cluster.py Outdated Show resolved Hide resolved
src/core/cluster.py Show resolved Hide resolved
src/core/cluster.py Show resolved Hide resolved
src/core/models.py Outdated Show resolved Hide resolved
src/core/models.py Outdated Show resolved Hide resolved
src/core/models.py Outdated Show resolved Hide resolved
Copy link
Contributor

@deusebio deusebio left a comment

Choose a reason for hiding this comment

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

Changes look mostly fine. I have a couple of points worth raising

  1. I'm a bit concerned about the update_client_data which seems out of place and a bit off the new pattern.
  2. Instantiation of KafkaClient using some "general" properties rather than the databag value.

Of the two, 1. is definitely more important than 2. But actually, these two may also nicely connect into one point, as I would suggest to refactor a bit the KafkaClient class such that:

  1. it aligns a bit closer with the actual data shared with client applications over the relations
  2. It also provides implementation of writing to the databag, therefore having in the update_client_data method, and not spread databag schema/information in the charm code

.github/workflows/ci.yaml Show resolved Hide resolved
src/charm.py Show resolved Hide resolved
src/core/cluster.py Outdated Show resolved Hide resolved
src/core/models.py Show resolved Hide resolved
src/core/models.py Show resolved Hide resolved
src/core/models.py Show resolved Hide resolved
src/core/models.py Show resolved Hide resolved
src/core/cluster.py Show resolved Hide resolved
src/core/models.py Show resolved Hide resolved
src/charm.py Show resolved Hide resolved
@deusebio
Copy link
Contributor

Just discussed offline with Marc. Actually I did not realized that this pattern is also like this in ZK, here. Although also Marc is not fond of this, we can just keep it as-is for the time being, but I would like to improve this in the next weeks when we are together in Madrid.

Copy link
Contributor

@zmraul zmraul left a comment

Choose a reason for hiding this comment

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

Very thorough! I stole some changes that you added back to the Karapace PRs :)

lib/charms/zookeeper/v0/client.py Outdated Show resolved Hide resolved
src/charm.py Show resolved Hide resolved
src/charm.py Show resolved Hide resolved
src/core/cluster.py Outdated Show resolved Hide resolved
src/core/models.py Show resolved Hide resolved
tests/integration/ha/continuous_writes.py Show resolved Hide resolved
@deusebio deusebio self-requested a review May 2, 2024 10:41
@marcoppenheimer marcoppenheimer merged commit 3e813ac into main May 2, 2024
16 checks passed
@marcoppenheimer marcoppenheimer deleted the feat/secrets branch May 2, 2024 17:36
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.

None yet

5 participants