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.

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

Add sops file flag to diagnostics script #2250

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Add sops file flag to diagnostics script #2250

wants to merge 7 commits into from

Conversation

anders-elastisys
Copy link
Contributor

@anders-elastisys anders-elastisys commented Aug 28, 2024 •
edited
Loading

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?

Want to simplify the usage of the diagnostics script, the script will now by default import GPG keys from a file $CK8S_CONFIG_PATH/diagnostics_receiver.gpg and use their fingerprints if CK8S_PGP_FP is not already set.

Information to reviewers

I am planning to update the public docs to reflect this change.

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

@anders-elastisys anders-elastisys force-pushed the anders-elastisys/diagnostics-support-additional-sops-file branch from 823a5d0 to 50e02c3 Compare August 28, 2024 07:19
@anders-elastisys anders-elastisys requested review from cristiklein and a team August 28, 2024 07:39
@anders-elastisys anders-elastisys force-pushed the anders-elastisys/diagnostics-support-additional-sops-file branch from 50e02c3 to 231ded5 Compare August 28, 2024 08:31
@cristiklein
Copy link
Contributor

@anders-elastisys I guess you wanted some early feedback on your proposal.

Overall, I think this is a good idea. It would be great if we could bring the script to the point where the on-call person only needs to type:

./bin/ck8s diagnostics sc
./bin/ck8s diagnostics wc

The expression convention over configuration comes to my mind. The on-call person should be provided with "sensible defaults".

@anders-elastisys anders-elastisys force-pushed the anders-elastisys/diagnostics-support-additional-sops-file branch 2 times, most recently from 20d8c62 to 13414e1 Compare September 4, 2024 13:03
@anders-elastisys
Copy link
Contributor Author

@anders-elastisys I guess you wanted some early feedback on your proposal.

Overall, I think this is a good idea. It would be great if we could bring the script to the point where the on-call person only needs to type:

./bin/ck8s diagnostics sc
./bin/ck8s diagnostics wc

The expression convention over configuration comes to my mind. The on-call person should be provided with "sensible defaults".

Maybe 13414e1 is more in line with what we want? I removed the sops-config file flag and instead added so that by default the script will look for the file ${CK8S_CONFIG_PATH}/diagnostics_receiver.gpg containing GPG keys that are then imported to the operators keys and used by SOPS later in the script. It is still possible to set CK8S_PGP_FP manually, but I removed the support for using .sops.yaml file as I deemed it not necessary for what we want this command to achieve. Let me know what you think.

@anders-elastisys anders-elastisys marked this pull request as ready for review September 4, 2024 13:09
cristiklein
cristiklein approved these changes Sep 5, 2024
View reviewed changes
Copy link
Contributor

@cristiklein cristiklein left a comment

Choose a reason for hiding this comment

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

I like the transparency of the error message and that we provide the platform administrator with sane defaults.

salehsedghpour
salehsedghpour approved these changes Sep 24, 2024
View reviewed changes
bin/diagnostics.bash

echo -e "\tCK8S_PGP_FP=<fingerprint provided during onboarding> ./bin/ck8s diagnostics [sc|wc]\n" 1>&2
echo -e "\tIf in doubt, contact support@elastisys.com\n" 1>&2
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't our customer contact sme-support@elastisys.com?

simonklb
simonklb requested changes Sep 26, 2024
View reviewed changes
bin/diagnostics.bash
echo -e "\tEncrypting using the fingerprints: $fingerprints." 1>&2
echo -e "\tIf you want to send diagnostic data to Elastisys, make sure to do:\n" 1>&2
echo -e "\tIf you want to send diagnostic data to Elastisys, make sure to store GPG keys" 1>&2
echo -e "\tretrieved during onboarding in a file named:\n" 1>&2
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not appropriate for this repository since it is open source. We should not assume that Elastisys is owning the platform.

Copy link
Contributor Author

@anders-elastisys anders-elastisys Oct 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The notice was added when the script was first created, see the discussion here. Maybe @cristiklein can chip in regarding this?

bin/diagnostics.bash

echo -e "\tCK8S_PGP_FP=<fingerprint provided during onboarding> ./bin/ck8s diagnostics [sc|wc]\n" 1>&2
echo -e "\tIf in doubt, contact support@elastisys.com\n" 1>&2
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not appropriate for this repository since it is open source. We should not assume that Elastisys is owning the platform.

@Elias-elastisys Elias-elastisys force-pushed the eliasl/prom-query-diagnostics-script branch from 05df19f to 2257d0c Compare September 27, 2024 08:16
Base automatically changed from eliasl/prom-query-diagnostics-script to main September 27, 2024 08:37
@anders-elastisys anders-elastisys force-pushed the anders-elastisys/diagnostics-support-additional-sops-file branch from 13414e1 to 7dfc403 Compare October 15, 2024 11:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simonklb simonklb simonklb requested changes

@Elias-elastisys Elias-elastisys Elias-elastisys approved these changes

@salehsedghpour salehsedghpour salehsedghpour approved these changes

@cristiklein cristiklein cristiklein approved these changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@anders-elastisys @cristiklein @simonklb @salehsedghpour @Elias-elastisys