Merge libs

[Abuelodelanada]

Related to #63

This PR merge log_proxy lib into loki_push_api.

• LogProxyConsumer was moved to loki_push_api.py and LogProxyProviderremoved.
• LokiPushApiConsumer now inherits from ConsumerBase.
• ConsumerBase handles alert rules stuff.
• Relation data now is shared over App relation data bags

[mmanciop]

I'll look at this again when the alert rules support is in :(

[mmanciop]

When I use the library like this:

        self._log_proxy = LogProxyConsumer(
            charm=self,
            enable_syslog=True,
        )

I get:

TypeError: __init__() missing 2 required positional arguments: 'log_files' and 'container_name'

I really do not believe these arguments should be required. The documentation says that container_name can be omitted if there is only one workload container. And having to specify an empty array of log files feels wrong.

[mmanciop]

The LogProxyConsumer has a remarkably different API than LogPushApiConsumer, in that is does not emit events, and I really do not like this. I feel very strongly that the charm author experience should be virtually the same.

[Abuelodelanada]

The LogProxyConsumer has a remarkably different API than LogPushApiConsumer, in that is does not emit events, and I really do not like this. I feel very strongly that the charm author experience should be virtually the same.

LogProxyConsumer works this way since its first implementation in November. Let's follow this change in #70

[Abuelodelanada]

When I use the library like this:

        self._log_proxy = LogProxyConsumer(
            charm=self,
            enable_syslog=True,
        )

I get:

TypeError: __init__() missing 2 required positional arguments: 'log_files' and 'container_name'

I really do not believe these arguments should be required. The documentation says that container_name can be omitted if there is only one workload container. And having to specify an empty array of log files feels wrong.

Fixed

[rbarry82]

Easier for me to just commit a bunch of doc updates than comment on them one by one.

In general, though, the Provider and Consumer class names all appear to be backwards from our normal conventions unless I missed something.

I'm also a little concerned about the usage of event.app as a general pattern -- since it's only the URI and some basic data, it's probably ok, but event.unit may be better.

Similarly, the dict keys could be better:

(interface)"loki_push_api" -> relation.data[app]["loki_push_api"]["url"]

May be too many "loki_push_api", especially when that's also the name of a StoredState variable. Using loki_push_uri or similar may be less confusing for readers, but this is just a nit.

It is very concerning that the library, by default, tries to fetch Promtail from the internet, and that the documented suggestion "rebuild the charm". For end-users in airgapped environments, this is serious thing, and rebuilding the charm may not be practical (to say nothing of any hashing guarantees we may eventually give on "signed" charms). It would be far preferable to default to using a resource.

[mmanciop]

May be too many "loki_push_api", especially when that's also the name of a StoredState variable. Using loki_push_uri or similar may be less confusing for readers, but this is just a nit.

It actually started out as the URL being the value of the loki_push_api, but I changed it a while back because, when we start working on TLS, we will need a place to put CA and server certs. Having the loki_push_api key have a dict as a value is a basic form of guarantee of being able to evolve the relation contract in the future.

[mmanciop]

It is very concerning that the library, by default, tries to fetch Promtail from the internet, and that the documented suggestion "rebuild the charm". For end-users in airgapped environments, this is serious thing, and rebuilding the charm may not be practical (to say nothing of any hashing guarantees we may eventually give on "signed" charms). It would be far preferable to default to using a resource.

To my understanding, the library will try to fetch from the internet UNLESS you passed the promtail binary as resource to the charm when building it and releasing it. We have had a lot of discussions around this with @jnsgruk and @simskij back in the day, and it is the least of all evils. And yes, it makes for a poor UX with 1st party charms in air-gapped environments. I really wish we could specify resources as dependencies of charm libraries, and have those resources automatically added to the charm at build tine.

[mmanciop]

In general, though, the Provider and Consumer class names all appear to be backwards from our normal conventions unless I missed something.

No, I think they are fine:

- `LogProxyConsumer`: This object can be used by any Charmed Operator which needs to send telemetry, such as logs, to Loki through a Log Proxy by implementing the consumer side of the `loki_push_api` relation interface.

The direction of what is consumer and what is producer follows the roles of the relation: provides and requires. The Provider, because of how juju expose works, is the one with the server-socket: in this case, the Loki Push API server. I understand that it is counter-intuitive from the perspective of a log (the app produces the logs, after all), and I am open to discussing whether we should use different names for the Python API objects.

[jnsgruk]

It is very concerning that the library, by default, tries to fetch Promtail from the internet, and that the documented suggestion "rebuild the charm". For end-users in airgapped environments, this is serious thing, and rebuilding the charm may not be practical (to say nothing of any hashing guarantees we may eventually give on "signed" charms). It would be far preferable to default to using a resource.

To my understanding, the library will try to fetch from the internet UNLESS you passed the promtail binary as resource to the charm when building it and releasing it. We have had a lot of discussions around this with @jnsgruk and @simskij back in the day, and it is the least of all evils. And yes, it makes for a poor UX with 1st party charms in air-gapped environments. I really wish we could specify resources as dependencies of charm libraries, and have those resources automatically added to the charm at build tine.

Indeed - this is a bit of a mess that will eventually be fixed through a combination of subordinates on Kubernetes, and by being able to specify dependencies or resources for charm libraries.

@mmanciop is right though -- there should be no attempts to reach out to Github if a resource is attached, and we should make sure the documentation on how to achieve that is first class.

[Abuelodelanada]

Indeed - this is a bit of a mess that will eventually be fixed through a combination of subordinates on Kubernetes, and by being able to specify dependencies or resources for charm libraries.

@mmanciop is right though -- there should be no attempts to reach out to Github if a resource is attached, and we should make sure the documentation on how to achieve that is first class.

It works that way.

First we check if Promtail is attached. If it's attached we use that file.
If it's not attached, we check whether Promtail must be downloaded or not. We download Promtail if binary it's not in the container or the sha256 mismatch.

    def _obtain_promtail(self, event) -> None:
        """Obtain promtail binary from an attached resource or download it."""
        if self._is_promtail_attached():
            return

        if self._promtail_must_be_downloaded():
            self._download_and_push_promtail_to_workload(event)
        else:
            self._push_binary_to_workload()

Following in #70