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

Bug 1706576: Eliminate ec2 metadata dependency #238

Conversation

Projects
None yet
7 participants
@ironcladlou
Copy link
Member

commented May 14, 2019

Access to ec2 metadata will soon be restricted
(openshift/origin#22826). Eliminate the ec2 metadata
dependency by discovering AWS region information from cluster config. This
commit uses the deprecated install config for metatadata; once
openshift/installer#1725 merges, supported cluster
config will provide the region information and the code can be refactored.

@ironcladlou

This comment has been minimized.

Copy link
Member Author

commented May 14, 2019

This is an alternative to #235.

Eliminate ec2 metadata dependency
Access to ec2 metadata will soon be restricted
(openshift/origin#22826). Eliminate the ec2 metadata
dependency by discovering AWS region information from cluster config. This
commit uses the deprecated install config for metatadata; once
openshift/installer#1725 merges, supported cluster
config will provide the region information and the code can be refactored.

@ironcladlou ironcladlou force-pushed the ironcladlou:remove-aws-metadata-lookups branch from d4d6b41 to b6063eb May 14, 2019

@@ -122,6 +138,7 @@ func createDNSManager(cl client.Client, operatorConfig operatorconfig.Config, in
AccessID: string(awsCreds.Data["aws_access_key_id"]),
AccessKey: string(awsCreds.Data["aws_secret_access_key"]),
DNS: dnsConfig,
Region: installConfig.Platform.AWS.Region,

This comment has been minimized.

Copy link
@danehans

danehans May 14, 2019

Contributor

We get the cloud provider from infraConfig and the aws region from installConfig. Is it possible to get the cloud provider from the installConfig too and do away with infraConfig?

This comment has been minimized.

Copy link
@ironcladlou

ironcladlou May 14, 2019

Author Member

You may be missing some context here — the kube-system/cluster-config-v1 ConfigMap is deprecated, and all its usages have (for the most part) been replaced by versioned public config API (e.g. configv1.Infrastructure). In that respect, this PR is actually a regression as we had already finished the API migration and now we're once again using the deprecated stuff (because ingress didn't need region in the public API until today when metadata became off-limits).

So, when region lands in the public API, we need a followup to revert this PR and switch to the new public API version.

@Miciah
Copy link
Contributor

left a comment

Looks good.

region = config.Region
log.Info("using region from operator config", "region name", region)
}
if len(region) == 0 {

This comment has been minimized.

Copy link
@Miciah

Miciah May 14, 2019

Contributor

What do you think of using a switch with three cases (case len(aws.StringValue(sess.Config.Region)) > 0:, case len(config.Region) > 0:, and default:)?

This comment has been minimized.

Copy link
@ironcladlou

ironcladlou May 14, 2019

Author Member

Worth another CI run?

This comment has been minimized.

Copy link
@ironcladlou

ironcladlou May 14, 2019

Author Member

(related: might be able to remove sess.Config.Region support entirely and eliminate the branching — unless someone can remember why it's useful)

This comment has been minimized.

Copy link
@ironcladlou

ironcladlou May 14, 2019

Author Member

(in a followup)

This comment has been minimized.

Copy link
@Miciah

Miciah May 14, 2019

Contributor

Probably not.

@ironcladlou

This comment has been minimized.

Copy link
Member Author

commented May 14, 2019

@Miciah

This comment has been minimized.

Copy link
Contributor

commented May 15, 2019

/lgtm

1 similar comment
@knobunc

This comment has been minimized.

Copy link

commented May 15, 2019

/lgtm

@openshift-ci-robot

This comment has been minimized.

Copy link

commented May 15, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ironcladlou, knobunc, Miciah

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [Miciah,ironcladlou,knobunc]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ironcladlou

This comment has been minimized.

Copy link
Member Author

commented May 15, 2019

/cherrypick release-4.1

@openshift-cherrypick-robot

This comment has been minimized.

Copy link

commented May 15, 2019

@ironcladlou: once the present PR merges, I will cherry-pick it on top of release-4.1 in a new PR and assign it to you.

In response to this:

/cherrypick release-4.1

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@ironcladlou ironcladlou changed the title Eliminate ec2 metadata dependency Bug 1706576: Eliminate ec2 metadata dependency May 15, 2019

@openshift-merge-robot openshift-merge-robot merged commit c75352e into openshift:master May 15, 2019

6 checks passed

ci/prow/e2e-aws Job succeeded.
Details
ci/prow/e2e-aws-operator Job succeeded.
Details
ci/prow/e2e-aws-upgrade Job succeeded.
Details
ci/prow/images Job succeeded.
Details
ci/prow/unit Job succeeded.
Details
tide In merge pool.
Details
@openshift-cherrypick-robot

This comment has been minimized.

Copy link

commented May 15, 2019

@ironcladlou: new pull request created: #239

In response to this:

/cherrypick release-4.1

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.