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

Remove S3 params from AWS OIDC IdP set up script #51127

Merged
merged 1 commit into from
Jan 17, 2025

Conversation

marcoandredinis
Copy link
Contributor

This is dead code.
We don't allow setting up the S3 buckets from the UI. They were also being ignored if used in the teleport command.

This is dead code.
We don't allow setting up the S3 buckets from the UI.
They were also being ignored if used in the teleport command.
@marcoandredinis marcoandredinis added no-changelog Indicates that a PR does not require a changelog entry backport/branch/v17 labels Jan 16, 2025
// The script must execute the following command:
// teleport integration configure awsoidc-idp
argsList := []string{
"integration", "configure", "awsoidc-idp",
fmt.Sprintf("--cluster=%s", shsprintf.EscapeDefaultContext(clusterName)),
fmt.Sprintf("--name=%s", shsprintf.EscapeDefaultContext(integrationName)),
fmt.Sprintf("--role=%s", shsprintf.EscapeDefaultContext(role)),
fmt.Sprintf("--proxy-public-url=%s", shsprintf.EscapeDefaultContext(proxyAddr)),
Copy link
Member

Choose a reason for hiding this comment

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

What is this for? The PR description makes it seem like it's not strictly related to dead code removal. ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Instead of doing a argsList = append(argsList, "--proxy-public-url=%s"), I'm now adding it to the initial slice
We were appending here (note that s3Bucket and s3Prefix were always empty).

	case s3Bucket == "" && s3Prefix == "":
		proxyAddr, err := oidc.IssuerFromPublicAddress(h.cfg.PublicProxyAddr, "")
		if err != nil {
			return nil, trace.Wrap(err)
		}

		argsList = append(argsList,
			fmt.Sprintf("--proxy-public-url=%s", shsprintf.EscapeDefaultContext(proxyAddr)),
		)

Copy link
Member

Choose a reason for hiding this comment

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

Oh apologies, I didn't bother to Cmd+F proxy-public-url which would make my comment obsolete.

Copy link
Contributor

@flyinghermit flyinghermit left a comment

Choose a reason for hiding this comment

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

As pointed out in https://github.com/gravitational/teleport/pull/51127/files#r1920312799, please update PR description as it only says to removes dead code but we are also adding some new code.

@marcoandredinis marcoandredinis added this pull request to the merge queue Jan 17, 2025
Merged via the queue into master with commit 68078ee Jan 17, 2025
46 checks passed
@marcoandredinis marcoandredinis deleted the marco/remove_awsoidc_idpsetup_s3params branch January 17, 2025 17:27
@public-teleport-github-review-bot

@marcoandredinis See the table below for backport results.

Branch Result
branch/v17 Create PR

mvbrock pushed a commit that referenced this pull request Jan 18, 2025
This is dead code.
We don't allow setting up the S3 buckets from the UI.
They were also being ignored if used in the teleport command.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/branch/v17 no-changelog Indicates that a PR does not require a changelog entry size/sm ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants