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 optional TLS certificate checking when talking to Proxmox #336

Merged
merged 3 commits into from
Dec 9, 2024

Conversation

Luzifer
Copy link
Contributor

@Luzifer Luzifer commented Nov 26, 2024

Issue #, if available: #192

Description of changes:

  • Add environment variables / flags to main process to enable certificate checking / providing root-ca when talking to Proxmox
  • Add secret keys for the Proxmox credential secret to enable certificate checking / providing root-ca when talking to Proxmox
  • Retain default (TLS-Insecure) in order not to introduce a breaking change (RFC)

Testing performed:

@wikkyk wikkyk added this to the v0.7.0 milestone Nov 26, 2024
Copy link
Collaborator

@65278 65278 left a comment

Choose a reason for hiding this comment

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

Couple of questions, PR looks good unto itself (minus the boolean cast).
On top of the questions, we're missing documentation and work on how to map secrets as files into a deployment into SSL_CERT_DIR. That could be scoped to a different PR.

cmd/main.go Outdated Show resolved Hide resolved
cmd/main.go Outdated Show resolved Hide resolved
cmd/main.go Show resolved Hide resolved
pkg/scope/cluster.go Outdated Show resolved Hide resolved
pkg/scope/cluster.go Outdated Show resolved Hide resolved
65278
65278 previously approved these changes Dec 4, 2024
Copy link
Collaborator

@65278 65278 left a comment

Choose a reason for hiding this comment

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

I'm okay with this going in. Do or do not implement checking on whether data["insecure"] exists at your own leisure.

@wikkyk wikkyk deleted the branch main December 6, 2024 10:21
@wikkyk wikkyk closed this Dec 6, 2024
@wikkyk wikkyk reopened this Dec 6, 2024
@wikkyk wikkyk changed the base branch from 0.7 to main December 6, 2024 10:29
@wikkyk wikkyk dismissed 65278’s stale review December 6, 2024 10:29

The base branch was changed.

@Luzifer Luzifer marked this pull request as ready for review December 9, 2024 11:11
Copy link

sonarqubecloud bot commented Dec 9, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
72.9% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

@wikkyk
Copy link
Collaborator

wikkyk commented Dec 9, 2024

I'm ok with the barely-missed coverage% target in this case.

@wikkyk wikkyk merged commit 96eeffd into main Dec 9, 2024
8 of 9 checks passed
@wikkyk wikkyk deleted the tls-cert-checking branch December 9, 2024 12:19
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