Skip to content
This repository was archived by the owner on Feb 5, 2020. It is now read-only.

Conversation

@crawford
Copy link
Contributor

No description provided.

@coreosbot
Copy link

Can one of the admins verify this patch?

@crawford
Copy link
Contributor Author

@crawford
Copy link
Contributor Author

retest this please

@yifan-gu
Copy link
Contributor

retest this please.

kube_ca_key_pem = "${local.kube_ca_key_pem}"
kubelet_cert_pem = "${local.kubelet_cert_pem}"
kubelet_key_pem = "${local.kubelet_key_pem}"
tnc_cert_pem = "${local.tnc_cert_pem}"
Copy link
Contributor

Choose a reason for hiding this comment

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

where are these locals being defined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They were added in 37f29a5.

}
u = func() *url.URL {
return &url.URL{
Scheme: "https",
Copy link
Contributor

Choose a reason for hiding this comment

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

this won't work for the bootstrap node request resolved to s3 as the tls aws certs does not know about this domain

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Christ, we need to kill off this S3-pivot insanity.

@crawford
Copy link
Contributor Author

The masters are still trying to use HTTPS to fetch the config. I have no idea why. This works on my local system.

// XXX: The bootstrap node on AWS uses a CNAME to redirect TNC-bound
// traffic to S3. Because of this, HTTPS cannot be used.
scheme := "https"
if c.Platform == "AWS" && role == "master" {
Copy link
Contributor

Choose a reason for hiding this comment

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

aws?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Also calling I think it's advised to c.Platform.String()

i=$((i+1))
[ $i -eq 10 ] && echo "etcdctl failed too many times." && exit 1

echo "etcdctl failed. Retrying in 5 seconds..."
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't etcdctl endpoint health have its own retry logic?
cc @gyuho

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 observed it failing to retry.

Copy link

Choose a reason for hiding this comment

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

The retry logic would depend on the error types. Since endpoint health is just a simple get request, it's safe to retry here.

crawford added 7 commits June 18, 2018 09:27
For some reason, bcrypt and blowfish were left around the last time
glide was run.
This version implements the v2.2.0 config spec, which is needed in order
to customize the certificate authorities. There were a number of code
changes that needed to be made as well due to the change in types.
Instead of comparing against strings, this should use the platform
constants.
This injects the root certificate authority into the stub configs and
switches the TNC URL to HTTPS.
The new TNCO now uses HTTPS after bootstrap.
When this secret was created by tectonic.service, there was a race
condition triggered when TNCO created TNC too quickly. tectonic.sh would
get stuck waiting for all pods to get to the Running state because TNC
would never leave the ContainerCreating state. TNC needed the TLS assets
in order to complete, but those aren't created by tectonic.sh until
after all of the pods are running.

This moves the secret creation earlier in the process to avoid the race.
The call to etcdctl sometimes fails with a TCP error. Retry this
operation a few times before giving up.
@crawford
Copy link
Contributor Author

retest this please

3 similar comments
@yifan-gu
Copy link
Contributor

retest this please

@yifan-gu
Copy link
Contributor

retest this please

@enxebre
Copy link
Contributor

enxebre commented Jun 19, 2018

retest this please

@enxebre enxebre merged commit 17fbef2 into coreos:master Jun 19, 2018
@crawford crawford deleted the https branch June 19, 2018 17:08
wking added a commit to wking/openshift-installer that referenced this pull request Jul 23, 2018
Catching up with c71394d (vendor: bump Ignition to v0.26.0,
2018-06-14, coreos/tectonic-installer#3294).

It would be nice if we could prune the vendored Go for older ignition
config versions, but they're currently chained for translation:

  $ git grep coreos/ignition/config/v2_1 | grep '/v2_2/.*.go:'
  vendor/github.com/coreos/ignition/config/v2_2/config.go:				"github.com/coreos/ignition/config/v2_1"
  vendor/github.com/coreos/ignition/config/v2_2/translate.go:			v2_1 "github.com/coreos/ignition/config/v2_1/types"

I've filed [1] to ask for that.

[1]: coreos/ignition#602
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants