Skip to content

External variables and verbosity #6

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

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

Conversation

ksamaschke
Copy link
Collaborator

Summary

  • Removed hardcoded values by introducing environment variables
  • Make CCM load balancer type configurable through environment variables
  • Simplify cluster status checking script to be more user-friendly

Changes Made

Configuration Enhancement

  • Added CS_CCMLB parameter to both cluster-settings-template.env and cluster-settings.env.sample
  • This parameter allows users to configure the Cloud Controller Manager load balancer type (octavia-ovn or octavia-amphora)
  • Removed hardcoded OCTOVN setting in 04-cloud-secret.sh, replacing it with dynamic configuration based on CS_CCMLB value

Script Simplification

  • Refactored 08-wait-cluster.sh to check cluster status once instead of looping
  • Script now shows current status and saves kubeconfig if cluster is ready
  • Removed continuous polling logic in favor of a simpler check-and-report approach
  • Removed .yaml extension from kubeconfig filename to follow Kubernetes conventions

@ksamaschke ksamaschke requested a review from garloff May 17, 2025 03:06
@ksamaschke ksamaschke added the enhancement New feature or request label May 17, 2025
ksamaschke and others added 4 commits May 17, 2025 05:09
Signed-off-by: Karsten Samaschke <[email protected]>
- Add 99-prepare-files.sh to automate certificate and clouds.yaml setup
- Update README with prerequisites and instructions for config preparation
- Clarify that only the last certificate from ca-certificates.crt is needed
- Remove unused CL_WAIT_TIMEOUT parameter from all configuration files
- Document how to obtain and extract Cloud-in-a-Box certificate
@berendt
Copy link

berendt commented May 17, 2025

Since this is to be published as CC-BY-SA, I would prefer to mark code generated with AI as such in the commit messages. For example, we use AI-assisted: Claude Code in the last line of the commit message.

Also I think it makes sense to have the CLAUDE.md file in the repository when working with Claude Code.

Copy link
Member

@garloff garloff May 20, 2025

Choose a reason for hiding this comment

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

Why do we need a sample file when we have a template?
And it copies all the comments (which is documentation),
so we now need to adjust one more file when we update them ...

region_name: "RegionOne"
interface: "public"
identity_api_version: 3
verify: false # Skip TLS verification for self-signed certs
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, let's comment this one out and rather suggest to fill in a valid ca-bundle.crt, hinting that it would be the last cert in /etc/ssl/certs/ca-certificates.crt on the system with self-signed certs typically.
verify: false is an insecure workaround for cases where you lack it.


* 17-delete-cluster.sh: Remove cluster again.
* `08-wait-cluster.sh`: Check cluster status and save kubeconfig if ready.
This script no longer waits in a loop but provides immediate status
Copy link
Member

Choose a reason for hiding this comment

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

It never did.
But that's what's expected from a script with the name wait.
So call it poll or implement waiting ...


### Configuration Parameters

#### Registry and Repository Settings
Copy link
Member

Choose a reason for hiding this comment

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

I like the way you structure the settings in different groups.
We probably should be thinking about new prefixes for the first two categories.
We also need to make sure we don't duplicate information too much, as divergence is otherwise hard to avoid.
We have the main documentation for the parameters in the -template.env file. It would be my preferred place, TBH.
Additional documentation then should NOT repeat the explanations in there, but introduce higher level concepts etc.


### Obtaining the Cloud-in-a-Box Certificate

If you're using SCS Cloud-in-a-Box (CiaB), you'll need to obtain the certificate. The certificates are located on the CiaB manager host at `/etc/ssl/certs/ca-certificates.crt`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
If you're using SCS Cloud-in-a-Box (CiaB), you'll need to obtain the certificate. The certificates are located on the CiaB manager host at `/etc/ssl/certs/ca-certificates.crt`.
If you're using SCS Cloud-in-a-Box (CiaB), you'll need to obtain the certificate. The certificates are located on the CiaB manager host at `/etc/ssl/certs/ca-certificates.crt`.
This step is only required because CiaB uses self-signed certificates -- please skip it on clouds that use TLS certificates that are signed by a proper certificate authority (CA).

@@ -16,7 +16,7 @@ if test -z "$CS_MAINVER"; then echo "Configure CS_MAINVER"; exit 2; fi
if test -z "$CS_VERSION"; then echo "Configure CS_VERSION"; exit 3; fi
if test -z "$CL_PATCHVER"; then echo "Configure CL_PATCHVER"; exit 4; fi
if test -z "$CL_NAME"; then echo "Configure CL_NAME"; exit 5; fi
if test -z "$CL_PODDIDR"; then echo "Configure CL_PODCIDR"; exit 6; fi
if test -z "$CL_PODCIDR"; then echo "Configure CL_PODCIDR"; exit 6; fi
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for spotting this!

@garloff
Copy link
Member

garloff commented May 20, 2025

Overall comments:

  • Your scripts seem to be way more verbose than mine.
    • This has the advantage of giving the user more information
    • It has the disadvantage of hiding information as things scroll out of a single screen
      So my request is to not overdo it.
  • Let's be careful to avoid duplicating comment/documentation too much. This creates pain down the line
    • This in particular refers to documenting all the variables. They are now documented at three places:
      • cluster-settings-template.env (this is where I meant to document them)
      • cluster-settings-sample.env (if we really need this file, we should strip it from comments and make it minimal, IMVHO)
      • README.md has a good structure, but probably should reference the -template file for details and limit itself to the concepts (as we have 4 categories of settings, internal script settings, per NS = target cloud project, per CS and per Cluster settings, which users should really understand to avoid confusion -- I can remember my own confusion)
    • I think we will revise the naming of the settings slightly in the future to reflect those four categories (and maybe to connect to the naming of KaaS-v1). So the maintenance burden of three places is real.
  • It would have been better to have smaller pull requests, so we could review and decide on a more granular basis.
  • There seems to be DCOs missing.
  • Please address @berendt's comment on Claude.

Sidenote: #9 adds a lot more robustness to the clouds.yaml handling, one thing that people will stumble upon.

garloff added 5 commits May 21, 2025 09:27
04-cloud-secret.sh can only append auth values to the end.

Signed-off-by: Kurt Garloff <[email protected]>
Adjust sample to be consistent.

Signed-off-by: Kurt Garloff <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants