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

Improve readme steps for installation of apps #2364

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 53 additions & 12 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,21 @@ There are two options when managing DNS records, manually or ExternalDNS.

Assuming you already have everything needed to install the apps, this is what you need to do.

> [!NOTE]
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm missing a section giving an overview of Apps. This is stuff that we already know and have in our DNA, which is why it's so hard to write it. Someone completely new to the project should first learn that:

  • Welkin follows the "configuration as code" principle and expects all configuration to be in a so-called configuration folder.
  • Welkin strongly recommends to have this configuration folder in .git. Apps may be a submodule, so that the configuration folder is the source of truth for how an environment is set up.
  • Welkin Apps' functionality is accessed threw the ./bin/ck8s command-line tool.

Makes sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pushed a change. Is that satisfactory? Hard to know where to exactly place it.

Copy link
Contributor

Choose a reason for hiding this comment

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

It feels like we need to rethink the structure here.
Because first we tell them to setup domains to something, which in the normal case (with infrastructure providers supporting LB SVC) won't work.
Then second we tell them to setup ingress controller without first setting up the environment directory, which you have to remember to go back to.

I feel like we need to better identify the happy path, and make the structure better at being referable should we need to jump somewhere.

> Depending on your infrastructure, you might utilize a Service of type LoadBalancer for the ingress controller. This means you will not have an IP for the domains before installing the ingress controller. After configuring and validating the config, you can install just the ingress controller before the rest of apps with the following command
>
> ```bash
> ./bin/ck8s ops helmfile sc apply -lapp=ingress-nginx --include-transitive-needs
> ./bin/ck8s ops helmfile wc apply -lapp=ingress-nginx --include-transitive-needs
Copy link
Contributor

Choose a reason for hiding this comment

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

I am wondering if this README somewhere and somehow should state that you might not be able to run commands for wc if you have not installed Dex in sc first, since we usually assume for Welkin that wc is configured to point to Dex in sc 🤔

> ```
>
> The IP is then available on the ingress controller Service
>
> ```bash
> ./bin/ck8s ops kubectl sc -n ingress-nginx get svc ingress-nginx-controller
> ./bin/ck8s ops kubectl wc -n ingress-nginx get svc ingress-nginx-controller
> ```

The other option is to let ExternalDNS manage your DNS records, currently only AWS Route 53 is supported.
You configure ExternalDNS later in the process.

Expand All @@ -132,10 +147,10 @@ You configure ExternalDNS later in the process.
export CK8S_K8S_INSTALLER=[kubespray|capi] # set this to whichever installer was used for the kubernetes layer
```

> [!NOTE]
> The `air-gapped` flavor has a lot of the same settings as the `prod` flavor but with some additional variables that you need to configure yourself (these are set to `set-me`).
> [!NOTE]
> The `air-gapped` flavor has a lot of the same settings as the `prod` flavor but with some additional variables that you need to configure yourself (these are set to `set-me`).

1. Then set the path to where the ck8s configuration should be stored and the PGP fingerprint of the key(s) to use for encryption:
2. Then set the path to where the ck8s configuration should be stored and the PGP fingerprint of the key(s) to use for encryption:
lucianvlad marked this conversation as resolved.
Show resolved Hide resolved

```bash
export CK8S_CONFIG_PATH=${HOME}/.ck8s/my-ck8s-cluster
Expand All @@ -154,22 +169,27 @@ You configure ExternalDNS later in the process.
./bin/ck8s init both
```

> [!NOTE]
> It is possible to initialize `wc` and `sc` clusters separately by replacing `both` when running the `init` command:
>
> ```bash
> ./bin/ck8s init wc
> ./bin/ck8s init sc
> ```
> [!NOTE]
> It is possible to initialize `wc` and `sc` clusters separately by replacing `both` when running the `init` command:
>
> ```bash
> ./bin/ck8s init wc
> ./bin/ck8s init sc
> ```

1. Edit the configuration files that have been initialized in the configuration path.
4. Edit the configuration files that have been initialized in the configuration path.
Make sure that the `objectStorage` values are set in `common-config.yaml` or `sc-config.yaml` and `wc-config.yaml`, as well as required credentials in `secrets.yaml` according to your `objectStorage.type`.
The type may already be set in the default configuration found in the `defaults/` directory depending on your selected cloud provider.
Set `objectStorage.s3.*` if you are using S3 or `objectStorage.gcs.*` if you are using GCS.
Enable ExternalDNS `externalDns.enabled` and set the required variables, if you want ExternalDNS to manage your records from inside your cluster.
It requires credentials to route53, `txtOwnerId`, `endpoints` if `externalDns.sources.crd` is enabled.

1. Create S3 buckets - optional
> [!NOTE]
> One important configuration is whether or not you need to use proxy protocol for the ingress controller which depends on what infrastructure you use. You enable it and need to set an annotation depending on your infrastructure. Example for openstack
> `ingressNginx.controller.config.useProxyProtocol: true`
> `ingressNginx.controller.service.annotations: { loadbalancer.openstack.org/proxy-protocol: "true" }`

5. Create S3 buckets - optional
If you have set `objectStorage.type: s3`, then you need to create the buckets specified under `objectStorage.buckets` in your configuration files.
You can run the script `scripts/S3/entry.sh create` to create the buckets required.
The script uses `s3cmd` in the background and it uses the `${HOME}/.s3cfg` file for configuration and authentication for your S3 provider.
Expand Down Expand Up @@ -205,6 +225,27 @@ You configure ExternalDNS later in the process.
)
```

1. Update Network Policies

```bash
./bin/ck8s update-ips both apply
lucianvlad marked this conversation as resolved.
Show resolved Hide resolved
```

1. Validate config and fill in missing values
This should indicate any missing configuration that still needs to be set.

```bash
./bin/ck8s validate sc
./bin/ck8s validate wc
```

1. **Optional step**, install ingress controller and setup DNS as explained above.

```bash
./bin/ck8s ops helmfile sc apply -lapp=ingress-nginx --include-transitive-needs
./bin/ck8s ops helmfile wc apply -lapp=ingress-nginx --include-transitive-needs
```

1. **Note**, for this step each cluster need to be up and running already.
Deploy the apps:

Expand Down
4 changes: 2 additions & 2 deletions bin/update-ips.bash
Original file line number Diff line number Diff line change
Expand Up @@ -161,8 +161,8 @@ get_dns_ips() {
local -a ips
mapfile -t ips < <(dig +short "${domain}" | grep '^[.0-9]*$')
if [ ${#ips[@]} -eq 0 ]; then
log_error "No IPs for ${domain} was found"
exit 1
log_error "No IPs for ${domain} was found. Will block all IPs"
echo "0.0.0.0"
Copy link
Contributor

@simonklb simonklb Dec 5, 2024

Choose a reason for hiding this comment

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

Will this not do the opposite, i.e. allow all IPs? This function is used to resolve a domain name to an IP so that it can be added to the allow list, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This eventually sets the CIDR in the NP to 0.0.0.0/32, i.e. just the 0.0.0.0 ip which would never be a real IP meaning it blocks everything.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The process_ips_to_cidrs seems to be the one adding the /32

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah of course 🤦

I'm still a bit worried that we might at some point rewrite this and not all IPs go through that same IP to CIDR transformation function.

Also, why would you want it not to fail if the domain can't be resolved? Is it not better to become aware of it?

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, when you use a LoadBalancer Service for ingress-nginx you can't create the domains before installing ingress-nginx. And before you could not run update-ips since it would fail and the schema validation would complain if you tried to apply anything. This seemed like a better approach then having to manually set all NPs to something first. Not perfect but I'm not sure if there is a better solution.

Although I just realized I forgot to mention you would have to re-run update-ips again after setting up the DNS.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could it not be left emptied instead to disallow everything that way? 🤔
(IIRC the associated rules shouldn't be rendered in that case, because empty netpols allow everything, but we have a split between the old and new netpol still.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe split the script change and the documentation changes into separate PRs, both good for changelog purposes and review purposes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could it not be left emptied instead to disallow everything that way? 🤔

If I leave it empty instead the CIDR ends up as just /32 which fails in the apply

fi
echo "${ips[@]}"
}
Expand Down
Loading