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

Add changelog for release v0.41.0 #2271

Merged
merged 18 commits into from
Oct 2, 2024
Merged

Add changelog for release v0.41.0 #2271

merged 18 commits into from
Oct 2, 2024

Conversation

lunkan93
Copy link
Contributor

@lunkan93 lunkan93 commented Sep 3, 2024

Warning

This is a public repository, ensure not to disclose:

  • personal data beyond what is necessary for interacting with this pull request, nor
  • business confidential information, such as customer names.

What kind of PR is this?

Required: Mark one of the following that is applicable:

  • kind/feature
  • kind/improvement
  • kind/deprecation
  • kind/documentation
  • kind/clean-up
  • kind/bug
  • kind/other

Optional: Mark one or more of the following that are applicable:

Important

Breaking changes should be marked kind/admin-change or kind/dev-change depending on type
Critical security fixes should be marked with kind/security

  • kind/admin-change
  • kind/dev-change
  • kind/security
  • kind/adr

What does this PR do / why do we need this PR?

...

  • Fixes #

Information to reviewers

Checklist

  • Proper commit message prefix on all commits
  • Change checks:
    • The change is transparent
    • The change is disruptive
    • The change requires no migration steps
    • The change requires migration steps
    • The change upgrades CRDs
    • The change updates the config and the schema
  • Metrics checks:
    • The metrics are still exposed and present in Grafana after the change
    • The metrics names didn't change (Grafana dashboards and Prometheus alerts are not affected)
    • The metrics names did change (Grafana dashboards and Prometheus alerts were fixed)
  • Logs checks:
    • The logs do not show any errors after the change
  • Pod Security Policy checks:
    • Any changed pod is covered by Pod Security Admission
    • Any changed pod is covered by Gatekeeper Pod Security Policies
    • The change does not cause any pods to be blocked by Pod Security Admission or Policies
  • Network Policy checks:
    • Any changed pod is covered by Network Policies
    • The change does not cause any dropped packets in the NetworkPolicy Dashboard
  • Audit checks:
    • The change does not cause any unnecessary Kubernetes audit events
    • The change requires changes to Kubernetes audit policy
  • Falco checks:
    • The change does not cause any alerts to be generated by Falco
  • Bug checks:
    • The bug fix is covered by regression tests

@lunkan93 lunkan93 self-assigned this Sep 3, 2024
@lunkan93 lunkan93 changed the base branch from main to release-0.41 September 9, 2024 06:20
@davidumea
Copy link
Contributor

It would be nice to include this fix in the v0.41 release 5e1ce52

@lunkan93
Copy link
Contributor Author

lunkan93 commented Sep 11, 2024

@OlleLarsson @Xartos @viktor-f
This commit fixes the "Containers without x requests" panels showing negative numbers, because the query was taking number of running pods - number of containers with requests. The problem is that this breaks the node label filtering we added in this commit, as the kube_pod_container_info metric does not have the node label, which is what prompted the change of metric in that commit. So if you try to filter on specific nodes it will just show 0, while filtering on all nodes works. This problem is also present on Apps v0.40. I spent some time trying to fix it but I was not able to get a quck fix in, how should we procceed?

Edit: Should be fixed now with help from @viktor-f

@Elias-elastisys Elias-elastisys self-assigned this Sep 24, 2024
@lunkan93 lunkan93 marked this pull request as ready for review September 24, 2024 12:32
@viktor-f
Copy link
Contributor

viktor-f commented Sep 24, 2024

Could you as part of this release also fix this typo https://github.com/elastisys/compliantkubernetes-apps/blame/e5d2b971b21ff2b340911637ca76f6d366370b2e/helmfile.d/values/grafana/grafana-dashboards.yaml.gotmpl#L42 ?
It should be uptime-dashboard, the name is missing a a in dashboard.

changelog/0.41.md Outdated Show resolved Hide resolved
Comment on lines 1017 to 1021
buckets:
items:
properties:
destinationType:
const: "swift"
Copy link
Contributor

Choose a reason for hiding this comment

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

This will not capture the case fully, since if we have thanos with swift enabled, and we haven't set destinationType then it should default to swift.

We could potentially rework so the condition only looks for destinationType: swift for it to be simpler, but that is a different thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So we should check, if any bucket has destinationType: swift, or if swift for thanos is enabled and we haven't specified destinationType for the bucket (and the same thing for harbor) - and if any of these are true, we should perform the check for networkPolicies.rclone.sync.objectStorageSwift?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And if that is the default behavior we want, then we would need to incorporate a lot into the conditional set-me.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that is true. Though the new schema addition is 👌 I didn't know it was possible to express it that succinct.

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be to expand to something like this .harbor.persistence.type == "swift" and (.objectStorage.sync.buckets[] | select(.source == "*harbor*") | .destinationType != {}) but I'm not sure we want to expand the set-mes, we should probably work towards them being expressed in the schema only.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Ajarmar Do you have any thoughts on this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have a monstrosity of a conditional set-me that seems to be working. However in making it I have found some issues with the schema changes I made that I'll have to try and sort out.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Ajarmar Do you have any thoughts on this?

It would be nice to only have to specify the condition in one place rather than two as we do now (config and schema), but this seems like a bigger initiative than what should be done in this PR. For instance, a schema validation failure does not halt an automatic apps upgrade, but an unset set-me does, so this is something that would be missed if we were to only express the condition in the schema right now.

@lunkan93
Copy link
Contributor Author

lunkan93 commented Oct 2, 2024

Any final reviews before this is merged?

@lunkan93 lunkan93 merged commit d57a6d6 into release-0.41 Oct 2, 2024
12 checks passed
@lunkan93 lunkan93 deleted the staging-0.41.0 branch October 2, 2024 12:16
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.